Re: [Qemu-devel] [PATCH 10/15] apb: remove pci_apb_init() and instantiate APB device using qdev

[Mark Cave-Ayland]

On 20/11/17 17:51, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 11/17/2017 10:42 AM, Mark Cave-Ayland wrote:
>> By making the special_base and mem_base values qdev properties, we can move
>> the remaining parts of pci_apb_init() into the pbm init() and realize()
>> functions.
>>
>> This finally allows us to instantiate the APB directly using standard qdev
>> create/init functions in sun4u.c.
>>
>> Signed-off-by: Mark Cave-Ayland 
>> ---
>>  hw/pci-host/apb.c |  123 
>> ++---
>>  hw/sparc64/sun4u.c|6 ++-
>>  include/hw/pci-host/apb.h |4 +-
>>  3 files changed, 68 insertions(+), 65 deletions(-)
>>
>> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
>> index 823661a..6c20285 100644
>> --- a/hw/pci-host/apb.c
>> +++ b/hw/pci-host/apb.c
>> @@ -611,41 +611,56 @@ static void apb_pci_bridge_realize(PCIDevice *dev, 
>> Error **errp)
>>  pci_bridge_update_mappings(PCI_BRIDGE(br));
>>  }
>>  
>> -APBState *pci_apb_init(hwaddr special_base,
>> -   hwaddr mem_base)
>> +static void pci_pbm_reset(DeviceState *d)
>>  {
>> -DeviceState *dev;
>> -SysBusDevice *s;
>> -PCIHostState *phb;
>> -APBState *d;
>> -IOMMUState *is;
>> +unsigned int i;
>> +APBState *s = APB_DEVICE(d);
>> +
>> +for (i = 0; i < 8; i++) {
>> +s->pci_irq_map[i] &= PBM_PCI_IMR_MASK;
>> +}
>> +for (i = 0; i < 32; i++) {
>> +s->obio_irq_map[i] &= PBM_PCI_IMR_MASK;
>> +}
>> +
>> +s->irq_request = NO_IRQ_REQUEST;
>> +s->pci_irq_in = 0ULL;
>> +
>> +if (s->nr_resets++ == 0) {
>> +/* Power on reset */
>> +s->reset_control = POR;
>> +}
>> +}
>> +
>> +static const MemoryRegionOps pci_config_ops = {
>> +.read = apb_pci_config_read,
>> +.write = apb_pci_config_write,
>> +.endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static void pci_pbm_realize(DeviceState *dev, Error **errp)
>> +{
>> +APBState *s = APB_DEVICE(dev);
>> +PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>> +SysBusDevice *sbd = SYS_BUS_DEVICE(s);
>>  PCIDevice *pci_dev;
>> +IOMMUState *is;
>>  
>> -/* Ultrasparc PBM main bus */
>> -dev = qdev_create(NULL, TYPE_APB);
>> -d = APB_DEVICE(dev);
>> -phb = PCI_HOST_BRIDGE(dev);
>> -phb->bus = pci_register_bus(DEVICE(phb), "pci",
>> -pci_apb_set_irq, pci_apb_map_irq, d,
>> ->pci_mmio,
>> ->pci_ioport,
>> -0, 32, TYPE_PCI_BUS);
>> -qdev_init_nofail(dev);
>> -s = SYS_BUS_DEVICE(dev);
>>  /* apb_config */
>> -sysbus_mmio_map(s, 0, special_base);
>> +sysbus_mmio_map(sbd, 0, s->special_base);
> 
> You might add a safety check than special_base is correctly initialize
> (compare to -1?)

I guess strictly speaking this would be good to have, however since the
same value is also hard-coded in OpenBIOS then you'll get a crash if you
change it to any other value anyhow. So for that reason I think it's
okay for the moment as it is.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 10/15] apb: remove pci_apb_init() and instantiate APB device using qdev

[Philippe Mathieu-Daudé]

Hi Mark,

On 11/17/2017 10:42 AM, Mark Cave-Ayland wrote:
> By making the special_base and mem_base values qdev properties, we can move
> the remaining parts of pci_apb_init() into the pbm init() and realize()
> functions.
> 
> This finally allows us to instantiate the APB directly using standard qdev
> create/init functions in sun4u.c.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/pci-host/apb.c |  123 
> ++---
>  hw/sparc64/sun4u.c|6 ++-
>  include/hw/pci-host/apb.h |4 +-
>  3 files changed, 68 insertions(+), 65 deletions(-)
> 
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 823661a..6c20285 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -611,41 +611,56 @@ static void apb_pci_bridge_realize(PCIDevice *dev, 
> Error **errp)
>  pci_bridge_update_mappings(PCI_BRIDGE(br));
>  }
>  
> -APBState *pci_apb_init(hwaddr special_base,
> -   hwaddr mem_base)
> +static void pci_pbm_reset(DeviceState *d)
>  {
> -DeviceState *dev;
> -SysBusDevice *s;
> -PCIHostState *phb;
> -APBState *d;
> -IOMMUState *is;
> +unsigned int i;
> +APBState *s = APB_DEVICE(d);
> +
> +for (i = 0; i < 8; i++) {
> +s->pci_irq_map[i] &= PBM_PCI_IMR_MASK;
> +}
> +for (i = 0; i < 32; i++) {
> +s->obio_irq_map[i] &= PBM_PCI_IMR_MASK;
> +}
> +
> +s->irq_request = NO_IRQ_REQUEST;
> +s->pci_irq_in = 0ULL;
> +
> +if (s->nr_resets++ == 0) {
> +/* Power on reset */
> +s->reset_control = POR;
> +}
> +}
> +
> +static const MemoryRegionOps pci_config_ops = {
> +.read = apb_pci_config_read,
> +.write = apb_pci_config_write,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void pci_pbm_realize(DeviceState *dev, Error **errp)
> +{
> +APBState *s = APB_DEVICE(dev);
> +PCIHostState *phb = PCI_HOST_BRIDGE(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(s);
>  PCIDevice *pci_dev;
> +IOMMUState *is;
>  
> -/* Ultrasparc PBM main bus */
> -dev = qdev_create(NULL, TYPE_APB);
> -d = APB_DEVICE(dev);
> -phb = PCI_HOST_BRIDGE(dev);
> -phb->bus = pci_register_bus(DEVICE(phb), "pci",
> -pci_apb_set_irq, pci_apb_map_irq, d,
> ->pci_mmio,
> ->pci_ioport,
> -0, 32, TYPE_PCI_BUS);
> -qdev_init_nofail(dev);
> -s = SYS_BUS_DEVICE(dev);
>  /* apb_config */
> -sysbus_mmio_map(s, 0, special_base);
> +sysbus_mmio_map(sbd, 0, s->special_base);

You might add a safety check than special_base is correctly initialize
(compare to -1?)

>  /* PCI configuration space */
> -sysbus_mmio_map(s, 1, special_base + 0x100ULL);
> +sysbus_mmio_map(sbd, 1, s->special_base + 0x100ULL);
>  /* pci_ioport */
> -sysbus_mmio_map(s, 2, special_base + 0x200ULL);
> +sysbus_mmio_map(sbd, 2, s->special_base + 0x200ULL);
>  
> -memory_region_init(>pci_mmio, OBJECT(s), "pci-mmio", 0x1ULL);
> -memory_region_add_subregion(get_system_memory(), mem_base, >pci_mmio);
> +memory_region_init(>pci_mmio, OBJECT(s), "pci-mmio", 0x1ULL);
> +memory_region_add_subregion(get_system_memory(), s->mem_base,

Ditto with mem_base

> +>pci_mmio);
>  
>  pci_create_simple(phb->bus, 0, "pbm-pci");
>  
>  /* APB IOMMU */
> -is = >iommu;
> +is = >iommu;
>  memset(is, 0, sizeof(IOMMUState));
>  
>  memory_region_init_iommu(>iommu, sizeof(is->iommu),
> @@ -657,52 +672,30 @@ APBState *pci_apb_init(hwaddr special_base,
>  /* APB secondary busses */
>  pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 0), true,
> TYPE_PBM_PCI_BRIDGE);
> -d->bridgeB = PCI_BRIDGE(pci_dev);
> -pci_bridge_map_irq(d->bridgeB, "pciB", pci_pbm_map_irq);
> +s->bridgeB = PCI_BRIDGE(pci_dev);
> +pci_bridge_map_irq(s->bridgeB, "pciB", pci_pbm_map_irq);
>  qdev_init_nofail(_dev->qdev);
>  
>  pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 1), true,
> TYPE_PBM_PCI_BRIDGE);
> -d->bridgeA = PCI_BRIDGE(pci_dev);
> -pci_bridge_map_irq(d->bridgeA, "pciA", pci_pbm_map_irq);
> +s->bridgeA = PCI_BRIDGE(pci_dev);
> +pci_bridge_map_irq(s->bridgeA, "pciA", pci_pbm_map_irq);
>  qdev_prop_set_bit(DEVICE(pci_dev), "busA", true);
>  qdev_init_nofail(_dev->qdev);
> -
> -return d;
>  }
>  
> -static void pci_pbm_reset(DeviceState *d)
> +static void pci_pbm_init(Object *obj)
>  {
> +APBState *s = APB_DEVICE(obj);
> +PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  unsigned int i;
> -APBState *s = APB_DEVICE(d);
> -
> -for (i = 0; i < 8; i++) {
> -s->pci_irq_map[i] &= PBM_PCI_IMR_MASK;
> -}
> -  

Re: [Qemu-devel] [PATCH 10/15] apb: remove pci_apb_init() and instantiate APB device using qdev

[Artyom Tarasenko]

On Fri, Nov 17, 2017 at 2:42 PM, Mark Cave-Ayland
 wrote:
> By making the special_base and mem_base values qdev properties, we can move
> the remaining parts of pci_apb_init() into the pbm init() and realize()
> functions.
>
> This finally allows us to instantiate the APB directly using standard qdev
> create/init functions in sun4u.c.

Nice!
>
> Signed-off-by: Mark Cave-Ayland 

Reviewed-by: Artyom Tarasenko 

> ---
>  hw/pci-host/apb.c |  123 
> ++---
>  hw/sparc64/sun4u.c|6 ++-
>  include/hw/pci-host/apb.h |4 +-
>  3 files changed, 68 insertions(+), 65 deletions(-)
>
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 823661a..6c20285 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -611,41 +611,56 @@ static void apb_pci_bridge_realize(PCIDevice *dev, 
> Error **errp)
>  pci_bridge_update_mappings(PCI_BRIDGE(br));
>  }
>
> -APBState *pci_apb_init(hwaddr special_base,
> -   hwaddr mem_base)
> +static void pci_pbm_reset(DeviceState *d)
>  {
> -DeviceState *dev;
> -SysBusDevice *s;
> -PCIHostState *phb;
> -APBState *d;
> -IOMMUState *is;
> +unsigned int i;
> +APBState *s = APB_DEVICE(d);
> +
> +for (i = 0; i < 8; i++) {
> +s->pci_irq_map[i] &= PBM_PCI_IMR_MASK;
> +}
> +for (i = 0; i < 32; i++) {
> +s->obio_irq_map[i] &= PBM_PCI_IMR_MASK;
> +}
> +
> +s->irq_request = NO_IRQ_REQUEST;
> +s->pci_irq_in = 0ULL;
> +
> +if (s->nr_resets++ == 0) {
> +/* Power on reset */
> +s->reset_control = POR;
> +}
> +}
> +
> +static const MemoryRegionOps pci_config_ops = {
> +.read = apb_pci_config_read,
> +.write = apb_pci_config_write,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void pci_pbm_realize(DeviceState *dev, Error **errp)
> +{
> +APBState *s = APB_DEVICE(dev);
> +PCIHostState *phb = PCI_HOST_BRIDGE(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(s);
>  PCIDevice *pci_dev;
> +IOMMUState *is;
>
> -/* Ultrasparc PBM main bus */
> -dev = qdev_create(NULL, TYPE_APB);
> -d = APB_DEVICE(dev);
> -phb = PCI_HOST_BRIDGE(dev);
> -phb->bus = pci_register_bus(DEVICE(phb), "pci",
> -pci_apb_set_irq, pci_apb_map_irq, d,
> ->pci_mmio,
> ->pci_ioport,
> -0, 32, TYPE_PCI_BUS);
> -qdev_init_nofail(dev);
> -s = SYS_BUS_DEVICE(dev);
>  /* apb_config */
> -sysbus_mmio_map(s, 0, special_base);
> +sysbus_mmio_map(sbd, 0, s->special_base);
>  /* PCI configuration space */
> -sysbus_mmio_map(s, 1, special_base + 0x100ULL);
> +sysbus_mmio_map(sbd, 1, s->special_base + 0x100ULL);
>  /* pci_ioport */
> -sysbus_mmio_map(s, 2, special_base + 0x200ULL);
> +sysbus_mmio_map(sbd, 2, s->special_base + 0x200ULL);
>
> -memory_region_init(>pci_mmio, OBJECT(s), "pci-mmio", 0x1ULL);
> -memory_region_add_subregion(get_system_memory(), mem_base, >pci_mmio);
> +memory_region_init(>pci_mmio, OBJECT(s), "pci-mmio", 0x1ULL);
> +memory_region_add_subregion(get_system_memory(), s->mem_base,
> +>pci_mmio);
>
>  pci_create_simple(phb->bus, 0, "pbm-pci");
>
>  /* APB IOMMU */
> -is = >iommu;
> +is = >iommu;
>  memset(is, 0, sizeof(IOMMUState));
>
>  memory_region_init_iommu(>iommu, sizeof(is->iommu),
> @@ -657,52 +672,30 @@ APBState *pci_apb_init(hwaddr special_base,
>  /* APB secondary busses */
>  pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 0), true,
> TYPE_PBM_PCI_BRIDGE);
> -d->bridgeB = PCI_BRIDGE(pci_dev);
> -pci_bridge_map_irq(d->bridgeB, "pciB", pci_pbm_map_irq);
> +s->bridgeB = PCI_BRIDGE(pci_dev);
> +pci_bridge_map_irq(s->bridgeB, "pciB", pci_pbm_map_irq);
>  qdev_init_nofail(_dev->qdev);
>
>  pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 1), true,
> TYPE_PBM_PCI_BRIDGE);
> -d->bridgeA = PCI_BRIDGE(pci_dev);
> -pci_bridge_map_irq(d->bridgeA, "pciA", pci_pbm_map_irq);
> +s->bridgeA = PCI_BRIDGE(pci_dev);
> +pci_bridge_map_irq(s->bridgeA, "pciA", pci_pbm_map_irq);
>  qdev_prop_set_bit(DEVICE(pci_dev), "busA", true);
>  qdev_init_nofail(_dev->qdev);
> -
> -return d;
>  }
>
> -static void pci_pbm_reset(DeviceState *d)
> +static void pci_pbm_init(Object *obj)
>  {
> +APBState *s = APB_DEVICE(obj);
> +PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  unsigned int i;
> -APBState *s = APB_DEVICE(d);
> -
> -for (i = 0; i < 8; i++) {
> -s->pci_irq_map[i] &= PBM_PCI_IMR_MASK;
> -}
> -for (i = 0; i < 32; i++) {
> -   

[Qemu-devel] [PATCH 10/15] apb: remove pci_apb_init() and instantiate APB device using qdev

[Mark Cave-Ayland]

By making the special_base and mem_base values qdev properties, we can move
the remaining parts of pci_apb_init() into the pbm init() and realize()
functions.

This finally allows us to instantiate the APB directly using standard qdev
create/init functions in sun4u.c.

Signed-off-by: Mark Cave-Ayland 
---
 hw/pci-host/apb.c |  123 ++---
 hw/sparc64/sun4u.c|6 ++-
 include/hw/pci-host/apb.h |4 +-
 3 files changed, 68 insertions(+), 65 deletions(-)

diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 823661a..6c20285 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -611,41 +611,56 @@ static void apb_pci_bridge_realize(PCIDevice *dev, Error 
**errp)
 pci_bridge_update_mappings(PCI_BRIDGE(br));
 }
 
-APBState *pci_apb_init(hwaddr special_base,
-   hwaddr mem_base)
+static void pci_pbm_reset(DeviceState *d)
 {
-DeviceState *dev;
-SysBusDevice *s;
-PCIHostState *phb;
-APBState *d;
-IOMMUState *is;
+unsigned int i;
+APBState *s = APB_DEVICE(d);
+
+for (i = 0; i < 8; i++) {
+s->pci_irq_map[i] &= PBM_PCI_IMR_MASK;
+}
+for (i = 0; i < 32; i++) {
+s->obio_irq_map[i] &= PBM_PCI_IMR_MASK;
+}
+
+s->irq_request = NO_IRQ_REQUEST;
+s->pci_irq_in = 0ULL;
+
+if (s->nr_resets++ == 0) {
+/* Power on reset */
+s->reset_control = POR;
+}
+}
+
+static const MemoryRegionOps pci_config_ops = {
+.read = apb_pci_config_read,
+.write = apb_pci_config_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void pci_pbm_realize(DeviceState *dev, Error **errp)
+{
+APBState *s = APB_DEVICE(dev);
+PCIHostState *phb = PCI_HOST_BRIDGE(dev);
+SysBusDevice *sbd = SYS_BUS_DEVICE(s);
 PCIDevice *pci_dev;
+IOMMUState *is;
 
-/* Ultrasparc PBM main bus */
-dev = qdev_create(NULL, TYPE_APB);
-d = APB_DEVICE(dev);
-phb = PCI_HOST_BRIDGE(dev);
-phb->bus = pci_register_bus(DEVICE(phb), "pci",
-pci_apb_set_irq, pci_apb_map_irq, d,
->pci_mmio,
->pci_ioport,
-0, 32, TYPE_PCI_BUS);
-qdev_init_nofail(dev);
-s = SYS_BUS_DEVICE(dev);
 /* apb_config */
-sysbus_mmio_map(s, 0, special_base);
+sysbus_mmio_map(sbd, 0, s->special_base);
 /* PCI configuration space */
-sysbus_mmio_map(s, 1, special_base + 0x100ULL);
+sysbus_mmio_map(sbd, 1, s->special_base + 0x100ULL);
 /* pci_ioport */
-sysbus_mmio_map(s, 2, special_base + 0x200ULL);
+sysbus_mmio_map(sbd, 2, s->special_base + 0x200ULL);
 
-memory_region_init(>pci_mmio, OBJECT(s), "pci-mmio", 0x1ULL);
-memory_region_add_subregion(get_system_memory(), mem_base, >pci_mmio);
+memory_region_init(>pci_mmio, OBJECT(s), "pci-mmio", 0x1ULL);
+memory_region_add_subregion(get_system_memory(), s->mem_base,
+>pci_mmio);
 
 pci_create_simple(phb->bus, 0, "pbm-pci");
 
 /* APB IOMMU */
-is = >iommu;
+is = >iommu;
 memset(is, 0, sizeof(IOMMUState));
 
 memory_region_init_iommu(>iommu, sizeof(is->iommu),
@@ -657,52 +672,30 @@ APBState *pci_apb_init(hwaddr special_base,
 /* APB secondary busses */
 pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 0), true,
TYPE_PBM_PCI_BRIDGE);
-d->bridgeB = PCI_BRIDGE(pci_dev);
-pci_bridge_map_irq(d->bridgeB, "pciB", pci_pbm_map_irq);
+s->bridgeB = PCI_BRIDGE(pci_dev);
+pci_bridge_map_irq(s->bridgeB, "pciB", pci_pbm_map_irq);
 qdev_init_nofail(_dev->qdev);
 
 pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 1), true,
TYPE_PBM_PCI_BRIDGE);
-d->bridgeA = PCI_BRIDGE(pci_dev);
-pci_bridge_map_irq(d->bridgeA, "pciA", pci_pbm_map_irq);
+s->bridgeA = PCI_BRIDGE(pci_dev);
+pci_bridge_map_irq(s->bridgeA, "pciA", pci_pbm_map_irq);
 qdev_prop_set_bit(DEVICE(pci_dev), "busA", true);
 qdev_init_nofail(_dev->qdev);
-
-return d;
 }
 
-static void pci_pbm_reset(DeviceState *d)
+static void pci_pbm_init(Object *obj)
 {
+APBState *s = APB_DEVICE(obj);
+PCIHostState *phb = PCI_HOST_BRIDGE(obj);
+SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 unsigned int i;
-APBState *s = APB_DEVICE(d);
-
-for (i = 0; i < 8; i++) {
-s->pci_irq_map[i] &= PBM_PCI_IMR_MASK;
-}
-for (i = 0; i < 32; i++) {
-s->obio_irq_map[i] &= PBM_PCI_IMR_MASK;
-}
-
-s->irq_request = NO_IRQ_REQUEST;
-s->pci_irq_in = 0ULL;
-
-if (s->nr_resets++ == 0) {
-/* Power on reset */
-s->reset_control = POR;
-}
-}
 
-static const MemoryRegionOps pci_config_ops = {
-.read = apb_pci_config_read,
-.write = apb_pci_config_write,
-.endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-static int