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 <m...@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 <da...@redhat.com>
> > > Signed-off-by: Ani Sinha <anisi...@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
> >
