[v6,03/10] i386/pc: pass pci_hole64_size to pc_memory_init()

Use the pre-initialized pci-host qdev and fetch the
pci-hole64-size into pc_memory_init() newly added argument.
piix needs a bit of care given all the !pci_enabled()
and that the pci_hole64_size is private to i440fx.

This is in preparation to determine that host-phys-bits are
enough and for pci-hole64-size to be considered to relocate
ram-above-4g to be at 1T (on AMD platforms).

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c                 | 3 ++-
 hw/i386/pc_piix.c            | 5 ++++-
 hw/i386/pc_q35.c             | 8 +++++++-
 hw/pci-host/i440fx.c         | 7 +++++++
 include/hw/i386/pc.h         | 3 ++-
 include/hw/pci-host/i440fx.h | 1 +
 6 files changed, 23 insertions(+), 4 deletions(-)

Am 1. Juli 2022 16:10:07 UTC schrieb Joao Martins <joao.m.martins@oracle.com>:
>Use the pre-initialized pci-host qdev and fetch the
>pci-hole64-size into pc_memory_init() newly added argument.
>piix needs a bit of care given all the !pci_enabled()
>and that the pci_hole64_size is private to i440fx.

It exposes this value as the property PCI_HOST_PROP_PCI_HOLE64_SIZE. Reusing it allows for not touching i440fx in this patch at all.

For code symmetry reasons the analogue property could be used for Q35 as well.

Best regards,
Bernhard

>
>This is in preparation to determine that host-phys-bits are
>enough and for pci-hole64-size to be considered to relocate
>ram-above-4g to be at 1T (on AMD platforms).
>
>Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>---
> hw/i386/pc.c                 | 3 ++-
> hw/i386/pc_piix.c            | 5 ++++-
> hw/i386/pc_q35.c             | 8 +++++++-
> hw/pci-host/i440fx.c         | 7 +++++++
> include/hw/i386/pc.h         | 3 ++-
> include/hw/pci-host/i440fx.h | 1 +
> 6 files changed, 23 insertions(+), 4 deletions(-)
>
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index a9d1bf95649a..1bb89a9c17ec 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -817,7 +817,8 @@ void xen_load_linux(PCMachineState *pcms)
> void pc_memory_init(PCMachineState *pcms,
>                     MemoryRegion *system_memory,
>                     MemoryRegion *rom_memory,
>-                    MemoryRegion **ram_memory)
>+                    MemoryRegion **ram_memory,
>+                    uint64_t pci_hole64_size)
> {
>     int linux_boot, i;
>     MemoryRegion *option_rom_mr;
>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>index 6186a1473755..f3c726e42400 100644
>--- a/hw/i386/pc_piix.c
>+++ b/hw/i386/pc_piix.c
>@@ -91,6 +91,7 @@ static void pc_init1(MachineState *machine,
>     MemoryRegion *pci_memory;
>     MemoryRegion *rom_memory;
>     ram_addr_t lowmem;
>+    uint64_t hole64_size;
>     DeviceState *i440fx_host;
> 
>     /*
>@@ -166,10 +167,12 @@ static void pc_init1(MachineState *machine,
>         memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>         rom_memory = pci_memory;
>         i440fx_host = qdev_new(host_type);
>+        hole64_size = i440fx_pci_hole64_size(i440fx_host);
>     } else {
>         pci_memory = NULL;
>         rom_memory = system_memory;
>         i440fx_host = NULL;
>+        hole64_size = 0;
>     }
> 
>     pc_guest_info_init(pcms);
>@@ -186,7 +189,7 @@ static void pc_init1(MachineState *machine,
>     /* allocate ram and load rom/bios */
>     if (!xen_enabled()) {
>         pc_memory_init(pcms, system_memory,
>-                       rom_memory, &ram_memory);
>+                       rom_memory, &ram_memory, hole64_size);
>     } else {
>         pc_system_flash_cleanup_unused(pcms);
>         if (machine->kernel_filename != NULL) {
>diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>index 46ea89e564de..5a4a737fe203 100644
>--- a/hw/i386/pc_q35.c
>+++ b/hw/i386/pc_q35.c
>@@ -138,6 +138,7 @@ static void pc_q35_init(MachineState *machine)
>     MachineClass *mc = MACHINE_GET_CLASS(machine);
>     bool acpi_pcihp;
>     bool keep_pci_slot_hpc;
>+    uint64_t pci_hole64_size = 0;
> 
>     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
>@@ -206,8 +207,13 @@ static void pc_q35_init(MachineState *machine)
>     /* create pci host bus */
>     q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));
> 
>+    if (pcmc->pci_enabled) {
>+        pci_hole64_size = q35_host->mch.pci_hole64_size;
>+    }
>+
>     /* allocate ram and load rom/bios */
>-    pc_memory_init(pcms, get_system_memory(), rom_memory, &ram_memory);
>+    pc_memory_init(pcms, get_system_memory(), rom_memory, &ram_memory,
>+                   pci_hole64_size);
> 
>     object_property_add_child(qdev_get_machine(), "q35", OBJECT(q35_host));
>     object_property_set_link(OBJECT(q35_host), MCH_HOST_PROP_RAM_MEM,
>diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
>index d5426ef4a53c..15680da7d709 100644
>--- a/hw/pci-host/i440fx.c
>+++ b/hw/pci-host/i440fx.c
>@@ -237,6 +237,13 @@ static void i440fx_realize(PCIDevice *dev, Error **errp)
>     }
> }
> 
>+uint64_t i440fx_pci_hole64_size(DeviceState *i440fx_dev)
>+{
>+        I440FXState *i440fx = I440FX_PCI_HOST_BRIDGE(i440fx_dev);
>+
>+        return i440fx->pci_hole64_size;
>+}
>+
> PCIBus *i440fx_init(const char *pci_type,
>                     DeviceState *dev,
>                     MemoryRegion *address_space_mem,
>diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>index b7735dccfc81..568c226d3034 100644
>--- a/include/hw/i386/pc.h
>+++ b/include/hw/i386/pc.h
>@@ -159,7 +159,8 @@ void xen_load_linux(PCMachineState *pcms);
> void pc_memory_init(PCMachineState *pcms,
>                     MemoryRegion *system_memory,
>                     MemoryRegion *rom_memory,
>-                    MemoryRegion **ram_memory);
>+                    MemoryRegion **ram_memory,
>+                    uint64_t pci_hole64_size);
> uint64_t pc_pci_hole64_start(void);
> DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
> void pc_basic_device_init(struct PCMachineState *pcms,
>diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
>index d02bf1ed6b93..2234dd5a2a6a 100644
>--- a/include/hw/pci-host/i440fx.h
>+++ b/include/hw/pci-host/i440fx.h
>@@ -45,5 +45,6 @@ PCIBus *i440fx_init(const char *pci_type,
>                     MemoryRegion *pci_memory,
>                     MemoryRegion *ram_memory);
> 
>+uint64_t i440fx_pci_hole64_size(DeviceState *i440fx_dev);
> 
> #endif

On 7/9/22 21:51, B wrote:
> Am 1. Juli 2022 16:10:07 UTC schrieb Joao Martins <joao.m.martins@oracle.com>:
>> Use the pre-initialized pci-host qdev and fetch the
>> pci-hole64-size into pc_memory_init() newly added argument.
>> piix needs a bit of care given all the !pci_enabled()
>> and that the pci_hole64_size is private to i440fx.
> 
> It exposes this value as the property PCI_HOST_PROP_PCI_HOLE64_SIZE. 

Indeed.

> Reusing it allows for not touching i440fx in this patch at all.
> 
> For code symmetry reasons the analogue property could be used for Q35 as well.
> 
Presumably you want me to change into below while deleting i440fx_pci_hole64_size()
from this patch (see snip below). IMHO, gotta say that in q35 the code symmetry
doesn't buy much readability here, albeit it does remove any need for that other
helper in i440fx.

@Igor let me know if you agree with the change and whether I can keep the Reviewed-by.

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 504ddd0deece..cc0855066d06 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -167,7 +167,9 @@ static void pc_init1(MachineState *machine,
         memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
         rom_memory = pci_memory;
         i440fx_host = qdev_new(host_type);
-        hole64_size = i440fx_pci_hole64_size(i440fx_host);
+        hole64_size = object_property_get_uint(OBJECT(i440fx_host),
+                                               PCI_HOST_PROP_PCI_HOLE64_SIZE,
+                                               &error_abort);
     } else {
         pci_memory = NULL;
         rom_memory = system_memory;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4b747c59c19a..466f3ef3c918 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -208,7 +208,9 @@ static void pc_q35_init(MachineState *machine)
     q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));

     if (pcmc->pci_enabled) {
-        pci_hole64_size = q35_host->mch.pci_hole64_size;
+        pci_hole64_size = object_property_get_uint(OBJECT(q35_host),
+                                                   PCI_HOST_PROP_PCI_HOLE64_SIZE,
+                                                   &error_abort);
     }

     /* allocate ram and load rom/bios */
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 15680da7d709..d5426ef4a53c 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -237,13 +237,6 @@ static void i440fx_realize(PCIDevice *dev, Error **errp)
     }
 }

-uint64_t i440fx_pci_hole64_size(DeviceState *i440fx_dev)
-{
-        I440FXState *i440fx = I440FX_PCI_HOST_BRIDGE(i440fx_dev);
-
-        return i440fx->pci_hole64_size;
-}
-
 PCIBus *i440fx_init(const char *pci_type,
                     DeviceState *dev,
                     MemoryRegion *address_space_mem,

Am 11. Juli 2022 10:01:49 UTC schrieb Joao Martins <joao.m.martins@oracle.com>:
>On 7/9/22 21:51, B wrote:
>> Am 1. Juli 2022 16:10:07 UTC schrieb Joao Martins <joao.m.martins@oracle.com>:
>>> Use the pre-initialized pci-host qdev and fetch the
>>> pci-hole64-size into pc_memory_init() newly added argument.
>>> piix needs a bit of care given all the !pci_enabled()
>>> and that the pci_hole64_size is private to i440fx.
>> 
>> It exposes this value as the property PCI_HOST_PROP_PCI_HOLE64_SIZE. 
>
>Indeed.
>
>> Reusing it allows for not touching i440fx in this patch at all.
>> 
>> For code symmetry reasons the analogue property could be used for Q35 as well.
>> 
>Presumably you want me to change into below while deleting i440fx_pci_hole64_size()
>from this patch (see snip below).

Yes, exactly.

>IMHO, gotta say that in q35 the code symmetry
>doesn't buy much readability here,

That's true. It communicates, though, that a value is used which was deliberately made public, IOW that the code isn't sneaky. (That's just my interpretation, not sure what the common understanding is) Feel free to do however you prefer.

Best regards,
Bernhard

>albeit it does remove any need for that other
>helper in i440fx.
>
>@Igor let me know if you agree with the change and whether I can keep the Reviewed-by.
>
>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>index 504ddd0deece..cc0855066d06 100644
>--- a/hw/i386/pc_piix.c
>+++ b/hw/i386/pc_piix.c
>@@ -167,7 +167,9 @@ static void pc_init1(MachineState *machine,
>         memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>         rom_memory = pci_memory;
>         i440fx_host = qdev_new(host_type);
>-        hole64_size = i440fx_pci_hole64_size(i440fx_host);
>+        hole64_size = object_property_get_uint(OBJECT(i440fx_host),
>+                                               PCI_HOST_PROP_PCI_HOLE64_SIZE,
>+                                               &error_abort);
>     } else {
>         pci_memory = NULL;
>         rom_memory = system_memory;
>diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>index 4b747c59c19a..466f3ef3c918 100644
>--- a/hw/i386/pc_q35.c
>+++ b/hw/i386/pc_q35.c
>@@ -208,7 +208,9 @@ static void pc_q35_init(MachineState *machine)
>     q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));
>
>     if (pcmc->pci_enabled) {
>-        pci_hole64_size = q35_host->mch.pci_hole64_size;
>+        pci_hole64_size = object_property_get_uint(OBJECT(q35_host),
>+                                                   PCI_HOST_PROP_PCI_HOLE64_SIZE,
>+                                                   &error_abort);
>     }
>
>     /* allocate ram and load rom/bios */
>diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
>index 15680da7d709..d5426ef4a53c 100644
>--- a/hw/pci-host/i440fx.c
>+++ b/hw/pci-host/i440fx.c
>@@ -237,13 +237,6 @@ static void i440fx_realize(PCIDevice *dev, Error **errp)
>     }
> }
>
>-uint64_t i440fx_pci_hole64_size(DeviceState *i440fx_dev)
>-{
>-        I440FXState *i440fx = I440FX_PCI_HOST_BRIDGE(i440fx_dev);
>-
>-        return i440fx->pci_hole64_size;
>-}
>-
> PCIBus *i440fx_init(const char *pci_type,
>                     DeviceState *dev,
>                     MemoryRegion *address_space_mem,

On 7/11/22 23:17, B wrote:
> Am 11. Juli 2022 10:01:49 UTC schrieb Joao Martins <joao.m.martins@oracle.com>:
>> On 7/9/22 21:51, B wrote:
>>> Am 1. Juli 2022 16:10:07 UTC schrieb Joao Martins <joao.m.martins@oracle.com>:
>>>> Use the pre-initialized pci-host qdev and fetch the
>>>> pci-hole64-size into pc_memory_init() newly added argument.
>>>> piix needs a bit of care given all the !pci_enabled()
>>>> and that the pci_hole64_size is private to i440fx.
>>>
>>> It exposes this value as the property PCI_HOST_PROP_PCI_HOLE64_SIZE. 
>>
>> Indeed.
>>
>>> Reusing it allows for not touching i440fx in this patch at all.
>>>
>>> For code symmetry reasons the analogue property could be used for Q35 as well.
>>>
>> Presumably you want me to change into below while deleting i440fx_pci_hole64_size()
>>from this patch (see snip below).
> 
> Yes, exactly.
> 
>> IMHO, gotta say that in q35 the code symmetry
>> doesn't buy much readability here,
> 
> That's true. It communicates, though, that a value is used which was deliberately made public, IOW that the code isn't sneaky. (That's just my interpretation, not sure what the common understanding is) Feel free to do however you prefer.
> 
I think it's a good improvement, as avoids duplicating this new helper in i440fx pcihost
which also means less code for the same thing.

> Best regards,
> Bernhard
> 
>> albeit it does remove any need for that other
>> helper in i440fx.
>>
>> @Igor let me know if you agree with the change and whether I can keep the Reviewed-by.
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 504ddd0deece..cc0855066d06 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -167,7 +167,9 @@ static void pc_init1(MachineState *machine,
>>         memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>>         rom_memory = pci_memory;
>>         i440fx_host = qdev_new(host_type);
>> -        hole64_size = i440fx_pci_hole64_size(i440fx_host);
>> +        hole64_size = object_property_get_uint(OBJECT(i440fx_host),
>> +                                               PCI_HOST_PROP_PCI_HOLE64_SIZE,
>> +                                               &error_abort);
>>     } else {
>>         pci_memory = NULL;
>>         rom_memory = system_memory;
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 4b747c59c19a..466f3ef3c918 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -208,7 +208,9 @@ static void pc_q35_init(MachineState *machine)
>>     q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));
>>
>>     if (pcmc->pci_enabled) {
>> -        pci_hole64_size = q35_host->mch.pci_hole64_size;
>> +        pci_hole64_size = object_property_get_uint(OBJECT(q35_host),
>> +                                                   PCI_HOST_PROP_PCI_HOLE64_SIZE,
>> +                                                   &error_abort);
>>     }
>>
>>     /* allocate ram and load rom/bios */
>> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
>> index 15680da7d709..d5426ef4a53c 100644
>> --- a/hw/pci-host/i440fx.c
>> +++ b/hw/pci-host/i440fx.c
>> @@ -237,13 +237,6 @@ static void i440fx_realize(PCIDevice *dev, Error **errp)
>>     }
>> }
>>
>> -uint64_t i440fx_pci_hole64_size(DeviceState *i440fx_dev)
>> -{
>> -        I440FXState *i440fx = I440FX_PCI_HOST_BRIDGE(i440fx_dev);
>> -
>> -        return i440fx->pci_hole64_size;
>> -}
>> -
>> PCIBus *i440fx_init(const char *pci_type,
>>                     DeviceState *dev,
>>                     MemoryRegion *address_space_mem,