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

On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.

This patch breaks some unit tests. I am not sure why it did not catch
it when I tested it before sending.
Will have to resend after fixing the tests.

>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  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;
>      }
>
>      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
>

On 18.09.23 17:22, Ani Sinha wrote:
> On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.
> 
> This patch breaks some unit tests. I am not sure why it did not catch
> it when I tested it before sending.
> Will have to resend after fixing the tests.

Probably because they supply more memory than the system can actually 
handle? (e.g., -m 4g on 32bit)?

Agreed with MST that we should glue this to compat machines.

-- 
Cheers,

David / dhildenb

On Mon, Sep 18, 2023 at 8:59 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 18.09.23 17:22, Ani Sinha wrote:
> > On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.
> >
> > This patch breaks some unit tests. I am not sure why it did not catch
> > it when I tested it before sending.
> > Will have to resend after fixing the tests.
>
> Probably because they supply more memory than the system can actually
> handle? (e.g., -m 4g on 32bit)?

cxl tests are failing for example.

$ ./qemu-system-i386 -display none -machine q35,cxl=on
qemu-system-i386: Address space limit 0xffffffff < 0x1000fffff
phys-bits too low (32)



>
> Agreed with MST that we should glue this to compat machines.
>
> --
> Cheers,
>
> David / dhildenb
>

On 18.09.23 17:56, Ani Sinha wrote:
> On Mon, Sep 18, 2023 at 8:59 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 18.09.23 17:22, Ani Sinha wrote:
>>> On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.
>>>
>>> This patch breaks some unit tests. I am not sure why it did not catch
>>> it when I tested it before sending.
>>> Will have to resend after fixing the tests.
>>
>> Probably because they supply more memory than the system can actually
>> handle? (e.g., -m 4g on 32bit)?
> 
> cxl tests are failing for example.
> 
> $ ./qemu-system-i386 -display none -machine q35,cxl=on
> qemu-system-i386: Address space limit 0xffffffff < 0x1000fffff
> phys-bits too low (32)

CXL with 32bit CPUs ... it might be reasonably to just disable such 
tests. Certainly does not exist in real HW ... :)

-- 
Cheers,

David / dhildenb

On Mon, Sep 18, 2023 at 9:28 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 18.09.23 17:56, Ani Sinha wrote:
> > On Mon, Sep 18, 2023 at 8:59 PM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 18.09.23 17:22, Ani Sinha wrote:
> >>> On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.
> >>>
> >>> This patch breaks some unit tests. I am not sure why it did not catch
> >>> it when I tested it before sending.
> >>> Will have to resend after fixing the tests.
> >>
> >> Probably because they supply more memory than the system can actually
> >> handle? (e.g., -m 4g on 32bit)?
> >
> > cxl tests are failing for example.
> >
> > $ ./qemu-system-i386 -display none -machine q35,cxl=on
> > qemu-system-i386: Address space limit 0xffffffff < 0x1000fffff
> > phys-bits too low (32)

also another thing is:

./qemu-system-i386 -machine pc -m 128
works but ...

$ ./qemu-system-i386 -machine pc -m 128,slots=3,maxmem=1G
qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
phys-bits too low (32)

or

$ ./qemu-system-i386 -machine pc-i440fx-8.2 -accel kvm -m 128,slots=3,maxmem=1G
qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
phys-bits too low (32)

but of course after the compat knob older pc machines work fine using
the old logic :

$ ./qemu-system-i386 -machine pc-i440fx-8.1 -accel kvm -m 128,slots=3,maxmem=1G
VNC server running on ::1:5900
^Cqemu-system-i386: terminating on signal 2




>
> CXL with 32bit CPUs ... it might be reasonably to just disable such
> tests. Certainly does not exist in real HW ... :)
>
> --
> Cheers,
>
> David / dhildenb
>

On Tue, Sep 19, 2023 at 9:20 AM Ani Sinha <anisinha@redhat.com> wrote:
>
> On Mon, Sep 18, 2023 at 9:28 PM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 18.09.23 17:56, Ani Sinha wrote:
> > > On Mon, Sep 18, 2023 at 8:59 PM David Hildenbrand <david@redhat.com> wrote:
> > >>
> > >> On 18.09.23 17:22, Ani Sinha wrote:
> > >>> On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.
> > >>>
> > >>> This patch breaks some unit tests. I am not sure why it did not catch
> > >>> it when I tested it before sending.
> > >>> Will have to resend after fixing the tests.
> > >>
> > >> Probably because they supply more memory than the system can actually
> > >> handle? (e.g., -m 4g on 32bit)?
> > >
> > > cxl tests are failing for example.
> > >
> > > $ ./qemu-system-i386 -display none -machine q35,cxl=on
> > > qemu-system-i386: Address space limit 0xffffffff < 0x1000fffff
> > > phys-bits too low (32)
>
> also another thing is:
>
> ./qemu-system-i386 -machine pc -m 128
> works but ...
>
> $ ./qemu-system-i386 -machine pc -m 128,slots=3,maxmem=1G
> qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
> phys-bits too low (32)
>
> or
>
> $ ./qemu-system-i386 -machine pc-i440fx-8.2 -accel kvm -m 128,slots=3,maxmem=1G
> qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
> phys-bits too low (32)
>
> but of course after the compat knob older pc machines work fine using
> the old logic :
>
> $ ./qemu-system-i386 -machine pc-i440fx-8.1 -accel kvm -m 128,slots=3,maxmem=1G
> VNC server running on ::1:5900
> ^Cqemu-system-i386: terminating on signal 2

I dpn't know if we always need to do this but this code adds 1 GiB per
slot for device memory :

    if (pcmc->enforce_aligned_dimm) {
         /* size device region assuming 1G page max alignment per slot */
         size += (1 * GiB) * machine->ram_slots;
     }

For a 32-bit machine that is a lot of memory consumed in just alignment.

On Tue, Sep 19, 2023 at 9:53 AM Ani Sinha <anisinha@redhat.com> wrote:
>
> On Tue, Sep 19, 2023 at 9:20 AM Ani Sinha <anisinha@redhat.com> wrote:
> >
> > On Mon, Sep 18, 2023 at 9:28 PM David Hildenbrand <david@redhat.com> wrote:
> > >
> > > On 18.09.23 17:56, Ani Sinha wrote:
> > > > On Mon, Sep 18, 2023 at 8:59 PM David Hildenbrand <david@redhat.com> wrote:
> > > >>
> > > >> On 18.09.23 17:22, Ani Sinha wrote:
> > > >>> On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.
> > > >>>
> > > >>> This patch breaks some unit tests. I am not sure why it did not catch
> > > >>> it when I tested it before sending.
> > > >>> Will have to resend after fixing the tests.
> > > >>
> > > >> Probably because they supply more memory than the system can actually
> > > >> handle? (e.g., -m 4g on 32bit)?
> > > >
> > > > cxl tests are failing for example.
> > > >
> > > > $ ./qemu-system-i386 -display none -machine q35,cxl=on
> > > > qemu-system-i386: Address space limit 0xffffffff < 0x1000fffff
> > > > phys-bits too low (32)
> >
> > also another thing is:
> >
> > ./qemu-system-i386 -machine pc -m 128
> > works but ...
> >
> > $ ./qemu-system-i386 -machine pc -m 128,slots=3,maxmem=1G
> > qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
> > phys-bits too low (32)
> >
> > or
> >
> > $ ./qemu-system-i386 -machine pc-i440fx-8.2 -accel kvm -m 128,slots=3,maxmem=1G
> > qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
> > phys-bits too low (32)
> >
> > but of course after the compat knob older pc machines work fine using
> > the old logic :
> >
> > $ ./qemu-system-i386 -machine pc-i440fx-8.1 -accel kvm -m 128,slots=3,maxmem=1G
> > VNC server running on ::1:5900
> > ^Cqemu-system-i386: terminating on signal 2
>
> I dpn't know if we always need to do this but this code adds 1 GiB per
> slot for device memory :
>
>     if (pcmc->enforce_aligned_dimm) {
>          /* size device region assuming 1G page max alignment per slot */
>          size += (1 * GiB) * machine->ram_slots;
>      }
>
> For a 32-bit machine that is a lot of memory consumed in just alignment.

Let's look at an example when we get rid of all alignment stuff.

$ ./qemu-system-i386 -machine pc-i440fx-8.2 -m 512M,slots=1,maxmem=1G
above 4G start: 0x100000000,above 4G size: 0x0
qemu-system-i386: Address space limit 0xffffffff < 0x11ffffffe
phys-bits too low (32)

So basically, above_4g_start = 4GiB. size = 0.
Then it is adding the device memory which is 1GiB - 0.5 GiB = 0.5 GiB.
So the  0x11ffffffe is exactly 4.5 GiB.

Anything above 4 GiB is beyond 32 bits.

  Hi,

> Anything above 4 GiB is beyond 32 bits.

It's not that simple.  Physical address space is 64G on processors with
PAE support.

take care,
  Gerd

On Tue, Sep 19, 2023 at 1:38 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > Anything above 4 GiB is beyond 32 bits.
>
> It's not that simple.  Physical address space is 64G on processors with
> PAE support.

yeah I sent a patch previously to fix that as well.

On 19.09.23 08:18, Ani Sinha wrote:
> On Tue, Sep 19, 2023 at 9:53 AM Ani Sinha <anisinha@redhat.com> wrote:
>>
>> On Tue, Sep 19, 2023 at 9:20 AM Ani Sinha <anisinha@redhat.com> wrote:
>>>
>>> On Mon, Sep 18, 2023 at 9:28 PM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 18.09.23 17:56, Ani Sinha wrote:
>>>>> On Mon, Sep 18, 2023 at 8:59 PM David Hildenbrand <david@redhat.com> wrote:
>>>>>>
>>>>>> On 18.09.23 17:22, Ani Sinha wrote:
>>>>>>> On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.
>>>>>>>
>>>>>>> This patch breaks some unit tests. I am not sure why it did not catch
>>>>>>> it when I tested it before sending.
>>>>>>> Will have to resend after fixing the tests.
>>>>>>
>>>>>> Probably because they supply more memory than the system can actually
>>>>>> handle? (e.g., -m 4g on 32bit)?
>>>>>
>>>>> cxl tests are failing for example.
>>>>>
>>>>> $ ./qemu-system-i386 -display none -machine q35,cxl=on
>>>>> qemu-system-i386: Address space limit 0xffffffff < 0x1000fffff
>>>>> phys-bits too low (32)
>>>
>>> also another thing is:
>>>
>>> ./qemu-system-i386 -machine pc -m 128
>>> works but ...
>>>
>>> $ ./qemu-system-i386 -machine pc -m 128,slots=3,maxmem=1G
>>> qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
>>> phys-bits too low (32)
>>>
>>> or
>>>
>>> $ ./qemu-system-i386 -machine pc-i440fx-8.2 -accel kvm -m 128,slots=3,maxmem=1G
>>> qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
>>> phys-bits too low (32)
>>>
>>> but of course after the compat knob older pc machines work fine using
>>> the old logic :
>>>
>>> $ ./qemu-system-i386 -machine pc-i440fx-8.1 -accel kvm -m 128,slots=3,maxmem=1G
>>> VNC server running on ::1:5900
>>> ^Cqemu-system-i386: terminating on signal 2
>>
>> I dpn't know if we always need to do this but this code adds 1 GiB per
>> slot for device memory :
>>
>>      if (pcmc->enforce_aligned_dimm) {
>>           /* size device region assuming 1G page max alignment per slot */
>>           size += (1 * GiB) * machine->ram_slots;
>>       }
>>
>> For a 32-bit machine that is a lot of memory consumed in just alignment.
> 
> Let's look at an example when we get rid of all alignment stuff.
> 
> $ ./qemu-system-i386 -machine pc-i440fx-8.2 -m 512M,slots=1,maxmem=1G
> above 4G start: 0x100000000,above 4G size: 0x0
> qemu-system-i386: Address space limit 0xffffffff < 0x11ffffffe
> phys-bits too low (32)
> 
> So basically, above_4g_start = 4GiB. size = 0.
> Then it is adding the device memory which is 1GiB - 0.5 GiB = 0.5 GiB.
> So the  0x11ffffffe is exactly 4.5 GiB.
> 
> Anything above 4 GiB is beyond 32 bits.
> 

It's not worth worrying about memory devices for 32bit at all. For 
example Linux doesn't support memory hotplug on any 32bit system (not 
even with PAE and friends).

-- 
Cheers,

David / dhildenb

On Tue, Sep 19, 2023 at 1:13 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 19.09.23 08:18, Ani Sinha wrote:
> > On Tue, Sep 19, 2023 at 9:53 AM Ani Sinha <anisinha@redhat.com> wrote:
> >>
> >> On Tue, Sep 19, 2023 at 9:20 AM Ani Sinha <anisinha@redhat.com> wrote:
> >>>
> >>> On Mon, Sep 18, 2023 at 9:28 PM David Hildenbrand <david@redhat.com> wrote:
> >>>>
> >>>> On 18.09.23 17:56, Ani Sinha wrote:
> >>>>> On Mon, Sep 18, 2023 at 8:59 PM David Hildenbrand <david@redhat.com> wrote:
> >>>>>>
> >>>>>> On 18.09.23 17:22, Ani Sinha wrote:
> >>>>>>> On Mon, Sep 18, 2023 at 7:25 PM Ani Sinha <anisinha@redhat.com> 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.
> >>>>>>>
> >>>>>>> This patch breaks some unit tests. I am not sure why it did not catch
> >>>>>>> it when I tested it before sending.
> >>>>>>> Will have to resend after fixing the tests.
> >>>>>>
> >>>>>> Probably because they supply more memory than the system can actually
> >>>>>> handle? (e.g., -m 4g on 32bit)?
> >>>>>
> >>>>> cxl tests are failing for example.
> >>>>>
> >>>>> $ ./qemu-system-i386 -display none -machine q35,cxl=on
> >>>>> qemu-system-i386: Address space limit 0xffffffff < 0x1000fffff
> >>>>> phys-bits too low (32)
> >>>
> >>> also another thing is:
> >>>
> >>> ./qemu-system-i386 -machine pc -m 128
> >>> works but ...
> >>>
> >>> $ ./qemu-system-i386 -machine pc -m 128,slots=3,maxmem=1G
> >>> qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
> >>> phys-bits too low (32)
> >>>
> >>> or
> >>>
> >>> $ ./qemu-system-i386 -machine pc-i440fx-8.2 -accel kvm -m 128,slots=3,maxmem=1G
> >>> qemu-system-i386: Address space limit 0xffffffff < 0x1f7ffffff
> >>> phys-bits too low (32)
> >>>
> >>> but of course after the compat knob older pc machines work fine using
> >>> the old logic :
> >>>
> >>> $ ./qemu-system-i386 -machine pc-i440fx-8.1 -accel kvm -m 128,slots=3,maxmem=1G
> >>> VNC server running on ::1:5900
> >>> ^Cqemu-system-i386: terminating on signal 2
> >>
> >> I dpn't know if we always need to do this but this code adds 1 GiB per
> >> slot for device memory :
> >>
> >>      if (pcmc->enforce_aligned_dimm) {
> >>           /* size device region assuming 1G page max alignment per slot */
> >>           size += (1 * GiB) * machine->ram_slots;
> >>       }
> >>
> >> For a 32-bit machine that is a lot of memory consumed in just alignment.
> >
> > Let's look at an example when we get rid of all alignment stuff.
> >
> > $ ./qemu-system-i386 -machine pc-i440fx-8.2 -m 512M,slots=1,maxmem=1G
> > above 4G start: 0x100000000,above 4G size: 0x0
> > qemu-system-i386: Address space limit 0xffffffff < 0x11ffffffe
> > phys-bits too low (32)
> >
> > So basically, above_4g_start = 4GiB. size = 0.
> > Then it is adding the device memory which is 1GiB - 0.5 GiB = 0.5 GiB.
> > So the  0x11ffffffe is exactly 4.5 GiB.
> >
> > Anything above 4 GiB is beyond 32 bits.
> >
>
> It's not worth worrying about memory devices for 32bit at all. For
> example Linux doesn't support memory hotplug on any 32bit system (not
> even with PAE and friends).
>

Ok fair enough. The existing scheme clearly does not support 32-bit
memory hotplug.

We do have a slight improvement with our original test case. I will
send an updated patch that passed all unit tests :

$ ./qemu-system-x86_64 -machine pc-i440fx-8.2 -cpu pentium -m
size=10G, -monitor stdio -qmp tcp:0:5555,server,nowait
QEMU 8.1.50 monitor - type 'help' for more information
qemu-system-x86_64: Address space limit 0xffffffff < 0x2bfffffff
phys-bits too low (32)

[anisinha@rhel9-box build]$ ./qemu-system-x86_64 -machine
pc-i440fx-8.1 -cpu pentium -m size=10G, -monitor stdio -qmp
tcp:0:5555,server,nowait
QEMU 8.1.50 monitor - type 'help' for more information
VNC server running on ::1:5900
(qemu)

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?

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

>      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

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
>

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