On 02/18/16 11:20, Laszlo Ersek wrote: > On 02/18/16 11:16, Laszlo Ersek wrote: >> On 02/18/16 10:13, Ard Biesheuvel wrote: >>> The legacy 32-bit SMBIOS entry point has little use on AARCH64 systems, >>> since many such systems have no 32-bit addressable physical RAM, and so >>> OSes that implement SMBIOS will have to be able to deal with the 64-bit >>> entry point anyway. So don't expose the 32-bit entry point at all, and >>> save some memory below 4 GB. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel <[email protected]> >>> --- >>> >>> Note to Laszlo: >>> For some background, this is not just about saving a couple of KB of memory, >>> it is about >>> a) saving memory below 4 GB for 32-bit DMA, this is not currently an issue >>> under >>> virt but it may be with direct device assignment and no IOMMU, >> >> Device assignment without an IOMMU should never be done. >> >>> but in the >>> general case, this is one reason we should standardize on the 64-bit >>> versions >>> of these structures >>> b) the OS will map main memory in 1 GB chunks if it can, and punching a 1 >>> page >>> hole (e.g., for SMBIOS data) into it will result in the whole page being >>> mapped using 2 MB and 4 KB blocks instead, and under virt, each *level* >>> of >>> lookup at stage 1 (the guest virtual page table) will result in a full >>> page >>> table walk at level 2 (the guest PA to host PA mapping), so being able >>> to map >>> RAM as efficiently as possible is important. >> >> I think reason (b) is important; please add (b) to the commit message. >> >> Also, there's a typo in it I think: instead of >> >> ... will result in the whole page being mapped using 2 MB ... >> >> it should say >> >> ... will result in the 1GB chunk being mapped using 2 MB ... >> >>> >>> ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc >>> index 7c6419fbb92f..5a77cb239076 100644 >>> --- a/ArmVirtPkg/ArmVirtQemu.dsc >>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc >>> @@ -224,6 +224,14 @@ [PcdsDynamicDefault.common] >>> gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0 >>> gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE >>> >>> +[PcdsFixedAtBuild.AARCH64] >>> + # QEMU/mach-virt provides at least version 3.0 SMBIOS entry point and >>> tables, >> >> Okay, so this aspect was something that I tried to validate extensively, >> before reading your comments here. :) Because, I thought it would be >> brittle to rely on this. >> >> We set the SMBIOS entry point version in >> "OvmfPkg/Library/SmbiosVersionLib". Theoretically, on the aarch64 target >> QEMU could export a 2.x entry point. (It would be a QEMU bug of course.) >> For some reason I felt that we shouldn't depend on QEMU being correct here. >> >> Turns out we don't actually depend on it. Namely, clearing BIT0 (and >> BIT0 *only*) in PcdSmbiosEntryPointProvideMethod has an effect only if >> the entry point version is >= 3.0 (see >> "MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c".) >> >> If the entry point version is < 3.0, for whatever reason, then clearing >> BIT0 in the PCD has no effect. >> >> ... I realize that in practice, "entry point version < 3.0" and >> "[PcdsFixedAtBuild.AARCH64]" never happen at the same time (because QEMU >> is not buggy), but I still feel that it's important that >> MdeModulePkg/Universal/SmbiosDxe will ignore BIT0, if the entry point is >> < 3.0 -- this makes our SmbiosVersionLib more robust. >> >> Initially I worried that by clearing BIT0, < 3.0 entry points could be >> broken *in general*. With that in mind, I wanted to ask you to add some >> ASSERT()s to SmbiosVersionLib -- when the entry point exported by QEMU >> is < 3.0, then BIT0 should be set --, but after all this is not >> necessary, due to SmbiosDxe giving first priority to the entry point >> version. >> >> Ultimately, my point is, please drop the above remark from the comment. >> We should instead say, >> >> Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry >> point, if the entry point version is >= 3.0. >> >>> + # and AARCH64 OSes cannot assume the presence of the 32-bit entry point >>> + # anyway (because many AARCH64 systems don't have 32-bit addressable >>> physical >>> + # RAM). Since the additional allocations below 4 GB needlessly fragment >>> the >>> + # memory map, expose the 64-bit entry point only. >> >> and here I suggest, for the end: >> >> ... expose the 64-bit entry point only, for entry point versions >> >= 3.0. >> >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosEntryPointProvideMethod|2 >> >> Because this is a bitmap, I propose 0x2. >> >>> + >>> >>> ################################################################################ >>> # >>> # Components Section - list of all EDK II Modules needed by this Platform >>> >> >> Summary: >> (1) please fix the "whole page" --> "1GB chunk" typo in note (b) >> (2) please merge note (b) into the commit message >> (3) drop the QEMU/mach-virt reference from the code comment, instead >> state what effect clearing BIT0 has, and only when >> (4) again restrict the last sentence with "for EP vers >= 3.0". > > (5) Please reflect the same in the subject of the patch -- sth like > > ArmVirtPkg: ArmVirtQemu: expose only 64-bit entry point for v3.0+ SMBIOS > > (72 characters.)
I apologize for responding in several emails. :( (6) In the "ArmVirtPkg/ArmVirtQemu.dsc" file, a section called [PcdsFixedAtBuild.AARCH64] already exists; please add this setting to it. Thanks! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
