[edk2-devel] [PATCH V3 29/29] OvmfPkg: Update IoMmuDxe to support TDX

[Gerd Hoffmann]

On Mon, Dec 13, 2021 at 02:39:53AM +0000, Xu, Min M wrote:
> Hi
> > 
> > > +  if (CC_GUEST_IS_SEV (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> > > +    //
> > > +    // Clear the memory encryption mask on the plaintext buffer.
> > > +    //
> > > +    Status = MemEncryptSevClearPageEncMask (
> > > +               0,
> > > +               MapInfo->PlainTextAddress,
> > > +               MapInfo->NumberOfPages
> > > +               );
> > > +  } else if (CC_GUEST_IS_TDX (PcdGet64
> > (PcdConfidentialComputingGuestAttr))) {
> > > +    //
> > > +    // Set the memory shared bit.
> > > +    //
> > > +    Status = MemEncryptTdxSetPageSharedBit (
> > > +               0,
> > > +               MapInfo->PlainTextAddress,
> > > +               MapInfo->NumberOfPages
> > > +               );
> > 
> > Again, this looks very simliar and like a great opportunity to share code.
> > 
> MemEncryptSevClearPageEncMask () is implemented in MemEncryptSevLib.
> MemEncryptTdxSetPageSharedBit () is implemented in MemEncryptTdxlib. 
> 
> Yes, we have considered to merge these 2 EncryptLib into one lib (for
> example: MemoryEncryptCcLib). But after investigation and some PoC, we
> find it will make the code complicated and hard to maintain. (many
> if-else checking in the code)

> 1. From the naming perspective (in SEV/TDX documentation), SEV's bit is Enc bit, but TDX's bit is shared bit.
> 2. In SEV's SetMemoryEncDec () it handles differently for the different version of SEV (for example,  Sev-Snp). I am not sure if there will be more specific process will be added in the future.
> 3. In TDX's SetMemorySharedOrPrivate, currently it is simple and clean. But there maybe some new features added in the future.

> I am thinking if it is a better choice that every vendor takes their responsibility to maintain their own lib/code? 

Well, I still think there is opportunity to share code, specifically the
page table handling.  Have a generic page table walker which is able to
set and clear bits for a given memory range.  Then the sev/tdx specific
code can just call that instead of both having their own, duplicated
page table walking logic.

Maybe the page table walking should even be a MdeModulePkg Library, i.e.
move the code for page table walking (and huge page splitting) in
MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c to a library so it can
be reused elsewhere without duplicating the code,

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84671): https://edk2.groups.io/g/devel/message/84671
Mute This Topic: https://groups.io/mt/86739898/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-