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

[Kevin O'Connor <kevin@koconnor.net>]

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 <kraxel@redhat.com>
> ---
>  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
> 
>