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

[Brijesh Singh <brijesh.singh@amd.com>]

Hi Andrew,

The goal is to clear the "C" bit in PTE for all the MMIO areas in the GCD memory
space map. I think Leo looked at SetMemoryAttributes() based on Mike's feedback,
but I believe SetMemoryAttribute may get called on any range without specifying
types (we are interested in MMIO ranges, which are specified in ADD/REMOVE operations).

-Brijesh

On 06/01/2017 08:48 AM, Andrew Fish wrote:
> 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 <mailto: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 <mailto:edk2-devel@lists.01.org>
>> https://lists.01.org/mailman/listinfo/edk2-devel
>