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

[Joao Martins]

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,