Re: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area

["Yao, Jiewen" <jiewen.yao@intel.com>]

Ah, sorry, I did not realize that you are using that in FDF.

If we have to define the PcdOvmfWorkAreaHeaderSize, then I would suggest:
1) Add detail comment in the PCD definition - it must be same as sizeof(Header).
2) Add ASSERT in the code to ensure PcdOvmfWorkAreaHeaderSize is same as sizeof(Header).

Just in case someone change only one value, it can be caught in the code.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Thursday, August 5, 2021 10:44 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: brijesh.singh@amd.com; James Bottomley <jejb@linux.ibm.com>; Xu, Min M
> <min.m.xu@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Erdem Aktas <erdemaktas@google.com>;
> Michael Roth <Michael.Roth@amd.com>
> Subject: Re: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area
> 
> Hi Jiewen,
> 
> On 8/4/21 9:18 PM, Yao, Jiewen wrote:
> > HI Brijesh
> > Thanks for the startup. Feedback below:
> >
> > 1) I don't think we need a PCD to indicate the header.
> >
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaHeaderSize|4|UINT32|0x51
> >
> > Instead, if we define a HEADER structure, we can use sizeof() naturally.
> Otherwise, when we update this header, we need update 2 different places,
> which is not preferred.
> >
> 
> Can you use the sizeof() inside the OvmfPkg.fdf ? I was not able to find
> a reference usage for it in this file. Also, can a .fdf refer the C
> header file ?
> 
> We need to know the size of the header so that we can set the fixed
> PcdOvmfSevEsWorkArea. In the current approach the .fdf sets the PCD as
> shown below:
> 
> SET PcdOvmfSevEsWorkArea = PcdOvmfWorkAreaBase + <HeaderSize>
> 
> I am hard coding the header size to be 4 so that pcd points to the
> correct location within the WorkArea.
> 
> thanks
> 
> 
> > typedef struct {
> >    UINT8                   GuestType;
> >    UINT8                   Reserved1[3];
> > } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> >
> > 2) I don't think we can define a common structure OVMF_WORK_AREA to
> contain SEV specific field.
> >
> > typedef struct _OVMF_WORK_AREA {
> >    UINT8                   GuestType;
> >    UINT8                   Reserved1[3];
> >
> >    SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
> > } OVMF_WORK_AREA;
> >
> > A common patter is to define each individual structure, then use UNION.
> >
> > For example,
> >
> > typedef struct {
> >    UINT8                   GuestType;
> >    UINT8                   Reserved1[3];
> >
> >    SEC_SEV_ES_WORK_AREA    SevEsWorkArea;
> > } SEV_WORK_AREA;
> >
> > typedef union {
> >    CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER Header;
> >    SEV_WORK_AREA  Sev;
> > } OVMF_WORK_AREA;
> >
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> >> Singh via groups.io
> >> Sent: Thursday, August 5, 2021 4:20 AM
> >> To: devel@edk2.groups.io
> >> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M
> <min.m.xu@intel.com>;
> >> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> >> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> >> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
> >> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>;
> Brijesh
> >> Singh <brijesh.singh@amd.com>
> >> Subject: [edk2-devel] [PATCH 0/3] reuse the SevEsWork area
> >>
> >> Based on the discussion on the mailing list, we agreed that instead
> >> of wasting extra page in the MEMFD, we can reuse the SevEsWorkArea
> >> buffer for the TDX. To avoid any confusion, lets introduce a OvmfWorkArea
> >> that will contains 32 bytes of header followed by the actual workarea.
> >>
> >> While at it, move the code to clear the GHCB page from PageTable build
> >> to AmdSev.asm.
> >>
> >> I have used the existing TDX BZ for it because the request came
> >> during the TDX patch review. if anyone have concern please let me know
> >> and I will happily create a new BZ.
> >>
> >> Full tree is at:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2FAMDESE%2Fovmf%2Ftree%2Fsev-new-work-
> area&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C4c55a642f1804a80
> 3c4e08d957b75e61%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6
> 37637267367225365%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
> LCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NSs
> UVfQodJMDUcpLCsHSpTaRDHM8et%2BWZJOS8lCS3Kw%3D&amp;reserved=0
> >>
> >> Brijesh Singh (3):
> >>    OvmfPkg: introduce a common work area
> >>    OvmfPkg/ResetVector: update SEV support to use new work area format
> >>    OvmfPkg/ResetVector: move the GHCB page setup in AmdSev.asm
> >>
> >> Cc: James Bottomley <jejb@linux.ibm.com>
> >> Cc: Min Xu <min.m.xu@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Erdem Aktas <erdemaktas@google.com>
> >>
> >>   OvmfPkg/OvmfPkg.dec                        |   6 ++
> >>   OvmfPkg/OvmfPkgX64.fdf                     |   9 +-
> >>   OvmfPkg/PlatformPei/PlatformPei.inf        |   4 +-
> >>   OvmfPkg/ResetVector/ResetVector.inf        |   1 +
> >>   OvmfPkg/Sec/SecMain.inf                    |   1 +
> >>   OvmfPkg/Include/Library/MemEncryptSevLib.h |  21 +---
> >>   OvmfPkg/Include/WorkArea.h                 |  53 ++++++++++
> >>   OvmfPkg/PlatformPei/MemDetect.c            |  32 +++---
> >>   OvmfPkg/Sec/SecMain.c                      |  25 ++++-
> >>   OvmfPkg/ResetVector/Ia32/AmdSev.asm        | 111 +++++++++++++++++----
> >>   OvmfPkg/ResetVector/Ia32/PageTables64.asm  |  57 ++---------
> >>   OvmfPkg/ResetVector/ResetVector.nasmb      |   1 +
> >>   12 files changed, 213 insertions(+), 108 deletions(-)
> >>   create mode 100644 OvmfPkg/Include/WorkArea.h
> >>
> >> --
> >> 2.17.1
> >>
> >>
> >>
> >>
> >>
> >
> 
> 
> 
>