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

[Igor Mammedov]

On Mon, 14 Feb 2022 15:18:43 +0000
Joao Martins <joao.m.martins@oracle.com> wrote:

> On 2/14/22 15:03, Igor Mammedov wrote:
> > 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. 
> >   
> Sounds better to be conservative here.
> 
> I will change from warn_report() to error_report()
> and exit.
> 
> >>
> >> 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?
> >   
> It is HT fixup related. Because we are relocating the whole above-4g-ram,
> on what used to be enough with just 40 phys bits (default).

what if user starts QEMU with amount of RAM that won't fit into
configured phys bits (whether it's default one or one that comes from host)
and not on AMD host at that so no relocation happens but we still have
broken configuration. What matters here is the end address that might
be used by guest (pci64_bit hole end) is reachable.

That's why I suggested to make it generic check instead of AMD specific one. 
 
> > 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  
> 
> When booted with +host-phys-bits and/or with a host-phys-bits-limit=X, the 
> @phys_bits
> value will be either set to host, and ultimately bound to a maximum of
> host_phys_bits_limit (if at all set).
> 
> So essentially the selected phys_bits that we're checking above is the only 
> thing
> we need to care about IIUC.
>