[Qemu-devel] [PATCH 0/4] spapr-pci: prepare for vfio

[Qemu-devel] [PATCH 0/4] spapr-pci: prepare for vfio

[Alexey Kardashevskiy]

Here are few reworks for spapr-pci PHB which I'd like to have to support VFIO.
QOM, errors printing, traces, nothing really serious. Thanks!

Alexey Kardashevskiy (4):
  spapr-pci: convert init() callback to realize()
  spapr-pci: introduce a finish_realize() callback
  spapr-pci: add spapr_pci trace
  spapr-pci: converts fprintf to error_report

 hw/ppc/spapr_pci.c          | 90 ++++++++++++++++++++++++++-------------------
 include/hw/pci-host/spapr.h | 18 ++++++++-
 trace-events                |  1 +
 3 files changed, 69 insertions(+), 40 deletions(-)

-- 
1.8.4.rc4

[Qemu-devel] [PATCH 1/4] spapr-pci: convert init() callback to realize()

[Alexey Kardashevskiy]

This converts the old-style init() callback to a new style realize()
callback as init() now is supposed to do only trivial initialization.

As a part of convertion, this replaces fprintf(stderr) with error_setg()
as realize() does not "return" any value, instead it puts the extended
error into **errp.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v5:
* finish_finalize() moved to a separate patch
---
 hw/ppc/spapr_pci.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7763149..aeb012d 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -32,6 +32,7 @@
 #include "exec/address-spaces.h"
 #include <libfdt.h>
 #include "trace.h"
+#include "qemu/error-report.h"
 
 #include "hw/pci/pci_bus.h"
 
@@ -494,9 +495,9 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &phb->iommu_as;
 }
 
-static int spapr_phb_init(SysBusDevice *s)
+static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
-    DeviceState *dev = DEVICE(s);
+    SysBusDevice *s = SYS_BUS_DEVICE(dev);
     sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
     const char *busname;
@@ -510,9 +511,9 @@ static int spapr_phb_init(SysBusDevice *s)
         if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
             || (sphb->mem_win_addr != -1)
             || (sphb->io_win_addr != -1)) {
-            fprintf(stderr, "Either \"index\" or other parameters must"
-                    " be specified for PAPR PHB, not both\n");
-            return -1;
+            error_setg(errp, "Either \"index\" or other parameters must"
+                       " be specified for PAPR PHB, not both");
+            return;
         }
 
         sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
@@ -525,28 +526,28 @@ static int spapr_phb_init(SysBusDevice *s)
     }
 
     if (sphb->buid == -1) {
-        fprintf(stderr, "BUID not specified for PHB\n");
-        return -1;
+        error_setg(errp, "BUID not specified for PHB");
+        return;
     }
 
     if (sphb->dma_liobn == -1) {
-        fprintf(stderr, "LIOBN not specified for PHB\n");
-        return -1;
+        error_setg(errp, "LIOBN not specified for PHB");
+        return;
     }
 
     if (sphb->mem_win_addr == -1) {
-        fprintf(stderr, "Memory window address not specified for PHB\n");
-        return -1;
+        error_setg(errp, "Memory window address not specified for PHB");
+        return;
     }
 
     if (sphb->io_win_addr == -1) {
-        fprintf(stderr, "IO window address not specified for PHB\n");
-        return -1;
+        error_setg(errp, "IO window address not specified for PHB");
+        return;
     }
 
     if (find_phb(spapr, sphb->buid)) {
-        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
-        return -1;
+        error_setg(errp, "PCI host bridges must have unique BUIDs");
+        return;
     }
 
     sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
@@ -613,8 +614,9 @@ static int spapr_phb_init(SysBusDevice *s)
     sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
                                      sphb->dma_window_size);
     if (!sphb->tcet) {
-        fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
-        return -1;
+        error_setg(errp, "Unable to create TCE table for %s",
+                   sphb->dtbusname);
+        return;
     }
     address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
                        sphb->dtbusname);
@@ -631,13 +633,12 @@ static int spapr_phb_init(SysBusDevice *s)
 
         irq = spapr_allocate_lsi(0);
         if (!irq) {
-            return -1;
+            error_setg(errp, "spapr_allocate_lsi failed");
+            return;
         }
 
         sphb->lsi_table[i].irq = irq;
     }
-
-    return 0;
 }
 
 static void spapr_phb_reset(DeviceState *qdev)
@@ -720,11 +721,10 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
 static void spapr_phb_class_init(ObjectClass *klass, void *data)
 {
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
-    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     hc->root_bus_path = spapr_phb_root_bus_path;
-    sdc->init = spapr_phb_init;
+    dc->realize = spapr_phb_realize;
     dc->props = spapr_phb_properties;
     dc->reset = spapr_phb_reset;
     dc->vmsd = &vmstate_spapr_pci;
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 2/4] spapr-pci: introduce a finish_realize() callback

[Alexey Kardashevskiy]

The spapr-pci PHB initializes IOMMU for emulataed devices only.
The upcoming VFIO support will do it different. However both emulated
and VFIO PHB types share most of the initialization code.
For the type specific things a new finish_realize() callback is
introduced.

This introduces sPAPRPHBClass derived from PCIHostBridgeClass and
adds the callback pointer.

This implements finish_realize() for emulated devices.

This changes initialization steps order to have the finish_realize()
call at the end of the spapr_finalize().

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v5:
* this is a new patch in the series, it was a part of a previous patch
---
 hw/ppc/spapr_pci.c          | 46 +++++++++++++++++++++++++++++----------------
 include/hw/pci-host/spapr.h | 18 ++++++++++++++++--
 2 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index aeb012d..963841c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -500,6 +500,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
+    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s);
     const char *busname;
     char *namebuf;
     int i;
@@ -609,22 +610,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
                            PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
     phb->bus = bus;
 
-    sphb->dma_window_start = 0;
-    sphb->dma_window_size = 0x40000000;
-    sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
-                                     sphb->dma_window_size);
-    if (!sphb->tcet) {
-        error_setg(errp, "Unable to create TCE table for %s",
-                   sphb->dtbusname);
-        return;
-    }
-    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
-                       sphb->dtbusname);
-
-    pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
-
-    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
-
     QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
 
     /* Initialize the LSI table */
@@ -639,6 +624,32 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
 
         sphb->lsi_table[i].irq = irq;
     }
+
+    pci_setup_iommu(sphb->parent_obj.bus, spapr_pci_dma_iommu, sphb);
+
+    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
+
+    if (!info->finish_realize) {
+        error_setg(errp, "finish_realize not defined");
+        return;
+    }
+
+    info->finish_realize(sphb, errp);
+}
+
+static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
+{
+    sphb->dma_window_start = 0;
+    sphb->dma_window_size = 0x40000000;
+    sphb->tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
+                                     sphb->dma_window_size);
+    if (!sphb->tcet) {
+        error_setg(errp, "Unable to create TCE table for %s",
+                   sphb->dtbusname);
+        return ;
+    }
+    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
+                       sphb->dtbusname);
 }
 
 static void spapr_phb_reset(DeviceState *qdev)
@@ -722,12 +733,14 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
 {
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
+    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
 
     hc->root_bus_path = spapr_phb_root_bus_path;
     dc->realize = spapr_phb_realize;
     dc->props = spapr_phb_properties;
     dc->reset = spapr_phb_reset;
     dc->vmsd = &vmstate_spapr_pci;
+    spc->finish_realize = spapr_phb_finish_realize;
 }
 
 static const TypeInfo spapr_phb_info = {
@@ -735,6 +748,7 @@ static const TypeInfo spapr_phb_info = {
     .parent        = TYPE_PCI_HOST_BRIDGE,
     .instance_size = sizeof(sPAPRPHBState),
     .class_init    = spapr_phb_class_init,
+    .class_size    = sizeof(sPAPRPHBClass),
 };
 
 PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 970b4a9..0f428a1 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -34,7 +34,21 @@
 #define SPAPR_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
 
-typedef struct sPAPRPHBState {
+#define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
+#define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(sPAPRPHBClass, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
+
+typedef struct sPAPRPHBClass sPAPRPHBClass;
+typedef struct sPAPRPHBState sPAPRPHBState;
+
+struct sPAPRPHBClass {
+    PCIHostBridgeClass parent_class;
+
+    void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
+};
+
+struct sPAPRPHBState {
     PCIHostState parent_obj;
 
     int32_t index;
@@ -62,7 +76,7 @@ typedef struct sPAPRPHBState {
     } msi_table[SPAPR_MSIX_MAX_DEVS];
 
     QLIST_ENTRY(sPAPRPHBState) list;
-} sPAPRPHBState;
+};
 
 #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
 
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 3/4] spapr-pci: add spapr_pci trace

[Alexey Kardashevskiy]

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 trace-events | 1 +
 1 file changed, 1 insertion(+)

diff --git a/trace-events b/trace-events
index 8695e9e..ba5f76c 100644
--- a/trace-events
+++ b/trace-events
@@ -1114,6 +1114,7 @@ qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride,
 qxl_render_update_area_done(void *cookie) "%p"
 
 # hw/ppc/spapr_pci.c
+spapr_pci(const char *msg1, const char *msg2) "%s%s"
 spapr_pci_msi(const char *msg, uint32_t n, uint32_t ca) "%s (device#%d, cfg=%x)"
 spapr_pci_msi_setup(const char *name, unsigned vector, uint64_t addr) "dev\"%s\" vector %u, addr=%"PRIx64
 spapr_pci_rtas_ibm_change_msi(unsigned func, unsigned req) "func %u, requested %u"
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 4/4] spapr-pci: converts fprintf to error_report

[Alexey Kardashevskiy]

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 963841c..d102d82 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -293,7 +293,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
         ret_intr_type = RTAS_TYPE_MSIX;
         break;
     default:
-        fprintf(stderr, "rtas_ibm_change_msi(%u) is not implemented\n", func);
+        error_report("rtas_ibm_change_msi(%u) is not implemented", func);
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
@@ -327,7 +327,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     /* Find a device number in the map to add or reuse the existing one */
     ndev = spapr_msicfg_find(phb, config_addr, true);
     if (ndev >= SPAPR_MSIX_MAX_DEVS || ndev < 0) {
-        fprintf(stderr, "No free entry for a new MSI device\n");
+        error_report("No free entry for a new MSI device");
         rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
         return;
     }
@@ -336,7 +336,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     /* Check if there is an old config and MSI number has not changed */
     if (phb->msi_table[ndev].nvec && (req_num != phb->msi_table[ndev].nvec)) {
         /* Unexpected behaviour */
-        fprintf(stderr, "Cannot reuse MSI config for device#%d", ndev);
+        error_report("Cannot reuse MSI config for device#%d", ndev);
         rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
         return;
     }
@@ -346,7 +346,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
         irq = spapr_allocate_irq_block(req_num, false,
                                        ret_intr_type == RTAS_TYPE_MSI);
         if (irq < 0) {
-            fprintf(stderr, "Cannot allocate MSIs for device#%d", ndev);
+            error_report("Cannot allocate MSIs for device#%d", ndev);
             rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
             return;
         }
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH 0/4] spapr-pci: prepare for vfio

[Alexey Kardashevskiy]

On 11/21/2013 03:08 PM, Alexey Kardashevskiy wrote:
> Here are few reworks for spapr-pci PHB which I'd like to have to support VFIO.
> QOM, errors printing, traces, nothing really serious. Thanks!
> 
> Alexey Kardashevskiy (4):
>   spapr-pci: convert init() callback to realize()
>   spapr-pci: introduce a finish_realize() callback
>   spapr-pci: add spapr_pci trace
>   spapr-pci: converts fprintf to error_report
> 
>  hw/ppc/spapr_pci.c          | 90 ++++++++++++++++++++++++++-------------------
>  include/hw/pci-host/spapr.h | 18 ++++++++-
>  trace-events                |  1 +
>  3 files changed, 69 insertions(+), 40 deletions(-)


Ping?



-- 
Alexey

Re: [Qemu-devel] [PATCH 0/4] spapr-pci: prepare for vfio

[Alexey Kardashevskiy]

On 12/05/2013 08:39 PM, Alexey Kardashevskiy wrote:
> On 11/21/2013 03:08 PM, Alexey Kardashevskiy wrote:
>> Here are few reworks for spapr-pci PHB which I'd like to have to support VFIO.
>> QOM, errors printing, traces, nothing really serious. Thanks!
>>
>> Alexey Kardashevskiy (4):
>>   spapr-pci: convert init() callback to realize()
>>   spapr-pci: introduce a finish_realize() callback
>>   spapr-pci: add spapr_pci trace
>>   spapr-pci: converts fprintf to error_report
>>
>>  hw/ppc/spapr_pci.c          | 90 ++++++++++++++++++++++++++-------------------
>>  include/hw/pci-host/spapr.h | 18 ++++++++-
>>  trace-events                |  1 +
>>  3 files changed, 69 insertions(+), 40 deletions(-)
> 
> 
> Ping?

Ping?


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/4] spapr-pci: prepare for vfio

[Alexey Kardashevskiy]

On 12/20/2013 01:47 PM, Alexey Kardashevskiy wrote:
> On 12/05/2013 08:39 PM, Alexey Kardashevskiy wrote:
>> On 11/21/2013 03:08 PM, Alexey Kardashevskiy wrote:
>>> Here are few reworks for spapr-pci PHB which I'd like to have to support VFIO.
>>> QOM, errors printing, traces, nothing really serious. Thanks!
>>>
>>> Alexey Kardashevskiy (4):
>>>   spapr-pci: convert init() callback to realize()
>>>   spapr-pci: introduce a finish_realize() callback
>>>   spapr-pci: add spapr_pci trace
>>>   spapr-pci: converts fprintf to error_report
>>>
>>>  hw/ppc/spapr_pci.c          | 90 ++++++++++++++++++++++++++-------------------
>>>  include/hw/pci-host/spapr.h | 18 ++++++++-
>>>  trace-events                |  1 +
>>>  3 files changed, 69 insertions(+), 40 deletions(-)
>>
>>
>> Ping?
> 
> Ping?

Ping?


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/4] spapr-pci: prepare for vfio

[Alexey Kardashevskiy]

On 01/15/2014 06:10 PM, Alexey Kardashevskiy wrote:
> On 12/20/2013 01:47 PM, Alexey Kardashevskiy wrote:
>> On 12/05/2013 08:39 PM, Alexey Kardashevskiy wrote:
>>> On 11/21/2013 03:08 PM, Alexey Kardashevskiy wrote:
>>>> Here are few reworks for spapr-pci PHB which I'd like to have to support VFIO.
>>>> QOM, errors printing, traces, nothing really serious. Thanks!
>>>>
>>>> Alexey Kardashevskiy (4):
>>>>   spapr-pci: convert init() callback to realize()
>>>>   spapr-pci: introduce a finish_realize() callback
>>>>   spapr-pci: add spapr_pci trace
>>>>   spapr-pci: converts fprintf to error_report
>>>>
>>>>  hw/ppc/spapr_pci.c          | 90 ++++++++++++++++++++++++++-------------------
>>>>  include/hw/pci-host/spapr.h | 18 ++++++++-
>>>>  trace-events                |  1 +
>>>>  3 files changed, 69 insertions(+), 40 deletions(-)
>>>
>>>
>>> Ping?
>>
>> Ping?
> 
> Ping?

Alex?



-- 
Alexey

Re: [Qemu-devel] [PATCH 0/4] spapr-pci: prepare for vfio

[Alexey Kardashevskiy]

On 02/12/2014 10:44 PM, Alexey Kardashevskiy wrote:
> On 01/15/2014 06:10 PM, Alexey Kardashevskiy wrote:
>> On 12/20/2013 01:47 PM, Alexey Kardashevskiy wrote:
>>> On 12/05/2013 08:39 PM, Alexey Kardashevskiy wrote:
>>>> On 11/21/2013 03:08 PM, Alexey Kardashevskiy wrote:
>>>>> Here are few reworks for spapr-pci PHB which I'd like to have to support VFIO.
>>>>> QOM, errors printing, traces, nothing really serious. Thanks!
>>>>>
>>>>> Alexey Kardashevskiy (4):
>>>>>   spapr-pci: convert init() callback to realize()
>>>>>   spapr-pci: introduce a finish_realize() callback
>>>>>   spapr-pci: add spapr_pci trace
>>>>>   spapr-pci: converts fprintf to error_report
>>>>>
>>>>>  hw/ppc/spapr_pci.c          | 90 ++++++++++++++++++++++++++-------------------
>>>>>  include/hw/pci-host/spapr.h | 18 ++++++++-
>>>>>  trace-events                |  1 +
>>>>>  3 files changed, 69 insertions(+), 40 deletions(-)
>>>>
>>>>
>>>> Ping?
>>>
>>> Ping?
>>
>> Ping?
> 
> Alex?

I do realize Alex is in vacation and when he is not, he is busy and
everything like that but this set is really simple and it is blocking me
from posting other VFIO bits so if anyone could find a minute and have a
look, that would be great. Thanks!



-- 
Alexey

Re: [Qemu-devel] spapr-pci: convert init() callback to realize()

[Mike Day]

On 21/11/13 15:08 +1100, Alexey Kardashevskiy wrote:
> This converts the old-style init() callback to a new style realize()
> callback as init() now is supposed to do only trivial initialization.
> 
> As a part of convertion, this replaces fprintf(stderr) with error_setg()
> as realize() does not "return" any value, instead it puts the extended
> error into **errp.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Mike Day <ncmike@ncultra.org>

> ---
> Changes:
> v5:
> * finish_finalize() moved to a separate patch
> ---
>  hw/ppc/spapr_pci.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7763149..aeb012d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -32,6 +32,7 @@
>  #include "exec/address-spaces.h"
>  #include <libfdt.h>
>  #include "trace.h"
> +#include "qemu/error-report.h"
>  
>  #include "hw/pci/pci_bus.h"
>  
> @@ -494,9 +495,9 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>      return &phb->iommu_as;
>  }
>  
> -static int spapr_phb_init(SysBusDevice *s)
> +static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
> -    DeviceState *dev = DEVICE(s);
> +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
>      const char *busname;
> @@ -510,9 +511,9 @@ static int spapr_phb_init(SysBusDevice *s)
>          if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
>              || (sphb->mem_win_addr != -1)
>              || (sphb->io_win_addr != -1)) {
> -            fprintf(stderr, "Either \"index\" or other parameters must"
> -                    " be specified for PAPR PHB, not both\n");
> -            return -1;
> +            error_setg(errp, "Either \"index\" or other parameters must"
> +                       " be specified for PAPR PHB, not both");
> +            return;
 
shouldn't these return an int? Or if the error is returned by pointer
perhaps the function should have a void return?

>          }
>  
>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
> @@ -525,28 +526,28 @@ static int spapr_phb_init(SysBusDevice *s)
>      }
>  
>      if (sphb->buid == -1) {
> -        fprintf(stderr, "BUID not specified for PHB\n");
> -        return -1;
> +        error_setg(errp, "BUID not specified for PHB");
> +        return;
>      }
>  
>      if (sphb->dma_liobn == -1) {
> -        fprintf(stderr, "LIOBN not specified for PHB\n");
> -        return -1;
> +        error_setg(errp, "LIOBN not specified for PHB");
> +        return;
>      }
>  
>      if (sphb->mem_win_addr == -1) {
> -        fprintf(stderr, "Memory window address not specified for PHB\n");
> -        return -1;
> +        error_setg(errp, "Memory window address not specified for PHB");
> +        return;
>      }
>  
>      if (sphb->io_win_addr == -1) {
> -        fprintf(stderr, "IO window address not specified for PHB\n");
> -        return -1;
> +        error_setg(errp, "IO window address not specified for PHB");
> +        return;
>      }
>  
>      if (find_phb(spapr, sphb->buid)) {
> -        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
> -        return -1;
> +        error_setg(errp, "PCI host bridges must have unique BUIDs");
> +        return;
>      }
>  
>      sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
> @@ -613,8 +614,9 @@ static int spapr_phb_init(SysBusDevice *s)
>      sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
>                                       sphb->dma_window_size);
>      if (!sphb->tcet) {
> -        fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
> -        return -1;
> +        error_setg(errp, "Unable to create TCE table for %s",
> +                   sphb->dtbusname);
> +        return;
>      }
>      address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
>                         sphb->dtbusname);
> @@ -631,13 +633,12 @@ static int spapr_phb_init(SysBusDevice *s)
>  
>          irq = spapr_allocate_lsi(0);
>          if (!irq) {
> -            return -1;
> +            error_setg(errp, "spapr_allocate_lsi failed");
> +            return;
>          }
>  
>          sphb->lsi_table[i].irq = irq;
>      }
> -
> -    return 0;
>  }
>  
>  static void spapr_phb_reset(DeviceState *qdev)
> @@ -720,11 +721,10 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
>  static void spapr_phb_class_init(ObjectClass *klass, void *data)
>  {
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> -    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      hc->root_bus_path = spapr_phb_root_bus_path;
> -    sdc->init = spapr_phb_init;
> +    dc->realize = spapr_phb_realize;
>      dc->props = spapr_phb_properties;
>      dc->reset = spapr_phb_reset;
>      dc->vmsd = &vmstate_spapr_pci;
> -- 
> 1.8.4.rc4
> 
> 
> 
> 

Re: [Qemu-devel] spapr-pci: convert init() callback to realize()

[Mike Day]

On 21/11/13 15:08 +1100, Alexey Kardashevskiy wrote:
> This converts the old-style init() callback to a new style realize()
> callback as init() now is supposed to do only trivial initialization.
> 
> As a part of convertion, this replaces fprintf(stderr) with error_setg()
> as realize() does not "return" any value, instead it puts the extended
> error into **errp.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Mike Day <ncmike@ncultra.org>

> ---
> Changes:
> v5:
> * finish_finalize() moved to a separate patch
> ---
>  hw/ppc/spapr_pci.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7763149..aeb012d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -32,6 +32,7 @@
>  #include "exec/address-spaces.h"
>  #include <libfdt.h>
>  #include "trace.h"
> +#include "qemu/error-report.h"
>  
>  #include "hw/pci/pci_bus.h"
>  
> @@ -494,9 +495,9 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>      return &phb->iommu_as;
>  }
>  
> -static int spapr_phb_init(SysBusDevice *s)
> +static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
> -    DeviceState *dev = DEVICE(s);
> +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
>      const char *busname;
> @@ -510,9 +511,9 @@ static int spapr_phb_init(SysBusDevice *s)
>          if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
>              || (sphb->mem_win_addr != -1)
>              || (sphb->io_win_addr != -1)) {
> -            fprintf(stderr, "Either \"index\" or other parameters must"
> -                    " be specified for PAPR PHB, not both\n");
> -            return -1;
> +            error_setg(errp, "Either \"index\" or other parameters must"
> +                       " be specified for PAPR PHB, not both");
> +            return;
>          }

Seems like these exit clauses should return an integer here and below.
  
>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
> @@ -525,28 +526,28 @@ static int spapr_phb_init(SysBusDevice *s)
>      }
>  
>      if (sphb->buid == -1) {
> -        fprintf(stderr, "BUID not specified for PHB\n");
> -        return -1;
> +        error_setg(errp, "BUID not specified for PHB");
> +        return;
>      }
>  
>      if (sphb->dma_liobn == -1) {
> -        fprintf(stderr, "LIOBN not specified for PHB\n");
> -        return -1;
> +        error_setg(errp, "LIOBN not specified for PHB");
> +        return;
>      }
>  
>      if (sphb->mem_win_addr == -1) {
> -        fprintf(stderr, "Memory window address not specified for PHB\n");
> -        return -1;
> +        error_setg(errp, "Memory window address not specified for PHB");
> +        return;
>      }
>  
>      if (sphb->io_win_addr == -1) {
> -        fprintf(stderr, "IO window address not specified for PHB\n");
> -        return -1;
> +        error_setg(errp, "IO window address not specified for PHB");
> +        return;
>      }
>  
>      if (find_phb(spapr, sphb->buid)) {
> -        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
> -        return -1;
> +        error_setg(errp, "PCI host bridges must have unique BUIDs");
> +        return;
>      }
>  
>      sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
> @@ -613,8 +614,9 @@ static int spapr_phb_init(SysBusDevice *s)
>      sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
>                                       sphb->dma_window_size);
>      if (!sphb->tcet) {
> -        fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
> -        return -1;
> +        error_setg(errp, "Unable to create TCE table for %s",
> +                   sphb->dtbusname);
> +        return;
>      }
>      address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
>                         sphb->dtbusname);
> @@ -631,13 +633,12 @@ static int spapr_phb_init(SysBusDevice *s)
>  
>          irq = spapr_allocate_lsi(0);
>          if (!irq) {
> -            return -1;
> +            error_setg(errp, "spapr_allocate_lsi failed");
> +            return;
>          }
>  
>          sphb->lsi_table[i].irq = irq;
>      }
> -
> -    return 0;
>  }
>  
>  static void spapr_phb_reset(DeviceState *qdev)
> @@ -720,11 +721,10 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
>  static void spapr_phb_class_init(ObjectClass *klass, void *data)
>  {
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> -    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      hc->root_bus_path = spapr_phb_root_bus_path;
> -    sdc->init = spapr_phb_init;
> +    dc->realize = spapr_phb_realize;
>      dc->props = spapr_phb_properties;
>      dc->reset = spapr_phb_reset;
>      dc->vmsd = &vmstate_spapr_pci;
> -- 
> 1.8.4.rc4
> 
> 
> 
> 

Re: [Qemu-devel] spapr-pci: introduce a finish_realize() callback

[Mike Day]

On 21/11/13 15:08 +1100, Alexey Kardashevskiy wrote:
> The spapr-pci PHB initializes IOMMU for emulataed devices only.
> The upcoming VFIO support will do it different. However both emulated
> and VFIO PHB types share most of the initialization code.
> For the type specific things a new finish_realize() callback is
> introduced.
> 
> This introduces sPAPRPHBClass derived from PCIHostBridgeClass and
> adds the callback pointer.
> 
> This implements finish_realize() for emulated devices.
> 
> This changes initialization steps order to have the finish_realize()
> call at the end of the spapr_finalize().
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: Mike Day <ncmike@ncultra.org>

> ---
> Changes:
> v5:
> * this is a new patch in the series, it was a part of a previous patch
> ---
>  hw/ppc/spapr_pci.c          | 46 +++++++++++++++++++++++++++++----------------
>  include/hw/pci-host/spapr.h | 18 ++++++++++++++++--
>  2 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index aeb012d..963841c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -500,6 +500,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
> +    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s);
>      const char *busname;
>      char *namebuf;
>      int i;
> @@ -609,22 +610,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
>      phb->bus = bus;
>  
> -    sphb->dma_window_start = 0;
> -    sphb->dma_window_size = 0x40000000;
> -    sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
> -                                     sphb->dma_window_size);
> -    if (!sphb->tcet) {
> -        error_setg(errp, "Unable to create TCE table for %s",
> -                   sphb->dtbusname);
> -        return;
> -    }
> -    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
> -                       sphb->dtbusname);
> -
> -    pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
> -
> -    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
> -
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>  
>      /* Initialize the LSI table */
> @@ -639,6 +624,32 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  
>          sphb->lsi_table[i].irq = irq;
>      }
> +
> +    pci_setup_iommu(sphb->parent_obj.bus, spapr_pci_dma_iommu, sphb);
> +
> +    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
> +
> +    if (!info->finish_realize) {
> +        error_setg(errp, "finish_realize not defined");
> +        return;
> +    }
> +
> +    info->finish_realize(sphb, errp);
> +}
> +
> +static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
> +{
> +    sphb->dma_window_start = 0;
> +    sphb->dma_window_size = 0x40000000;
> +    sphb->tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
> +                                     sphb->dma_window_size);
> +    if (!sphb->tcet) {
> +        error_setg(errp, "Unable to create TCE table for %s",
> +                   sphb->dtbusname);
> +        return ;
> +    }
> +    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
> +                       sphb->dtbusname);
>  }
>  
>  static void spapr_phb_reset(DeviceState *qdev)
> @@ -722,12 +733,14 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>  {
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
>  
>      hc->root_bus_path = spapr_phb_root_bus_path;
>      dc->realize = spapr_phb_realize;
>      dc->props = spapr_phb_properties;
>      dc->reset = spapr_phb_reset;
>      dc->vmsd = &vmstate_spapr_pci;
> +    spc->finish_realize = spapr_phb_finish_realize;
>  }
>  
>  static const TypeInfo spapr_phb_info = {
> @@ -735,6 +748,7 @@ static const TypeInfo spapr_phb_info = {
>      .parent        = TYPE_PCI_HOST_BRIDGE,
>      .instance_size = sizeof(sPAPRPHBState),
>      .class_init    = spapr_phb_class_init,
> +    .class_size    = sizeof(sPAPRPHBClass),
>  };
>  
>  PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 970b4a9..0f428a1 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -34,7 +34,21 @@
>  #define SPAPR_PCI_HOST_BRIDGE(obj) \
>      OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
>  
> -typedef struct sPAPRPHBState {
> +#define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
> +#define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(sPAPRPHBClass, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
> +
> +typedef struct sPAPRPHBClass sPAPRPHBClass;
> +typedef struct sPAPRPHBState sPAPRPHBState;
> +
> +struct sPAPRPHBClass {
> +    PCIHostBridgeClass parent_class;
> +
> +    void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
> +};
> +
> +struct sPAPRPHBState {
>      PCIHostState parent_obj;
>  
>      int32_t index;
> @@ -62,7 +76,7 @@ typedef struct sPAPRPHBState {
>      } msi_table[SPAPR_MSIX_MAX_DEVS];
>  
>      QLIST_ENTRY(sPAPRPHBState) list;
> -} sPAPRPHBState;
> +};
>  
>  #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
>  
> -- 
> 1.8.4.rc4
> 
> 
> 
> 

Re: [Qemu-devel] spapr-pci: converts fprintf to error_report

[Mike Day]

On 21/11/13 15:08 +1100, Alexey Kardashevskiy wrote:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: Mike Day <ncmike@ncultra.org>

> ---
>  hw/ppc/spapr_pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 963841c..d102d82 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -293,7 +293,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>          ret_intr_type = RTAS_TYPE_MSIX;
>          break;
>      default:
> -        fprintf(stderr, "rtas_ibm_change_msi(%u) is not implemented\n", func);
> +        error_report("rtas_ibm_change_msi(%u) is not implemented", func);
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>          return;
>      }
> @@ -327,7 +327,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>      /* Find a device number in the map to add or reuse the existing one */
>      ndev = spapr_msicfg_find(phb, config_addr, true);
>      if (ndev >= SPAPR_MSIX_MAX_DEVS || ndev < 0) {
> -        fprintf(stderr, "No free entry for a new MSI device\n");
> +        error_report("No free entry for a new MSI device");
>          rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>          return;
>      }
> @@ -336,7 +336,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>      /* Check if there is an old config and MSI number has not changed */
>      if (phb->msi_table[ndev].nvec && (req_num != phb->msi_table[ndev].nvec)) {
>          /* Unexpected behaviour */
> -        fprintf(stderr, "Cannot reuse MSI config for device#%d", ndev);
> +        error_report("Cannot reuse MSI config for device#%d", ndev);
>          rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>          return;
>      }
> @@ -346,7 +346,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>          irq = spapr_allocate_irq_block(req_num, false,
>                                         ret_intr_type == RTAS_TYPE_MSI);
>          if (irq < 0) {
> -            fprintf(stderr, "Cannot allocate MSIs for device#%d", ndev);
> +            error_report("Cannot allocate MSIs for device#%d", ndev);
>              rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>              return;
>          }
> -- 
> 1.8.4.rc4
> 
> 
> 
> 

Re: [Qemu-devel] spapr-pci: convert init() callback to realize()

[Alexey Kardashevskiy]

On 03/13/2014 12:56 AM, Mike Day wrote:
> On 21/11/13 15:08 +1100, Alexey Kardashevskiy wrote:
>> This converts the old-style init() callback to a new style realize()
>> callback as init() now is supposed to do only trivial initialization.
>>
>> As a part of convertion, this replaces fprintf(stderr) with error_setg()
>> as realize() does not "return" any value, instead it puts the extended
>> error into **errp.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: Mike Day <ncmike@ncultra.org>
> 
>> ---
>> Changes:
>> v5:
>> * finish_finalize() moved to a separate patch
>> ---
>>  hw/ppc/spapr_pci.c | 44 ++++++++++++++++++++++----------------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 7763149..aeb012d 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -32,6 +32,7 @@
>>  #include "exec/address-spaces.h"
>>  #include <libfdt.h>
>>  #include "trace.h"
>> +#include "qemu/error-report.h"
>>  
>>  #include "hw/pci/pci_bus.h"
>>  
>> @@ -494,9 +495,9 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>      return &phb->iommu_as;
>>  }
>>  
>> -static int spapr_phb_init(SysBusDevice *s)
>> +static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>  {
>> -    DeviceState *dev = DEVICE(s);
>> +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
>>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
>>      const char *busname;
>> @@ -510,9 +511,9 @@ static int spapr_phb_init(SysBusDevice *s)
>>          if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
>>              || (sphb->mem_win_addr != -1)
>>              || (sphb->io_win_addr != -1)) {
>> -            fprintf(stderr, "Either \"index\" or other parameters must"
>> -                    " be specified for PAPR PHB, not both\n");
>> -            return -1;
>> +            error_setg(errp, "Either \"index\" or other parameters must"
>> +                       " be specified for PAPR PHB, not both");
>> +            return;
>>          }
> 
> Seems like these exit clauses should return an integer here and below.


Nope. Bad initfn (which returns int) became good realizefn (which does not
return anything and uses Error** instead) :)

Thanks for review!

>   
>>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
>> @@ -525,28 +526,28 @@ static int spapr_phb_init(SysBusDevice *s)
>>      }
>>  
>>      if (sphb->buid == -1) {
>> -        fprintf(stderr, "BUID not specified for PHB\n");
>> -        return -1;
>> +        error_setg(errp, "BUID not specified for PHB");
>> +        return;
>>      }
>>  
>>      if (sphb->dma_liobn == -1) {
>> -        fprintf(stderr, "LIOBN not specified for PHB\n");
>> -        return -1;
>> +        error_setg(errp, "LIOBN not specified for PHB");
>> +        return;
>>      }
>>  
>>      if (sphb->mem_win_addr == -1) {
>> -        fprintf(stderr, "Memory window address not specified for PHB\n");
>> -        return -1;
>> +        error_setg(errp, "Memory window address not specified for PHB");
>> +        return;
>>      }
>>  
>>      if (sphb->io_win_addr == -1) {
>> -        fprintf(stderr, "IO window address not specified for PHB\n");
>> -        return -1;
>> +        error_setg(errp, "IO window address not specified for PHB");
>> +        return;
>>      }
>>  
>>      if (find_phb(spapr, sphb->buid)) {
>> -        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
>> -        return -1;
>> +        error_setg(errp, "PCI host bridges must have unique BUIDs");
>> +        return;
>>      }
>>  
>>      sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
>> @@ -613,8 +614,9 @@ static int spapr_phb_init(SysBusDevice *s)
>>      sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
>>                                       sphb->dma_window_size);
>>      if (!sphb->tcet) {
>> -        fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
>> -        return -1;
>> +        error_setg(errp, "Unable to create TCE table for %s",
>> +                   sphb->dtbusname);
>> +        return;
>>      }
>>      address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
>>                         sphb->dtbusname);
>> @@ -631,13 +633,12 @@ static int spapr_phb_init(SysBusDevice *s)
>>  
>>          irq = spapr_allocate_lsi(0);
>>          if (!irq) {
>> -            return -1;
>> +            error_setg(errp, "spapr_allocate_lsi failed");
>> +            return;
>>          }
>>  
>>          sphb->lsi_table[i].irq = irq;
>>      }
>> -
>> -    return 0;
>>  }
>>  
>>  static void spapr_phb_reset(DeviceState *qdev)
>> @@ -720,11 +721,10 @@ static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
>>  static void spapr_phb_class_init(ObjectClass *klass, void *data)
>>  {
>>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>> -    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>>      hc->root_bus_path = spapr_phb_root_bus_path;
>> -    sdc->init = spapr_phb_init;
>> +    dc->realize = spapr_phb_realize;
>>      dc->props = spapr_phb_properties;
>>      dc->reset = spapr_phb_reset;
>>      dc->vmsd = &vmstate_spapr_pci;
>> -- 
>> 1.8.4.rc4
>>
>>
>>
>>


-- 
Alexey

Re: [Qemu-devel] [PATCH 1/4] spapr-pci: convert init() callback to realize()

[Andreas Färber]

Am 21.11.2013 05:08, schrieb Alexey Kardashevskiy:
> This converts the old-style init() callback to a new style realize()
> callback as init() now is supposed to do only trivial initialization.
> 
> As a part of convertion, this replaces fprintf(stderr) with error_setg()
> as realize() does not "return" any value, instead it puts the extended
> error into **errp.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v5:
> * finish_finalize() moved to a separate patch
> ---
>  hw/ppc/spapr_pci.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7763149..aeb012d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -32,6 +32,7 @@
>  #include "exec/address-spaces.h"
>  #include <libfdt.h>
>  #include "trace.h"
> +#include "qemu/error-report.h"
>  
>  #include "hw/pci/pci_bus.h"
>  
[snip]

Thanks, applied to ppc-next without the above hunk:
https://github.com/afaerber/qemu-cpu/commits/ppc-next

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/4] spapr-pci: add spapr_pci trace

[Andreas Färber]

Am 21.11.2013 05:08, schrieb Alexey Kardashevskiy:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  trace-events | 1 +
>  1 file changed, 1 insertion(+)

Unused trace event?

Andreas

> 
> diff --git a/trace-events b/trace-events
> index 8695e9e..ba5f76c 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1114,6 +1114,7 @@ qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride,
>  qxl_render_update_area_done(void *cookie) "%p"
>  
>  # hw/ppc/spapr_pci.c
> +spapr_pci(const char *msg1, const char *msg2) "%s%s"
>  spapr_pci_msi(const char *msg, uint32_t n, uint32_t ca) "%s (device#%d, cfg=%x)"
>  spapr_pci_msi_setup(const char *name, unsigned vector, uint64_t addr) "dev\"%s\" vector %u, addr=%"PRIx64
>  spapr_pci_rtas_ibm_change_msi(unsigned func, unsigned req) "func %u, requested %u"
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/4] spapr-pci: converts fprintf to error_report

[Andreas Färber]

Am 21.11.2013 05:08, schrieb Alexey Kardashevskiy:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/ppc/spapr_pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Thanks, applied to ppc-next with the #include:
https://github.com/afaerber/qemu-cpu/commits/ppc-next

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/4] spapr-pci: introduce a finish_realize() callback

[Andreas Färber]

Am 21.11.2013 05:08, schrieb Alexey Kardashevskiy:
> The spapr-pci PHB initializes IOMMU for emulataed devices only.

"emulated"

> The upcoming VFIO support will do it different. However both emulated
> and VFIO PHB types share most of the initialization code.
> For the type specific things a new finish_realize() callback is
> introduced.
> 
> This introduces sPAPRPHBClass derived from PCIHostBridgeClass and
> adds the callback pointer.
> 
> This implements finish_realize() for emulated devices.
> 
> This changes initialization steps order to have the finish_realize()
> call at the end of the spapr_finalize().
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v5:
> * this is a new patch in the series, it was a part of a previous patch
> ---
>  hw/ppc/spapr_pci.c          | 46 +++++++++++++++++++++++++++++----------------
>  include/hw/pci-host/spapr.h | 18 ++++++++++++++++--
>  2 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index aeb012d..963841c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -500,6 +500,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
> +    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s);
>      const char *busname;
>      char *namebuf;
>      int i;
> @@ -609,22 +610,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
>      phb->bus = bus;
>  
> -    sphb->dma_window_start = 0;
> -    sphb->dma_window_size = 0x40000000;
> -    sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
> -                                     sphb->dma_window_size);
> -    if (!sphb->tcet) {
> -        error_setg(errp, "Unable to create TCE table for %s",
> -                   sphb->dtbusname);
> -        return;
> -    }
> -    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
> -                       sphb->dtbusname);
> -
> -    pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
> -
> -    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
> -
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>  
>      /* Initialize the LSI table */
> @@ -639,6 +624,32 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  
>          sphb->lsi_table[i].irq = irq;
>      }
> +
> +    pci_setup_iommu(sphb->parent_obj.bus, spapr_pci_dma_iommu, sphb);

Accessing ->parent_obj anywhere but in VMState is very likely wrong,
here it is definitely.

Also due to time pressure I'm not comfortable taking this into 2.0-rc0
and would like to see how this works for the second implementation.

Regards,
Andreas

> +
> +    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
> +
> +    if (!info->finish_realize) {
> +        error_setg(errp, "finish_realize not defined");
> +        return;
> +    }
> +
> +    info->finish_realize(sphb, errp);
> +}
> +
> +static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
> +{
> +    sphb->dma_window_start = 0;
> +    sphb->dma_window_size = 0x40000000;
> +    sphb->tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
> +                                     sphb->dma_window_size);
> +    if (!sphb->tcet) {
> +        error_setg(errp, "Unable to create TCE table for %s",
> +                   sphb->dtbusname);
> +        return ;
> +    }
> +    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
> +                       sphb->dtbusname);
>  }
>  
>  static void spapr_phb_reset(DeviceState *qdev)
> @@ -722,12 +733,14 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>  {
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
>  
>      hc->root_bus_path = spapr_phb_root_bus_path;
>      dc->realize = spapr_phb_realize;
>      dc->props = spapr_phb_properties;
>      dc->reset = spapr_phb_reset;
>      dc->vmsd = &vmstate_spapr_pci;
> +    spc->finish_realize = spapr_phb_finish_realize;
>  }
>  
>  static const TypeInfo spapr_phb_info = {
> @@ -735,6 +748,7 @@ static const TypeInfo spapr_phb_info = {
>      .parent        = TYPE_PCI_HOST_BRIDGE,
>      .instance_size = sizeof(sPAPRPHBState),
>      .class_init    = spapr_phb_class_init,
> +    .class_size    = sizeof(sPAPRPHBClass),
>  };
>  
>  PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 970b4a9..0f428a1 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -34,7 +34,21 @@
>  #define SPAPR_PCI_HOST_BRIDGE(obj) \
>      OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
>  
> -typedef struct sPAPRPHBState {
> +#define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
> +#define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(sPAPRPHBClass, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
> +
> +typedef struct sPAPRPHBClass sPAPRPHBClass;
> +typedef struct sPAPRPHBState sPAPRPHBState;
> +
> +struct sPAPRPHBClass {
> +    PCIHostBridgeClass parent_class;
> +
> +    void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
> +};
> +
> +struct sPAPRPHBState {
>      PCIHostState parent_obj;
>  
>      int32_t index;
> @@ -62,7 +76,7 @@ typedef struct sPAPRPHBState {
>      } msi_table[SPAPR_MSIX_MAX_DEVS];
>  
>      QLIST_ENTRY(sPAPRPHBState) list;
> -} sPAPRPHBState;
> +};
>  
>  #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/4] spapr-pci: introduce a finish_realize() callback

[Alexey Kardashevskiy]

On 03/13/2014 12:56 PM, Andreas Färber wrote:
> Am 21.11.2013 05:08, schrieb Alexey Kardashevskiy:
>> The spapr-pci PHB initializes IOMMU for emulataed devices only.
> 
> "emulated"
> 
>> The upcoming VFIO support will do it different. However both emulated
>> and VFIO PHB types share most of the initialization code.
>> For the type specific things a new finish_realize() callback is
>> introduced.
>>
>> This introduces sPAPRPHBClass derived from PCIHostBridgeClass and
>> adds the callback pointer.
>>
>> This implements finish_realize() for emulated devices.
>>
>> This changes initialization steps order to have the finish_realize()
>> call at the end of the spapr_finalize().
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v5:
>> * this is a new patch in the series, it was a part of a previous patch
>> ---
>>  hw/ppc/spapr_pci.c          | 46 +++++++++++++++++++++++++++++----------------
>>  include/hw/pci-host/spapr.h | 18 ++++++++++++++++--
>>  2 files changed, 46 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index aeb012d..963841c 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -500,6 +500,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
>>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>>      PCIHostState *phb = PCI_HOST_BRIDGE(s);
>> +    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s);
>>      const char *busname;
>>      char *namebuf;
>>      int i;
>> @@ -609,22 +610,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>                             PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
>>      phb->bus = bus;
>>  
>> -    sphb->dma_window_start = 0;
>> -    sphb->dma_window_size = 0x40000000;
>> -    sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
>> -                                     sphb->dma_window_size);
>> -    if (!sphb->tcet) {
>> -        error_setg(errp, "Unable to create TCE table for %s",
>> -                   sphb->dtbusname);
>> -        return;
>> -    }
>> -    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
>> -                       sphb->dtbusname);
>> -
>> -    pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
>> -
>> -    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
>> -
>>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>>  
>>      /* Initialize the LSI table */
>> @@ -639,6 +624,32 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>  
>>          sphb->lsi_table[i].irq = irq;
>>      }
>> +
>> +    pci_setup_iommu(sphb->parent_obj.bus, spapr_pci_dma_iommu, sphb);
> 
> Accessing ->parent_obj anywhere but in VMState is very likely wrong,
> here it is definitely.

This is just a victim of multiple cut-n-paste's. My bad, missed it... It
should be:
pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);


> Also due to time pressure I'm not comfortable taking this into 2.0-rc0
> and would like to see how this works for the second implementation.

Repost it? Or I better wait for something (what?)? Thanks.


> 
> Regards,
> Andreas
> 
>> +
>> +    pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
>> +
>> +    if (!info->finish_realize) {
>> +        error_setg(errp, "finish_realize not defined");
>> +        return;
>> +    }
>> +
>> +    info->finish_realize(sphb, errp);
>> +}
>> +
>> +static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
>> +{
>> +    sphb->dma_window_start = 0;
>> +    sphb->dma_window_size = 0x40000000;
>> +    sphb->tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
>> +                                     sphb->dma_window_size);
>> +    if (!sphb->tcet) {
>> +        error_setg(errp, "Unable to create TCE table for %s",
>> +                   sphb->dtbusname);
>> +        return ;
>> +    }
>> +    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
>> +                       sphb->dtbusname);
>>  }
>>  
>>  static void spapr_phb_reset(DeviceState *qdev)
>> @@ -722,12 +733,14 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>>  {
>>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
>>  
>>      hc->root_bus_path = spapr_phb_root_bus_path;
>>      dc->realize = spapr_phb_realize;
>>      dc->props = spapr_phb_properties;
>>      dc->reset = spapr_phb_reset;
>>      dc->vmsd = &vmstate_spapr_pci;
>> +    spc->finish_realize = spapr_phb_finish_realize;
>>  }
>>  
>>  static const TypeInfo spapr_phb_info = {
>> @@ -735,6 +748,7 @@ static const TypeInfo spapr_phb_info = {
>>      .parent        = TYPE_PCI_HOST_BRIDGE,
>>      .instance_size = sizeof(sPAPRPHBState),
>>      .class_init    = spapr_phb_class_init,
>> +    .class_size    = sizeof(sPAPRPHBClass),
>>  };
>>  
>>  PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
>> index 970b4a9..0f428a1 100644
>> --- a/include/hw/pci-host/spapr.h
>> +++ b/include/hw/pci-host/spapr.h
>> @@ -34,7 +34,21 @@
>>  #define SPAPR_PCI_HOST_BRIDGE(obj) \
>>      OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
>>  
>> -typedef struct sPAPRPHBState {
>> +#define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
>> +     OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
>> +#define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(sPAPRPHBClass, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
>> +
>> +typedef struct sPAPRPHBClass sPAPRPHBClass;
>> +typedef struct sPAPRPHBState sPAPRPHBState;
>> +
>> +struct sPAPRPHBClass {
>> +    PCIHostBridgeClass parent_class;
>> +
>> +    void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
>> +};
>> +
>> +struct sPAPRPHBState {
>>      PCIHostState parent_obj;
>>  
>>      int32_t index;
>> @@ -62,7 +76,7 @@ typedef struct sPAPRPHBState {
>>      } msi_table[SPAPR_MSIX_MAX_DEVS];
>>  
>>      QLIST_ENTRY(sPAPRPHBState) list;
>> -} sPAPRPHBState;
>> +};
>>  
>>  #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
>>  
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 3/4] spapr-pci: add spapr_pci trace

[Alexey Kardashevskiy]

On 03/13/2014 12:46 PM, Andreas Färber wrote:
> Am 21.11.2013 05:08, schrieb Alexey Kardashevskiy:
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  trace-events | 1 +
>>  1 file changed, 1 insertion(+)
> 
> Unused trace event?

I'll better drop it at all since Paolo dislikes generalized trace events
and this one is used in vfio code :)

> 
> Andreas
> 
>>
>> diff --git a/trace-events b/trace-events
>> index 8695e9e..ba5f76c 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1114,6 +1114,7 @@ qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride,
>>  qxl_render_update_area_done(void *cookie) "%p"
>>  
>>  # hw/ppc/spapr_pci.c
>> +spapr_pci(const char *msg1, const char *msg2) "%s%s"
>>  spapr_pci_msi(const char *msg, uint32_t n, uint32_t ca) "%s (device#%d, cfg=%x)"
>>  spapr_pci_msi_setup(const char *name, unsigned vector, uint64_t addr) "dev\"%s\" vector %u, addr=%"PRIx64
>>  spapr_pci_rtas_ibm_change_msi(unsigned func, unsigned req) "func %u, requested %u"
>>
> 
> 


-- 
Alexey