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

[Andrew Fish <afish@apple.com>]

Laszlo,

The current design is DXE IPL and gEfiCpuArchProtocolGuid abstract the CPU specifics from the DXE Core. 

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Gcd/Gcd.c#L866
  if (Operation == GCD_SET_ATTRIBUTES_MEMORY_OPERATION) {
    //
    // Call CPU Arch Protocol to attempt to set attributes on the range
    //
    CpuArchAttributes = ConverToCpuArchAttributes (Attributes);
    if (CpuArchAttributes != INVALID_CPU_ARCH_ATTRIBUTES) {
      if (gCpu == NULL) {
        Status = EFI_NOT_AVAILABLE_YET;
      } else {
        Status = gCpu->SetMemoryAttributes (
                         gCpu,
                         BaseAddress,
                         Length,
                         CpuArchAttributes
                         );
      }
      if (EFI_ERROR (Status)) {
        CoreFreePool (TopEntry);
        CoreFreePool (BottomEntry);
        goto Done;
      }
    }
  }

Maybe the issue is there is an attempt to change attributes too early and they currently get sent to the bit bucket? I guess they could get queued up and replayed after gEfiCpuArchProtocolGuid is preset?

Thanks,

Andrew Fish


> On Jun 1, 2017, at 2:10 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 06/01/17 09:40, Jordan Justen wrote:
>> On 2017-05-29 14:59:46, Brijesh Singh wrote:
>>> 
>>> 
>>> On 5/29/17 3:38 PM, Jordan Justen wrote:
>>>> On 2017-05-29 04:16:15, Laszlo Ersek wrote:
>>>>> (looks like I was the one to comment as second reviewer after all :) )
>>>>> 
>>>>> On 05/26/17 23:05, Jordan Justen wrote:
>>>>>> On 2017-05-26 07:43:48, Brijesh Singh wrote:
>>>>>>> Changes since v4:
>>>>>>> - decouple IoMmu protocol implementation from AmdSevDxe into a seperate
>>>>>>>   IoMmuDxe driver. And introduce a placeholder protocol to provide the
>>>>>>>   dependency support for the dependent modules.
>>>>>> I think you split IoMmuDxe out from AmdSevDxe based on my feedback
>>>>>> regarding APRIORI, but I don't think this helped.
>>>>>> 
>>>>>> Ideally I would like to see one driver named IoMmuDxe that is *not* in
>>>>>> APRIORI.
>>>>> There are two separate goals here:
>>>>> 
>>>>> (1) Make sure that any driver that adds MMIO ranges will automatically
>>>>> add those ranges with the C bit cleared in the PTEs, without actually
>>>>> knowing about SEV.
>>>> Ok, this sounds reasonable.
>>>> 
>>>> The APRIORI method looks like a hack. Why is this not being handled at
>>>> the time the page tables are being built, in DxeIpl? Couldn't we
>>>> define a platform Page Tables library to allow a platform to somehow
>>>> modify the page tables as they are built? Or, maybe just after? This
>>>> would also make sure it happens before DXE runs.
>>> 
>>> Before introducing  AmdSevDxe driver, we did proposed patches to clear
>>> the C-bit during the page table creation time. In the first patch [1],
>>> Leo tried to  teach gcd.c to clear the C-bit from MMIO. IIRC, the main
>>> concern was -- typically Dxecore does not do any CPU specific thing
>>> hence we should try to find some alternative approach.
>> 
>> DxeCore doesn't build the page tables. DxeIpl builds them. I agree
>> that DxeCore is not the right place to handle this. In
>> https://lists.01.org/pipermail/edk2-devel/2017-March/008987.html
>> Jiewen suggested that DxeIpl could be updated during page table
>> creation time.
>> 
>> 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?
> 
> 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