[Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Cao jin]

v6 changelog:
1. split modification of xen_host_pci_sysfs_path() into a separate new patch
   as 1/6 shows.
2. 'bug' fix of qemu_strtoul(), in patch 2/6 & 3/6
3. Grammar fix in patch 4/6
4. 'msg' --> 'message' in commit message.

Cao jin (6):
  Change xen_host_pci_sysfs_path() to return void
  Xen: use qemu_strtoul instead of strtol
  Add Error **errp for xen_host_pci_device_get()
  Add Error **errp for xen_pt_setup_vga()
  Add Error **errp for xen_pt_config_init()
  Xen PCI passthru: convert to realize()

 hw/xen/xen-host-pci-device.c | 149 +++++++++++++++++++++----------------------
 hw/xen/xen-host-pci-device.h |   5 +-
 hw/xen/xen_pt.c              |  77 ++++++++++++----------
 hw/xen/xen_pt.h              |   5 +-
 hw/xen/xen_pt_config_init.c  |  51 ++++++++-------
 hw/xen/xen_pt_graphics.c     |  11 ++--
 6 files changed, 155 insertions(+), 143 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v6 1/6] Change xen_host_pci_sysfs_path() to return void

[Cao jin]

And assert the snprintf() error, because user can do nothing in case of
snprintf() fail.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen-host-pci-device.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 7d8a023..9c342e7 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -31,19 +31,14 @@
 #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
 #define IORESOURCE_MEM_64       0x00100000
 
-static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
-                                   const char *name, char *buf, ssize_t size)
+static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
+                                    const char *name, char *buf, ssize_t size)
 {
     int rc;
 
     rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
                   d->domain, d->bus, d->dev, d->func, name);
-
-    if (rc >= size || rc < 0) {
-        /* The output is truncated, or some other error was encountered */
-        return -ENODEV;
-    }
-    return 0;
+    assert(rc >= 0 && rc < size);
 }
 
 
@@ -58,10 +53,8 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
     char *endptr, *s;
     uint8_t type;
 
-    rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
-    if (rc) {
-        return rc;
-    }
+    xen_host_pci_sysfs_path(d, "resource", path, sizeof(path));
+
     fd = open(path, O_RDONLY);
     if (fd == -1) {
         XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
@@ -150,10 +143,8 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
     unsigned long value;
     char *endptr;
 
-    rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
-    if (rc) {
-        return rc;
-    }
+    xen_host_pci_sysfs_path(d, name, path, sizeof(path));
+
     fd = open(path, O_RDONLY);
     if (fd == -1) {
         XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
@@ -200,21 +191,17 @@ static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
     char path[PATH_MAX];
     struct stat buf;
 
-    if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path))) {
-        return false;
-    }
+    xen_host_pci_sysfs_path(d, "physfn", path, sizeof(path));
+
     return !stat(path, &buf);
 }
 
 static int xen_host_pci_config_open(XenHostPCIDevice *d)
 {
     char path[PATH_MAX];
-    int rc;
 
-    rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
-    if (rc) {
-        return rc;
-    }
+    xen_host_pci_sysfs_path(d, "config", path, sizeof(path));
+
     d->config_fd = open(path, O_RDWR);
     if (d->config_fd < 0) {
         return -errno;
-- 
2.1.0

[Qemu-devel] [PATCH v6 2/6] Xen: use qemu_strtoul instead of strtol

[Cao jin]

No need to roll our own (with slightly incorrect handling of errno),
when we can use the common version.

Change signed parsing to unsigned, because what it read are values in
PCI config space, which are non-negative.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen-host-pci-device.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 9c342e7..83da9c4 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -141,7 +141,7 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
     char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
     int fd, rc;
     unsigned long value;
-    char *endptr;
+    const char *endptr;
 
     xen_host_pci_sysfs_path(d, name, path, sizeof(path));
 
@@ -158,13 +158,9 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
         }
     } while (rc < 0);
     buf[rc] = 0;
-    value = strtol(buf, &endptr, base);
-    if (endptr == buf || *endptr != '\n') {
-        rc = -1;
-    } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
-        rc = -errno;
-    } else {
-        rc = 0;
+    rc = qemu_strtoul(buf, &endptr, base, &value);
+    if (!rc) {
+        assert(value <= UINT_MAX);
         *pvalue = value;
     }
 out:
-- 
2.1.0

[Qemu-devel] [PATCH v6 3/6] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

To catch the error message. Also modify the caller

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen-host-pci-device.c | 102 ++++++++++++++++++++++++-------------------
 hw/xen/xen-host-pci-device.h |   5 ++-
 hw/xen/xen_pt.c              |  13 +++---
 3 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 83da9c4..3827ca7 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -44,7 +44,7 @@ static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
 
 /* This size should be enough to read the first 7 lines of a resource file */
 #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
-static int xen_host_pci_get_resource(XenHostPCIDevice *d)
+static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
 {
     int i, rc, fd;
     char path[PATH_MAX];
@@ -57,19 +57,18 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
 
     fd = open(path, O_RDONLY);
     if (fd == -1) {
-        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
-        return -errno;
+        error_setg_file_open(errp, errno, path);
+        return;
     }
 
     do {
-        rc = read(fd, &buf, sizeof (buf) - 1);
+        rc = read(fd, &buf, sizeof(buf) - 1);
         if (rc < 0 && errno != EINTR) {
-            rc = -errno;
+            error_setg_errno(errp, errno, "read err");
             goto out;
         }
     } while (rc < 0);
     buf[rc] = 0;
-    rc = 0;
 
     s = buf;
     for (i = 0; i < PCI_NUM_REGIONS; i++) {
@@ -122,20 +121,19 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
             d->rom.bus_flags = flags & IORESOURCE_BITS;
         }
     }
+
     if (i != PCI_NUM_REGIONS) {
-        /* Invalid format or input to short */
-        rc = -ENODEV;
+        error_setg(errp, "Invalid format or input too short: %s", buf);
     }
 
 out:
     close(fd);
-    return rc;
 }
 
 /* This size should be enough to read a long from a file */
 #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
-static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
-                                  unsigned int *pvalue, int base)
+static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
+                                   unsigned int *pvalue, int base, Error **errp)
 {
     char path[PATH_MAX];
     char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
@@ -147,39 +145,45 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
 
     fd = open(path, O_RDONLY);
     if (fd == -1) {
-        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
-        return -errno;
+        error_setg_file_open(errp, errno, path);
+        return;
     }
+
     do {
-        rc = read(fd, &buf, sizeof (buf) - 1);
+        rc = read(fd, &buf, sizeof(buf) - 1);
         if (rc < 0 && errno != EINTR) {
-            rc = -errno;
+            error_setg_errno(errp, errno, "read err");
             goto out;
         }
     } while (rc < 0);
+
     buf[rc] = 0;
     rc = qemu_strtoul(buf, &endptr, base, &value);
     if (!rc) {
         assert(value <= UINT_MAX);
         *pvalue = value;
+    } else {
+        error_setg_errno(errp, -rc, "failed to parse value '%s'", buf);
     }
+
 out:
     close(fd);
-    return rc;
 }
 
-static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
-                                             const char *name,
-                                             unsigned int *pvalue)
+static inline void xen_host_pci_get_hex_value(XenHostPCIDevice *d,
+                                              const char *name,
+                                              unsigned int *pvalue,
+                                              Error **errp)
 {
-    return xen_host_pci_get_value(d, name, pvalue, 16);
+    xen_host_pci_get_value(d, name, pvalue, 16, errp);
 }
 
-static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
-                                             const char *name,
-                                             unsigned int *pvalue)
+static inline void xen_host_pci_get_dec_value(XenHostPCIDevice *d,
+                                              const char *name,
+                                              unsigned int *pvalue,
+                                              Error **errp)
 {
-    return xen_host_pci_get_value(d, name, pvalue, 10);
+    xen_host_pci_get_value(d, name, pvalue, 10, errp);
 }
 
 static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
@@ -192,17 +196,16 @@ static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
     return !stat(path, &buf);
 }
 
-static int xen_host_pci_config_open(XenHostPCIDevice *d)
+static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
 {
     char path[PATH_MAX];
 
     xen_host_pci_sysfs_path(d, "config", path, sizeof(path));
 
     d->config_fd = open(path, O_RDWR);
-    if (d->config_fd < 0) {
-        return -errno;
+    if (d->config_fd == -1) {
+        error_setg_file_open(errp, errno, path);
     }
-    return 0;
 }
 
 static int xen_host_pci_config_read(XenHostPCIDevice *d,
@@ -324,11 +327,12 @@ int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
     return -1;
 }
 
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
-                            uint8_t bus, uint8_t dev, uint8_t func)
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+                             uint8_t bus, uint8_t dev, uint8_t func,
+                             Error **errp)
 {
     unsigned int v;
-    int rc = 0;
+    Error *err = NULL;
 
     d->config_fd = -1;
     d->domain = domain;
@@ -336,43 +340,51 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
     d->dev = dev;
     d->func = func;
 
-    rc = xen_host_pci_config_open(d);
-    if (rc) {
+    xen_host_pci_config_open(d, &err);
+    if (err) {
         goto error;
     }
-    rc = xen_host_pci_get_resource(d);
-    if (rc) {
+
+    xen_host_pci_get_resource(d, &err);
+    if (err) {
         goto error;
     }
-    rc = xen_host_pci_get_hex_value(d, "vendor", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "vendor", &v, &err);
+    if (err) {
         goto error;
     }
     d->vendor_id = v;
-    rc = xen_host_pci_get_hex_value(d, "device", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "device", &v, &err);
+    if (err) {
         goto error;
     }
     d->device_id = v;
-    rc = xen_host_pci_get_dec_value(d, "irq", &v);
-    if (rc) {
+
+    xen_host_pci_get_dec_value(d, "irq", &v, &err);
+    if (err) {
         goto error;
     }
     d->irq = v;
-    rc = xen_host_pci_get_hex_value(d, "class", &v);
-    if (rc) {
+
+    xen_host_pci_get_hex_value(d, "class", &v, &err);
+    if (err) {
         goto error;
     }
     d->class_code = v;
+
     d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
 
-    return 0;
+    return;
+
 error:
+    error_propagate(errp, err);
+
     if (d->config_fd >= 0) {
         close(d->config_fd);
         d->config_fd = -1;
     }
-    return rc;
 }
 
 bool xen_host_pci_device_closed(XenHostPCIDevice *d)
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index 3d44e04..6acf36e 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -36,8 +36,9 @@ typedef struct XenHostPCIDevice {
     int config_fd;
 } XenHostPCIDevice;
 
-int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
-                            uint8_t bus, uint8_t dev, uint8_t func);
+void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
+                             uint8_t bus, uint8_t dev, uint8_t func,
+                             Error **errp);
 void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
 bool xen_host_pci_device_closed(XenHostPCIDevice *d);
 
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index aa96288..53b5bca 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -767,6 +767,7 @@ static int xen_pt_initfn(PCIDevice *d)
     uint8_t machine_irq = 0, scratch;
     uint16_t cmd = 0;
     int pirq = XEN_PT_UNASSIGNED_PIRQ;
+    Error *err = NULL;
 
     /* register real device */
     XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
@@ -774,11 +775,13 @@ static int xen_pt_initfn(PCIDevice *d)
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
                s->dev.devfn);
 
-    rc = xen_host_pci_device_get(&s->real_device,
-                                 s->hostaddr.domain, s->hostaddr.bus,
-                                 s->hostaddr.slot, s->hostaddr.function);
-    if (rc) {
-        XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
+    xen_host_pci_device_get(&s->real_device,
+                            s->hostaddr.domain, s->hostaddr.bus,
+                            s->hostaddr.slot, s->hostaddr.function,
+                            &err);
+    if (err) {
+        error_append_hint(&err, "Failed to \"open\" the real pci device");
+        error_report_err(err);
         return -1;
     }
 
-- 
2.1.0

[Qemu-devel] [PATCH v6 4/6] Add Error **errp for xen_pt_setup_vga()

[Cao jin]

To catch the error message. Also modify the caller

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/xen/xen_pt.c          |  7 +++++--
 hw/xen/xen_pt.h          |  3 ++-
 hw/xen/xen_pt_graphics.c | 11 ++++++-----
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 53b5bca..07bfcec 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -808,8 +808,11 @@ static int xen_pt_initfn(PCIDevice *d)
             return -1;
         }
 
-        if (xen_pt_setup_vga(s, &s->real_device) < 0) {
-            XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+        xen_pt_setup_vga(s, &s->real_device, &err);
+        if (err) {
+            error_append_hint(&err, "Setup VGA BIOS of passthrough"
+                    " GFX failed");
+            error_report_err(err);
             xen_host_pci_device_put(&s->real_device);
             return -1;
         }
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 3749711..26f74f8 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -330,5 +330,6 @@ static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
 }
 int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
 int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);
-int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev);
+void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
+                     Error **errp);
 #endif /* !XEN_PT_H */
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index df6069b..e7a7c7e 100644
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -161,7 +161,8 @@ struct pci_data {
     uint16_t reserved;
 } __attribute__((packed));
 
-int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
+void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
+                     Error **errp)
 {
     unsigned char *bios = NULL;
     struct rom_header *rom;
@@ -172,13 +173,14 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
     struct pci_data *pd = NULL;
 
     if (!is_igd_vga_passthrough(dev)) {
-        return -1;
+        error_setg(errp, "Need to enable igd-passthrough");
+        return;
     }
 
     bios = get_vgabios(s, &bios_size, dev);
     if (!bios) {
-        XEN_PT_ERR(&s->dev, "VGA: Can't getting VBIOS!\n");
-        return -1;
+        error_setg(errp, "VGA: Can't get VBIOS");
+        return;
     }
 
     /* Currently we fixed this address as a primary. */
@@ -203,7 +205,6 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
 
     /* Currently we fixed this address as a primary for legacy BIOS. */
     cpu_physical_memory_rw(0xc0000, bios, bios_size, 1);
-    return 0;
 }
 
 uint32_t igd_read_opregion(XenPCIPassthroughState *s)
-- 
2.1.0

[Qemu-devel] [PATCH v6 5/6] Add Error **errp for xen_pt_config_init()

[Cao jin]

To catch the error message. Also modify the caller

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/xen/xen_pt.c             |  8 ++++---
 hw/xen/xen_pt.h             |  2 +-
 hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++---------------------
 3 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 07bfcec..9eef3df 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -825,9 +825,11 @@ static int xen_pt_initfn(PCIDevice *d)
     xen_pt_register_regions(s, &cmd);
 
     /* reinitialize each config register to be emulated */
-    rc = xen_pt_config_init(s);
-    if (rc) {
-        XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
+    xen_pt_config_init(s, &err);
+    if (err) {
+        error_append_hint(&err, "PCI Config space initialisation failed");
+        error_report_err(err);
+        rc = -1;
         goto err_out;
     }
 
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 26f74f8..c2f8e1f 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -230,7 +230,7 @@ struct XenPCIPassthroughState {
     bool listener_set;
 };
 
-int xen_pt_config_init(XenPCIPassthroughState *s);
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp);
 void xen_pt_config_delete(XenPCIPassthroughState *s);
 XenPTRegGroup *xen_pt_find_reg_grp(XenPCIPassthroughState *s, uint32_t address);
 XenPTReg *xen_pt_find_reg(XenPTRegGroup *reg_grp, uint32_t address);
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 185a698..81c6721 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1887,8 +1887,9 @@ static uint8_t find_cap_offset(XenPCIPassthroughState *s, uint8_t cap)
     return 0;
 }
 
-static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
-                                  XenPTRegGroup *reg_grp, XenPTRegInfo *reg)
+static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
+                                   XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
+                                   Error **errp)
 {
     XenPTReg *reg_entry;
     uint32_t data = 0;
@@ -1907,12 +1908,13 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
                        reg_grp->base_offset + reg->offset, &data);
         if (rc < 0) {
             g_free(reg_entry);
-            return rc;
+            error_setg(errp, "Init emulate register fail");
+            return;
         }
         if (data == XEN_PT_INVALID_REG) {
             /* free unused BAR register entry */
             g_free(reg_entry);
-            return 0;
+            return;
         }
         /* Sync up the data to dev.config */
         offset = reg_grp->base_offset + reg->offset;
@@ -1930,7 +1932,8 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
         if (rc) {
             /* Serious issues when we cannot read the host values! */
             g_free(reg_entry);
-            return rc;
+            error_setg(errp, "Cannot read host values");
+            return;
         }
         /* Set bits in emu_mask are the ones we emulate. The dev.config shall
          * contain the emulated view of the guest - therefore we flip the mask
@@ -1955,10 +1958,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
             val = data;
 
         if (val & ~size_mask) {
-            XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
-                       offset, val, reg->size);
+            error_setg(errp, "Offset 0x%04x:0x%04x expands past"
+                    " register size (%d)", offset, val, reg->size);
             g_free(reg_entry);
-            return -ENXIO;
+            return;
         }
         /* This could be just pci_set_long as we don't modify the bits
          * past reg->size, but in case this routine is run in parallel or the
@@ -1978,13 +1981,12 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
     }
     /* list add register entry */
     QLIST_INSERT_HEAD(&reg_grp->reg_tbl_list, reg_entry, entries);
-
-    return 0;
 }
 
-int xen_pt_config_init(XenPCIPassthroughState *s)
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
 {
     int i, rc;
+    Error *err = NULL;
 
     QLIST_INIT(&s->reg_grps);
 
@@ -2027,11 +2029,12 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
                                                   reg_grp_offset,
                                                   &reg_grp_entry->size);
             if (rc < 0) {
-                XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
-                           i, ARRAY_SIZE(xen_pt_emu_reg_grps),
+                error_setg(&err, "Failed to initialize %d/%zu, type = 0x%x,"
+                           " rc: %d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
                            xen_pt_emu_reg_grps[i].grp_type, rc);
+                error_propagate(errp, err);
                 xen_pt_config_delete(s);
-                return rc;
+                return;
             }
         }
 
@@ -2039,24 +2042,24 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
             if (xen_pt_emu_reg_grps[i].emu_regs) {
                 int j = 0;
                 XenPTRegInfo *regs = xen_pt_emu_reg_grps[i].emu_regs;
+
                 /* initialize capability register */
                 for (j = 0; regs->size != 0; j++, regs++) {
-                    /* initialize capability register */
-                    rc = xen_pt_config_reg_init(s, reg_grp_entry, regs);
-                    if (rc < 0) {
-                        XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
-                                   j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
-                                   regs->offset, xen_pt_emu_reg_grps[i].grp_type,
-                                   i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
+                    xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
+                    if (err) {
+                        error_append_hint(&err, "Failed to initialize %d/%zu"
+                                " reg 0x%x in grp_type = 0x%x (%d/%zu)",
+                                j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
+                                regs->offset, xen_pt_emu_reg_grps[i].grp_type,
+                                i, ARRAY_SIZE(xen_pt_emu_reg_grps));
+                        error_propagate(errp, err);
                         xen_pt_config_delete(s);
-                        return rc;
+                        return;
                     }
                 }
             }
         }
     }
-
-    return 0;
 }
 
 /* delete all emulate register */
-- 
2.1.0

[Qemu-devel] [PATCH v6 6/6] Xen PCI passthru: convert to realize()

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/xen/xen_pt.c | 53 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 9eef3df..d33221b 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -760,10 +760,10 @@ static void xen_pt_destroy(PCIDevice *d) {
 }
 /* init */
 
-static int xen_pt_initfn(PCIDevice *d)
+static void xen_pt_realize(PCIDevice *d, Error **errp)
 {
     XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
-    int rc = 0;
+    int i, rc = 0;
     uint8_t machine_irq = 0, scratch;
     uint16_t cmd = 0;
     int pirq = XEN_PT_UNASSIGNED_PIRQ;
@@ -781,8 +781,8 @@ static int xen_pt_initfn(PCIDevice *d)
                             &err);
     if (err) {
         error_append_hint(&err, "Failed to \"open\" the real pci device");
-        error_report_err(err);
-        return -1;
+        error_propagate(errp, err);
+        return;
     }
 
     s->is_virtfn = s->real_device.is_virtfn;
@@ -802,19 +802,19 @@ static int xen_pt_initfn(PCIDevice *d)
     if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
         (s->real_device.dev == 2) && (s->real_device.func == 0)) {
         if (!is_igd_vga_passthrough(&s->real_device)) {
-            XEN_PT_ERR(d, "Need to enable igd-passthru if you're trying"
-                       " to passthrough IGD GFX.\n");
+            error_setg(errp, "Need to enable igd-passthru if you're trying"
+                    " to passthrough IGD GFX");
             xen_host_pci_device_put(&s->real_device);
-            return -1;
+            return;
         }
 
         xen_pt_setup_vga(s, &s->real_device, &err);
         if (err) {
             error_append_hint(&err, "Setup VGA BIOS of passthrough"
                     " GFX failed");
-            error_report_err(err);
+            error_propagate(errp, err);
             xen_host_pci_device_put(&s->real_device);
-            return -1;
+            return;
         }
 
         /* Register ISA bridge for passthrough GFX. */
@@ -836,20 +836,19 @@ static int xen_pt_initfn(PCIDevice *d)
     /* Bind interrupt */
     rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
     if (rc) {
-        XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
+        error_setg_errno(errp, errno, "Failed to read PCI_INTERRUPT_PIN");
         goto err_out;
     }
     if (!scratch) {
-        XEN_PT_LOG(d, "no pin interrupt\n");
+        error_setg(errp, "no pin interrupt");
         goto out;
     }
 
     machine_irq = s->real_device.irq;
     rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
-
     if (rc < 0) {
-        XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
-                   machine_irq, pirq, errno);
+        error_setg_errno(errp, errno, "Mapping machine irq %u to"
+                         " pirq %i failed", machine_irq, pirq);
 
         /* Disable PCI intx assertion (turn on bit10 of devctl) */
         cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -870,8 +869,8 @@ static int xen_pt_initfn(PCIDevice *d)
                                        PCI_SLOT(d->devfn),
                                        e_intx);
         if (rc < 0) {
-            XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
-                       e_intx, errno);
+            error_setg_errno(errp, errno, "Binding of interrupt %u failed",
+                             e_intx);
 
             /* Disable PCI intx assertion (turn on bit10 of devctl) */
             cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -879,8 +878,8 @@ static int xen_pt_initfn(PCIDevice *d)
 
             if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
                 if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
-                    XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
-                               " (err: %d)\n", machine_irq, errno);
+                    error_setg_errno(errp, errno, "Unmapping of machine"
+                            " interrupt %u failed", machine_irq);
                 }
             }
             s->machine_irq = 0;
@@ -893,14 +892,14 @@ out:
 
         rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
         if (rc) {
-            XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
+            error_setg_errno(errp, errno, "Failed to read PCI_COMMAND");
             goto err_out;
         } else {
             val |= cmd;
             rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
             if (rc) {
-                XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
-                           val, rc);
+                error_setg_errno(errp, errno, "Failed to write PCI_COMMAND"
+                                 " val = 0x%x", val);
                 goto err_out;
             }
         }
@@ -910,15 +909,19 @@ out:
     memory_listener_register(&s->io_listener, &address_space_io);
     s->listener_set = true;
     XEN_PT_LOG(d,
-               "Real physical device %02x:%02x.%d registered successfully!\n",
+               "Real physical device %02x:%02x.%d registered successfully\n",
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
 
-    return 0;
+    return;
 
 err_out:
+    for (i = 0; i < PCI_ROM_SLOT; i++) {
+        object_unparent(OBJECT(&s->bar[i]));
+    }
+    object_unparent(OBJECT(&s->rom));
+
     xen_pt_destroy(d);
     assert(rc);
-    return rc;
 }
 
 static void xen_pt_unregister_device(PCIDevice *d)
@@ -937,7 +940,7 @@ static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = xen_pt_initfn;
+    k->realize = xen_pt_realize;
     k->exit = xen_pt_unregister_device;
     k->config_read = xen_pt_pci_read_config;
     k->config_write = xen_pt_pci_write_config;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v6 1/6] Change xen_host_pci_sysfs_path() to return void

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

On 01/17/2016 05:13 AM, Cao jin wrote:
> And assert the snprintf() error, because user can do nothing in case of
> snprintf() fail.

s/fail/failure/

> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen-host-pci-device.c | 35 +++++++++++------------------------
>  1 file changed, 11 insertions(+), 24 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Cao jin]

Seems I forgot to CC Eric, but I checked my command line history, I do 
include Eric...weird

On 01/17/2016 08:13 PM, Cao jin wrote:
> v6 changelog:
> 1. split modification of xen_host_pci_sysfs_path() into a separate new patch
>     as 1/6 shows.
> 2. 'bug' fix of qemu_strtoul(), in patch 2/6 & 3/6
> 3. Grammar fix in patch 4/6
> 4. 'msg' --> 'message' in commit message.
>
> Cao jin (6):
>    Change xen_host_pci_sysfs_path() to return void
>    Xen: use qemu_strtoul instead of strtol
>    Add Error **errp for xen_host_pci_device_get()
>    Add Error **errp for xen_pt_setup_vga()
>    Add Error **errp for xen_pt_config_init()
>    Xen PCI passthru: convert to realize()
>
>   hw/xen/xen-host-pci-device.c | 149 +++++++++++++++++++++----------------------
>   hw/xen/xen-host-pci-device.h |   5 +-
>   hw/xen/xen_pt.c              |  77 ++++++++++++----------
>   hw/xen/xen_pt.h              |   5 +-
>   hw/xen/xen_pt_config_init.c  |  51 ++++++++-------
>   hw/xen/xen_pt_graphics.c     |  11 ++--
>   6 files changed, 155 insertions(+), 143 deletions(-)
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Markus Armbruster]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> Seems I forgot to CC Eric, but I checked my command line history, I do
> include Eric...weird

Probably the broken duplicate suppression feature.  Eric, are you sure
the reduction in e-mail for you is worth the occasional confusion for
others?

Re: [Qemu-devel] [PATCH v6 2/6] Xen: use qemu_strtoul instead of strtol

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On 01/17/2016 05:13 AM, Cao jin wrote:
> No need to roll our own (with slightly incorrect handling of errno),
> when we can use the common version.
> 
> Change signed parsing to unsigned, because what it read are values in
> PCI config space, which are non-negative.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen-host-pci-device.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Cao jin]

Hi Stefano,

     Just FYI: The series have been reviewed by Eric, I guess it could 
be picked up;)

On 01/17/2016 08:13 PM, Cao jin wrote:
> v6 changelog:
> 1. split modification of xen_host_pci_sysfs_path() into a separate new patch
>     as 1/6 shows.
> 2. 'bug' fix of qemu_strtoul(), in patch 2/6 & 3/6
> 3. Grammar fix in patch 4/6
> 4. 'msg' --> 'message' in commit message.
>
> Cao jin (6):
>    Change xen_host_pci_sysfs_path() to return void
>    Xen: use qemu_strtoul instead of strtol
>    Add Error **errp for xen_host_pci_device_get()
>    Add Error **errp for xen_pt_setup_vga()
>    Add Error **errp for xen_pt_config_init()
>    Xen PCI passthru: convert to realize()
>
>   hw/xen/xen-host-pci-device.c | 149 +++++++++++++++++++++----------------------
>   hw/xen/xen-host-pci-device.h |   5 +-
>   hw/xen/xen_pt.c              |  77 ++++++++++++----------
>   hw/xen/xen_pt.h              |   5 +-
>   hw/xen/xen_pt_config_init.c  |  51 ++++++++-------
>   hw/xen/xen_pt_graphics.c     |  11 ++--
>   6 files changed, 155 insertions(+), 143 deletions(-)
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Stefano Stabellini]

Hi Cao,

I appreciate the reminder, but it looks like Eric hasn't reviewed patch
3/5. Am I wrong?

On Thu, 21 Jan 2016, Cao jin wrote:
> Hi Stefano,
> 
>     Just FYI: The series have been reviewed by Eric, I guess it could be
> picked up;)
> 
> On 01/17/2016 08:13 PM, Cao jin wrote:
> > v6 changelog:
> > 1. split modification of xen_host_pci_sysfs_path() into a separate new patch
> >     as 1/6 shows.
> > 2. 'bug' fix of qemu_strtoul(), in patch 2/6 & 3/6
> > 3. Grammar fix in patch 4/6
> > 4. 'msg' --> 'message' in commit message.
> > 
> > Cao jin (6):
> >    Change xen_host_pci_sysfs_path() to return void
> >    Xen: use qemu_strtoul instead of strtol
> >    Add Error **errp for xen_host_pci_device_get()
> >    Add Error **errp for xen_pt_setup_vga()
> >    Add Error **errp for xen_pt_config_init()
> >    Xen PCI passthru: convert to realize()
> > 
> >   hw/xen/xen-host-pci-device.c | 149
> > +++++++++++++++++++++----------------------
> >   hw/xen/xen-host-pci-device.h |   5 +-
> >   hw/xen/xen_pt.c              |  77 ++++++++++++----------
> >   hw/xen/xen_pt.h              |   5 +-
> >   hw/xen/xen_pt_config_init.c  |  51 ++++++++-------
> >   hw/xen/xen_pt_graphics.c     |  11 ++--
> >   6 files changed, 155 insertions(+), 143 deletions(-)
> > 
> 
> -- 
> Yours Sincerely,
> 
> Cao jin
> 
> 
> 

Re: [Qemu-devel] [PATCH v6 3/6] Add Error **errp for xen_host_pci_device_get()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

On 01/17/2016 05:13 AM, Cao jin wrote:
> To catch the error message. Also modify the caller
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen-host-pci-device.c | 102 ++++++++++++++++++++++++-------------------
>  hw/xen/xen-host-pci-device.h |   5 ++-
>  hw/xen/xen_pt.c              |  13 +++---
>  3 files changed, 68 insertions(+), 52 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On 01/21/2016 08:41 AM, Stefano Stabellini wrote:
> Hi Cao,
> 
> I appreciate the reminder, but it looks like Eric hasn't reviewed patch
> 3/5. Am I wrong?

I've done it now. Thanks for your patience, and for letting me jump in
with a late review (since I remember you were about to push an earlier
version when I started looking at it).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Stefano Stabellini]

On Thu, 21 Jan 2016, Eric Blake wrote:
> On 01/21/2016 08:41 AM, Stefano Stabellini wrote:
> > Hi Cao,
> > 
> > I appreciate the reminder, but it looks like Eric hasn't reviewed patch
> > 3/5. Am I wrong?
> 
> I've done it now. Thanks for your patience, and for letting me jump in
> with a late review (since I remember you were about to push an earlier
> version when I started looking at it).
 
Thank you for reviewing :-)

Re: [Qemu-devel] [PATCH v6 0/6] Xen PCI passthru: Convert to realize()

[Cao jin]

On 01/21/2016 11:41 PM, Stefano Stabellini wrote:
> Hi Cao,
>
> I appreciate the reminder, but it looks like Eric hasn't reviewed patch
> 3/5. Am I wrong?
>

Seems I made a little mistake:-[
Already see your pull request, Thank you:)
And thanks for Eric`s review:)

-- 
Yours Sincerely,

Cao jin