From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: citrix.com, ip: 216.71.155.175, mailfrom: roger.pau@citrix.com) Received: from esa6.hc3370-68.iphmx.com (esa6.hc3370-68.iphmx.com [216.71.155.175]) by groups.io with SMTP; Thu, 08 Aug 2019 03:40:38 -0700 Authentication-Results: esa6.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=roger.pau@citrix.com; spf=Pass smtp.mailfrom=roger.pau@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa6.hc3370-68.iphmx.com: no sender authenticity information available from domain of roger.pau@citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa6.hc3370-68.iphmx.com; envelope-from="roger.pau@citrix.com"; x-sender="roger.pau@citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa6.hc3370-68.iphmx.com: domain of roger.pau@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa6.hc3370-68.iphmx.com; envelope-from="roger.pau@citrix.com"; x-sender="roger.pau@citrix.com"; x-conformance=sidf_compatible; x-record-type="v=spf1"; x-record-text="v=spf1 ip4:209.167.231.154 ip4:178.63.86.133 ip4:195.66.111.40/30 ip4:85.115.9.32/28 ip4:199.102.83.4 ip4:192.28.146.160 ip4:192.28.146.107 ip4:216.52.6.88 ip4:216.52.6.188 ip4:162.221.158.21 ip4:162.221.156.83 ~all" Received-SPF: None (esa6.hc3370-68.iphmx.com: no sender authenticity information available from domain of postmaster@mail.citrix.com) identity=helo; client-ip=162.221.158.21; receiver=esa6.hc3370-68.iphmx.com; envelope-from="roger.pau@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: elcTG9IXHm2qaXodC/GfoqeL9gx/v/fzf0Z/43p+n4pSmrE/CRF8e2DNw+wbMJ1YGqKzJH4WlJ lX2kcE0qfsTjwvjeFXMMFu9tVStRJN3tVYMuuK9xwnzhv8buX5Xe6lT3JF5fbP75wx3RhXZ5M/ RRwT91tfIzsm26mDpWvz70bm7oiH0miMvg9D5TQgGxr+3mBIeea8Zpt3G0Igb5xXH8Z+IKmJ20 kiO7YgtbC+k6kEnDuTR7EphHp1u5rfgsvzG/wdcNukM0duPXUsIzdfTL1WRzDx4BtGJbV1ibKc Yi4= X-SBRS: 2.7 X-MesageID: 4183112 X-Ironport-Server: esa6.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.64,360,1559534400"; d="scan'208";a="4183112" Date: Thu, 8 Aug 2019 12:40:31 +0200 From: =?UTF-8?B?Um9nZXIgUGF1IE1vbm7DqQ==?= To: Anthony PERARD CC: , Julien Grall , , Jordan Justen , Ard Biesheuvel , Laszlo Ersek Subject: Re: [edk2-devel] [PATCH v4 12/35] OvmfPkg/XenPlatformPei: Grab RSDP from PVH guest start of day struct Message-ID: <20190808104031.yhqdjgawwkddwxac@Air-de-Roger> References: <20190729153944.24239-1-anthony.perard@citrix.com> <20190729153944.24239-13-anthony.perard@citrix.com> <20190807143558.uxha44jflgmstdkj@Air-de-Roger> <20190808102641.GQ1242@perard.uk.xensource.com> MIME-Version: 1.0 In-Reply-To: <20190808102641.GQ1242@perard.uk.xensource.com> User-Agent: NeoMutt/20180716 Return-Path: roger.pau@citrix.com X-ClientProxiedBy: AMSPEX02CAS02.citrite.net (10.69.22.113) To AMSPEX02CL02.citrite.net (10.69.22.126) Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: 8bit On Thu, Aug 08, 2019 at 11:26:41AM +0100, Anthony PERARD wrote: > On Wed, Aug 07, 2019 at 04:35:58PM +0200, Roger Pau Monné wrote: > > On Mon, Jul 29, 2019 at 04:39:21PM +0100, Anthony PERARD wrote: > > > Check if there's a start of the day struct provided to PVH guest, save > > > the ACPI RSDP address for later. > > > > > > This patch import import arch-x86/hvm/start_info.h from xen.git. > > > > You seem to change the types when importing start_info.h, is that > > absolutely necessary? > > I guess one could try to map xen's types to EDKII's type with typedefs, > but I'm not sur how well that would work. Importing the xen headers is > documented so changing the types is fairly easy, see > https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/IndustryStandard/Xen/README > > Also, changing the header further might have been something useful to > do, we could have match EDKII's naming convention and source files would > have looked a bit less weird. Ack, didn't know there was a README for this procedure. > > From my experience working with different projects when importing such > > headers it's better to keep them verbatim, this makes importing future > > versions from upstream easier. > > > > I have a comment below, but it's more like a question. > > > > > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > > > index 5c7d7ddc1c..b366139a0a 100644 > > > --- a/OvmfPkg/XenPlatformPei/Xen.c > > > +++ b/OvmfPkg/XenPlatformPei/Xen.c > > > @@ -25,6 +25,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #include "Platform.h" > > > #include "Xen.h" > > > @@ -86,6 +87,7 @@ XenConnect ( > > > UINT32 XenVersion; > > > EFI_XEN_OVMF_INFO *Info; > > > CHAR8 Sig[sizeof (Info->Signature) + 1]; > > > + UINT32 *PVHResetVectorData; > > > > > > AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL); > > > mXenInfo.HyperPages = AllocatePages (TransferPages); > > > @@ -121,6 +123,29 @@ XenConnect ( > > > mXenHvmloaderInfo = NULL; > > > } > > > > > > + mXenInfo.RsdpPvh = NULL; > > > + > > > + // > > > + // Locate and use information from the start of day structure if we have > > > + // booted via the PVH entry point. > > > + // > > > + > > > + PVHResetVectorData = (VOID *)(UINTN) PcdGet32 (PcdXenPvhStartOfDayStructPtr); > > > + // > > > + // That magic value is written in XenResetVector/Ia32/XenPVHMain.asm > > > + // > > > + if (PVHResetVectorData[1] == SIGNATURE_32 ('X', 'P', 'V', 'H')) { > > > + struct hvm_start_info *HVMStartInfo; > > > + > > > + HVMStartInfo = (VOID *)(UINTN) PVHResetVectorData[0]; > > > + if (HVMStartInfo->magic == XEN_HVM_START_MAGIC_VALUE) { > > > + ASSERT (HVMStartInfo->rsdp_paddr != 0); > > > + if (HVMStartInfo->rsdp_paddr != 0) { > > > + mXenInfo.RsdpPvh = (VOID *)(UINTN)HVMStartInfo->rsdp_paddr; > > > > I guess you can do this because OVMF has an identity map of virtual > > addresses to physical addresses? > > I think so, yes. I know that early code does created page table like > that, but I don't know if later code rework those page table or not. > > > I wonder the size of such identity map, and whether you need to check > > that the rsdp address is indeed inside of that region before > > converting it to a pointer. The same would apply to > > PcdXenPvhStartOfDayStructPtr, but that's likely safe because it's > > always < 4GB, which I assume OVMF always has identity mapped? > > PcdXenPvhStartOfDayStructPtr is safe because OVMF owns it. As for the > rspd_paddr* and the HVMStartInfo*, I need to check. As you say, it's > probably fine as long as it's <4GB. > > I've looked at the comment here: > https://github.com/tianocore/edk2/blob/master/OvmfPkg/ResetVector/Ia32/PageTables64.asm#L94 > This mean that the code executed in the patch (accessing the hvm start > info struct) is executed while the id map is setup up to 4GB. So as long > as the struct is below 4G, it's fine. Yes, the start_info struct is guaranteed to be below 4GB because the physical memory address of it is passed on a register when starting the kernel in 32bit protected mode, so the address cannot be greater than 4GB or it would be truncated. > As for the RSDP, I think that pointer is accessed much later, when a > different page table is setup, I think that would be that code: > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > But I'm not sure how much is setup. But I'm guessing that whatever is > pointed by RSDP, it will be in the 1:1, because we tell the UEFI about > it while parsing the e820. OK, as long as we know it's safe to access. Note it's quite likely the rsdp is also below 4GB anyway, but better be safe than sorry. Thanks, Roger.