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

["Lendacky, Thomas via groups.io" <thomas.lendacky=amd.com@groups.io>]

On 4/24/24 06:54, 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?

InitializedApicTimer() in OvmfPkg/Sec/SecMain.c

> 
>> +/**
>> +  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 ...

Agreed. SEV needs to clean up the pagetable manipulation in general in 
order to support 5-level paging and remove redundant code. That will be 
a patch series in itself.

But without this modification, the SNP support no longer works with the 
KVM/gmem support that will be upstream. This change gets OVMF SNP 
support working again.

> 
> Ideally CpuPageTableLib should be used for this.

CpuPageTableLib will need to be modified in order for it to be used at 
this (Sec) stage. In order to work in Sec - either the caller will have 
to supply a list of pages that can be used if pagetable entries need to 
be allocated for splits or new entries or by providing some kind of SEC 
pagetable allocation pool.

So it will take significant work to get SEV support updated to using 
CpuPageTableLib and that's why with this single patch we can get OVMF 
SNP support working again.

Thanks,
Tom

> 
> take care,
>    Gerd
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118215): https://edk2.groups.io/g/devel/message/118215
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]
-=-=-=-=-=-=-=-=-=-=-=-