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];