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