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