Hello Jian, On 10/23/17 08:50, Jian J Wang wrote: > More than one entry of RT_CODE memory might cause boot problem for some > old OSs. This patch will fix this issue to keep OS compatibility as much > as possible. > > Cc: Eric Dong <[email protected]> > Cc: Jiewen Yao <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <[email protected]> > --- > UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-)
Thank you again for the explanation elsewhere in this thread. I filed the following TianoCore Bugzilla entry about this issue, and assigned it to you: https://bugzilla.tianocore.org/show_bug.cgi?id=753 Can you please read the BZ, and add corrections (in further comments) if you think any such are necessary? I suggest that the BZ reference be included in the commit message. (If there is no v2 necessary for this patch, then Eric can do that as well, when he commits / pushes your patch.) I think the patch is good, but I have one technical question below: > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c > b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index d312eb66f8..0802464b9d 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -829,6 +829,15 @@ RefreshGcdMemoryAttributesFromPaging ( > // Sync real page attributes to GCD > BaseAddress = MemorySpaceMap[Index].BaseAddress; > MemorySpaceLength = MemorySpaceMap[Index].Length; > + Capabilities = MemorySpaceMap[Index].Capabilities | > + EFI_MEMORY_PAGETYPE_MASK; > + Status = gDS->SetMemorySpaceCapabilities ( > + BaseAddress, > + MemorySpaceLength, > + Capabilities > + ); > + ASSERT_EFI_ERROR (Status); > + This logic -- i.e. the addition of the EFI_MEMORY_PAGETYPE_MASK capabilities -- will be applied to *all* GCD memory space map entries that have a type different from "EfiGcdMemoryTypeNonExistent". I wonder if that's a good idea -- for example I don't think it makes much sense for "EfiGcdMemoryTypeMemoryMappedIo". How about the following alternatives: (1) *Either* restrict this capability addition to EfiGcdMemoryTypeSystemMemory (and maybe some other GCD types as well?), (2) *or*, remove this change, and: > while (MemorySpaceLength > 0) { > if (PageLength == 0) { > PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, > &PageAttribute); > @@ -846,7 +855,6 @@ RefreshGcdMemoryAttributesFromPaging ( > if (Attributes != (MemorySpaceMap[Index].Attributes & > EFI_MEMORY_PAGETYPE_MASK)) { > DoUpdate = TRUE; > Attributes |= (MemorySpaceMap[Index].Attributes & > ~EFI_MEMORY_PAGETYPE_MASK); > - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities; > } else { > DoUpdate = FALSE; > } > @@ -854,8 +862,8 @@ RefreshGcdMemoryAttributesFromPaging ( > > Length = MIN (PageLength, MemorySpaceLength); > if (DoUpdate) { > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities); > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes); > + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, > Attributes); > + ASSERT_EFI_ERROR (Status); > DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - > %016lx (%08lx -> %08lx)\r\n", > Index, BaseAddress, BaseAddress + Length - 1, > MemorySpaceMap[Index].Attributes, Attributes)); > keep the SetMemorySpaceCapabilities() call here, but use the following arguments instead: MemorySpaceMap[Index].BaseAddress MemorySpaceMap[Index].Length This will ensure that the capabilities are changed on the *entire* containing GCD memory space entry, and no entry splitting will take place. Yes, it is possible that the SetMemorySpaceCapabilities() function will be invoked multiple times, on the same GCD memory space entry, but that's not a problem, IMO. The Capabilities value (bitmask) should be the exact same. In fact, you could set Capabilities just before the inner loop, and then only *grow* it in the inner loop. Something like this: > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c > b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index d312eb66f87c..5a0eb2900cd5 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -829,6 +829,7 @@ RefreshGcdMemoryAttributesFromPaging ( > // Sync real page attributes to GCD > BaseAddress = MemorySpaceMap[Index].BaseAddress; > MemorySpaceLength = MemorySpaceMap[Index].Length; > + Capabilities = MemorySpaceMap[Index].Capabilities; > while (MemorySpaceLength > 0) { > if (PageLength == 0) { > PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, > &PageAttribute); > @@ -846,7 +847,7 @@ RefreshGcdMemoryAttributesFromPaging ( > if (Attributes != (MemorySpaceMap[Index].Attributes & > EFI_MEMORY_PAGETYPE_MASK)) { > DoUpdate = TRUE; > Attributes |= (MemorySpaceMap[Index].Attributes & > ~EFI_MEMORY_PAGETYPE_MASK); > - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities; > + Capabilities |= Attributes; > } else { > DoUpdate = FALSE; > } > @@ -854,8 +855,20 @@ RefreshGcdMemoryAttributesFromPaging ( > > Length = MIN (PageLength, MemorySpaceLength); > if (DoUpdate) { > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities); > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes); > + Status = gDS->SetMemorySpaceCapabilities ( > + MemorySpaceMap[Index].BaseAddress, > + MemorySpaceMap[Index].Length, > + Capabilities > + ); > + ASSERT_EFI_ERROR (Status); > + > + Status = gDS->SetMemorySpaceAttributes ( > + BaseAddress, > + Length, > + Attributes > + ); > + ASSERT_EFI_ERROR (Status); > + > DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - > %016lx (%08lx -> %08lx)\r\n", > Index, BaseAddress, BaseAddress + Length - 1, > MemorySpaceMap[Index].Attributes, Attributes)); What do you think? Meta comment: can you please CC me on the next version of the patch (if you send one)? Looks like I'm now a Reviewer for UefiCpuPkg :) , I just have to commit the patch for "Maintainers.txt". In addition, if you send a v2, please locate the web link for it in the mailing list archive: https://lists.01.org/pipermail/edk2-devel/2017-October/thread.html and add the link to the Bugzilla, in a new comment -- this way someone who looks at the bugzilla can see all the versions and the discussion threads. Thanks! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
