Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Eduardo Habkost]

On Fri, Aug 25, 2017 at 07:57:57PM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 25, 2017 at 12:17:42PM -0300, Eduardo Habkost wrote:
> > On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> > > From: Mao Zhongyi 
> > > 
> > > Convert i82801b11, io3130_upstream, io3130_downstream and
> > > pcie_root_port devices to realize.
> > > 
> > > Cc: m...@redhat.com
> > > Cc: mar...@redhat.com
> > > Cc: arm...@redhat.com
> > > Signed-off-by: Mao Zhongyi 
> > > Reviewed-by: Marcel Apfelbaum 
> > > Reviewed-by: Michael S. Tsirkin 
> > > Signed-off-by: Michael S. Tsirkin 
> > > ---
> > [...]
> > > diff --git a/hw/pci-bridge/xio3130_downstream.c 
> > > b/hw/pci-bridge/xio3130_downstream.c
> > > index cfe8a36..e706f36 100644
> > > --- a/hw/pci-bridge/xio3130_downstream.c
> > > +++ b/hw/pci-bridge/xio3130_downstream.c
> > > @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState 
> > > *qdev)
> > >  pci_bridge_reset(qdev);
> > >  }
> > >  
> > > -static int xio3130_downstream_initfn(PCIDevice *d)
> > > +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
> > >  {
> > [...]
> > >  pcie_chassis_create(s->chassis);
> > >  rc = pcie_chassis_add_slot(s);
> > >  if (rc < 0) {
> > >  goto err_pcie_cap;
> > 
> > Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
> > won't report an error properly.  Causes crash with:
> > 
> > $ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device 
> > xio3130-downstream
> > qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: 
> > memory_region_del_subregion: Assertion `subregion->container == mr' failed.
> > Aborted (core dumped)
> > 
> > 
> > >  }
> > >  
> > >  rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> > > -   PCI_ERR_SIZEOF, );
> > > +   PCI_ERR_SIZEOF, errp);
> > >  if (rc < 0) {
> > > -error_report_err(err);
> > >  goto err;
> > >  }
> > >  
> > > -return 0;
> > > +return;
> > >  
> > >  err:
> > >  pcie_chassis_del_slot(s);
> > > @@ -114,7 +113,6 @@ err_msi:
> > >  msi_uninit(d);
> > >  err_bridge:
> > >  pci_bridge_exitfn(d);
> > > -return rc;
> > >  }
> > >  
> > [...]
> 
> Good catch!  Patch?

I will send one later today.

-- 
Eduardo

Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Michael S. Tsirkin]

On Fri, Aug 25, 2017 at 12:17:42PM -0300, Eduardo Habkost wrote:
> On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> > From: Mao Zhongyi 
> > 
> > Convert i82801b11, io3130_upstream, io3130_downstream and
> > pcie_root_port devices to realize.
> > 
> > Cc: m...@redhat.com
> > Cc: mar...@redhat.com
> > Cc: arm...@redhat.com
> > Signed-off-by: Mao Zhongyi 
> > Reviewed-by: Marcel Apfelbaum 
> > Reviewed-by: Michael S. Tsirkin 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> [...]
> > diff --git a/hw/pci-bridge/xio3130_downstream.c 
> > b/hw/pci-bridge/xio3130_downstream.c
> > index cfe8a36..e706f36 100644
> > --- a/hw/pci-bridge/xio3130_downstream.c
> > +++ b/hw/pci-bridge/xio3130_downstream.c
> > @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
> >  pci_bridge_reset(qdev);
> >  }
> >  
> > -static int xio3130_downstream_initfn(PCIDevice *d)
> > +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
> >  {
> [...]
> >  pcie_chassis_create(s->chassis);
> >  rc = pcie_chassis_add_slot(s);
> >  if (rc < 0) {
> >  goto err_pcie_cap;
> 
> Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
> won't report an error properly.  Causes crash with:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device 
> xio3130-downstream
> qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: 
> memory_region_del_subregion: Assertion `subregion->container == mr' failed.
> Aborted (core dumped)
> 
> 
> >  }
> >  
> >  rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> > -   PCI_ERR_SIZEOF, );
> > +   PCI_ERR_SIZEOF, errp);
> >  if (rc < 0) {
> > -error_report_err(err);
> >  goto err;
> >  }
> >  
> > -return 0;
> > +return;
> >  
> >  err:
> >  pcie_chassis_del_slot(s);
> > @@ -114,7 +113,6 @@ err_msi:
> >  msi_uninit(d);
> >  err_bridge:
> >  pci_bridge_exitfn(d);
> > -return rc;
> >  }
> >  
> [...]

Good catch!  Patch?

> -- 
> Eduardo

Re: [Qemu-devel] [PULL 09/21] pci: Convert to realize

[Eduardo Habkost]

On Mon, Jul 03, 2017 at 10:45:16PM +0300, Michael S. Tsirkin wrote:
> From: Mao Zhongyi 
> 
> Convert i82801b11, io3130_upstream, io3130_downstream and
> pcie_root_port devices to realize.
> 
> Cc: m...@redhat.com
> Cc: mar...@redhat.com
> Cc: arm...@redhat.com
> Signed-off-by: Mao Zhongyi 
> Reviewed-by: Marcel Apfelbaum 
> Reviewed-by: Michael S. Tsirkin 
> Signed-off-by: Michael S. Tsirkin 
> ---
[...]
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index cfe8a36..e706f36 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
>  pci_bridge_reset(qdev);
>  }
>  
> -static int xio3130_downstream_initfn(PCIDevice *d)
> +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>  {
[...]
>  pcie_chassis_create(s->chassis);
>  rc = pcie_chassis_add_slot(s);
>  if (rc < 0) {
>  goto err_pcie_cap;

Missing error_setg() call here.  If pcie_chassis_add_slot() fails, realize
won't report an error properly.  Causes crash with:

$ ./x86_64-softmmu/qemu-system-x86_64 -device ioh3420 -device xio3130-downstream
qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/memory.c:2166: 
memory_region_del_subregion: Assertion `subregion->container == mr' failed.
Aborted (core dumped)


>  }
>  
>  rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> -   PCI_ERR_SIZEOF, );
> +   PCI_ERR_SIZEOF, errp);
>  if (rc < 0) {
> -error_report_err(err);
>  goto err;
>  }
>  
> -return 0;
> +return;
>  
>  err:
>  pcie_chassis_del_slot(s);
> @@ -114,7 +113,6 @@ err_msi:
>  msi_uninit(d);
>  err_bridge:
>  pci_bridge_exitfn(d);
> -return rc;
>  }
>  
[...]

-- 
Eduardo

[Qemu-devel] [PULL 09/21] pci: Convert to realize

[Michael S. Tsirkin]

From: Mao Zhongyi 

Convert i82801b11, io3130_upstream, io3130_downstream and
pcie_root_port devices to realize.

Cc: m...@redhat.com
Cc: mar...@redhat.com
Cc: arm...@redhat.com
Signed-off-by: Mao Zhongyi 
Reviewed-by: Marcel Apfelbaum 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 include/hw/pci/pci_bridge.h|  3 ++-
 include/hw/pci/pcie.h  |  3 ++-
 hw/pci-bridge/i82801b11.c  | 11 +--
 hw/pci-bridge/pcie_root_port.c | 18 +-
 hw/pci-bridge/xio3130_downstream.c | 20 +---
 hw/pci-bridge/xio3130_upstream.c   | 20 +---
 hw/pci/pci_bridge.c|  7 +++
 hw/pci/pcie.c  | 24 +---
 8 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index d5891cd..ff7cbaa 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -33,7 +33,8 @@
 #define PCI_BRIDGE_DEV_PROP_SHPC   "shpc"
 
 int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
-  uint16_t svid, uint16_t ssid);
+  uint16_t svid, uint16_t ssid,
+  Error **errp);
 
 PCIDevice *pci_bridge_get_device(PCIBus *bus);
 PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 3d8f24b..b71e369 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -84,7 +84,8 @@ struct PCIExpressDevice {
 #define COMPAT_PROP_PCP "power_controller_present"
 
 /* PCI express capability helper functions */
-int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port);
+int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
+  uint8_t port, Error **errp);
 int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset,
  uint8_t type, uint8_t port);
 int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 2c065c3..2c1b747 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -59,24 +59,23 @@ typedef struct I82801b11Bridge {
 /*< public >*/
 } I82801b11Bridge;
 
-static int i82801b11_bridge_initfn(PCIDevice *d)
+static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
 {
 int rc;
 
 pci_bridge_initfn(d, TYPE_PCI_BUS);
 
 rc = pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
-   I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
+   I82801ba_SSVID_SVID, I82801ba_SSVID_SSID,
+   errp);
 if (rc < 0) {
 goto err_bridge;
 }
 pci_config_set_prog_interface(d->config, PCI_CLASS_BRIDGE_PCI_INF_SUB);
-return 0;
+return;
 
 err_bridge:
 pci_bridge_exitfn(d);
-
-return rc;
 }
 
 static const VMStateDescription i82801b11_bridge_dev_vmstate = {
@@ -96,7 +95,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, 
void *data)
 k->vendor_id = PCI_VENDOR_ID_INTEL;
 k->device_id = PCI_DEVICE_ID_INTEL_82801BA_11;
 k->revision = ICH9_D2P_A2_REVISION;
-k->init = i82801b11_bridge_initfn;
+k->realize = i82801b11_bridge_realize;
 k->config_write = pci_bridge_write_config;
 dc->vmsd = _bridge_dev_vmstate;
 set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index cf36318..4d588cb 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -59,29 +59,30 @@ static void rp_realize(PCIDevice *d, Error **errp)
 PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
 PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
 int rc;
-Error *local_err = NULL;
 
 pci_config_set_interrupt_pin(d->config, 1);
 pci_bridge_initfn(d, TYPE_PCIE_BUS);
 pcie_port_init_reg(d);
 
-rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, rpc->ssid);
+rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
+   rpc->ssid, errp);
 if (rc < 0) {
-error_setg(errp, "Can't init SSV ID, error %d", rc);
+error_append_hint(errp, "Can't init SSV ID, error %d\n", rc);
 goto err_bridge;
 }
 
 if (rpc->interrupts_init) {
-rc = rpc->interrupts_init(d, _err);
+rc = rpc->interrupts_init(d, errp);
 if (rc < 0) {
-error_propagate(errp, local_err);
 goto err_bridge;
 }
 }
 
-rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT, p->port);
+rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
+   p->port, errp);
 if (rc < 0) {
-error_setg(errp, "Can't add Root Port capability, error %d", rc);
+error_append_hint(errp,