Re: [PATCH v2] hw/i386/pc: improve physical address space bound check for 32-bit systems

[Ani Sinha]

On Tue, Sep 19, 2023 at 6:10 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 19.09.23 13:52, Ani Sinha wrote:
> > On Tue, Sep 19, 2023 at 4:08 PM Ani Sinha <anisinha@redhat.com> wrote:
> >>
> >> 32-bit systems do not have a reserved memory for hole64 and memory hotplug 
> >> is
> >> not supported on those systems. Therefore, the maximum limit of the guest
> >> physical address coincides with the end of "above 4G memory space" region.
> >> Make sure that the end of "above 4G memory" is still addressible by the
> >> guest processor with its available address bits. For example, previously 
> >> this
> >> was allowed:
> >>
> >> $ ./qemu-system-x86_64 -cpu pentium -m size=10G
> >>
> >> Now it is no longer allowed:
> >>
> >> $ ./qemu-system-x86_64 -cpu pentium -m size=10G
> >> qemu-system-x86_64: Address space limit 0xffffffff < 0x2bfffffff phys-bits 
> >> too low (32)
> >>
> >> After calling CPUID with EAX=0x80000001, all AMD64 compliant processors
> >> have the longmode-capable-bit turned on in the extended feature flags (bit 
> >> 29)
> >> in EDX. The absence of CPUID longmode can be used to differentiate between
> >> 32-bit and 64-bit processors and is the recommended approach. QEMU takes 
> >> this
> >> approach elsewhere (for example, please see x86_cpu_realizefn()) and with
> >> this change, pc_max_used_gpa() also takes the same approach to detect 
> >> 32-bit
> >> processors.
> >>
> >> Finally, a new compatibility flag is introduced to retain the old behavior
> >> for pc_max_used_gpa() for macines 8.1 and older.
> > typo - will fix in v3                   ^^^^^^^^
> >
> > btw, does this patch break it for processors < 32-bit? For them clearly
> >
> > x86ms->above_4g_mem_start = 4 * GiB;
> >
> > does not work.
> >
> >
> >>
> >> Suggested-by: David Hildenbrand <david@redhat.com>
> >> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >> ---
> >>   hw/i386/pc.c         | 17 ++++++++++++++---
> >>   hw/i386/pc_piix.c    |  4 ++++
> >>   include/hw/i386/pc.h |  3 +++
> >>   3 files changed, 21 insertions(+), 3 deletions(-)
> >>
> >> changelog:
> >> v2: removed memory hotplug region from max_gpa. added compat knobs.
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 54838c0c41..fea97ee258 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -907,10 +907,20 @@ static uint64_t pc_get_cxl_range_end(PCMachineState 
> >> *pcms)
> >>   static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t 
> >> pci_hole64_size)
> >>   {
> >>       X86CPU *cpu = X86_CPU(first_cpu);
> >> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >>
> >> -    /* 32-bit systems don't have hole64 thus return max CPU address */
> >> -    if (cpu->phys_bits <= 32) {
> >> -        return ((hwaddr)1 << cpu->phys_bits) - 1;
> >> +    /*
> >> +     * 32-bit systems don't have hole64 and does not support
> >> +     * memory hotplug.
> >> +     */
> >> +    if (pcmc->fixed_32bit_mem_addr_check) {
> >> +        if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) {
> >> +            return pc_above_4g_end(pcms) - 1;
> >> +        }
>
> I think you should use the logic from v1.
>
> My comment regarding memory hotplug was primarily about (Linux) guest
> support.
>
> We should not optimize for 32bit processors (e.g., try placing the
> device memory region below 4g), but you can still consider the region
> and properly account it.

I am confused. So maybe you can send a patch.