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

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

[Wei Liu]

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.

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

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

[Ian Campbell]

On Mon, 2014-11-17 at 12:10 +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.

FWIW I think "devid" here is effectively the PCI bus ID, and no
toolstack I know of has ever supported multiple PCI buses. In theory it
would be possible though. This means that the 0 corresponds to the
"0000:" too, I think.

This doesn't invalidate your reasoning, just FYI.

>  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>

Acked-by: 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.
> 
> 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.

I don't think you need any kind of special h/w support to do PV pci
hotplug to guests, do you?

Ian.

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

[Konrad Rzeszutek Wilk]

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
> 

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

[Wei Liu]

On Wed, Nov 19, 2014 at 04:01:54PM -0500, Konrad Rzeszutek Wilk wrote:
> 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

Yes.

> 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.
> 

Thanks for testing.

Wei.

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

[Konrad Rzeszutek Wilk]

On Wed, Nov 19, 2014 at 09:21:23PM +0000, Wei Liu wrote:
> On Wed, Nov 19, 2014 at 04:01:54PM -0500, Konrad Rzeszutek Wilk wrote:
> > 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
> 
> Yes.

Then Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> > 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.
> > 
> 
> Thanks for testing.
> 
> Wei.

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

[Ian Campbell]

On Wed, 2014-11-19 at 16:28 -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 19, 2014 at 09:21:23PM +0000, Wei Liu wrote:
> > On Wed, Nov 19, 2014 at 04:01:54PM -0500, Konrad Rzeszutek Wilk wrote:
> > > 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
> > 
> > Yes.
> 
> Then Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Applied.

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

[Hu, Robert]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Ian Campbell
> Sent: Thursday, November 20, 2014 11:58 PM
> To: Konrad Rzeszutek Wilk
> Cc: Ian Jackson; Li, Liang Z; Wei Liu; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH for-4.5] libxl: remove existence check for PCI
> device hotplug
> 
> On Wed, 2014-11-19 at 16:28 -0500, Konrad Rzeszutek Wilk wrote:
> > On Wed, Nov 19, 2014 at 09:21:23PM +0000, Wei Liu wrote:
> > > On Wed, Nov 19, 2014 at 04:01:54PM -0500, Konrad Rzeszutek Wilk wrote:
> > > > 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
> > >
> > > Yes.
> >
> > Then Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Applied.
Hi, I'm new here. Shall I ask where does this patch apply to? shall I expect to see this issue fixed in Xen 4.5-RC3?
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Wei Liu]

On Mon, Nov 24, 2014 at 08:13:16AM +0000, Hu, Robert wrote:
[...]
> 
> > Applied.

> Hi, I'm new here. Shall I ask where does this patch apply to? shall I

http://xenbits.xen.org/gitweb/?p=xen.git;a=summary

Patches are applied to staging branch and then pushed to master branch
after they pass tests.

> expect to see this issue fixed in Xen 4.5-RC3?

Yes.

Wei.

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