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

[Min Xu]

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,

Thanks for the suggestion. I will carefully think about it and figure out if it is feasible. (A Poc as the first step )

Thanks
Min


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