Re: [Xen-devel] [PATCH v4] igd-passthrough-i440FX: convert to realize()

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

Unfortunately I don't have a setup to test this either. Maybe Lars can
find out who should be involved on the Intel side on this.

In any case, the patch looks good to me. It would be nice to get
feedback from somebody at Intel before applying it, but if we don't get
any to your next version of the patch, I'll probably apply it anyway to
my next branch.

On Tue, 29 Dec 2015, Cao jin wrote:
> Agree with your review point. Since can`t get contact with author
> tiejun.chen@xxxxxxxxx, maybe only can ask for test help from Stefano, I will
> send next version when we get in touch with stefano.
> 
> On 12/29/2015 07:25 PM, Michael S. Tsirkin wrote:
> > On Tue, Dec 29, 2015 at 04:46:03PM +0800, Cao jin wrote:
> > > Signed-off-by: Cao jin <caoj.fnst@xxxxxxxxxxxxxx>
> > > ---
> > > changelog v4:
> > > 1. strip the bugfix code, but I guess this patch should be applied after
> > >     that bugfix patch.
> > > 2. Other little fix as per previous review
> > > 
> > > Some explanation to previous review:
> > > 1. error_setg_errno() I use in this patch will print the strerror(errno),
> > > so I
> > >     guess it will be informative enough? example:
> > > 
> > >     error_setg_errno(errp, errno, "open %s fail", path); will print:
> > >     "open bluhbluh fail: string_from_strerrer(errno)"
> > 
> > I'd prefer some mention of why this is attempted too.
> > E.g. "igd passthrough : open bluhbluh fail"
> > 
> > > 2. snprintf() has tiny tiny chance to fail,
> > 
> > It just formats a string into a buffer. the buffer
> > is pre-allocated, and we know it's large enough for the data.
> > How can it fail?
> > 
> > 
> > > I don`t assert here, because this
> > >     device is on-board, it init during machine initialization, in case it
> > > fails,
> > >     the errp we set will results in error_report(err) and exit(1), at
> > > least can
> > >     let user know why fail. see the call-chain:
> > > 
> > >     pc_xen_hvm_init_pci->pc_init1->i440fx_init->pci_create_simple->
> > > pci_create_simple_multifunction->qdev_init_nofail
> > 
> > unlike open failing, there's nothing user can do, this means there's
> > a coding bug somewhere, and assert is easier to debug.
> > 
> > > About test:
> > > 1. Compiled
> > > 2. Did a dirty hack to force create/realize this device in pc_init1(),
> > > prove
> > >     that the realizing process is ok, I will reply this mail later to
> > > attch the
> > >     screenshot evidence
> > 
> > Good job, thanks!
> > You should also Cc Tiejun Chen <tiejun.chen@xxxxxxxxx> who wrote this
> > code, presumably with access to hardware to test on.
> > 
> > 
> > >   hw/pci-host/piix.c | 30 +++++++++++++++---------------
> > >   1 file changed, 15 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > index a9cb983..e91570f 100644
> > > --- a/hw/pci-host/piix.c
> > > +++ b/hw/pci-host/piix.c
> > > @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
> > >       {0xa8, 4},  /* SNB: base of GTT stolen memory */
> > >   };
> > > 
> > > -static int host_pci_config_read(int pos, int len, uint32_t *val)
> > > +static void host_pci_config_read(int pos, int len, uint32_t *val, Error
> > > **errp)
> > >   {
> > >       char path[PATH_MAX];
> > >       int config_fd;
> > > @@ -769,19 +769,19 @@ static int host_pci_config_read(int pos, int len,
> > > uint32_t *val)
> > >       /* Access real host bridge. */
> > >       int rc = snprintf(path, size,
> > > "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
> > >                         0, 0, 0, 0, "config");
> > > -    int ret = 0;
> > > 
> > >       if (rc >= size || rc < 0) {
> > > -        return -ENODEV;
> > > +        error_setg_errno(errp, errno, "snprintf err");
> > >       }
> > > 
> > >       config_fd = open(path, O_RDWR);
> > >       if (config_fd < 0) {
> > > -        return -ENODEV;
> > > +        error_setg_errno(errp, errno, "open %s fail", path);
> > > +        return;
> > >       }
> > > 
> > >       if (lseek(config_fd, pos, SEEK_SET) != pos) {
> > > -        ret = -errno;
> > > +        error_setg_errno(errp, errno, "lseek err");
> > >           goto out;
> > >       }
> > > 
> > > @@ -789,32 +789,32 @@ static int host_pci_config_read(int pos, int len,
> > > uint32_t *val)
> > >           rc = read(config_fd, (uint8_t *)val, len);
> > >       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> > >       if (rc != len) {
> > > -        ret = -errno;
> > > +        error_setg_errno(errp, errno, "read err");
> > >       }
> > > 
> > >   out:
> > >       close(config_fd);
> > > -    return ret;
> > >   }
> > > 
> > > -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> > > +static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error
> > > **errp)
> > >   {
> > >       uint32_t val = 0;
> > > -    int rc, i, num;
> > > +    int i, num;
> > >       int pos, len;
> > > +    Error *local_err = NULL;
> > > 
> > >       num = ARRAY_SIZE(igd_host_bridge_infos);
> > >       for (i = 0; i < num; i++) {
> > >           pos = igd_host_bridge_infos[i].offset;
> > >           len = igd_host_bridge_infos[i].len;
> > > -        rc = host_pci_config_read(pos, len, &val);
> > > -        if (rc) {
> > > -            return -ENODEV;
> > > +
> > > +        host_pci_config_read(pos, len, &val, &local_err);
> > > +        if (local_err) {
> > > +            error_propagate(errp, local_err);
> > > +            return;
> > >           }
> > >           pci_default_write_config(pci_dev, pos, cpu_to_le32(val), len);
> > >       }
> > > -
> > > -    return 0;
> > >   }
> > > 
> > >   static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void
> > > *data)
> > > @@ -822,7 +822,7 @@ static void
> > > igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> > >       DeviceClass *dc = DEVICE_CLASS(klass);
> > >       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > 
> > > -    k->init = igd_pt_i440fx_initfn;
> > > +    k->realize = igd_pt_i440fx_realize;
> > >       dc->desc = "IGD Passthrough Host bridge";
> > >   }
> > > 
> > > --
> > > 2.1.0
> > > 
> > > 
> > 
> > 
> > .
> > 
> 
> -- 
> Yours Sincerely,
> 
> Cao Jin
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel