From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 11 Apr 2019 04:47:07 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C8EA71F560; Thu, 11 Apr 2019 11:47:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-225.rdu2.redhat.com [10.10.120.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1764B1001E67; Thu, 11 Apr 2019 11:47:04 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 10/31] OvmfPkg/XenPlatformPei: Detect OVMF_INFO from hvmloader To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Jordan Justen , Ard Biesheuvel , Julien Grall , xen-devel@lists.xenproject.org References: <20190409110844.14746-1-anthony.perard@citrix.com> <20190409110844.14746-11-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <10573b3b-dfb8-c9e3-fb0c-6ea6f07586de@redhat.com> Date: Thu, 11 Apr 2019 13:47:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190409110844.14746-11-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 11 Apr 2019 11:47:06 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/09/19 13:08, Anthony PERARD wrote: > This struct is only useful to retrieve the E820 table. The (1) please replace "this struct" with the name of the struct. > mXenHvmloaderInfo isn't used yet, but will be use in a further patch to > retrieve the E820 table. > > Also remove the unused pointer from the XenInfo HOB as that information > is only useful in the XenPlatformPei. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Anthony PERARD > --- > OvmfPkg/Include/Guid/XenInfo.h | 4 ---- > OvmfPkg/PlatformPei/Xen.c | 3 --- > OvmfPkg/XenPlatformPei/Xen.c | 23 ++++++++++++++++++-- > 3 files changed, 21 insertions(+), 9 deletions(-) > > diff --git a/OvmfPkg/Include/Guid/XenInfo.h b/OvmfPkg/Include/Guid/XenInfo.h > index d512b0b63f..5d4579f244 100644 > --- a/OvmfPkg/Include/Guid/XenInfo.h > +++ b/OvmfPkg/Include/Guid/XenInfo.h > @@ -24,10 +24,6 @@ typedef struct { > /// > VOID *HyperPages; > /// > - /// Location of the hvm_info page. > - /// > - VOID *HvmInfo; > - /// > /// Hypervisor major version. > /// > UINT16 VersionMajor; > diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c > index ab38f97a67..75181be2fd 100644 > --- a/OvmfPkg/PlatformPei/Xen.c > +++ b/OvmfPkg/PlatformPei/Xen.c > @@ -104,9 +104,6 @@ XenConnect ( > mXenInfo.VersionMajor = (UINT16)(XenVersion >> 16); > mXenInfo.VersionMinor = (UINT16)(XenVersion & 0xFFFF); > > - /* TBD: Locate hvm_info and reserve it away. */ > - mXenInfo.HvmInfo = NULL; > - > BuildGuidDataHob ( > &gEfiXenInfoGuid, > &mXenInfo, > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > index 7c82e5b69b..a866641b67 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -39,6 +39,12 @@ STATIC UINT32 mXenLeaf = 0; > > EFI_XEN_INFO mXenInfo; > > +// > +// Location of the firmware info struct setup by hvmloader. > +// Only the E820 table is used by OVMF. > +// > +EFI_XEN_OVMF_INFO *mXenHvmloaderInfo; > + > /** > Returns E820 map provided by Xen > > @@ -84,6 +90,8 @@ XenConnect ( > UINT32 TransferReg; > UINT32 TransferPages; > UINT32 XenVersion; > + EFI_XEN_OVMF_INFO *Info; > + CHAR8 Sig[sizeof (Info->Signature) + 1]; > > AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL); > mXenInfo.HyperPages = AllocatePages (TransferPages); > @@ -103,8 +111,19 @@ XenConnect ( > mXenInfo.VersionMajor = (UINT16)(XenVersion >> 16); > mXenInfo.VersionMinor = (UINT16)(XenVersion & 0xFFFF); > > - /* TBD: Locate hvm_info and reserve it away. */ > - mXenInfo.HvmInfo = NULL; > + // > + // Check if there are information left by hvmloader > + // > + > + Info = (EFI_XEN_OVMF_INFO *)(UINTN) OVMF_INFO_PHYSICAL_ADDRESS; > + // Copy the signature, and make it null-terminated. (2) Please use the following style: // // Copy the signature, and make it null-terminated. // > + AsciiStrnCpyS(Sig, sizeof (Sig), > + (CHAR8 *) &Info->Signature, sizeof (Info->Signature)); (3) This should be indented as: AsciiStrnCpyS ( Sig, sizeof (Sig), (CHAR8 *) &Info->Signature, sizeof (Info->Signature) ); or: AsciiStrnCpyS (Sig, sizeof (Sig), (CHAR8 *) &Info->Signature, sizeof (Info->Signature)); (note the whitespace before the opening paren, after the function designator) I'm not checking the correctness of the parameters, for now. With (1) and (3) addressed: Acked-by: Laszlo Ersek If you can, please verify the comment style and the indentation style in the other patches too. Thanks Laszlo > + if (AsciiStrCmp (Sig, "XenHVMOVMF") == 0) { > + mXenHvmloaderInfo = Info; > + } else { > + mXenHvmloaderInfo = NULL; > + } > > BuildGuidDataHob ( > &gEfiXenInfoGuid, >