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 <[email protected]> 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
