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

["Gerd Hoffmann" <kraxel@redhat.com>]

On Fri, Apr 26, 2024 at 02:50:07PM GMT, Roth, Michael via groups.io 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. In the case of SEV-SNP, this requires special
> handling on the hypervisor side which may not be available in the
> future[1], so make the necessary changes in the SEC-configured page
> table to clear the encryption bit for 4K region containing the APIC
> base address.
> 
> Since CpuPageTableLib is used to handle the splitting, some additional
> care must be taken to clear the C-bit in all non-leaf PTEs since the
> library expects that to be the case. Add handling for that when setting
> up the SEC page table.
> 
> While here, drop special handling for the APIC base address in the
> SEV-ES/SNP #VC handler.

Please split this into smaller patches (one for each commit message
paragraph).

> -0x011000|0x00F000
> +0x011000|0x001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableSize

Hmm, I think we need to figure how we'll organize page table allocation
best.  This becomes a bit messy, even more so once we enable 5-level
paging for sev because one page isn't enough then ...

Is it possible to move the GHCB page setup to SecCoreStartupWithStack()
too?  If so it would be nice to setup both GHCB and APIC page in C code,
and it should also allow to cleanup the page table memory reservation
and simplify the reset vector.

Note: I'm fine with this as-is for the next stable tag, the comments are
more for the next patch series, when enabling 5-level paging for sev,
where this must be touched again.

> +  ApicAddress = (UINT64)GetLocalApicBaseAddress ();
> +  Buffer      = (VOID *)(UINTN)FixedPcdGet32 (PcdOvmfSecApicPageTableBase);
> +  Cr3         = AsmReadCr3 ();
> +
> +  MapAttribute.Uint64         = ApicAddress;
> +  MapAttribute.Bits.Present   = 1;
> +  MapAttribute.Bits.ReadWrite = 1;
> +  MapMask.Uint64              = MAX_UINT64;
> +  BufferSize                  = SIZE_4KB;
> +
> +  Status = PageTableMap (
> +             (UINTN *)&Cr3,
> +             Paging4Level,
> +             Buffer,
> +             &BufferSize,
> +             ApicAddress,
> +             SIZE_4KB,
> +             &MapAttribute,
> +             &MapMask,
> +             NULL
> +             );

Very nice.

take care,
  Gerd



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