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

[Michael S. Tsirkin]

On Mon, Sep 18, 2023 at 07:40:45PM +0530, Ani Sinha wrote:
> 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?)

migration mostly.

> >
> > > ---
> > >  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
> >