Re: [PATCH v5 3/4] OvmfPkg/PlatformPei: Mark SEC GHCB page as unencrypted via hypercall

["Ashish Kalra" <ashish.kalra@amd.com>]

Hello Tom,

On Fri, Jul 16, 2021 at 09:22:20AM -0500, Tom Lendacky wrote:
> On 7/8/21 9:08 AM, Ashish Kalra wrote:
> > From: Ashish Kalra <ashish.kalra@amd.com>
> > 
> > Mark the SEC GHCB page (that is mapped as unencrypted in
> > ResetVector code) in the hypervisor page status tracking.
> > 
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  OvmfPkg/PlatformPei/AmdSev.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> > index a8bf610022..1ec0de48fe 100644
> > --- a/OvmfPkg/PlatformPei/AmdSev.c
> > +++ b/OvmfPkg/PlatformPei/AmdSev.c
> > @@ -52,6 +52,15 @@ AmdSevEsInitialize (
> >    PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
> >    ASSERT_RETURN_ERROR (PcdStatus);
> >  
> > +  //
> > +  // GHCB_BASE setup during reset-vector needs to be marked as
> 
> s/GHCB_BASE/The SEC Ghcb/
> 
> > +  // decrypted in the hypervisor page encryption bitmap.
> 
> Is the "hypervisor page encryption bitmap" valid anymore? This gets passed
> up to userspace now, right?
> 
> You should go through all the patches to be sure you aren't talking about
> a bitmap anymore and just state that you're updating the encryption state
> with the hypervisor.
> 

Ok, i will fix that.

> > +  //
> > +  SetMemoryEncDecHypercall3 (FixedPcdGet32 (PcdOvmfSecGhcbBase),
> 
> The first argument needs to be moved down to a line of its own and
> indented like the following arguments.
> 
> > +    EFI_SIZE_TO_PAGES(FixedPcdGet32 (PcdOvmfSecGhcbSize)),
> > +    KVM_MAP_GPA_RANGE_DECRYPTED
> 
> Ah, now I see this #define used, but you should be passing a 0 or 1,
> right? This happens to evaluate to 0, but it's the wrong way to call this
> function.
> 

Ok.

Thanks,
Ashish

> > +    );
> > +
> >    //
> >    // Allocate GHCB and per-CPU variable pages.
> >    //   Since the pages must survive across the UEFI to OS transition
> >