Re: [Qemu-devel] [PATCH] Xen PCI passthrough: convert to realize()

[Stefano Stabellini]

On Wed, 23 Dec 2015, Cao jin wrote:
> Signed-off-by: Cao jin <address@hidden>
> ---
> 
> 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(&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;
>  
> @@ -2039,11 +2041,11 @@ 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(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
> 
> 
>