[SeaBIOS] Re: [PATCH v2 2/6] detect physical address space size

[Kevin O'Connor]

On Wed, May 03, 2023 at 11:20:54AM +0200, Gerd Hoffmann wrote:
> Check for pae and long mode using cpuid.  If present also read the
> physical address bits.  Apply some qemu sanity checks (see below).
> Record results in PhysBits and LongMode variables.  In case we are not
> sure what the address space size is leave the PhysBits variable unset.
>
> On qemu we have the problem that for historical reasons x86_64
> processors advertise 40 physical address space bits by default, even in
> case the host supports less than that so actually using the whole
> address space will not work.
>
> Because of that the code applies some extra sanity checks in case we
> find 40 (or less) physical address space bits advertised.  Only
> known-good values (which is 40 for amd processors and 36+39 for intel
> processors) will be accepted as valid.
>
> Recommendation is to use 'qemu -cpu ${name},host-phys-bits=on' to
> advertise valid physical address space bits to the guest.  Some distro
> builds enable this by default, and most likely the qemu default will
> change in near future too.
> Signed-off-by: Gerd Hoffmann
>
> src/fw/paravirt.h |  2 ++
>  src/fw/paravirt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
>
> diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
> index 4e2e993ba9d3..7ca3fdcc06d4 100644
> --- a/src/fw/paravirt.h
> +++ b/src/fw/paravirt.h
> @@ -31,6 +31,8 @@ typedef struct QemuCfgDmaAccess {
>  extern u32 RamSize;
>  extern u64 RamSizeOver4G;
>  extern int PlatformRunningOn;
> +extern u8 PhysBits;
> +extern int LongMode;
>
> static inline int runningOnQEMU(void) {
>      return CONFIG_QEMU || (
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index c880cb10a1bc..a84968661aee 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -32,6 +32,10 @@
>  u32 RamSize;
>  // Amount of continuous ram >4Gig
>  u64 RamSizeOver4G;
> +// physical address space bits
> +u8 PhysBits;
> +// 64bit processor
> +int LongMode;

Odd that LongMode is an "int" while PhysBits is a u8..  Would probably
be simpler to do: LongMode = !!(edx & (1 << 29))  - and then treat it as
a flag variable.

Since these variables are exported, it might also be worthwhile to
give them more descriptive names - for example CPUPhysBits and
CPULongMode.

Separately, series looks fine to me.
-Kevin
> // Type of emulator platform.
>  int PlatformRunningOn VARFSEG;
>  // cfg enabled
> @@ -129,6 +133,58 @@ static void kvmclock_init(void)
>      tsctimer_setfreq(MHz * 1000, "kvmclock");
>  }
>
> +static void physbits(int qemu_quirk)
> +{
> unsigned int max, eax, ebx, ecx, edx;
> unsigned int physbits;
> char signature[13];
> int pae = 0, valid = 0;
> cpuid(0, &eax, &ebx, &ecx, &edx);
> memcpy(signature + 0, &ebx, 4);
> memcpy(signature + 4, &edx, 4);
> memcpy(signature + 8, &ecx, 4);
> signature[12] = 0;
> if (eax >= 1) {
>    cpuid(1, &eax, &ebx, &ecx, &edx);
>    pae = (edx & (1 << 6));
> }
> cpuid(0x80000000, &eax, &ebx, &ecx, &edx);
> max = eax;
> if (max >= 0x80000001) {
>    cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
>    LongMode = (edx & (1 << 29));
> }
> if (pae && LongMode && max >= 0x80000008) {
>    cpuid(0x80000008, &eax, &ebx, &ecx, &edx);
>    physbits = (u8)eax;
>    if (!qemu_quirk) {
>        valid = 1;
>    } else if (physbits >= 41) {
>        valid = 1;
>    } else if (strcmp(signature, "GenuineIntel") == 0) {
>        if ((physbits == 36) || (physbits == 39))
>            valid = 1;
>    } else if (strcmp(signature, "AuthenticAMD") == 0) {
>        if (physbits == 40)
>            valid = 1;
>    }
> } else {
>    physbits = pae ? 36 : 32;
>    valid = 1;
> }
> dprintf(1, "%s: signature="%s", pae=%s, lm=%s, phys-bits=%d, valid=%s\n",
>        __func__, signature, pae ? "yes" : "no", LongMode ? "yes" : "no",
>        physbits, valid ? "yes" : "no");
> if (valid)
>    PhysBits = physbits;
>
> +}
>
> static void qemu_detect(void)
>  {
>      if (!CONFIG_QEMU_HARDWARE)
> @@ -161,6 +217,7 @@ static void qemu_detect(void)
>          dprintf(1, "Running on QEMU (unknown nb: %04x:%04x)\n", v, d);
>          break;
>      }
> physbits(1);
>
> }
> static int qemu_early_e820(void);
>
> 2.40.1