[v2] hw: Strengthen SysBus & QBus API

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

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

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.

Transformation done using the following coccinelle script:

  @@
  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);

and manually adding the local 'host_mem' variable to
avoid multiple calls to get_system_memory().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/bonito.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index ee6cb85e97..96bd028671 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -654,10 +654,10 @@ 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);
+    MemoryRegion *host_mem = get_system_memory();
 
     /*
      * Bonito North Bridge, built on FPGA,
@@ -668,48 +668,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(host_mem, 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(host_mem, 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(host_mem, 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(host_mem, 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(host_mem, 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(host_mem, 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(host_mem, 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,
@@ -719,7 +719,7 @@ static void bonito_pci_realize(PCIDevice *dev, Error **errp)
 
     memory_region_init_alias(pcimem_alias, NULL, "pci.mem.alias",
                              &bs->pci_mem, 0, BONITO_PCIHI_SIZE);
-    memory_region_add_subregion(get_system_memory(),
+    memory_region_add_subregion(host_mem,
                                 BONITO_PCIHI_BASE, pcimem_alias);
     create_unimplemented_device("PCI_2",
                                 (hwaddr)BONITO_PCIHI_BASE + BONITO_PCIHI_SIZE,
-- 
2.41.0

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

Posted by Thomas Huth 1 year, 1 month ago

On 19/10/2023 09.16, 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.
> 
> Transformation done using the following coccinelle script:
> 
>    @@
>    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);
> 
> and manually adding the local 'host_mem' variable to
> avoid multiple calls to get_system_memory().

Thanks for updating it!

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/bonito.c | 32 ++++++++++++++++----------------
>   1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index ee6cb85e97..96bd028671 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -654,10 +654,10 @@ 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);
> +    MemoryRegion *host_mem = get_system_memory();
>   
>       /*
>        * Bonito North Bridge, built on FPGA,
> @@ -668,48 +668,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(host_mem, 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(host_mem, 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(host_mem, 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(host_mem, 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(host_mem, 0x1fe00300,
> +                                &s->iomem_cop);

At least the above two hunks look like they could now fit into one line?

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>

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

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

On 19/10/23 09:26, Thomas Huth wrote:
> On 19/10/2023 09.16, 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.
>>
>> Transformation done using the following coccinelle script:
>>
>>    @@
>>    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);
>>
>> and manually adding the local 'host_mem' variable to
>> avoid multiple calls to get_system_memory().
> 
> Thanks for updating it!
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/pci-host/bonito.c | 32 ++++++++++++++++----------------
>>   1 file changed, 16 insertions(+), 16 deletions(-)


>>       /* 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(host_mem, 0x1fe00300,
>> +                                &s->iomem_cop);
> 
> At least the above two hunks look like they could now fit into one line?

This file will be heavily reworked soon, but meanwhile I did what you
suggested.

> Anyway:
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!