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

[B]

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,