Re: [PATCH for-4.5] libxl: remove existence check for PCI device hotplug

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Nov 17, 2014 at 12:10:34PM +0000, Wei Liu wrote:
> The existence check is to make sure a device is not added to a guest
> multiple times.
> 
> PCI device backend path has different rules from vif, disk etc. For
> example:
> /local/domain/0/backend/pci/9/0/dev-1/0000:03:10.1
> /local/domain/0/backend/pci/9/0/key-1/0000:03:10.1
> /local/domain/0/backend/pci/9/0/dev-2/0000:03:10.2
> /local/domain/0/backend/pci/9/0/key-2/0000:03:10.2
> 
> The devid for PCI devices is hardcoded 0. libxl__device_exists only
> checks up to /local/.../9/0 so it always returns true even the device is
> assignable.
> 
> Remove invocation of libxl__device_exists. We're sure at this point that
> the PCI device is assignable (hence no xenstore entry or JSON entry).
> The check is done before hand. For HVM guest it's done by calling
> xc_test_assign_device and for PV guest it's done by calling
> pciback_dev_is_assigned.
> 
> Reported-by: Li, Liang Z <liang.z.li@intel.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Wilk <konrad.wilk@oracle.com>
> ---
> This patch fixes a regression in 4.5.

Ouch! That needs then to be fixed.

Is the version you would want to commit? I did test it - and it
looked to do the right thing - thought the xen-pciback is stuck in the
7 state. However that is a seperate issue that I believe is due to
Xen pciback not your patches.

> 
> The risk is that I misunderstood semantics of xc_test_assign_device and
> pciback_dev_is_assigned and end up adding several entries to JSON config
> template. But if the assignable tests are incorrect I think we have a
> bigger problem to worry about than duplicated entries in JSON template.
> 
> It would be good for someone to have PCI hotplug setup to run a quick test.  I
> think Liang confirmed (indrectly) that xc_test_assign_device worked well for
> him so I think there's won't be multiple JSON template entries for HVM guests.
> However PV side still remains to be tested.
> ---
>  tools/libxl/libxl_pci.c |    8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 9f40100..316643c 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -175,14 +175,6 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
>          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;
> -        }
> -
>          rc = libxl__set_domain_configuration(gc, domid, &d_config);
>          if (rc) goto out;
>  
> -- 
> 1.7.10.4
>