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:28 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8571C307D966; Thu, 11 Apr 2019 11:47:27 +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 E450960BF7; Thu, 11 Apr 2019 11:47:25 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 10/31] OvmfPkg/XenPlatformPei: Detect OVMF_INFO from hvmloader From: "Laszlo Ersek" 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> <10573b3b-dfb8-c9e3-fb0c-6ea6f07586de@redhat.com> Message-ID: Date: Thu, 11 Apr 2019 13:47:25 +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: <10573b3b-dfb8-c9e3-fb0c-6ea6f07586de@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Thu, 11 Apr 2019 11:47:27 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/11/19 13:47, Laszlo Ersek wrote: > 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: sigh, I meant "(1) through (3)". > 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, >> >