Re: [Xen-devel] [PATCH v3 07/15] libxl: disallow attaching the same device more than once

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Nov 17, 2014 at 07:53:44AM +0000, Li, Liang Z wrote:
> > Originally the code allowed users to attach the same device more than
> > once. It just stupidly overwrites xenstore entries. This is bogus as
> > frontend will be very confused.
> >
> > Introduce a helper function to check if the device to be written to
> > xenstore already exists. A new error code is also introduced.
> >
> > The check and add are within one xs transaction.
> >
> > Signed-off-by: Wei Liu <wei....@xxxxxxxxxx>
> > ---
> 
> I find this patch will cause the pci-attach failed if more than one virtual 
> function devices are attached to the guest.
> 

Is this a regression? I think so but I would like to confirm.

> 
> >  @@ -148,15 +150,32 @@ static int libxl__device_pci_add_xenstore(libxl__gc 
> > *gc,
> >  uint32_t domid, libxl_d
> >       if (!starting)
> >           flexarray_append_pair(back, "state", libxl__sprintf(gc, "%d", 7));
> >  
> >  -retry_transaction:
> >  -    t = xs_transaction_start(ctx->xsh);
> >  -    libxl__xs_writev(gc, t, be_path,
> >  -                    libxl__xs_kvs_of_flexarray(gc, back, back->count));
> >  -    if (!xs_transaction_end(ctx->xsh, t, 0))
> >  -        if (errno == EAGAIN)
> >  -            goto retry_transaction;
> >  +    GCNEW(device);
> >  +    libxl__device_from_pcidev(gc, domid, pcidev, device);
> 
> >  -    return 0;
> >  +    for (;;) {
> >  +        rc = libxl__xs_transaction_start(gc, &t);
> >  +        if (rc) goto out;
> >  +
> >  +        rc = libxl__device_exists(gc, t, device);
> >  +        if (rc < 0) goto out;
> >  +        if (rc == 1) {
> >  +            LOG(ERROR, "device already exists in xenstore");
> >  +            rc = ERROR_DEVICE_EXISTS;
> >  +            goto out;
> >  +        } 
> 
> The libxl__device_exists will return 1 if more than one PCI devices are 
> attached to the guest, no matter the BDFs are identical or not.

That means this check is problematic. I think the original intention was
to check on BDFs, however it wasn't thoroughly tested. Sorry.

> I don't understand why to check this condition here, if the same device was 
> attached more than once the xc_test_assign_device() will return error,
> and the libxl__device_pci_add_xenstore() will not be called. It seems 
> unnecessary.
> 

It was added to be in line with other devices. However from the look of
it PCI devices need special treatment? Do you have some PCI dev backend
paths at hand?

Does reverting the said patch help?

Wei.

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