[Qemu-devel] [PATCH 0/4] QOM'ify PCIDevices

[Qemu-devel] [PATCH 0/4] QOM'ify PCIDevices

[Philippe Mathieu-Daudé]

Hi,

In this series we QOM'ify the last few PCI devices still using
PCIDeviceClass->init() by converting this init() to realize(),

Then we finally remove the then PCIDeviceClass->init() method for good.

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  hw/block/nvme: QOM'ify PCI NVME
  hw/pci-host/piix: QOM'ify the IGD Passthrough host bridge
  hw/pci-host/xilinx: QOM'ify the AXI-PCIe host bridge
  hw/pci: remove obsolete PCIDevice->init()

 include/hw/pci/pci.h      |  1 -
 hw/block/nvme.c           | 32 +++++++++++++++++---------------
 hw/pci-host/piix.c        | 31 +++++++++++++++----------------
 hw/pci-host/xilinx-pcie.c | 19 +++++++++----------
 hw/pci/pci.c              | 14 --------------
 5 files changed, 41 insertions(+), 56 deletions(-)

-- 
2.15.1

[Qemu-devel] [PATCH 1/4] hw/block/nvme: QOM'ify PCI NVME

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/nvme.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 441e21ed1f..9c5f898da0 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -920,9 +920,9 @@ static const MemoryRegionOps nvme_cmb_ops = {
     },
 };
 
-static int nvme_init(PCIDevice *pci_dev)
+static void nvme_realize(PCIDevice *pci, Error **errp)
 {
-    NvmeCtrl *n = NVME(pci_dev);
+    NvmeCtrl *n = NVME(pci);
     NvmeIdCtrl *id = &n->id_ctrl;
 
     int i;
@@ -931,30 +931,33 @@ static int nvme_init(PCIDevice *pci_dev)
     Error *local_err = NULL;
 
     if (!n->conf.blk) {
-        return -1;
+        error_setg(errp, "Block device missing");
+        return;
     }
 
     bs_size = blk_getlength(n->conf.blk);
     if (bs_size < 0) {
-        return -1;
+        error_setg_errno(errp, -bs_size, "Could not get length of device");
+        return;
     }
 
     blkconf_serial(&n->conf, &n->serial);
     if (!n->serial) {
-        return -1;
+        error_setg(errp, "Could not get device serial number");
+        return;
     }
     blkconf_blocksizes(&n->conf);
     blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
                                   false, &local_err);
     if (local_err) {
-        error_report_err(local_err);
-        return -1;
+        error_propagate(errp, local_err);
+        return;
     }
 
-    pci_conf = pci_dev->config;
+    pci_conf = pci->config;
     pci_conf[PCI_INTERRUPT_PIN] = 1;
-    pci_config_set_prog_interface(pci_dev->config, 0x2);
-    pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
+    pci_config_set_prog_interface(pci->config, 0x2);
+    pci_config_set_class(pci->config, PCI_CLASS_STORAGE_EXPRESS);
     pcie_endpoint_cap_init(&n->parent_obj, 0x80);
 
     n->num_namespaces = 1;
@@ -1046,12 +1049,11 @@ static int nvme_init(PCIDevice *pci_dev)
             cpu_to_le64(n->ns_size >>
                 id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds);
     }
-    return 0;
 }
 
-static void nvme_exit(PCIDevice *pci_dev)
+static void nvme_exit(PCIDevice *pci)
 {
-    NvmeCtrl *n = NVME(pci_dev);
+    NvmeCtrl *n = NVME(pci);
 
     nvme_clear_ctrl(n);
     g_free(n->namespaces);
@@ -1061,7 +1063,7 @@ static void nvme_exit(PCIDevice *pci_dev)
         memory_region_unref(&n->ctrl_mem);
     }
 
-    msix_uninit_exclusive_bar(pci_dev);
+    msix_uninit_exclusive_bar(pci);
 }
 
 static Property nvme_props[] = {
@@ -1081,7 +1083,7 @@ static void nvme_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
 
-    pc->init = nvme_init;
+    pc->realize = nvme_realize;
     pc->exit = nvme_exit;
     pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
     pc->vendor_id = PCI_VENDOR_ID_INTEL;
-- 
2.15.1

[Qemu-devel] [PATCH 2/4] hw/pci-host/piix: QOM'ify the IGD Passthrough host bridge

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/piix.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index a684a7cca9..0153f32a32 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -804,7 +804,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
     {0xa8, 4},  /* SNB: base of GTT stolen memory */
 };
 
-static int host_pci_config_read(int pos, int len, uint32_t *val)
+static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
 {
     char path[PATH_MAX];
     int config_fd;
@@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)
     /* Access real host bridge. */
     int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
                       0, 0, 0, 0, "config");
-    int ret = 0;
 
     if (rc >= size || rc < 0) {
-        return -ENODEV;
+        error_setg(&error_abort, "Invalid PCI device");
     }
 
     config_fd = open(path, O_RDWR);
     if (config_fd < 0) {
-        return -ENODEV;
+        error_setg_errno(errp, errno, "Failed to open: %s", path);
+        return;
     }
 
     if (lseek(config_fd, pos, SEEK_SET) != pos) {
-        ret = -errno;
+        error_setg_errno(errp, errno, "Failed to seek: %s", path);
         goto out;
     }
 
@@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)
         rc = read(config_fd, (uint8_t *)val, len);
     } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
     if (rc != len) {
-        ret = -errno;
+        error_setg_errno(errp, errno, "Failed to read: %s", path);
     }
 
 out:
     close(config_fd);
-    return ret;
 }
 
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+static void igd_pt_i440fx_realize(PCIDevice *pci, Error **errp)
 {
     uint32_t val = 0;
-    int rc, i, num;
+    int i, num;
     int pos, len;
+    Error *local_err = NULL;
 
     num = ARRAY_SIZE(igd_host_bridge_infos);
     for (i = 0; i < num; i++) {
         pos = igd_host_bridge_infos[i].offset;
         len = igd_host_bridge_infos[i].len;
-        rc = host_pci_config_read(pos, len, &val);
-        if (rc) {
-            return -ENODEV;
+        host_pci_config_read(pos, len, &val, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
         }
-        pci_default_write_config(pci_dev, pos, val, len);
+        pci_default_write_config(pci, pos, val, len);
     }
-
-    return 0;
 }
 
 static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
@@ -865,7 +864,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = igd_pt_i440fx_initfn;
+    k->realize = igd_pt_i440fx_realize;
     dc->desc = "IGD Passthrough Host bridge";
 }
 
-- 
2.15.1

[Qemu-devel] [PATCH 3/4] hw/pci-host/xilinx: QOM'ify the AXI-PCIe host bridge

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/xilinx-pcie.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 7659253090..756db39fd5 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -267,24 +267,23 @@ static void xilinx_pcie_root_config_write(PCIDevice *d, uint32_t address,
     }
 }
 
-static int xilinx_pcie_root_init(PCIDevice *dev)
+static void xilinx_pcie_root_realize(PCIDevice *pci, Error **errp)
 {
-    BusState *bus = qdev_get_parent_bus(DEVICE(dev));
+    DeviceState *dev = DEVICE(pci);
+    BusState *bus = qdev_get_parent_bus(dev);
     XilinxPCIEHost *s = XILINX_PCIE_HOST(bus->parent);
 
-    pci_set_word(dev->config + PCI_COMMAND,
+    pci_set_word(pci->config + PCI_COMMAND,
                  PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
-    pci_set_word(dev->config + PCI_MEMORY_BASE, s->mmio_base >> 16);
-    pci_set_word(dev->config + PCI_MEMORY_LIMIT,
+    pci_set_word(pci->config + PCI_MEMORY_BASE, s->mmio_base >> 16);
+    pci_set_word(pci->config + PCI_MEMORY_LIMIT,
                  ((s->mmio_base + s->mmio_size - 1) >> 16) & 0xfff0);
 
-    pci_bridge_initfn(dev, TYPE_PCI_BUS);
+    pci_bridge_initfn(pci, TYPE_PCI_BUS);
 
-    if (pcie_endpoint_cap_v1_init(dev, 0x80) < 0) {
+    if (pcie_endpoint_cap_v1_init(pci, 0x80) < 0) {
         hw_error("Failed to initialize PCIe capability");
     }
-
-    return 0;
 }
 
 static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
@@ -300,7 +299,7 @@ static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_BRIDGE_HOST;
     k->is_express = true;
     k->is_bridge = true;
-    k->init = xilinx_pcie_root_init;
+    k->realize = xilinx_pcie_root_realize;
     k->exit = pci_bridge_exitfn;
     dc->reset = pci_bridge_reset;
     k->config_read = xilinx_pcie_root_config_read;
-- 
2.15.1

[Qemu-devel] [PATCH 4/4] hw/pci: remove obsolete PCIDevice->init()

[Philippe Mathieu-Daudé]

All PCI devices are now QOM'ified.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/pci/pci.h |  1 -
 hw/pci/pci.c         | 14 --------------
 2 files changed, 15 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8d02a0a383..0f1ed64c2f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -217,7 +217,6 @@ typedef struct PCIDeviceClass {
     DeviceClass parent_class;
 
     void (*realize)(PCIDevice *dev, Error **errp);
-    int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
     PCIUnregisterFunc *exit;
     PCIConfigReadFunc *config_read;
     PCIConfigWriteFunc *config_write;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b2d139bd9a..cd25ab6f6b 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2054,18 +2054,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
     }
 }
 
-static void pci_default_realize(PCIDevice *dev, Error **errp)
-{
-    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
-
-    if (pc->init) {
-        if (pc->init(dev) < 0) {
-            error_setg(errp, "Device initialization failed");
-            return;
-        }
-    }
-}
-
 PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
                                     const char *name)
 {
@@ -2538,13 +2526,11 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
 static void pci_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
-    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
 
     k->realize = pci_qdev_realize;
     k->unrealize = pci_qdev_unrealize;
     k->bus_type = TYPE_PCI_BUS;
     k->props = pci_props;
-    pc->realize = pci_default_realize;
 }
 
 static void pci_device_class_base_init(ObjectClass *klass, void *data)
-- 
2.15.1

Re: [Qemu-devel] [PATCH 2/4] hw/pci-host/piix: QOM'ify the IGD Passthrough host bridge

[Marcel Apfelbaum]

Hi Philippe,

Thanks for the patch.

On 17/12/2017 22:49, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/pci-host/piix.c | 31 +++++++++++++++----------------
>   1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index a684a7cca9..0153f32a32 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -804,7 +804,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>       {0xa8, 4},  /* SNB: base of GTT stolen memory */
>   };
>   
> -static int host_pci_config_read(int pos, int len, uint32_t *val)
> +static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
>   {
>       char path[PATH_MAX];
>       int config_fd;
> @@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)
>       /* Access real host bridge. */
>       int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
>                         0, 0, 0, 0, "config");
> -    int ret = 0;
>   
>       if (rc >= size || rc < 0) {
> -        return -ENODEV;
> +        error_setg(&error_abort, "Invalid PCI device");

I think a return statement is missing here.

>       }
>   
>       config_fd = open(path, O_RDWR);
>       if (config_fd < 0) {
> -        return -ENODEV;
> +        error_setg_errno(errp, errno, "Failed to open: %s", path);
> +        return;
>       }
>   
>       if (lseek(config_fd, pos, SEEK_SET) != pos) {
> -        ret = -errno;
> +        error_setg_errno(errp, errno, "Failed to seek: %s", path);
>           goto out;
>       }
>   
> @@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)
>           rc = read(config_fd, (uint8_t *)val, len);
>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>       if (rc != len) {
> -        ret = -errno;
> +        error_setg_errno(errp, errno, "Failed to read: %s", path);
>       }
>   
>   out:
>       close(config_fd);
> -    return ret;
>   }
>   
> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> +static void igd_pt_i440fx_realize(PCIDevice *pci, Error **errp)

I would keep pci_dev, pci is too "broad".
Also is kind of convention, try to grep the source code.


Other than that the patch looks good to me.
Thanks,
Marcel

>   {
>       uint32_t val = 0;
> -    int rc, i, num;
> +    int i, num;
>       int pos, len;
> +    Error *local_err = NULL;
>   
>       num = ARRAY_SIZE(igd_host_bridge_infos);
>       for (i = 0; i < num; i++) {
>           pos = igd_host_bridge_infos[i].offset;
>           len = igd_host_bridge_infos[i].len;
> -        rc = host_pci_config_read(pos, len, &val);
> -        if (rc) {
> -            return -ENODEV;
> +        host_pci_config_read(pos, len, &val, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
>           }
> -        pci_default_write_config(pci_dev, pos, val, len);
> +        pci_default_write_config(pci, pos, val, len);
>       }
> -
> -    return 0;
>   }
>   
>   static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> @@ -865,7 +864,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>   
> -    k->init = igd_pt_i440fx_initfn;
> +    k->realize = igd_pt_i440fx_realize;
>       dc->desc = "IGD Passthrough Host bridge";
>   }
>   
> 

Re: [Qemu-devel] [PATCH 4/4] hw/pci: remove obsolete PCIDevice->init()

[Marcel Apfelbaum]

On 17/12/2017 22:49, Philippe Mathieu-Daudé wrote:
> All PCI devices are now QOM'ified.
> 

Finally!


> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/pci/pci.h |  1 -
>   hw/pci/pci.c         | 14 --------------
>   2 files changed, 15 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 8d02a0a383..0f1ed64c2f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -217,7 +217,6 @@ typedef struct PCIDeviceClass {
>       DeviceClass parent_class;
>   
>       void (*realize)(PCIDevice *dev, Error **errp);
> -    int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
>       PCIUnregisterFunc *exit;
>       PCIConfigReadFunc *config_read;
>       PCIConfigWriteFunc *config_write;
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b2d139bd9a..cd25ab6f6b 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2054,18 +2054,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>       }
>   }
>   
> -static void pci_default_realize(PCIDevice *dev, Error **errp)
> -{
> -    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> -
> -    if (pc->init) {
> -        if (pc->init(dev) < 0) {
> -            error_setg(errp, "Device initialization failed");
> -            return;
> -        }
> -    }
> -}
> -
>   PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
>                                       const char *name)
>   {
> @@ -2538,13 +2526,11 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>   static void pci_device_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *k = DEVICE_CLASS(klass);
> -    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
>   
>       k->realize = pci_qdev_realize;
>       k->unrealize = pci_qdev_unrealize;
>       k->bus_type = TYPE_PCI_BUS;
>       k->props = pci_props;
> -    pc->realize = pci_default_realize;
>   }
>   
>   static void pci_device_class_base_init(ObjectClass *klass, void *data)
> 

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 3/4] hw/pci-host/xilinx: QOM'ify the AXI-PCIe host bridge

[Marcel Apfelbaum]

On 17/12/2017 22:49, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/pci-host/xilinx-pcie.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> index 7659253090..756db39fd5 100644
> --- a/hw/pci-host/xilinx-pcie.c
> +++ b/hw/pci-host/xilinx-pcie.c
> @@ -267,24 +267,23 @@ static void xilinx_pcie_root_config_write(PCIDevice *d, uint32_t address,
>       }
>   }
>   
> -static int xilinx_pcie_root_init(PCIDevice *dev)
> +static void xilinx_pcie_root_realize(PCIDevice *pci, Error **errp)
>   {

Same comment here, if "dev" seems not good enough maybe
you can use the pci_dev convention.

> -    BusState *bus = qdev_get_parent_bus(DEVICE(dev));
> +    DeviceState *dev = DEVICE(pci);
> +    BusState *bus = qdev_get_parent_bus(dev);

Not sure the above worth the effort :)

>       XilinxPCIEHost *s = XILINX_PCIE_HOST(bus->parent);
>   
> -    pci_set_word(dev->config + PCI_COMMAND,
> +    pci_set_word(pci->config + PCI_COMMAND,
>                    PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> -    pci_set_word(dev->config + PCI_MEMORY_BASE, s->mmio_base >> 16);
> -    pci_set_word(dev->config + PCI_MEMORY_LIMIT,
> +    pci_set_word(pci->config + PCI_MEMORY_BASE, s->mmio_base >> 16);
> +    pci_set_word(pci->config + PCI_MEMORY_LIMIT,
>                    ((s->mmio_base + s->mmio_size - 1) >> 16) & 0xfff0);
>   
> -    pci_bridge_initfn(dev, TYPE_PCI_BUS);
> +    pci_bridge_initfn(pci, TYPE_PCI_BUS);
>   
> -    if (pcie_endpoint_cap_v1_init(dev, 0x80) < 0) {
> +    if (pcie_endpoint_cap_v1_init(pci, 0x80) < 0) {
>           hw_error("Failed to initialize PCIe capability");

I think here you need to use the error_setg (after/instead hw_error).
It is supposed to return -1 if pcie capability was not added.


Thanks,
Marcel

>       }
> -
> -    return 0;
>   }
>   
>   static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
> @@ -300,7 +299,7 @@ static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
>       k->class_id = PCI_CLASS_BRIDGE_HOST;
>       k->is_express = true;
>       k->is_bridge = true;
> -    k->init = xilinx_pcie_root_init;
> +    k->realize = xilinx_pcie_root_realize;
>       k->exit = pci_bridge_exitfn;
>       dc->reset = pci_bridge_reset;
>       k->config_read = xilinx_pcie_root_config_read;
> 

Re: [Qemu-devel] [PATCH 2/4] hw/pci-host/piix: QOM'ify the IGD Passthrough host bridge

[Philippe Mathieu-Daudé]

Hi Marcel,

On 12/18/2017 04:05 AM, Marcel Apfelbaum wrote:
> Hi Philippe,
> 
> Thanks for the patch.
> 
> On 17/12/2017 22:49, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/pci-host/piix.c | 31 +++++++++++++++----------------
>>   1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index a684a7cca9..0153f32a32 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -804,7 +804,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>>       {0xa8, 4},  /* SNB: base of GTT stolen memory */
>>   };
>>   -static int host_pci_config_read(int pos, int len, uint32_t *val)
>> +static void host_pci_config_read(int pos, int len, uint32_t *val,
>> Error **errp)
>>   {
>>       char path[PATH_MAX];
>>       int config_fd;
>> @@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int
>> len, uint32_t *val)
>>       /* Access real host bridge. */
>>       int rc = snprintf(path, size,
>> "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
>>                         0, 0, 0, 0, "config");
>> -    int ret = 0;
>>         if (rc >= size || rc < 0) {
>> -        return -ENODEV;
>> +        error_setg(&error_abort, "Invalid PCI device");
> 
> I think a return statement is missing here.

Well, error_setg(&error_abort) will never return, however checking the
documentation again I noticed I forgot these recommendations:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.
 * Likewise, don't error_setg(&error_abort, ...), use assert().

abort() might be OK but a bit harsh, so I'll use error_report() + exit()
here.

>>       }
>>         config_fd = open(path, O_RDWR);
>>       if (config_fd < 0) {
>> -        return -ENODEV;
>> +        error_setg_errno(errp, errno, "Failed to open: %s", path);
>> +        return;
>>       }
>>         if (lseek(config_fd, pos, SEEK_SET) != pos) {
>> -        ret = -errno;
>> +        error_setg_errno(errp, errno, "Failed to seek: %s", path);
>>           goto out;
>>       }
>>   @@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int
>> len, uint32_t *val)
>>           rc = read(config_fd, (uint8_t *)val, len);
>>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>       if (rc != len) {
>> -        ret = -errno;
>> +        error_setg_errno(errp, errno, "Failed to read: %s", path);
>>       }
>>     out:
>>       close(config_fd);
>> -    return ret;
>>   }
>>   -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>> +static void igd_pt_i440fx_realize(PCIDevice *pci, Error **errp)
> 
> I would keep pci_dev, pci is too "broad".
> Also is kind of convention, try to grep the source code.

Ok.

> Other than that the patch looks good to me.

Thanks!

> Thanks,
> Marcel
> 
>>   {
>>       uint32_t val = 0;
>> -    int rc, i, num;
>> +    int i, num;
>>       int pos, len;
>> +    Error *local_err = NULL;
>>         num = ARRAY_SIZE(igd_host_bridge_infos);
>>       for (i = 0; i < num; i++) {
>>           pos = igd_host_bridge_infos[i].offset;
>>           len = igd_host_bridge_infos[i].len;
>> -        rc = host_pci_config_read(pos, len, &val);
>> -        if (rc) {
>> -            return -ENODEV;
>> +        host_pci_config_read(pos, len, &val, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return;
>>           }
>> -        pci_default_write_config(pci_dev, pos, val, len);
>> +        pci_default_write_config(pci, pos, val, len);
>>       }
>> -
>> -    return 0;
>>   }
>>     static void igd_passthrough_i440fx_class_init(ObjectClass *klass,
>> void *data)
>> @@ -865,7 +864,7 @@ static void
>> igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>   -    k->init = igd_pt_i440fx_initfn;
>> +    k->realize = igd_pt_i440fx_realize;
>>       dc->desc = "IGD Passthrough Host bridge";
>>   }
>>  
> 

Re: [Qemu-devel] [PATCH 2/4] hw/pci-host/piix: QOM'ify the IGD Passthrough host bridge

[Philippe Mathieu-Daudé]

[...]
>>>   -static int host_pci_config_read(int pos, int len, uint32_t *val)
>>> +static void host_pci_config_read(int pos, int len, uint32_t *val,
>>> Error **errp)
>>>   {
>>>       char path[PATH_MAX];
>>>       int config_fd;
>>> @@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int
>>> len, uint32_t *val)
>>>       /* Access real host bridge. */
>>>       int rc = snprintf(path, size,
>>> "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
>>>                         0, 0, 0, 0, "config");
>>> -    int ret = 0;
>>>         if (rc >= size || rc < 0) {
>>> -        return -ENODEV;
>>> +        error_setg(&error_abort, "Invalid PCI device");
>>
>> I think a return statement is missing here.
>
> Well, error_setg(&error_abort) will never return, however checking the
> documentation again I noticed I forgot these recommendations:
>
>  * Please don't error_setg(&error_fatal, ...), use error_report() and
>  * exit(), because that's more obvious.
>  * Likewise, don't error_setg(&error_abort, ...), use assert().
>
> abort() might be OK but a bit harsh, so I'll use error_report() + exit()
> here.

Actually g_strdup_printf() is cleaner.

>>>       }
>>>         config_fd = open(path, O_RDWR);
>>>       if (config_fd < 0) {
>>> -        return -ENODEV;
>>> +        error_setg_errno(errp, errno, "Failed to open: %s", path);
>>> +        return;
>>>       }
>>>         if (lseek(config_fd, pos, SEEK_SET) != pos) {
>>> -        ret = -errno;
>>> +        error_setg_errno(errp, errno, "Failed to seek: %s", path);
>>>           goto out;
>>>       }
>>>   @@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int
>>> len, uint32_t *val)
>>>           rc = read(config_fd, (uint8_t *)val, len);
>>>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>>       if (rc != len) {
>>> -        ret = -errno;
>>> +        error_setg_errno(errp, errno, "Failed to read: %s", path);
>>>       }
>>>     out:
>>>       close(config_fd);
>>> -    return ret;
>>>   }
[...]

Re: [Qemu-devel] [PATCH 3/4] hw/pci-host/xilinx: QOM'ify the AXI-PCIe host bridge

[Philippe Mathieu-Daudé]

On 12/18/2017 04:15 AM, Marcel Apfelbaum wrote:
> On 17/12/2017 22:49, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/pci-host/xilinx-pcie.c | 19 +++++++++----------
>>   1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
>> index 7659253090..756db39fd5 100644
>> --- a/hw/pci-host/xilinx-pcie.c
>> +++ b/hw/pci-host/xilinx-pcie.c
>> @@ -267,24 +267,23 @@ static void
>> xilinx_pcie_root_config_write(PCIDevice *d, uint32_t address,
>>       }
>>   }
>>   -static int xilinx_pcie_root_init(PCIDevice *dev)
>> +static void xilinx_pcie_root_realize(PCIDevice *pci, Error **errp)
>>   {
> 
> Same comment here, if "dev" seems not good enough maybe
> you can use the pci_dev convention.
> 
>> -    BusState *bus = qdev_get_parent_bus(DEVICE(dev));
>> +    DeviceState *dev = DEVICE(pci);
>> +    BusState *bus = qdev_get_parent_bus(dev);
> 
> Not sure the above worth the effort :)
> 
>>       XilinxPCIEHost *s = XILINX_PCIE_HOST(bus->parent);
>>   -    pci_set_word(dev->config + PCI_COMMAND,
>> +    pci_set_word(pci->config + PCI_COMMAND,
>>                    PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
>> -    pci_set_word(dev->config + PCI_MEMORY_BASE, s->mmio_base >> 16);
>> -    pci_set_word(dev->config + PCI_MEMORY_LIMIT,
>> +    pci_set_word(pci->config + PCI_MEMORY_BASE, s->mmio_base >> 16);
>> +    pci_set_word(pci->config + PCI_MEMORY_LIMIT,
>>                    ((s->mmio_base + s->mmio_size - 1) >> 16) & 0xfff0);
>>   -    pci_bridge_initfn(dev, TYPE_PCI_BUS);
>> +    pci_bridge_initfn(pci, TYPE_PCI_BUS);
>>   -    if (pcie_endpoint_cap_v1_init(dev, 0x80) < 0) {
>> +    if (pcie_endpoint_cap_v1_init(pci, 0x80) < 0) {
>>           hw_error("Failed to initialize PCIe capability");
> 
> I think here you need to use the error_setg (after/instead hw_error).
> It is supposed to return -1 if pcie capability was not added.

Oops I missed that, I'll fix in v2.

> Thanks,
> Marcel
> 
>>       }
>> -
>> -    return 0;
>>   }
>>     static void xilinx_pcie_root_class_init(ObjectClass *klass, void
>> *data)
>> @@ -300,7 +299,7 @@ static void
>> xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
>>       k->class_id = PCI_CLASS_BRIDGE_HOST;
>>       k->is_express = true;
>>       k->is_bridge = true;
>> -    k->init = xilinx_pcie_root_init;
>> +    k->realize = xilinx_pcie_root_realize;
>>       k->exit = pci_bridge_exitfn;
>>       dc->reset = pci_bridge_reset;
>>       k->config_read = xilinx_pcie_root_config_read;
>>

Re: [Qemu-devel] [PATCH 4/4] hw/pci: remove obsolete PCIDevice->init()

[Philippe Mathieu-Daudé]

Hi Michael, Paolo.

Now than all other PCI QOM'ification patches got applied, can you take
this patch for 2.12?

Thanks,

Phil.

On 12/17/2017 09:49 PM, Philippe Mathieu-Daudé wrote:
> All PCI devices are now QOM'ified.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/pci/pci.h |  1 -
>  hw/pci/pci.c         | 14 --------------
>  2 files changed, 15 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 8d02a0a383..0f1ed64c2f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -217,7 +217,6 @@ typedef struct PCIDeviceClass {
>      DeviceClass parent_class;
>  
>      void (*realize)(PCIDevice *dev, Error **errp);
> -    int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
>      PCIUnregisterFunc *exit;
>      PCIConfigReadFunc *config_read;
>      PCIConfigWriteFunc *config_write;
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b2d139bd9a..cd25ab6f6b 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2054,18 +2054,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>      }
>  }
>  
> -static void pci_default_realize(PCIDevice *dev, Error **errp)
> -{
> -    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> -
> -    if (pc->init) {
> -        if (pc->init(dev) < 0) {
> -            error_setg(errp, "Device initialization failed");
> -            return;
> -        }
> -    }
> -}
> -
>  PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
>                                      const char *name)
>  {
> @@ -2538,13 +2526,11 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>  static void pci_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *k = DEVICE_CLASS(klass);
> -    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
>  
>      k->realize = pci_qdev_realize;
>      k->unrealize = pci_qdev_unrealize;
>      k->bus_type = TYPE_PCI_BUS;
>      k->props = pci_props;
> -    pc->realize = pci_default_realize;
>  }
>  
>  static void pci_device_class_base_init(ObjectClass *klass, void *data)
> 

Re: [Qemu-devel] [PATCH 4/4] hw/pci: remove obsolete PCIDevice->init()

[Michael S. Tsirkin]

On Sun, Mar 11, 2018 at 04:46:52PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Michael, Paolo.
> 
> Now than all other PCI QOM'ification patches got applied, can you take
> this patch for 2.12?
> 
> Thanks,
> 
> Phil.
> 
> On 12/17/2017 09:49 PM, Philippe Mathieu-Daudé wrote:
> > All PCI devices are now QOM'ified.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  include/hw/pci/pci.h |  1 -
> >  hw/pci/pci.c         | 14 --------------
> >  2 files changed, 15 deletions(-)
> > 
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 8d02a0a383..0f1ed64c2f 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -217,7 +217,6 @@ typedef struct PCIDeviceClass {
> >      DeviceClass parent_class;
> >  
> >      void (*realize)(PCIDevice *dev, Error **errp);
> > -    int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> >      PCIUnregisterFunc *exit;
> >      PCIConfigReadFunc *config_read;
> >      PCIConfigWriteFunc *config_write;
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index b2d139bd9a..cd25ab6f6b 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -2054,18 +2054,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >      }
> >  }
> >  
> > -static void pci_default_realize(PCIDevice *dev, Error **errp)
> > -{
> > -    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > -
> > -    if (pc->init) {
> > -        if (pc->init(dev) < 0) {
> > -            error_setg(errp, "Device initialization failed");
> > -            return;
> > -        }
> > -    }
> > -}
> > -
> >  PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
> >                                      const char *name)
> >  {
> > @@ -2538,13 +2526,11 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
> >  static void pci_device_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *k = DEVICE_CLASS(klass);
> > -    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
> >  
> >      k->realize = pci_qdev_realize;
> >      k->unrealize = pci_qdev_unrealize;
> >      k->bus_type = TYPE_PCI_BUS;
> >      k->props = pci_props;
> > -    pc->realize = pci_default_realize;

How about we assert !init here?

+	assert(!pc->init);

> >  }
> >  
> >  static void pci_device_class_base_init(ObjectClass *klass, void *data)
> > 

Re: [Qemu-devel] [PATCH 4/4] hw/pci: remove obsolete PCIDevice->init()

[Michael S. Tsirkin]

On Mon, Mar 12, 2018 at 02:03:17AM +0200, Michael S. Tsirkin wrote:
> On Sun, Mar 11, 2018 at 04:46:52PM +0100, Philippe Mathieu-Daudé wrote:
> > Hi Michael, Paolo.
> > 
> > Now than all other PCI QOM'ification patches got applied, can you take
> > this patch for 2.12?
> > 
> > Thanks,
> > 
> > Phil.
> > 
> > On 12/17/2017 09:49 PM, Philippe Mathieu-Daudé wrote:
> > > All PCI devices are now QOM'ified.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > >  include/hw/pci/pci.h |  1 -
> > >  hw/pci/pci.c         | 14 --------------
> > >  2 files changed, 15 deletions(-)
> > > 
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index 8d02a0a383..0f1ed64c2f 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -217,7 +217,6 @@ typedef struct PCIDeviceClass {
> > >      DeviceClass parent_class;
> > >  
> > >      void (*realize)(PCIDevice *dev, Error **errp);
> > > -    int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> > >      PCIUnregisterFunc *exit;
> > >      PCIConfigReadFunc *config_read;
> > >      PCIConfigWriteFunc *config_write;
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index b2d139bd9a..cd25ab6f6b 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -2054,18 +2054,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > >      }
> > >  }
> > >  
> > > -static void pci_default_realize(PCIDevice *dev, Error **errp)
> > > -{
> > > -    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > > -
> > > -    if (pc->init) {
> > > -        if (pc->init(dev) < 0) {
> > > -            error_setg(errp, "Device initialization failed");
> > > -            return;
> > > -        }
> > > -    }
> > > -}
> > > -
> > >  PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
> > >                                      const char *name)
> > >  {
> > > @@ -2538,13 +2526,11 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
> > >  static void pci_device_class_init(ObjectClass *klass, void *data)
> > >  {
> > >      DeviceClass *k = DEVICE_CLASS(klass);
> > > -    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
> > >  
> > >      k->realize = pci_qdev_realize;
> > >      k->unrealize = pci_qdev_unrealize;
> > >      k->bus_type = TYPE_PCI_BUS;
> > >      k->props = pci_props;
> > > -    pc->realize = pci_default_realize;
> 
> How about we assert !init here?
> 
> +	assert(!pc->init);

Oh sorry I missed the chunk that drops it completely.
Will apply.

> > >  }
> > >  
> > >  static void pci_device_class_base_init(ObjectClass *klass, void *data)
> > > 
> 
> 

Re: [Qemu-devel] [PATCH 4/4] hw/pci: remove obsolete PCIDevice->init()

[Philippe Mathieu-Daudé]

On 03/12/2018 01:04 AM, Michael S. Tsirkin wrote:
> On Mon, Mar 12, 2018 at 02:03:17AM +0200, Michael S. Tsirkin wrote:
>> On Sun, Mar 11, 2018 at 04:46:52PM +0100, Philippe Mathieu-Daudé wrote:
>>> Hi Michael, Paolo.
>>>
>>> Now than all other PCI QOM'ification patches got applied, can you take
>>> this patch for 2.12?
>>>
>>> Thanks,
>>>
>>> Phil.
>>>
>>> On 12/17/2017 09:49 PM, Philippe Mathieu-Daudé wrote:
>>>> All PCI devices are now QOM'ified.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  include/hw/pci/pci.h |  1 -
>>>>  hw/pci/pci.c         | 14 --------------
>>>>  2 files changed, 15 deletions(-)
>>>>
>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>>> index 8d02a0a383..0f1ed64c2f 100644
>>>> --- a/include/hw/pci/pci.h
>>>> +++ b/include/hw/pci/pci.h
>>>> @@ -217,7 +217,6 @@ typedef struct PCIDeviceClass {
>>>>      DeviceClass parent_class;
>>>>  
>>>>      void (*realize)(PCIDevice *dev, Error **errp);
>>>> -    int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
>>>>      PCIUnregisterFunc *exit;
>>>>      PCIConfigReadFunc *config_read;
>>>>      PCIConfigWriteFunc *config_write;
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index b2d139bd9a..cd25ab6f6b 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -2054,18 +2054,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>>      }
>>>>  }
>>>>  
>>>> -static void pci_default_realize(PCIDevice *dev, Error **errp)
>>>> -{
>>>> -    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
>>>> -
>>>> -    if (pc->init) {
>>>> -        if (pc->init(dev) < 0) {
>>>> -            error_setg(errp, "Device initialization failed");
>>>> -            return;
>>>> -        }
>>>> -    }
>>>> -}
>>>> -
>>>>  PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
>>>>                                      const char *name)
>>>>  {
>>>> @@ -2538,13 +2526,11 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
>>>>  static void pci_device_class_init(ObjectClass *klass, void *data)
>>>>  {
>>>>      DeviceClass *k = DEVICE_CLASS(klass);
>>>> -    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
>>>>  
>>>>      k->realize = pci_qdev_realize;
>>>>      k->unrealize = pci_qdev_unrealize;
>>>>      k->bus_type = TYPE_PCI_BUS;
>>>>      k->props = pci_props;
>>>> -    pc->realize = pci_default_realize;
>>
>> How about we assert !init here?
>>
>> +	assert(!pc->init);
> 
> Oh sorry I missed the chunk that drops it completely.
> Will apply.

Thanks!

> 
>>>>  }
>>>>  
>>>>  static void pci_device_class_base_init(ObjectClass *klass, void *data)
>>>>
>>
>>