| Message ID | 1450866192-31401-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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 > > >
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;
>> }
>>
[...]
>
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 > >
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. [...] >
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; }
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(-)