On 7/11/22 23:17, B wrote:
> Am 11. Juli 2022 10:01:49 UTC schrieb Joao Martins
> <joao.m.mart...@oracle.com>:
>> On 7/9/22 21:51, B wrote:
>>> Am 1. Juli 2022 16:10:07 UTC schrieb Joao Martins
>>> <joao.m.mart...@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,
