Re: [edk2-devel] [PATCH] OvmfPkg: Don't make APIC MMIO accesses with encryption bit set

["Roth, Michael via groups.io" <Michael.Roth=amd.com@groups.io>]

On Wed, Apr 24, 2024 at 01:54:01PM +0200, Gerd Hoffmann wrote:
> On Tue, Apr 23, 2024 at 03:59:58PM -0500, Michael Roth wrote:
> > For the most part, OVMF will clear the encryption bit for MMIO regions,
> > but there is currently one known exception during SEC when the APIC
> > base address is accessed via MMIO with the encryption bit set for
> > SEV-ES/SEV-SNP guests.
> 
> what exactly accesses the lapic that early?

This looks to be for InitializeDebugAgent() to set up a timer to handle the
debug console.

> 
> > +/**
> > +  Map known MMIO regions unencrypted if SEV-ES is active.
> > +
> > +  During early booting, page table entries default to having the encryption bit
> > +  set for SEV-ES/SEV-SNP guests. In cases where there is MMIO to an address, the
> > +  encryption bit should be cleared. Clear it here for any known MMIO accesses
> > +  during SEC, which is currently just the APIC base address.
> > +
> > +**/
> > +VOID
> > +SecMapApicBaseUnencrypted (
> > +  VOID
> > +  )
> > +{
> > +  PAGE_MAP_AND_DIRECTORY_POINTER  *Level4Entry;
> > +  PAGE_MAP_AND_DIRECTORY_POINTER  *Level3Entry;
> > +  PAGE_MAP_AND_DIRECTORY_POINTER  *Level2Entry;
> > +  PAGE_TABLE_4K_ENTRY             *Level1Entry;
> > +  SEC_SEV_ES_WORK_AREA            *SevEsWorkArea;
> > +  PHYSICAL_ADDRESS                Cr3;
> > +  UINT64                          ApicAddress;
> > +  UINT64                          PgTableMask;
> > +  UINT32                          Level1Page;
> > +  UINT64                          Level1Address;
> > +  UINT64                          Level1Flags;
> > +  UINTN                           PteIndex;
> > +
> > +  if (!SevEsIsEnabled ()) {
> > +    return;
> > +  }
> 
> That is incompatible with 5-level paging.  The current reset vector will
> never turn on 5-level paging in case SEV is active because we have more
> incompatibilities elsewhere (BaseMemEncryptSevLib IIRC).  But still,
> it's moving things into the wrong direction ...

Tom had mentioned this eventuality and we discussed it to an extent. AIUI
once we make that switch then most of this function could be replaced with
a call into the library to handle the splitting, and similar re-work would
need to be done for handling splitting the area for the GHCB page which is
also currently done with direct page table manipulation. So while it
does sort of move in the wrong direction, I don't think it would
significantly complicate things as far as making that transition.

> 
> Ideally CpuPageTableLib should be used for this.

What's the outlook for moving CpuPageTableLib before the next OVMF release?
My concern is that once SNP KVM support goes upstream (which is currently
looking to be within kernel 6.10 timeframe), SNP guest support in OVMF will
be completely broken without a fix like this for APIC MMIO accesses.

One thing to maybe get ahead of is the fact that splitting pages with
5-level paging will require having 2 pages reserved for GHCB instead of
the 1 we have currently, and 2 pages reserved for APIC range instead of
the 1 proposed by this patch (since we'd need to not only split a 2MB PTE
to 4KB, but the upper 1GB PTE to 2MB).

Do we know enough about what that sort of allocation/reserve logic would
look to start modifying PcdOvmfSecPageTablesBase,
PcdOvmfSecGhcbPageTableBase, and PcdOvmfSecApicPageTableBase to start
preping for such a change?

If so we could maybe take steps toward that to ease the transition. But
either way if the move to CpuPageTableLib is a ways out then I think we
need a fix before then.

-Mike

> 
> take care,
>   Gerd
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118217): https://edk2.groups.io/g/devel/message/118217
Mute This Topic: https://groups.io/mt/105698125/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-