Re: [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD)

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-06-05 14:56:04, Brijesh Singh wrote:
> On 06/01/2017 04:10 AM, Laszlo Ersek wrote:
> > On 06/01/17 09:40, Jordan Justen wrote:
> >> In https://lists.01.org/pipermail/edk2-devel/2017-April/009883.html
> >> Leo said that DxeIpl won't work because new I/O ranges might be added.
> >> I don't understand this, because isn't DxeIpl and an early APRIORI
> >> entry are roughly equivalent in the boot sequence?
> > 
> > I think you are right. I believe a patch for this exact idea hasn't been
> > posted yet. Jiewen's message that you linked above contains the expression
> > 
> >      always clear SEV mask for MMIO *and all rest*
> > 
> > (emphasis mine), which I think we may have missed *in combination with*
> > the DxeIpl.
> > 
> > So the idea would be to iterate over all the HOBs in the DxeIpl PEIM.
> > Keep the C bit set for system memory regions. Clear the C bit for MMIO
> > regions that are known from the HOB list. Also clear the C bit
> > everywhere else in the address space (known from the CPU HOB) where no
> > coverage is provided by any memory resource descriptor HOB.
> > 
> > This is going to be harder than the current approach, because:
> > 
> > - The current approach can work off of the GCD memory space map, which
> > provides explicit NonExistent entries, covering the entire address space
> > (according to the CPU HOB).
> > 
> > - However, the DxeIpl method would take place before entering DXE, so no
> > GCD memory space map would be available -- the "NonExistent" entries
> > would have to be synthesized manually from the address space size (known
> > from the CPU HOB) and the lack of coverage by memory resource descriptor
> > HOBs.
> > 
> > Basically, in order to move the current GCD memory space map traversal
> > from early DXE to late PEI, the memory space map building logic of the
> > DXE Core would have to be duplicated in the DxeIpl PEIM. If I understand
> > correctly. (The DxeIpl PEIM may already contain very similar code, for
> > the page table building, which might not be difficult to extend like
> > this -- I haven't looked.)
> > 
> > Is this what you have in mind?
> > 
> 
> Do you have any further thought on this?

Regarding Laszlo's feedback, I'm not convinced that it would be
excessively difficult to accomplish this in DxeIpl. (I'm not saying
that I couldn't be convinced. :)

As far as I can see, this is an architecturally defined AMD feature.
(Is this true, or is BaseMemcryptSevLib actually OVMF specific?)

You've asserted that it should work (SEV would not be detected) with
any Intel processor as well. Therefore, I don't see a good reason that
we shouldn't be able to support it in modules that already have
IA32/X64 specific code. (I'm recalling
881813d7a93d9009c873515b043c41c4554779e4.)

Since DxeIpl builds the IA32/X64 page tables, and you need to modify
the page tables for this feature (correct?), I think we should try to
support the feature there if it is feasible. I can understand the
argument that this doesn't apply to all non-VM platforms, so I think
we could add a PCD which disables this support by default.

I don't know that the owners of MdeModulePkg and UefiCpuPkg will agree
with me though.

> In meantime, I have been looking into MdeModule/Core/Dxe/DxeMain to see
> if I can invoke a platform dependent library to clear C-bit before DxeMain
> finishes its execution. As Laszlo pointed, current approach is using GCD memory
> space map to get MMIO and NonExistent entries. I have pushed two patches
> in my development branch to show what I have been doing:
> 
> 1) add a new null DxeGcdCorePlatformHookLib
> 
> https://github.com/codomania/edk2/commit/171f816376b3b0677cbfb90271a94a920d7ad72d
> 
> The library provides a function "DxeGcdCorePlatformHookReady" which can be called
> by DxeMain just after it initializes the GcdServices (which will guarantee that
> Gcd memory space map is available).

Regarding hooking into DxeCore, I don't think it is the best approach,
but it is better than APRIORI. I wonder if the MdeModulePkg owners
could jump in with an opinion. (Hopefully besides just pushing the
problem away via APRIORI.)

-Jordan

> 2) override DxeGcdCorePlatformHookLib inside the Ovmf to clear the C-bit when
>   SEV is detected.
> 
> https://github.com/codomania/edk2/commit/914ce904ca1b7647c966562596ba53c95949f659
> 
> I've tested the approach and it seems to work. Is this something aligned with your
> thinking?
> 
> 
> > Thanks
> > Laszlo
> > 
> >> -Jordan
> >>
> >>> In second patch
> >>> [2], Leo tried to introduce a new notify protocol to get MMIO add/remove
> >>> events. During discussion Jiewen suggested to look into adding a new
> >>> platform driver into APRIORI to avoid the need for any modifications
> >>> inside the Gcdcore - this seems workable solution which did not require
> >>> adding any CPU specific code inside the Gcd.
> >>>
> >>> [1] https://lists.01.org/pipermail/edk2-devel/2017-March/008974.html
> >>> [2] https://lists.01.org/pipermail/edk2-devel/2017-April/009852.html
> >>>
> > 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel