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