diff mbox series

sabre: use object_initialize_child() for iommu child object

Message ID 20201021114300.11579-1-mark.cave-ayland@ilande.co.uk
State New
Headers show
Series sabre: use object_initialize_child() for iommu child object | expand

Commit Message

Mark Cave-Ayland Oct. 21, 2020, 11:43 a.m. UTC 
Store the child object directly within the sabre object rather than using
link properties.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci-host/sabre.c         | 10 ++++------
 hw/sparc64/sun4u.c          |  8 +-------
 include/hw/pci-host/sabre.h |  2 +-
 3 files changed, 6 insertions(+), 14 deletions(-)

Comments

Mark Cave-Ayland Oct. 25, 2020, 11:11 a.m. UTC | #1 
On 21/10/2020 12:43, Mark Cave-Ayland wrote:

> Store the child object directly within the sabre object rather than using
> link properties.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/pci-host/sabre.c         | 10 ++++------
>   hw/sparc64/sun4u.c          |  8 +-------
>   include/hw/pci-host/sabre.h |  2 +-
>   3 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index f41a0cc301..aaa93acd6e 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -383,9 +383,10 @@ static void sabre_realize(DeviceState *dev, Error **errp)
>       pci_create_simple(phb->bus, 0, TYPE_SABRE_PCI_DEVICE);
>   
>       /* IOMMU */
> +    sysbus_realize(SYS_BUS_DEVICE(&s->iommu), &error_fatal);
>       memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
> -                    sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
> -    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
> +                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->iommu), 0), 1);
> +    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, &s->iommu);
>   
>       /* APB secondary busses */
>       pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), true,
> @@ -422,10 +423,7 @@ static void sabre_init(Object *obj)
>       s->pci_irq_in = 0ULL;
>   
>       /* IOMMU */
> -    object_property_add_link(obj, "iommu", TYPE_SUN4U_IOMMU,
> -                             (Object **) &s->iommu,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> +    object_initialize_child(obj, "iommu", &s->iommu, TYPE_SUN4U_IOMMU);
>   
>       /* sabre_config */
>       memory_region_init_io(&s->sabre_config, OBJECT(s), &sabre_config_ops, s,
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 2f8fc670cf..a33f1eccfd 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -562,7 +562,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>       PCIBus *pci_bus, *pci_busA, *pci_busB;
>       PCIDevice *ebus, *pci_dev;
>       SysBusDevice *s;
> -    DeviceState *iommu, *dev;
> +    DeviceState *dev;
>       FWCfgState *fw_cfg;
>       NICInfo *nd;
>       MACAddr macaddr;
> @@ -571,10 +571,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>       /* init CPUs */
>       cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
>   
> -    /* IOMMU */
> -    iommu = qdev_new(TYPE_SUN4U_IOMMU);
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(iommu), &error_fatal);
> -
>       /* set up devices */
>       ram_init(0, machine->ram_size);
>   
> @@ -584,8 +580,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>       sabre = SABRE(qdev_new(TYPE_SABRE));
>       qdev_prop_set_uint64(DEVICE(sabre), "special-base", PBM_SPECIAL_BASE);
>       qdev_prop_set_uint64(DEVICE(sabre), "mem-base", PBM_MEM_BASE);
> -    object_property_set_link(OBJECT(sabre), "iommu", OBJECT(iommu),
> -                             &error_abort);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(sabre), &error_fatal);
>   
>       /* sabre_config */
> diff --git a/include/hw/pci-host/sabre.h b/include/hw/pci-host/sabre.h
> index 01190241bb..05bf741cde 100644
> --- a/include/hw/pci-host/sabre.h
> +++ b/include/hw/pci-host/sabre.h
> @@ -34,7 +34,7 @@ struct SabreState {
>       MemoryRegion pci_mmio;
>       MemoryRegion pci_ioport;
>       uint64_t pci_irq_in;
> -    IOMMUState *iommu;
> +    IOMMUState iommu;
>       PCIBridge *bridgeA;
>       PCIBridge *bridgeB;
>       uint32_t pci_control[16];

No further comments (and I'm happier that this is a better solution than having an 
"optional" link property) so I've applied this to my qemu-sparc branch.


ATB,

Mark.
Philippe Mathieu-Daudé Oct. 25, 2020, 11:41 a.m. UTC | #2 
On 10/25/20 12:11 PM, Mark Cave-Ayland wrote:
> On 21/10/2020 12:43, Mark Cave-Ayland wrote:
> 
>> Store the child object directly within the sabre object rather than using
>> link properties.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/pci-host/sabre.c         | 10 ++++------
>>   hw/sparc64/sun4u.c          |  8 +-------
>>   include/hw/pci-host/sabre.h |  2 +-
>>   3 files changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
>> index f41a0cc301..aaa93acd6e 100644
>> --- a/hw/pci-host/sabre.c
>> +++ b/hw/pci-host/sabre.c
>> @@ -383,9 +383,10 @@ static void sabre_realize(DeviceState *dev, Error 
>> **errp)
>>       pci_create_simple(phb->bus, 0, TYPE_SABRE_PCI_DEVICE);
>>       /* IOMMU */
>> +    sysbus_realize(SYS_BUS_DEVICE(&s->iommu), &error_fatal);
>>       memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
>> -                    sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 
>> 0), 1);
>> -    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
>> +                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->iommu), 
>> 0), 1);
>> +    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, &s->iommu);
>>       /* APB secondary busses */
>>       pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), true,
>> @@ -422,10 +423,7 @@ static void sabre_init(Object *obj)
>>       s->pci_irq_in = 0ULL;
>>       /* IOMMU */
>> -    object_property_add_link(obj, "iommu", TYPE_SUN4U_IOMMU,
>> -                             (Object **) &s->iommu,
>> -                             qdev_prop_allow_set_link_before_realize,
>> -                             0);
>> +    object_initialize_child(obj, "iommu", &s->iommu, TYPE_SUN4U_IOMMU);
>>       /* sabre_config */
>>       memory_region_init_io(&s->sabre_config, OBJECT(s), 
>> &sabre_config_ops, s,
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index 2f8fc670cf..a33f1eccfd 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -562,7 +562,7 @@ static void sun4uv_init(MemoryRegion 
>> *address_space_mem,
>>       PCIBus *pci_bus, *pci_busA, *pci_busB;
>>       PCIDevice *ebus, *pci_dev;
>>       SysBusDevice *s;
>> -    DeviceState *iommu, *dev;
>> +    DeviceState *dev;
>>       FWCfgState *fw_cfg;
>>       NICInfo *nd;
>>       MACAddr macaddr;
>> @@ -571,10 +571,6 @@ static void sun4uv_init(MemoryRegion 
>> *address_space_mem,
>>       /* init CPUs */
>>       cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
>> -    /* IOMMU */
>> -    iommu = qdev_new(TYPE_SUN4U_IOMMU);
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(iommu), &error_fatal);
>> -
>>       /* set up devices */
>>       ram_init(0, machine->ram_size);
>> @@ -584,8 +580,6 @@ static void sun4uv_init(MemoryRegion 
>> *address_space_mem,
>>       sabre = SABRE(qdev_new(TYPE_SABRE));
>>       qdev_prop_set_uint64(DEVICE(sabre), "special-base", 
>> PBM_SPECIAL_BASE);
>>       qdev_prop_set_uint64(DEVICE(sabre), "mem-base", PBM_MEM_BASE);
>> -    object_property_set_link(OBJECT(sabre), "iommu", OBJECT(iommu),
>> -                             &error_abort);
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(sabre), &error_fatal);
>>       /* sabre_config */
>> diff --git a/include/hw/pci-host/sabre.h b/include/hw/pci-host/sabre.h
>> index 01190241bb..05bf741cde 100644
>> --- a/include/hw/pci-host/sabre.h
>> +++ b/include/hw/pci-host/sabre.h
>> @@ -34,7 +34,7 @@ struct SabreState {
>>       MemoryRegion pci_mmio;
>>       MemoryRegion pci_ioport;
>>       uint64_t pci_irq_in;
>> -    IOMMUState *iommu;
>> +    IOMMUState iommu;
>>       PCIBridge *bridgeA;
>>       PCIBridge *bridgeB;
>>       uint32_t pci_control[16];
> 
> No further comments (and I'm happier that this is a better solution than 
> having an "optional" link property) so I've applied this to my 
> qemu-sparc branch.

Sorry I had this patch tagged for review but am having trouble
managing that folder. This is certainly better, thanks for this
cleanup.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> 
> ATB,
> 
> Mark.
>
Mark Cave-Ayland Oct. 27, 2020, 9:33 a.m. UTC | #3 
On 25/10/2020 11:41, Philippe Mathieu-Daudé wrote:

> On 10/25/20 12:11 PM, Mark Cave-Ayland wrote:
>> On 21/10/2020 12:43, Mark Cave-Ayland wrote:
>>
>>> Store the child object directly within the sabre object rather than using
>>> link properties.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/pci-host/sabre.c         | 10 ++++------
>>>   hw/sparc64/sun4u.c          |  8 +-------
>>>   include/hw/pci-host/sabre.h |  2 +-
>>>   3 files changed, 6 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
>>> index f41a0cc301..aaa93acd6e 100644
>>> --- a/hw/pci-host/sabre.c
>>> +++ b/hw/pci-host/sabre.c
>>> @@ -383,9 +383,10 @@ static void sabre_realize(DeviceState *dev, Error **errp)
>>>       pci_create_simple(phb->bus, 0, TYPE_SABRE_PCI_DEVICE);
>>>       /* IOMMU */
>>> +    sysbus_realize(SYS_BUS_DEVICE(&s->iommu), &error_fatal);
>>>       memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
>>> -                    sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
>>> -    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
>>> +                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->iommu), 0), 1);
>>> +    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, &s->iommu);
>>>       /* APB secondary busses */
>>>       pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), true,
>>> @@ -422,10 +423,7 @@ static void sabre_init(Object *obj)
>>>       s->pci_irq_in = 0ULL;
>>>       /* IOMMU */
>>> -    object_property_add_link(obj, "iommu", TYPE_SUN4U_IOMMU,
>>> -                             (Object **) &s->iommu,
>>> -                             qdev_prop_allow_set_link_before_realize,
>>> -                             0);
>>> +    object_initialize_child(obj, "iommu", &s->iommu, TYPE_SUN4U_IOMMU);
>>>       /* sabre_config */
>>>       memory_region_init_io(&s->sabre_config, OBJECT(s), &sabre_config_ops, s,
>>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>>> index 2f8fc670cf..a33f1eccfd 100644
>>> --- a/hw/sparc64/sun4u.c
>>> +++ b/hw/sparc64/sun4u.c
>>> @@ -562,7 +562,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>>       PCIBus *pci_bus, *pci_busA, *pci_busB;
>>>       PCIDevice *ebus, *pci_dev;
>>>       SysBusDevice *s;
>>> -    DeviceState *iommu, *dev;
>>> +    DeviceState *dev;
>>>       FWCfgState *fw_cfg;
>>>       NICInfo *nd;
>>>       MACAddr macaddr;
>>> @@ -571,10 +571,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>>       /* init CPUs */
>>>       cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
>>> -    /* IOMMU */
>>> -    iommu = qdev_new(TYPE_SUN4U_IOMMU);
>>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(iommu), &error_fatal);
>>> -
>>>       /* set up devices */
>>>       ram_init(0, machine->ram_size);
>>> @@ -584,8 +580,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>>>       sabre = SABRE(qdev_new(TYPE_SABRE));
>>>       qdev_prop_set_uint64(DEVICE(sabre), "special-base", PBM_SPECIAL_BASE);
>>>       qdev_prop_set_uint64(DEVICE(sabre), "mem-base", PBM_MEM_BASE);
>>> -    object_property_set_link(OBJECT(sabre), "iommu", OBJECT(iommu),
>>> -                             &error_abort);
>>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(sabre), &error_fatal);
>>>       /* sabre_config */
>>> diff --git a/include/hw/pci-host/sabre.h b/include/hw/pci-host/sabre.h
>>> index 01190241bb..05bf741cde 100644
>>> --- a/include/hw/pci-host/sabre.h
>>> +++ b/include/hw/pci-host/sabre.h
>>> @@ -34,7 +34,7 @@ struct SabreState {
>>>       MemoryRegion pci_mmio;
>>>       MemoryRegion pci_ioport;
>>>       uint64_t pci_irq_in;
>>> -    IOMMUState *iommu;
>>> +    IOMMUState iommu;
>>>       PCIBridge *bridgeA;
>>>       PCIBridge *bridgeB;
>>>       uint32_t pci_control[16];
>>
>> No further comments (and I'm happier that this is a better solution than having an 
>> "optional" link property) so I've applied this to my qemu-sparc branch.
> 
> Sorry I had this patch tagged for review but am having trouble
> managing that folder. This is certainly better, thanks for this
> cleanup.
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Looks like this patch exposes another issue related to QOM lifecycle, so I'm dropping 
it from my qemu-sparc branch for now.


ATB,

Mark.
diff mbox series

Patch

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index f41a0cc301..aaa93acd6e 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -383,9 +383,10 @@  static void sabre_realize(DeviceState *dev, Error **errp)
     pci_create_simple(phb->bus, 0, TYPE_SABRE_PCI_DEVICE);
 
     /* IOMMU */
+    sysbus_realize(SYS_BUS_DEVICE(&s->iommu), &error_fatal);
     memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
-                    sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
-    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
+                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->iommu), 0), 1);
+    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, &s->iommu);
 
     /* APB secondary busses */
     pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), true,
@@ -422,10 +423,7 @@  static void sabre_init(Object *obj)
     s->pci_irq_in = 0ULL;
 
     /* IOMMU */
-    object_property_add_link(obj, "iommu", TYPE_SUN4U_IOMMU,
-                             (Object **) &s->iommu,
-                             qdev_prop_allow_set_link_before_realize,
-                             0);
+    object_initialize_child(obj, "iommu", &s->iommu, TYPE_SUN4U_IOMMU);
 
     /* sabre_config */
     memory_region_init_io(&s->sabre_config, OBJECT(s), &sabre_config_ops, s,
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 2f8fc670cf..a33f1eccfd 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -562,7 +562,7 @@  static void sun4uv_init(MemoryRegion *address_space_mem,
     PCIBus *pci_bus, *pci_busA, *pci_busB;
     PCIDevice *ebus, *pci_dev;
     SysBusDevice *s;
-    DeviceState *iommu, *dev;
+    DeviceState *dev;
     FWCfgState *fw_cfg;
     NICInfo *nd;
     MACAddr macaddr;
@@ -571,10 +571,6 @@  static void sun4uv_init(MemoryRegion *address_space_mem,
     /* init CPUs */
     cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
 
-    /* IOMMU */
-    iommu = qdev_new(TYPE_SUN4U_IOMMU);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(iommu), &error_fatal);
-
     /* set up devices */
     ram_init(0, machine->ram_size);
 
@@ -584,8 +580,6 @@  static void sun4uv_init(MemoryRegion *address_space_mem,
     sabre = SABRE(qdev_new(TYPE_SABRE));
     qdev_prop_set_uint64(DEVICE(sabre), "special-base", PBM_SPECIAL_BASE);
     qdev_prop_set_uint64(DEVICE(sabre), "mem-base", PBM_MEM_BASE);
-    object_property_set_link(OBJECT(sabre), "iommu", OBJECT(iommu),
-                             &error_abort);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(sabre), &error_fatal);
 
     /* sabre_config */
diff --git a/include/hw/pci-host/sabre.h b/include/hw/pci-host/sabre.h
index 01190241bb..05bf741cde 100644
--- a/include/hw/pci-host/sabre.h
+++ b/include/hw/pci-host/sabre.h
@@ -34,7 +34,7 @@  struct SabreState {
     MemoryRegion pci_mmio;
     MemoryRegion pci_ioport;
     uint64_t pci_irq_in;
-    IOMMUState *iommu;
+    IOMMUState iommu;
     PCIBridge *bridgeA;
     PCIBridge *bridgeB;
     uint32_t pci_control[16];