* [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()
@ 2015-12-23 10:23 Cao jin
2015-12-23 12:03 ` Stefano Stabellini
0 siblings, 1 reply; 5+ messages in thread
From: Cao jin @ 2015-12-23 10:23 UTC (permalink / raw)
To: qemu-devel; +Cc: stefano.stabellini
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
Since the callchain is pretty deep & error path is very much too, so I made the
patch based on the principal: catch/report the most necessary error msg with
smallest modification.(So you can see I don`t change some functions to void,
despite they coule be)
hw/xen/xen-host-pci-device.c | 79 +++++++++++++++++++++++++++-----------------
hw/xen/xen-host-pci-device.h | 5 +--
hw/xen/xen_pt.c | 67 +++++++++++++++++++------------------
hw/xen/xen_pt.h | 5 +--
hw/xen/xen_pt_config_init.c | 47 +++++++++++++-------------
hw/xen/xen_pt_graphics.c | 6 ++--
6 files changed, 118 insertions(+), 91 deletions(-)
diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 7d8a023..1ab6d97 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -43,13 +43,14 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
/* The output is truncated, or some other error was encountered */
return -ENODEV;
}
+
return 0;
}
/* 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];
@@ -60,23 +61,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
if (rc) {
- return rc;
+ error_setg_errno(errp, errno, "snprintf err");
+ return;
}
+
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_errno(errp, errno, "open err");
+ return;
}
do {
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++) {
@@ -129,20 +131,20 @@ 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 to short");
}
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)
+ unsigned int *pvalue, int base, Error **errp)
{
char path[PATH_MAX];
char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
@@ -152,30 +154,38 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
if (rc) {
+ error_setg_errno(errp, errno, "snprintf err");
return rc;
}
+
fd = open(path, O_RDONLY);
if (fd == -1) {
- XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
+ error_setg_errno(errp, errno, "open err");
return -errno;
}
+
do {
rc = read(fd, &buf, sizeof (buf) - 1);
if (rc < 0 && errno != EINTR) {
+ error_setg_errno(errp, errno, "read err");
rc = -errno;
goto out;
}
} while (rc < 0);
+
buf[rc] = 0;
value = strtol(buf, &endptr, base);
if (endptr == buf || *endptr != '\n') {
+ error_setg(errp, "format invalid");
rc = -1;
} else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
+ error_setg_errno(errp, errno, "strtol err");
rc = -errno;
} else {
rc = 0;
*pvalue = value;
}
+
out:
close(fd);
return rc;
@@ -183,16 +193,18 @@ out:
static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
const char *name,
- unsigned int *pvalue)
+ unsigned int *pvalue,
+ Error **errp)
{
- return xen_host_pci_get_value(d, name, pvalue, 16);
+ return 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)
+ unsigned int *pvalue,
+ Error **errp)
{
- return xen_host_pci_get_value(d, name, pvalue, 10);
+ return xen_host_pci_get_value(d, name, pvalue, 10, errp);
}
static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
@@ -206,20 +218,21 @@ 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];
int rc;
rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
if (rc) {
- return rc;
+ error_setg_errno(errp, errno, "snprintf err");
+ return;
}
+
d->config_fd = open(path, O_RDWR);
if (d->config_fd < 0) {
- return -errno;
+ error_setg_errno(errp, errno, "open err");
}
- return 0;
}
static int xen_host_pci_config_read(XenHostPCIDevice *d,
@@ -341,8 +354,9 @@ 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;
@@ -353,43 +367,48 @@ 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, errp);
+ if (*errp) {
goto error;
}
- rc = xen_host_pci_get_resource(d);
- if (rc) {
+
+ xen_host_pci_get_resource(d, errp);
+ if (*errp) {
goto error;
}
- rc = xen_host_pci_get_hex_value(d, "vendor", &v);
+
+ rc = xen_host_pci_get_hex_value(d, "vendor", &v, errp);
if (rc) {
goto error;
}
d->vendor_id = v;
- rc = xen_host_pci_get_hex_value(d, "device", &v);
+
+ rc = xen_host_pci_get_hex_value(d, "device", &v, errp);
if (rc) {
goto error;
}
d->device_id = v;
- rc = xen_host_pci_get_dec_value(d, "irq", &v);
+
+ rc = xen_host_pci_get_dec_value(d, "irq", &v, errp);
if (rc) {
goto error;
}
d->irq = v;
- rc = xen_host_pci_get_hex_value(d, "class", &v);
+
+ rc = xen_host_pci_get_hex_value(d, "class", &v, errp);
if (rc) {
goto error;
}
d->class_code = v;
+
d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
- return 0;
+ return;
error:
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..42b9138 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..5f4cbb7 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;
@@ -774,12 +774,11 @@ 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);
- return -1;
+ xen_host_pci_device_get(&s->real_device,
+ s->hostaddr.domain, s->hostaddr.bus,
+ s->hostaddr.slot, s->hostaddr.function, errp);
+ if (*errp) {
+ return;
}
s->is_virtfn = s->real_device.is_virtfn;
@@ -799,16 +798,17 @@ 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;
}
- if (xen_pt_setup_vga(s, &s->real_device) < 0) {
- XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
+ if (xen_pt_setup_vga(s, &s->real_device, errp) < 0) {
+ error_append_hint(errp, "Setup VGA BIOS of passthrough"
+ " GFX failed!");
xen_host_pci_device_put(&s->real_device);
- return -1;
+ return;
}
/* Register ISA bridge for passthrough GFX. */
@@ -819,29 +819,28 @@ 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, errp);
+ if (*errp) {
+ error_append_hint(errp, "PCI Config space initialisation failed.");
goto err_out;
}
/* 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;
@@ -862,8 +861,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 %i failed!",
+ e_intx);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -871,8 +870,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 %i failed!", machine_irq);
}
}
s->machine_irq = 0;
@@ -885,14 +884,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;
}
}
@@ -905,12 +904,16 @@ out:
"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)
@@ -929,7 +932,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;
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index c545280..b4b0c19 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -228,7 +228,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);
@@ -328,5 +328,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);
+int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
+ Error **errp);
#endif /* !XEN_PT_H */
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 3d8686d..dad2032 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1899,8 +1899,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;
@@ -1919,12 +1920,14 @@ 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;
+ error_setg(errp, "Invalid register value");
+ return;
}
/* Sync up the data to dev.config */
offset = reg_grp->base_offset + reg->offset;
@@ -1942,7 +1945,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
@@ -1967,10 +1971,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
@@ -1990,11 +1994,9 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
}
/* list add register entry */
QLIST_INSERT_HEAD(®_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;
@@ -2039,11 +2041,11 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
reg_grp_offset,
®_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(errp, "Failed to initialize %d/%ld, type=0x%x,"
+ " rc:%d\n", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
xen_pt_emu_reg_grps[i].grp_type, rc);
xen_pt_config_delete(s);
- return rc;
+ return;
}
}
@@ -2054,21 +2056,20 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
/* 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, errp);
+ if (*errp) {
+ error_append_hint(errp, "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_delete(s);
- return rc;
+ return;
}
}
}
}
}
-
- return 0;
}
/* delete all emulate register */
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index df6069b..17e7a59 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)
+int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
+ Error **errp)
{
unsigned char *bios = NULL;
struct rom_header *rom;
@@ -172,12 +173,13 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
struct pci_data *pd = NULL;
if (!is_igd_vga_passthrough(dev)) {
+ error_setg(errp, "Need to enable igd-passthrough");
return -1;
}
bios = get_vgabios(s, &bios_size, dev);
if (!bios) {
- XEN_PT_ERR(&s->dev, "VGA: Can't getting VBIOS!\n");
+ error_setg(errp, "VGA: Can't getting VBIOS!");
return -1;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()
2015-12-23 10:23 [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize() Cao jin
@ 2015-12-23 12:03 ` Stefano Stabellini
2015-12-23 13:08 ` Cao jin
0 siblings, 1 reply; 5+ messages in thread
From: Stefano Stabellini @ 2015-12-23 12:03 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, stefano.stabellini
[-- Attachment #1: Type: text/plain, Size: 24030 bytes --]
On Wed, 23 Dec 2015, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>
> Since the callchain is pretty deep & error path is very much too, so I made the
> patch based on the principal: catch/report the most necessary error msg with
> smallest modification.(So you can see I don`t change some functions to void,
> despite they coule be)
Thanks Cao.
For consistency with the other functions, I think it would be better to
change all functions to return void or none.
Also it might be nice to split the patch in a series.
The patch as is fails to build:
qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’:
qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used uninitialized in this func
> hw/xen/xen-host-pci-device.c | 79 +++++++++++++++++++++++++++-----------------
> hw/xen/xen-host-pci-device.h | 5 +--
> hw/xen/xen_pt.c | 67 +++++++++++++++++++------------------
> hw/xen/xen_pt.h | 5 +--
> hw/xen/xen_pt_config_init.c | 47 +++++++++++++-------------
> hw/xen/xen_pt_graphics.c | 6 ++--
> 6 files changed, 118 insertions(+), 91 deletions(-)
>
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 7d8a023..1ab6d97 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -43,13 +43,14 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> /* The output is truncated, or some other error was encountered */
> return -ENODEV;
> }
> +
> return 0;
> }
I would prefer to keep stylistic changes separate, especially the ones
in functions which would be otherwise left unmodified. Maybe you could
move them to a separate patch?
> /* 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];
> @@ -60,23 +61,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>
> rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
> + return;
> }
> +
> 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_errno(errp, errno, "open err");
> + return;
> }
>
> do {
> 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++) {
> @@ -129,20 +131,20 @@ 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 to short");
^too short
> }
>
> 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)
> + unsigned int *pvalue, int base, Error **errp)
As mentioned above, I would prefer if you could change this function to
return void too. Otherwise keep the return int everywhere.
> {
> char path[PATH_MAX];
> char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
> @@ -152,30 +154,38 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>
> rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
> if (rc) {
> + error_setg_errno(errp, errno, "snprintf err");
> return rc;
> }
> +
> fd = open(path, O_RDONLY);
> if (fd == -1) {
> - XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
> + error_setg_errno(errp, errno, "open err");
would it be possible to keep the path in the error message?
> return -errno;
> }
> +
> do {
> rc = read(fd, &buf, sizeof (buf) - 1);
> if (rc < 0 && errno != EINTR) {
> + error_setg_errno(errp, errno, "read err");
> rc = -errno;
> goto out;
> }
> } while (rc < 0);
> +
> buf[rc] = 0;
> value = strtol(buf, &endptr, base);
> if (endptr == buf || *endptr != '\n') {
> + error_setg(errp, "format invalid");
> rc = -1;
> } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> + error_setg_errno(errp, errno, "strtol err");
> rc = -errno;
> } else {
> rc = 0;
> *pvalue = value;
> }
> +
> out:
> close(fd);
> return rc;
> @@ -183,16 +193,18 @@ out:
>
> static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
> const char *name,
> - unsigned int *pvalue)
> + unsigned int *pvalue,
> + Error **errp)
same here about returning void
> {
> - return xen_host_pci_get_value(d, name, pvalue, 16);
> + return 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)
> + unsigned int *pvalue,
> + Error **errp)
same here
> {
> - return xen_host_pci_get_value(d, name, pvalue, 10);
> + return xen_host_pci_get_value(d, name, pvalue, 10, errp);
> }
>
> static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
> @@ -206,20 +218,21 @@ 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];
> int rc;
>
> rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
> + return;
> }
> +
> d->config_fd = open(path, O_RDWR);
> if (d->config_fd < 0) {
> - return -errno;
> + error_setg_errno(errp, errno, "open err");
> }
> - return 0;
> }
>
> static int xen_host_pci_config_read(XenHostPCIDevice *d,
> @@ -341,8 +354,9 @@ 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;
> @@ -353,43 +367,48 @@ 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, errp);
> + if (*errp) {
> goto error;
> }
> - rc = xen_host_pci_get_resource(d);
> - if (rc) {
> +
> + xen_host_pci_get_resource(d, errp);
> + if (*errp) {
> goto error;
> }
> - rc = xen_host_pci_get_hex_value(d, "vendor", &v);
> +
> + rc = xen_host_pci_get_hex_value(d, "vendor", &v, errp);
> if (rc) {
please stick to the same strategy to check for returned errors
> goto error;
> }
> d->vendor_id = v;
> - rc = xen_host_pci_get_hex_value(d, "device", &v);
> +
> + rc = xen_host_pci_get_hex_value(d, "device", &v, errp);
> if (rc) {
> goto error;
> }
> d->device_id = v;
> - rc = xen_host_pci_get_dec_value(d, "irq", &v);
> +
> + rc = xen_host_pci_get_dec_value(d, "irq", &v, errp);
> if (rc) {
> goto error;
> }
> d->irq = v;
> - rc = xen_host_pci_get_hex_value(d, "class", &v);
> +
> + rc = xen_host_pci_get_hex_value(d, "class", &v, errp);
> if (rc) {
> goto error;
> }
> d->class_code = v;
> +
> d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
>
> - return 0;
> + return;
> error:
> 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..42b9138 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..5f4cbb7 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;
> @@ -774,12 +774,11 @@ 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);
> - return -1;
> + xen_host_pci_device_get(&s->real_device,
> + s->hostaddr.domain, s->hostaddr.bus,
> + s->hostaddr.slot, s->hostaddr.function, errp);
> + if (*errp) {
> + return;
> }
>
> s->is_virtfn = s->real_device.is_virtfn;
> @@ -799,16 +798,17 @@ 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;
> }
>
> - if (xen_pt_setup_vga(s, &s->real_device) < 0) {
> - XEN_PT_ERR(d, "Setup VGA BIOS of passthrough GFX failed!\n");
> + if (xen_pt_setup_vga(s, &s->real_device, errp) < 0) {
> + error_append_hint(errp, "Setup VGA BIOS of passthrough"
> + " GFX failed!");
> xen_host_pci_device_put(&s->real_device);
> - return -1;
> + return;
> }
>
> /* Register ISA bridge for passthrough GFX. */
> @@ -819,29 +819,28 @@ 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, errp);
> + if (*errp) {
> + error_append_hint(errp, "PCI Config space initialisation failed.");
> goto err_out;
> }
>
> /* 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;
> @@ -862,8 +861,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 %i failed!",
> + e_intx);
>
> /* Disable PCI intx assertion (turn on bit10 of devctl) */
> cmd |= PCI_COMMAND_INTX_DISABLE;
> @@ -871,8 +870,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 %i failed!", machine_irq);
> }
> }
> s->machine_irq = 0;
> @@ -885,14 +884,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;
> }
> }
> @@ -905,12 +904,16 @@ out:
> "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)
> @@ -929,7 +932,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;
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index c545280..b4b0c19 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -228,7 +228,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);
> @@ -328,5 +328,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);
> +int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
> + Error **errp);
> #endif /* !XEN_PT_H */
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 3d8686d..dad2032 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1899,8 +1899,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;
> @@ -1919,12 +1920,14 @@ 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;
> + error_setg(errp, "Invalid register value");
> + return;
> }
> /* Sync up the data to dev.config */
> offset = reg_grp->base_offset + reg->offset;
> @@ -1942,7 +1945,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
> @@ -1967,10 +1971,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
> @@ -1990,11 +1994,9 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> }
> /* list add register entry */
> QLIST_INSERT_HEAD(®_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;
>
> @@ -2039,11 +2041,11 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
> reg_grp_offset,
> ®_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(errp, "Failed to initialize %d/%ld, type=0x%x,"
> + " rc:%d\n", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
> xen_pt_emu_reg_grps[i].grp_type, rc);
> xen_pt_config_delete(s);
> - return rc;
> + return;
> }
> }
>
> @@ -2054,21 +2056,20 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
> /* 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, errp);
> + if (*errp) {
> + error_append_hint(errp, "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_delete(s);
> - return rc;
> + return;
> }
> }
> }
> }
> }
> -
> - return 0;
> }
>
> /* delete all emulate register */
> diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
> index df6069b..17e7a59 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)
> +int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev,
> + Error **errp)
same here about returning void
> {
> unsigned char *bios = NULL;
> struct rom_header *rom;
> @@ -172,12 +173,13 @@ int xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev)
> struct pci_data *pd = NULL;
>
> if (!is_igd_vga_passthrough(dev)) {
> + error_setg(errp, "Need to enable igd-passthrough");
> return -1;
> }
>
> bios = get_vgabios(s, &bios_size, dev);
> if (!bios) {
> - XEN_PT_ERR(&s->dev, "VGA: Can't getting VBIOS!\n");
> + error_setg(errp, "VGA: Can't getting VBIOS!");
> return -1;
> }
>
> --
> 2.1.0
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()
2015-12-23 12:03 ` Stefano Stabellini
@ 2015-12-23 13:08 ` Cao jin
2015-12-23 14:03 ` Stefano Stabellini
0 siblings, 1 reply; 5+ messages in thread
From: Cao jin @ 2015-12-23 13:08 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel
Hi Stefano,
first of all, thanks for your quick response:)
On 12/23/2015 08:03 PM, Stefano Stabellini wrote:
> On Wed, 23 Dec 2015, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>
>> Since the callchain is pretty deep & error path is very much too, so I made the
>> patch based on the principal: catch/report the most necessary error msg with
>> smallest modification.(So you can see I don`t change some functions to void,
>> despite they coule be)
>
> Thanks Cao.
>
> For consistency with the other functions, I think it would be better to
> change all functions to return void or none.
>
Ok, I`ll select one style may with the smallest modification;)
> Also it might be nice to split the patch in a series.
>
Yup, and the patches should be independent from each other?
> The patch as is fails to build:
>
> qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’:
> qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used uninitialized in this func
>
really weird...last patch you remind me that it cannot compile, make me
find that my computer didn`t install xen-devel package, then I installed
it right away. But this time, it really can compile on my
computer....anyway, I will check it out later.
>
>> hw/xen/xen-host-pci-device.c | 79 +++++++++++++++++++++++++++-----------------
>> hw/xen/xen-host-pci-device.h | 5 +--
>> hw/xen/xen_pt.c | 67 +++++++++++++++++++------------------
>> hw/xen/xen_pt.h | 5 +--
>> hw/xen/xen_pt_config_init.c | 47 +++++++++++++-------------
>> hw/xen/xen_pt_graphics.c | 6 ++--
>> 6 files changed, 118 insertions(+), 91 deletions(-)
>>
>> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>> index 7d8a023..1ab6d97 100644
>> --- a/hw/xen/xen-host-pci-device.c
>> +++ b/hw/xen/xen-host-pci-device.c
>> @@ -43,13 +43,14 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>> /* The output is truncated, or some other error was encountered */
>> return -ENODEV;
>> }
>> +
>> return 0;
>> }
>
> I would prefer to keep stylistic changes separate, especially the ones
> in functions which would be otherwise left unmodified. Maybe you could
> move them to a separate patch?
>
I can do that.
>
[...]
>> +
>> if (i != PCI_NUM_REGIONS) {
>> /* Invalid format or input to short */
>> - rc = -ENODEV;
>> + error_setg(errp, "Invalid format or input to short");
>
> ^too short
How about printing all the string in buf? like:
"Invalid format or input to short: %s", buf
for all the other comments below: will fix them up:)
>
>> }
>>
>> out:
>> close(fd);
>> - return rc;
>> }
>>
[...]
>
--
Yours Sincerely,
Cao Jin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()
2015-12-23 13:08 ` Cao jin
@ 2015-12-23 14:03 ` Stefano Stabellini
2015-12-24 2:23 ` Cao jin
0 siblings, 1 reply; 5+ messages in thread
From: Stefano Stabellini @ 2015-12-23 14:03 UTC (permalink / raw)
To: Cao jin; +Cc: qemu-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 3400 bytes --]
On Wed, 23 Dec 2015, Cao jin wrote:
> Hi Stefano,
> first of all, thanks for your quick response:)
Thank you for the patch
> On 12/23/2015 08:03 PM, Stefano Stabellini wrote:
> > On Wed, 23 Dec 2015, Cao jin wrote:
> > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> > > ---
> > >
> > > Since the callchain is pretty deep & error path is very much too, so I
> > > made the
> > > patch based on the principal: catch/report the most necessary error msg
> > > with
> > > smallest modification.(So you can see I don`t change some functions to
> > > void,
> > > despite they coule be)
> >
> > Thanks Cao.
> >
> > For consistency with the other functions, I think it would be better to
> > change all functions to return void or none.
> >
>
> Ok, I`ll select one style may with the smallest modification;)
Fine by me
> > Also it might be nice to split the patch in a series.
> >
>
> Yup, and the patches should be independent from each other?
That would be best: each patch has to compile independently.
> > The patch as is fails to build:
> >
> > qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’:
> > qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used
> > uninitialized in this func
> >
>
> really weird...last patch you remind me that it cannot compile, make me find
> that my computer didn`t install xen-devel package, then I installed it right
> away. But this time, it really can compile on my computer....anyway, I will
> check it out later.
I bet you don't have Xen PCI passthrough enabled. Do you have
CONFIG_XEN_PCI_PASSTHROUGH=y in i386-softmmu/config-target.mak?
> >
> > > hw/xen/xen-host-pci-device.c | 79
> > > +++++++++++++++++++++++++++-----------------
> > > hw/xen/xen-host-pci-device.h | 5 +--
> > > hw/xen/xen_pt.c | 67 +++++++++++++++++++------------------
> > > hw/xen/xen_pt.h | 5 +--
> > > hw/xen/xen_pt_config_init.c | 47 +++++++++++++-------------
> > > hw/xen/xen_pt_graphics.c | 6 ++--
> > > 6 files changed, 118 insertions(+), 91 deletions(-)
> > >
> > > diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> > > index 7d8a023..1ab6d97 100644
> > > --- a/hw/xen/xen-host-pci-device.c
> > > +++ b/hw/xen/xen-host-pci-device.c
> > > @@ -43,13 +43,14 @@ static int xen_host_pci_sysfs_path(const
> > > XenHostPCIDevice *d,
> > > /* The output is truncated, or some other error was encountered
> > > */
> > > return -ENODEV;
> > > }
> > > +
> > > return 0;
> > > }
> >
> > I would prefer to keep stylistic changes separate, especially the ones
> > in functions which would be otherwise left unmodified. Maybe you could
> > move them to a separate patch?
> >
>
> I can do that.
>
> >
> [...]
> > > +
> > > if (i != PCI_NUM_REGIONS) {
> > > /* Invalid format or input to short */
> > > - rc = -ENODEV;
> > > + error_setg(errp, "Invalid format or input to short");
> >
> > ^too short
>
> How about printing all the string in buf? like:
> "Invalid format or input to short: %s", buf
Sound good
> for all the other comments below: will fix them up:)
Thanks!
> > > }
> > >
> > > out:
> > > close(fd);
> > > - return rc;
> > > }
> > >
> [...]
> >
>
> --
> Yours Sincerely,
>
> Cao Jin
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()
2015-12-23 14:03 ` Stefano Stabellini
@ 2015-12-24 2:23 ` Cao jin
0 siblings, 0 replies; 5+ messages in thread
From: Cao jin @ 2015-12-24 2:23 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel
On 12/23/2015 10:03 PM, Stefano Stabellini wrote:
> On Wed, 23 Dec 2015, Cao jin wrote:
[...]
>
>
>>> The patch as is fails to build:
>>>
>>> qemu/hw/xen/xen_pt_config_init.c: In function ‘xen_pt_config_init’:
>>> qemu/hw/xen/xen_pt_config_init.c:2061:42: error: ‘rc’ may be used
>>> uninitialized in this func
>>>
>>
>> really weird...last patch you remind me that it cannot compile, make me find
>> that my computer didn`t install xen-devel package, then I installed it right
>> away. But this time, it really can compile on my computer....anyway, I will
>> check it out later.
>
"it really can compile on my computer" means when I press make, the lines:
CC x86_64-softmmu/hw/xen/xen_pt_config_init.o
LINK x86_64-softmmu/qemu-system-x86_64
flash into the screen.
> I bet you don't have Xen PCI passthrough enabled. Do you have
> CONFIG_XEN_PCI_PASSTHROUGH=y in i386-softmmu/config-target.mak?
>
I am not sure before, I am not aware of this before:-[ this time, I did
with:
$ ./configure --enable-debug --enable-xen-pci-passthrough
--target-list=x86_64-softmmu
$ make clean; make
It can compile. Then I checked x86_64-softmmu/config-target.mak: the
CONFIG_XEN_PCI_PASSTHROUGH=y does exist.
[...]
>
--
Yours Sincerely,
Cao Jin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-24 2:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-23 10:23 [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize() Cao jin
2015-12-23 12:03 ` Stefano Stabellini
2015-12-23 13:08 ` Cao jin
2015-12-23 14:03 ` Stefano Stabellini
2015-12-24 2:23 ` Cao jin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).