Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low

From: Igor Mammedov <imammedo@redhat.com>
To: Joao Martins <joao.m.martins@oracle.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S . Tsirkin" <mst@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, Daniel Jordan <daniel.m.jordan@oracle.com>,
	David Edmondson <david.edmondson@oracle.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Ani Sinha <ani@anisinha.ca>, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
Date: Mon, 14 Feb 2022 16:03:25 +0100	[thread overview]
Message-ID: <20220214160325.62b6436d@redhat.com> (raw)
In-Reply-To: <20220207202422.31582-4-joao.m.martins@oracle.com>

On Mon,  7 Feb 2022 20:24:21 +0000
Joao Martins <joao.m.martins@oracle.com> wrote:

> Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough
> to address 1Tb (0xff ffff ffff). On AMD platforms, if a
> ram-above-4g relocation happens and the CPU wasn't configured
> with a big enough phys-bits, warn the user. There isn't a
> catastrophic failure exactly, the guest will still boot, but
> most likely won't be able to use more than ~4G of RAM.

how 'unable to use" would manifest?
It might be better to prevent QEMU startup with broken setup (CLI)
rather then letting guest run and trying to figure out what's
going wrong when users start to complain. 

> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  hw/i386/pc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b060aedd38f3..f8712eb8427e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -842,6 +842,7 @@ static void relocate_4g(MachineState *machine, PCMachineState *pcms)
>      X86MachineState *x86ms = X86_MACHINE(pcms);
>      ram_addr_t device_mem_size = 0;
>      uint32_t eax, vendor[3];
> +    hwaddr maxphysaddr;
>  
>      host_cpuid(0x0, 0, &eax, &vendor[0], &vendor[2], &vendor[1]);
>      if (!IS_AMD_VENDOR(vendor)) {
> @@ -858,6 +859,12 @@ static void relocate_4g(MachineState *machine, PCMachineState *pcms)
>          return;
>      }
>  
> +    maxphysaddr = ((hwaddr)1 << X86_CPU(first_cpu)->phys_bits) - 1;
> +    if (maxphysaddr < AMD_ABOVE_1TB_START)
> +        warn_report("Relocated RAM above 4G to start at %lu "
> +                    "phys-bits too low (%u)",
> +                    AMD_ABOVE_1TB_START, X86_CPU(first_cpu)->phys_bits);

perhaps this hunk belongs to the end of pc_memory_init(),
it's not HT fixup specific at all?

Also I'm not sure but there are host_phys_bits/host_phys_bits_limit properties,
perhaps they need to be checked/verified as well

>      x86ms->above_4g_mem_start = AMD_ABOVE_1TB_START;
>  }
>  