Re: [PATCH] hw/i386/pc: fix max_used_gpa for 32-bit systems

[Ani Sinha]

On Mon, Sep 18, 2023 at 7:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Sep 18, 2023 at 07:24:48PM +0530, Ani Sinha wrote:
> > 32-bit systems do not have a reserved memory for hole64 but they may have a
> > reserved memory space for memory hotplug. Since, hole64 starts after the
> > reserved hotplug memory, the unaligned hole64 start address gives us the
> > end address for this memory hotplug region that the processor may use.
> > Fix this. This ensures that the physical address space bound checking works
> > correctly for 32-bit systems as well.
> >
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
>
>
> I doubt we can make changes like this without compat machinery. No?

Is that for not breaking migration or being backward compatible
(something which was broken in the first place used to work but now
its doesnt because we fixed it?)

>
> > ---
> >  hw/i386/pc.c | 60 ++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 35 insertions(+), 25 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 54838c0c41..c8abcabd53 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -904,13 +904,43 @@ static uint64_t pc_get_cxl_range_end(PCMachineState 
> > *pcms)
> >      return start;
> >  }
> >
> > +/*
> > + * The 64bit pci hole starts after "above 4G RAM" and
> > + * potentially the space reserved for memory hotplug.
> > + * This function returns unaligned start address.
> > + */
> > +static uint64_t pc_pci_hole64_start_unaligned(void)
> > +{
> > +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> > +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> > +    MachineState *ms = MACHINE(pcms);
> > +    uint64_t hole64_start = 0;
> > +    ram_addr_t size = 0;
> > +
> > +    if (pcms->cxl_devices_state.is_enabled) {
> > +        hole64_start = pc_get_cxl_range_end(pcms);
> > +    } else if (pcmc->has_reserved_memory && (ms->ram_size < 
> > ms->maxram_size)) {
> > +        pc_get_device_memory_range(pcms, &hole64_start, &size);
> > +        if (!pcmc->broken_reserved_end) {
> > +            hole64_start += size;
> > +        }
> > +    } else {
> > +        hole64_start = pc_above_4g_end(pcms);
> > +    }
> > +
> > +    return hole64_start;
> > +}
> > +
> >  static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t 
> > pci_hole64_size)
> >  {
> >      X86CPU *cpu = X86_CPU(first_cpu);
> >
> > -    /* 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, but we might have a region for
> > +     * memory hotplug.
> > +     */
> > +    if (!(cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM)) {
> > +        return pc_pci_hole64_start_unaligned() - 1;
> >      }
> >
>
> I see you are changing cpu->phys_bits to a CPUID check.
> Could you explain why in the commit log?

Yeah missed that but will do in v2.

>
> >      return pc_pci_hole64_start() + pci_hole64_size - 1;
> > @@ -1147,30 +1177,10 @@ void pc_memory_init(PCMachineState *pcms,
> >      pcms->memhp_io_base = ACPI_MEMORY_HOTPLUG_BASE;
> >  }
> >
> > -/*
> > - * The 64bit pci hole starts after "above 4G RAM" and
> > - * potentially the space reserved for memory hotplug.
> > - */
> > +/* returns 1 GiB aligned hole64 start address */
> >  uint64_t pc_pci_hole64_start(void)
> >  {
> > -    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> > -    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> > -    MachineState *ms = MACHINE(pcms);
> > -    uint64_t hole64_start = 0;
> > -    ram_addr_t size = 0;
> > -
> > -    if (pcms->cxl_devices_state.is_enabled) {
> > -        hole64_start = pc_get_cxl_range_end(pcms);
> > -    } else if (pcmc->has_reserved_memory && (ms->ram_size < 
> > ms->maxram_size)) {
> > -        pc_get_device_memory_range(pcms, &hole64_start, &size);
> > -        if (!pcmc->broken_reserved_end) {
> > -            hole64_start += size;
> > -        }
> > -    } else {
> > -        hole64_start = pc_above_4g_end(pcms);
> > -    }
> > -
> > -    return ROUND_UP(hole64_start, 1 * GiB);
> > +    return ROUND_UP(pc_pci_hole64_start_unaligned(), 1 * GiB);
> >  }
> >
> >  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus)
> > --
> > 2.39.1
>