diff mbox series

[v3,2/9] hw/pci-host/q35: Inline sysbus_add_io()

Message ID 20230204151027.39007-3-shentey@gmail.com
State New
Headers show
Series PC cleanups | expand

Commit Message

Bernhard Beschow Feb. 4, 2023, 3:10 p.m. UTC 
sysbus_add_io() just wraps memory_region_add_subregion() while also
obscuring where the memory is attached. So use
memory_region_add_subregion() directly and attach it to the existing
memory region s->mch.address_space_io which is set as an alias to
get_system_io() by the q35 machine.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/pci-host/q35.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 5, 2023, 11:12 a.m. UTC | #1 
On 4/2/23 16:10, Bernhard Beschow wrote:
> sysbus_add_io() just wraps memory_region_add_subregion() while also
> obscuring where the memory is attached. So use
> memory_region_add_subregion() directly and attach it to the existing
> memory region s->mch.address_space_io which is set as an alias to
> get_system_io() by the q35 machine.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/pci-host/q35.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 26390863d6..fa05844319 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -50,10 +50,12 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>       Q35PCIHost *s = Q35_HOST_DEVICE(dev);
>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>   
> -    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
> +    memory_region_add_subregion(s->mch.address_space_io,
> +                                MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
>       sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);

This makes me wonder why MCH_PCI_DEVICE doesn't use the bus I/O space
via pci_address_space_io(). IOW, why the MR like is in MCH_PCI_DEVICE
and not Q35_HOST_DEVICE?
Bernhard Beschow Feb. 6, 2023, 12:15 a.m. UTC | #2 
Am 5. Februar 2023 11:12:26 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 4/2/23 16:10, Bernhard Beschow wrote:
>> sysbus_add_io() just wraps memory_region_add_subregion() while also
>> obscuring where the memory is attached. So use
>> memory_region_add_subregion() directly and attach it to the existing
>> memory region s->mch.address_space_io which is set as an alias to
>> get_system_io() by the q35 machine.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/pci-host/q35.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 26390863d6..fa05844319 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -50,10 +50,12 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>>       Q35PCIHost *s = Q35_HOST_DEVICE(dev);
>>       SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>   -    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
>> +    memory_region_add_subregion(s->mch.address_space_io,
>> +                                MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
>>       sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
>
>This makes me wonder why MCH_PCI_DEVICE doesn't use the bus I/O space
>via pci_address_space_io(). IOW, why the MR like is in MCH_PCI_DEVICE
>and not Q35_HOST_DEVICE?

I think I have follow-up patches in the pipeline moving the MemoryRegion pointers to the host device. Interestingly, these pointers are only used during the realize phase and just needlessly occupy memory during the rest of the device's lifetime...
diff mbox series

Patch

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 26390863d6..fa05844319 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -50,10 +50,12 @@  static void q35_host_realize(DeviceState *dev, Error **errp)
     Q35PCIHost *s = Q35_HOST_DEVICE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
+    memory_region_add_subregion(s->mch.address_space_io,
+                                MCH_HOST_BRIDGE_CONFIG_ADDR, &pci->conf_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
 
-    sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
+    memory_region_add_subregion(s->mch.address_space_io,
+                                MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
 
     /* register q35 0xcf8 port as coalesced pio */