Re: [PATCH 05/12] hw/pci-host/bonito: Do not use SysBus API to map local MMIO region

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 18/10/23 18:05, Thomas Huth wrote:
> On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
>> There is no point in exposing an internal MMIO region via
>> SysBus and directly mapping it in the very same device.
>>
>> Just map it without using the SysBus API.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/pci-host/bonito.c | 29 ++++++++++++++---------------
>>   1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>> index ee6cb85e97..3b803bcad3 100644
>> --- a/hw/pci-host/bonito.c
>> +++ b/hw/pci-host/bonito.c
>> @@ -654,7 +654,6 @@ static void bonito_host_realize(DeviceState *dev, 
>> Error **errp)
>>   static void bonito_pci_realize(PCIDevice *dev, Error **errp)
>>   {
>>       PCIBonitoState *s = PCI_BONITO(dev);
>> -    SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
>>       PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
>>       BonitoState *bs = s->pcihost;
>>       MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
>> @@ -668,48 +667,48 @@ static void bonito_pci_realize(PCIDevice *dev, 
>> Error **errp)
>>       /* set the north bridge register mapping */
>>       memory_region_init_io(&s->iomem, OBJECT(s), &bonito_ops, s,
>>                             "north-bridge-register", 
>> BONITO_INTERNAL_REG_SIZE);
>> -    sysbus_init_mmio(sysbus, &s->iomem);
>> -    sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_INTERNAL_REG_BASE,
>> +                                &s->iomem);
>>       /* set the north bridge pci configure  mapping */
>>       memory_region_init_io(&phb->conf_mem, OBJECT(s), 
>> &bonito_pciconf_ops, s,
>>                             "north-bridge-pci-config", 
>> BONITO_PCICONFIG_SIZE);
>> -    sysbus_init_mmio(sysbus, &phb->conf_mem);
>> -    sysbus_mmio_map(sysbus, 1, BONITO_PCICONFIG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_PCICONFIG_BASE,
>> +                                &phb->conf_mem);
>>       /* set the south bridge pci configure  mapping */
>>       memory_region_init_io(&phb->data_mem, OBJECT(s), 
>> &bonito_spciconf_ops, s,
>>                             "south-bridge-pci-config", 
>> BONITO_SPCICONFIG_SIZE);
>> -    sysbus_init_mmio(sysbus, &phb->data_mem);
>> -    sysbus_mmio_map(sysbus, 2, BONITO_SPCICONFIG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_SPCICONFIG_BASE,
>> +                                &phb->data_mem);
>>       create_unimplemented_device("bonito", BONITO_REG_BASE, 
>> BONITO_REG_SIZE);
>>       memory_region_init_io(&s->iomem_ldma, OBJECT(s), 
>> &bonito_ldma_ops, s,
>>                             "ldma", 0x100);
>> -    sysbus_init_mmio(sysbus, &s->iomem_ldma);
>> -    sysbus_mmio_map(sysbus, 3, 0x1fe00200);
>> +    memory_region_add_subregion(get_system_memory(), 0x1fe00200,
>> +                                &s->iomem_ldma);
>>       /* PCI copier */
>>       memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
>>                             "cop", 0x100);
>> -    sysbus_init_mmio(sysbus, &s->iomem_cop);
>> -    sysbus_mmio_map(sysbus, 4, 0x1fe00300);
>> +    memory_region_add_subregion(get_system_memory(), 0x1fe00300,
>> +                                &s->iomem_cop);
>>       create_unimplemented_device("ROMCS", BONITO_FLASH_BASE, 60 * MiB);
>>       /* Map PCI IO Space  0x1fd0 0000 - 0x1fd1 0000 */
>>       memory_region_init_alias(&s->bonito_pciio, OBJECT(s), "isa_mmio",
>>                                get_system_io(), 0, BONITO_PCIIO_SIZE);
>> -    sysbus_init_mmio(sysbus, &s->bonito_pciio);
>> -    sysbus_mmio_map(sysbus, 5, BONITO_PCIIO_BASE);
>> +    memory_region_add_subregion(get_system_memory(), BONITO_PCIIO_BASE,
>> +                                &s->bonito_pciio);
>>       /* add pci local io mapping */
>>       memory_region_init_alias(&s->bonito_localio, OBJECT(s), "IOCS[0]",
>>                                get_system_io(), 0, 256 * KiB);
>> -    sysbus_init_mmio(sysbus, &s->bonito_localio);
>> -    sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
>> +    memory_region_add_subregion(get_system_memory(), BONITO_DEV_BASE,
>> +                                &s->bonito_localio);
>>       create_unimplemented_device("IOCS[1]", BONITO_DEV_BASE + 1 * 256 
>> * KiB,
>>                                   256 * KiB);
>>       create_unimplemented_device("IOCS[2]", BONITO_DEV_BASE + 2 * 256 
>> * KiB,
> 
> Would it make sense to cache the return value of get_system_memory() in 
> a local variable instead of calling it again and again ...?


This is done later, at this point I'm just doing mechanical
transformation with:

     @@
     expression sbdev;
     expression index;
     expression addr;
     expression subregion;
     @@
     -    sysbus_init_mmio(sbdev, subregion);
          ... when != sbdev
     -    sysbus_mmio_map(sbdev, index, addr);
     +    memory_region_add_subregion(get_system_memory(), addr, subregion);

     @@
     expression sbdev;
     expression index;
     expression addr;
     expression subregion;
     expression priority;
     @@
     -    sysbus_init_mmio(sbdev, subregion);
          ... when != sbdev
     -    sysbus_mmio_map_overlap(sbdev, index, addr, priority);
     +    memory_region_add_subregion_overlap(get_system_memory(), addr,
     +                                        subregion, priority);