[PATCH 0/3] Fix PCI passthrough for HVM with stubdomain

[PATCH 0/3] Fix PCI passthrough for HVM with stubdomain

[Marek Marczykowski-Górecki]

This series is follow up to previous attempts to fix this. Related threads:
 - http://markmail.org/thread/dwjcdfk3y7s5c5kl "PCI passthrough with stubdomain"
 - http://xen.markmail.org/thread/l7tvqcxbiyc2grvr "Json config and stubdomain"

With those patches applied, HVM finally have PCI devices working. Tested on
Linux 4.4.14 and Window 7 pro guests (both 64bit).


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

[PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Marek Marczykowski-Górecki]

HVM domains use IOMMU and device model assistance for communicating with
PCI devices, xen-pcifront/pciback is used only in PV domains.
When HVM domain has device model in stubdomain, attaching xen-pciback to
the target domain itself is not only useless, but also may prevent
attaching xen-pciback to the stubdomain, effectively breaking PCI
passthrough.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 6f8f49c..2ae1bc4 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1111,7 +1111,7 @@ out:
         }
     }
 
-    if (!starting)
+    if (!starting && !hvm)
         rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
     else
         rc = 0;
@@ -1306,7 +1306,8 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
         }
     }
 
-    if (d_config->num_pcidevs > 0) {
+    if (d_config->num_pcidevs > 0
+            && d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
         rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
             d_config->num_pcidevs);
         if (rc < 0) {
-- 
2.5.5


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

Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Konrad Rzeszutek Wilk]

On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-Górecki wrote:
> HVM domains use IOMMU and device model assistance for communicating with
> PCI devices, xen-pcifront/pciback is used only in PV domains.
> When HVM domain has device model in stubdomain, attaching xen-pciback to
> the target domain itself is not only useless, but also may prevent
> attaching xen-pciback to the stubdomain, effectively breaking PCI
> passthrough.

This has the consequence that the "reset" of the device that
pciback does will no longer be done.

That is the FLR functionality will not be exercised anymore.

> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl_pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 6f8f49c..2ae1bc4 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -1111,7 +1111,7 @@ out:
>          }
>      }
>  
> -    if (!starting)
> +    if (!starting && !hvm)
>          rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
>      else
>          rc = 0;
> @@ -1306,7 +1306,8 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
>          }
>      }
>  
> -    if (d_config->num_pcidevs > 0) {
> +    if (d_config->num_pcidevs > 0
> +            && d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
>          rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
>              d_config->num_pcidevs);
>          if (rc < 0) {
> -- 
> 2.5.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2227 bytes --]

On Tue, Oct 18, 2016 at 04:52:29PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-Górecki wrote:
> > HVM domains use IOMMU and device model assistance for communicating with
> > PCI devices, xen-pcifront/pciback is used only in PV domains.
> > When HVM domain has device model in stubdomain, attaching xen-pciback to
> > the target domain itself is not only useless, but also may prevent
> > attaching xen-pciback to the stubdomain, effectively breaking PCI
> > passthrough.
> 
> This has the consequence that the "reset" of the device that
> pciback does will no longer be done.
> 
> That is the FLR functionality will not be exercised anymore.

Are you sure about that? libxl__device_pci_add calls
libxl__device_pci_reset, regardless of my patch.

> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> >  tools/libxl/libxl_pci.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 6f8f49c..2ae1bc4 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -1111,7 +1111,7 @@ out:
> >          }
> >      }
> >  
> > -    if (!starting)
> > +    if (!starting && !hvm)
> >          rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
> >      else
> >          rc = 0;
> > @@ -1306,7 +1306,8 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> >          }
> >      }
> >  
> > -    if (d_config->num_pcidevs > 0) {
> > +    if (d_config->num_pcidevs > 0
> > +            && d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
> >          rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
> >              d_config->num_pcidevs);
> >          if (rc < 0) {
> > -- 
> > 2.5.5
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Wei Liu]

On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-Górecki wrote:
> HVM domains use IOMMU and device model assistance for communicating with
> PCI devices, xen-pcifront/pciback is used only in PV domains.

This bit of description is in line with my understanding of how PCI
passthrough works.

> When HVM domain has device model in stubdomain, attaching xen-pciback to
> the target domain itself is not only useless, but also may prevent
> attaching xen-pciback to the stubdomain, effectively breaking PCI
> passthrough.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl_pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 6f8f49c..2ae1bc4 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -1111,7 +1111,7 @@ out:
>          }
>      }
>  
> -    if (!starting)
> +    if (!starting && !hvm)
>          rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
>      else
>          rc = 0;
> @@ -1306,7 +1306,8 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
>          }
>      }
>  
> -    if (d_config->num_pcidevs > 0) {
> +    if (d_config->num_pcidevs > 0
> +            && d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {

Please move the indentation forward.

>          rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
>              d_config->num_pcidevs);
>          if (rc < 0) {
> -- 
> 2.5.5
> 

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

Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Konrad Rzeszutek Wilk]

On Wed, Oct 19, 2016 at 10:37:52AM +0100, Wei Liu wrote:
> On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-Górecki wrote:
> > HVM domains use IOMMU and device model assistance for communicating with
> > PCI devices, xen-pcifront/pciback is used only in PV domains.
> 
> This bit of description is in line with my understanding of how PCI
> passthrough works.

Kind of. Pciback is also used to "own" the PCI devices. And in fact
they do an important job of resetting the PCI device when the
device is "bind" to pciback:

echo <Bdf> > bind

And .. this is the important part - when device changes ownership.
That is when you disconnect it from one guest and assign to another.
You need to reset the device in between. The code that calls
the pci_reset_function is called by:

}                                                                               
                                                                                
/*                                                                              
 * Called when:                                                                 
 *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
 *  - XenBus state has been disconnected (guest shutdown). See xen_pcibk_xenbus_remove
 *  - 'echo BDF > unbind' on pciback module with no guest attached. See pcistub_remove
 *  - 'echo BDF > unbind' with a guest still using it. See pcistub_remove       
 *                                                                              
 *  As such we have to be careful.                                              
 *                                                                              
 *  To make this easier, the caller has to hold the device lock.                
 */                                                                             
void pcistub_put_pci_dev(struct pci_dev *dev)

The first two are done when XenStore 'pci' entries are active - which
this patch will remove and introduce a potential security problem.

Unless libxl does an 'unbind' followed by an 'bind'?

> 
> > When HVM domain has device model in stubdomain, attaching xen-pciback to
> > the target domain itself is not only useless, but also may prevent
> > attaching xen-pciback to the stubdomain, effectively breaking PCI
> > passthrough.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> >  tools/libxl/libxl_pci.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 6f8f49c..2ae1bc4 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -1111,7 +1111,7 @@ out:
> >          }
> >      }
> >  
> > -    if (!starting)
> > +    if (!starting && !hvm)
> >          rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
> >      else
> >          rc = 0;
> > @@ -1306,7 +1306,8 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> >          }
> >      }
> >  
> > -    if (d_config->num_pcidevs > 0) {
> > +    if (d_config->num_pcidevs > 0
> > +            && d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
> 
> Please move the indentation forward.
> 
> >          rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
> >              d_config->num_pcidevs);
> >          if (rc < 0) {
> > -- 
> > 2.5.5
> > 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 4479 bytes --]

On Wed, Oct 19, 2016 at 04:46:26PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Oct 19, 2016 at 10:37:52AM +0100, Wei Liu wrote:
> > On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-Górecki wrote:
> > > HVM domains use IOMMU and device model assistance for communicating with
> > > PCI devices, xen-pcifront/pciback is used only in PV domains.
> > 
> > This bit of description is in line with my understanding of how PCI
> > passthrough works.
> 
> Kind of. Pciback is also used to "own" the PCI devices. And in fact
> they do an important job of resetting the PCI device when the
> device is "bind" to pciback:
> 
> echo <Bdf> > bind

This part is still done.

> And .. this is the important part - when device changes ownership.
> That is when you disconnect it from one guest and assign to another.
> You need to reset the device in between. The code that calls
> the pci_reset_function is called by:
> 
> }                                                                               
>                                                                                 
> /*                                                                              
>  * Called when:                                                                 
>  *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
>  *  - XenBus state has been disconnected (guest shutdown). See xen_pcibk_xenbus_remove

But this, in case of HVM without stubdomain, is not.

>  *  - 'echo BDF > unbind' on pciback module with no guest attached. See pcistub_remove
>  *  - 'echo BDF > unbind' with a guest still using it. See pcistub_remove       
>  *                                                                              
>  *  As such we have to be careful.                                              
>  *                                                                              
>  *  To make this easier, the caller has to hold the device lock.                
>  */                                                                             
> void pcistub_put_pci_dev(struct pci_dev *dev)
> 
> The first two are done when XenStore 'pci' entries are active - which
> this patch will remove and introduce a potential security problem.
> 
> Unless libxl does an 'unbind' followed by an 'bind'?

What about libxl__device_pci_reset, which is called (at least) before
attaching device to some domain, even after my patch and even if the
device is already bound to pciback. It tries to reset the device using
'reset' entry in sysfs. I see this isn't available for some devices -
can pci_reset_function do any better?


> 
> > 
> > > When HVM domain has device model in stubdomain, attaching xen-pciback to
> > > the target domain itself is not only useless, but also may prevent
> > > attaching xen-pciback to the stubdomain, effectively breaking PCI
> > > passthrough.
> > > 
> > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > ---
> > >  tools/libxl/libxl_pci.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > > index 6f8f49c..2ae1bc4 100644
> > > --- a/tools/libxl/libxl_pci.c
> > > +++ b/tools/libxl/libxl_pci.c
> > > @@ -1111,7 +1111,7 @@ out:
> > >          }
> > >      }
> > >  
> > > -    if (!starting)
> > > +    if (!starting && !hvm)
> > >          rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
> > >      else
> > >          rc = 0;
> > > @@ -1306,7 +1306,8 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> > >          }
> > >      }
> > >  
> > > -    if (d_config->num_pcidevs > 0) {
> > > +    if (d_config->num_pcidevs > 0
> > > +            && d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
> > 
> > Please move the indentation forward.
> > 
> > >          rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
> > >              d_config->num_pcidevs);
> > >          if (rc < 0) {
> > > -- 
> > > 2.5.5
> > > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Konrad Rzeszutek Wilk]

On Thu, Oct 20, 2016 at 12:42:33AM +0200, Marek Marczykowski-Górecki wrote:
> On Wed, Oct 19, 2016 at 04:46:26PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Oct 19, 2016 at 10:37:52AM +0100, Wei Liu wrote:
> > > On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-Górecki wrote:
> > > > HVM domains use IOMMU and device model assistance for communicating with
> > > > PCI devices, xen-pcifront/pciback is used only in PV domains.
> > > 
> > > This bit of description is in line with my understanding of how PCI
> > > passthrough works.
> > 
> > Kind of. Pciback is also used to "own" the PCI devices. And in fact
> > they do an important job of resetting the PCI device when the
> > device is "bind" to pciback:
> > 
> > echo <Bdf> > bind
> 
> This part is still done.
> 
> > And .. this is the important part - when device changes ownership.
> > That is when you disconnect it from one guest and assign to another.
> > You need to reset the device in between. The code that calls
> > the pci_reset_function is called by:
> > 
> > }                                                                               
> >                                                                                 
> > /*                                                                              
> >  * Called when:                                                                 
> >  *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
> >  *  - XenBus state has been disconnected (guest shutdown). See xen_pcibk_xenbus_remove
> 
> But this, in case of HVM without stubdomain, is not.
> 
> >  *  - 'echo BDF > unbind' on pciback module with no guest attached. See pcistub_remove
> >  *  - 'echo BDF > unbind' with a guest still using it. See pcistub_remove       
> >  *                                                                              
> >  *  As such we have to be careful.                                              
> >  *                                                                              
> >  *  To make this easier, the caller has to hold the device lock.                
> >  */                                                                             
> > void pcistub_put_pci_dev(struct pci_dev *dev)
> > 
> > The first two are done when XenStore 'pci' entries are active - which
> > this patch will remove and introduce a potential security problem.
> > 
> > Unless libxl does an 'unbind' followed by an 'bind'?
> 
> What about libxl__device_pci_reset, which is called (at least) before
> attaching device to some domain, even after my patch and even if the
> device is already bound to pciback. It tries to reset the device using
> 'reset' entry in sysfs. I see this isn't available for some devices -
> can pci_reset_function do any better?

My vague recollection was that it tried to do it but it aborted
earlier due to holding locks (dev_lock is held when you do any
operation on the SysFS). But I may be forgetting the details.

I need to look in the Linux code to confirm what the tricky part was.

> 
> 
> > 
> > > 
> > > > When HVM domain has device model in stubdomain, attaching xen-pciback to
> > > > the target domain itself is not only useless, but also may prevent
> > > > attaching xen-pciback to the stubdomain, effectively breaking PCI
> > > > passthrough.
> > > > 
> > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > > ---
> > > >  tools/libxl/libxl_pci.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > > > index 6f8f49c..2ae1bc4 100644
> > > > --- a/tools/libxl/libxl_pci.c
> > > > +++ b/tools/libxl/libxl_pci.c
> > > > @@ -1111,7 +1111,7 @@ out:
> > > >          }
> > > >      }
> > > >  
> > > > -    if (!starting)
> > > > +    if (!starting && !hvm)
> > > >          rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
> > > >      else
> > > >          rc = 0;
> > > > @@ -1306,7 +1306,8 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> > > >          }
> > > >      }
> > > >  
> > > > -    if (d_config->num_pcidevs > 0) {
> > > > +    if (d_config->num_pcidevs > 0
> > > > +            && d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
> > > 
> > > Please move the indentation forward.
> > > 
> > > >          rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
> > > >              d_config->num_pcidevs);
> > > >          if (rc < 0) {
> > > > -- 
> > > > 2.5.5
> > > > 
> > > 
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > https://lists.xen.org/xen-devel
> 
> -- 
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?


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

Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 4151 bytes --]

On Tue, Oct 25, 2016 at 09:10:02AM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Oct 20, 2016 at 12:42:33AM +0200, Marek Marczykowski-Górecki wrote:
> > On Wed, Oct 19, 2016 at 04:46:26PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Oct 19, 2016 at 10:37:52AM +0100, Wei Liu wrote:
> > > > On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-Górecki wrote:
> > > > > HVM domains use IOMMU and device model assistance for communicating with
> > > > > PCI devices, xen-pcifront/pciback is used only in PV domains.
> > > > 
> > > > This bit of description is in line with my understanding of how PCI
> > > > passthrough works.
> > > 
> > > Kind of. Pciback is also used to "own" the PCI devices. And in fact
> > > they do an important job of resetting the PCI device when the
> > > device is "bind" to pciback:
> > > 
> > > echo <Bdf> > bind
> > 
> > This part is still done.
> > 
> > > And .. this is the important part - when device changes ownership.
> > > That is when you disconnect it from one guest and assign to another.
> > > You need to reset the device in between. The code that calls
> > > the pci_reset_function is called by:
> > > 
> > > }                                                                               
> > >                                                                                 
> > > /*                                                                              
> > >  * Called when:                                                                 
> > >  *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
> > >  *  - XenBus state has been disconnected (guest shutdown). See xen_pcibk_xenbus_remove
> > 
> > But this, in case of HVM without stubdomain, is not.
> > 
> > >  *  - 'echo BDF > unbind' on pciback module with no guest attached. See pcistub_remove
> > >  *  - 'echo BDF > unbind' with a guest still using it. See pcistub_remove       
> > >  *                                                                              
> > >  *  As such we have to be careful.                                              
> > >  *                                                                              
> > >  *  To make this easier, the caller has to hold the device lock.                
> > >  */                                                                             
> > > void pcistub_put_pci_dev(struct pci_dev *dev)
> > > 
> > > The first two are done when XenStore 'pci' entries are active - which
> > > this patch will remove and introduce a potential security problem.
> > > 
> > > Unless libxl does an 'unbind' followed by an 'bind'?
> > 
> > What about libxl__device_pci_reset, which is called (at least) before
> > attaching device to some domain, even after my patch and even if the
> > device is already bound to pciback. It tries to reset the device using
> > 'reset' entry in sysfs. I see this isn't available for some devices -
> > can pci_reset_function do any better?
> 
> My vague recollection was that it tried to do it but it aborted
> earlier due to holding locks (dev_lock is held when you do any
> operation on the SysFS). But I may be forgetting the details.
> 
> I need to look in the Linux code to confirm what the tricky part was.

Thanks. This is the last thing holding me from sending v2.

Anyway, if attaching xen-pciback to /something/ is needed, how should it
look? We have 3 cases:
1. PV - without qemu
2. HVM - with qemu in dom0
3. HVM - with qemu in stubdomain
And soon there will be 4th: PVH - without qemu

For 1 and 4 the device should be attached (in terms of xenstore) to the
target domain, as xen-pcifront (or equivalent) running there will be
used. BTW is that true for PVHv2?
For 3 - it should be attached to stubdomain (which is the case).
The question is what about 2 - should it be attached to the target domain,
even though it will not be used?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains

[Andrew Cooper]

On 25/10/16 20:22, Marek Marczykowski-Górecki wrote:
> On Tue, Oct 25, 2016 at 09:10:02AM -0400, Konrad Rzeszutek Wilk wrote:
>> On Thu, Oct 20, 2016 at 12:42:33AM +0200, Marek Marczykowski-Górecki wrote:
>>> On Wed, Oct 19, 2016 at 04:46:26PM -0400, Konrad Rzeszutek Wilk wrote:
>>>> On Wed, Oct 19, 2016 at 10:37:52AM +0100, Wei Liu wrote:
>>>>> On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-Górecki wrote:
>>>>>> HVM domains use IOMMU and device model assistance for communicating with
>>>>>> PCI devices, xen-pcifront/pciback is used only in PV domains.
>>>>> This bit of description is in line with my understanding of how PCI
>>>>> passthrough works.
>>>> Kind of. Pciback is also used to "own" the PCI devices. And in fact
>>>> they do an important job of resetting the PCI device when the
>>>> device is "bind" to pciback:
>>>>
>>>> echo <Bdf> > bind
>>> This part is still done.
>>>
>>>> And .. this is the important part - when device changes ownership.
>>>> That is when you disconnect it from one guest and assign to another.
>>>> You need to reset the device in between. The code that calls
>>>> the pci_reset_function is called by:
>>>>
>>>> }                                                                               
>>>>                                                                                 
>>>> /*                                                                              
>>>>  * Called when:                                                                 
>>>>  *  - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remove_device
>>>>  *  - XenBus state has been disconnected (guest shutdown). See xen_pcibk_xenbus_remove
>>> But this, in case of HVM without stubdomain, is not.
>>>
>>>>  *  - 'echo BDF > unbind' on pciback module with no guest attached. See pcistub_remove
>>>>  *  - 'echo BDF > unbind' with a guest still using it. See pcistub_remove       
>>>>  *                                                                              
>>>>  *  As such we have to be careful.                                              
>>>>  *                                                                              
>>>>  *  To make this easier, the caller has to hold the device lock.                
>>>>  */                                                                             
>>>> void pcistub_put_pci_dev(struct pci_dev *dev)
>>>>
>>>> The first two are done when XenStore 'pci' entries are active - which
>>>> this patch will remove and introduce a potential security problem.
>>>>
>>>> Unless libxl does an 'unbind' followed by an 'bind'?
>>> What about libxl__device_pci_reset, which is called (at least) before
>>> attaching device to some domain, even after my patch and even if the
>>> device is already bound to pciback. It tries to reset the device using
>>> 'reset' entry in sysfs. I see this isn't available for some devices -
>>> can pci_reset_function do any better?
>> My vague recollection was that it tried to do it but it aborted
>> earlier due to holding locks (dev_lock is held when you do any
>> operation on the SysFS). But I may be forgetting the details.
>>
>> I need to look in the Linux code to confirm what the tricky part was.
> Thanks. This is the last thing holding me from sending v2.
>
> Anyway, if attaching xen-pciback to /something/ is needed, how should it
> look? We have 3 cases:
> 1. PV - without qemu
> 2. HVM - with qemu in dom0
> 3. HVM - with qemu in stubdomain
> And soon there will be 4th: PVH - without qemu
>
> For 1 and 4 the device should be attached (in terms of xenstore) to the
> target domain, as xen-pcifront (or equivalent) running there will be
> used. BTW is that true for PVHv2?
> For 3 - it should be attached to stubdomain (which is the case).
> The question is what about 2 - should it be attached to the target domain,
> even though it will not be used?

PVH(v2) is a little complicated.  For dom0 support, there are some bits
of basic bridge emulation moving into the hypervisor so qemu isn't
required at all.  In practice, this means that SRIOV passthrough to
plain PVH(v2) domU's will also work without qemu.

There are specific plans to not use pcifront in PVH(v2) guests, making
it closer to how real hardware works.  If however there are complicated
bits of faking up required (e.g. Graphics IO-bars so windows doesn't
refuse to load the driver), then that will be better relegated to a very
small ioreq server driver in dom0, similar to how demu currently works.

As for the other cases.  The one and only legitimate case where a guest
can find any information about its pci devices in xenstore is PV
guests.  HVM guests must under no circumstance be in a position to use
pci-front.  Both pci-back and qemu have their own model of PCI state,
and an HVM guest must not be able to do thinks like half an update via
one method and another half via the other.

It is unfortunate that xen-pciback has dual unrelated functionality. 
The "binding to arbitrary devices" should be split out into a separate
device, leaving xen-pciback as only the back half of the shared protocol.

~Andrew

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

[PATCH 2/3] libxl: attach PCI device to qemu only after setting pciback/pcifront

[Marek Marczykowski-Górecki]

When qemu is running in stubdomain, handling "pci-ins" command will fail
if pcifront is not initialized already. Fix this by sending such command
only after confirming that pciback/front is running.

It is possible to handle this case in mini-os code, by delaying
"pci-ins" handling until pcifront_thread finishes devices discovery. But
the same problem most likely will apply to any other stubdomain
implementations when they come (Rumprun, Linux) - so better handle this
at libxl level.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 2ae1bc4..3805d30 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1195,6 +1195,7 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int orig_vdev, pfunc_mask;
+    char *be_path;
     libxl_device_pci *assigned;
     int num_assigned, i, rc;
     int stubdomid = 0;
@@ -1249,6 +1250,14 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
         rc = do_pci_add(gc, stubdomid, &pcidev_s, 0);
         if ( rc )
             goto out;
+        /* wait for the device actually being connected, otherwise device model
+         * running there will fail to find the device */
+        be_path = libxl__sprintf(gc, "%s/backend/pci/%d/0",
+                libxl__xs_get_dompath(gc, 0), stubdomid);
+        rc = libxl__wait_for_backend(gc, be_path,
+                    GCSPRINTF("%d", XenbusStateConnected));
+        if ( rc )
+            goto out;
     }
 
     orig_vdev = pcidev->vdevfn & ~7U;
-- 
2.5.5


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

Re: [PATCH 2/3] libxl: attach PCI device to qemu only after setting pciback/pcifront

[Wei Liu]

On Tue, Oct 18, 2016 at 03:53:32AM +0200, Marek Marczykowski-Górecki wrote:
> When qemu is running in stubdomain, handling "pci-ins" command will fail
> if pcifront is not initialized already. Fix this by sending such command
> only after confirming that pciback/front is running.
> 
> It is possible to handle this case in mini-os code, by delaying
> "pci-ins" handling until pcifront_thread finishes devices discovery. But
> the same problem most likely will apply to any other stubdomain
> implementations when they come (Rumprun, Linux) - so better handle this
> at libxl level.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Mostly some coding style issues.

> ---
>  tools/libxl/libxl_pci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 2ae1bc4..3805d30 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -1195,6 +1195,7 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      unsigned int orig_vdev, pfunc_mask;
> +    char *be_path;
>      libxl_device_pci *assigned;
>      int num_assigned, i, rc;
>      int stubdomid = 0;
> @@ -1249,6 +1250,14 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
>          rc = do_pci_add(gc, stubdomid, &pcidev_s, 0);
>          if ( rc )
>              goto out;
> +        /* wait for the device actually being connected, otherwise device model
> +         * running there will fail to find the device */

Hmm... I don't think we have written down style requirement for code
comment, but I would like this comment to be formatted a bit.

        /* Wait for the device actually being connected, otherwise device model
         * running there will fail to find the device. */

That is to capitalise the first word and make it a proper sentence.

> +        be_path = libxl__sprintf(gc, "%s/backend/pci/%d/0",
> +                libxl__xs_get_dompath(gc, 0), stubdomid);
> +        rc = libxl__wait_for_backend(gc, be_path,
> +                    GCSPRINTF("%d", XenbusStateConnected));

Please indent this a bit.

> +        if ( rc )

Not really your problem. This file is ridden with such style violation.
Can you please remove the space before and after "rc"?

Wei.

> +            goto out;
>      }
>  
>      orig_vdev = pcidev->vdevfn & ~7U;
> -- 
> 2.5.5
> 

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

[PATCH 3/3] libxl: don't try to manipulate json config for stubdomain

[Marek Marczykowski-Górecki]

Stubdomain do not have it's own config file - its configuration is
derived from target domains. Do not try to manipulate it when attaching
PCI device.

This bug prevented starting HVM with stubdomain and PCI passthrough
device attached.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_pci.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 3805d30..5ad70c5 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -151,14 +151,18 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     GCNEW(device);
     libxl__device_from_pcidev(gc, domid, pcidev, device);
 
-    lock = libxl__lock_domain_userdata(gc, domid);
-    if (!lock) {
-        rc = ERROR_LOCK_FAIL;
-        goto out;
-    }
+    /* Stubdomain config is derived from its target domain, it doesn't have
+       its own file */
+    if (!libxl_is_stubdom(CTX, domid, NULL)) {
+        lock = libxl__lock_domain_userdata(gc, domid);
+        if (!lock) {
+            rc = ERROR_LOCK_FAIL;
+            goto out;
+        }
 
-    rc = libxl__get_domain_configuration(gc, domid, &d_config);
-    if (rc) goto out;
+        rc = libxl__get_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
+    }
 
     DEVICE_ADD(pci, pcidevs, domid, &pcidev_saved, COMPARE_PCI, &d_config);
 
@@ -169,8 +173,10 @@ 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__set_domain_configuration(gc, domid, &d_config);
-        if (rc) goto out;
+        if (lock) {
+            rc = libxl__set_domain_configuration(gc, domid, &d_config);
+            if (rc) goto out;
+        }
 
         libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back));
 
-- 
2.5.5


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

Re: [PATCH 3/3] libxl: don't try to manipulate json config for stubdomain

[Wei Liu]

On Tue, Oct 18, 2016 at 03:53:33AM +0200, Marek Marczykowski-Górecki wrote:
> Stubdomain do not have it's own config file - its configuration is
> derived from target domains. Do not try to manipulate it when attaching
> PCI device.
> 
> This bug prevented starting HVM with stubdomain and PCI passthrough
> device attached.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl_pci.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 3805d30..5ad70c5 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -151,14 +151,18 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
>      GCNEW(device);
>      libxl__device_from_pcidev(gc, domid, pcidev, device);
>  
> -    lock = libxl__lock_domain_userdata(gc, domid);
> -    if (!lock) {
> -        rc = ERROR_LOCK_FAIL;
> -        goto out;
> -    }
> +    /* Stubdomain config is derived from its target domain, it doesn't have
> +       its own file */
> +    if (!libxl_is_stubdom(CTX, domid, NULL)) {
> +        lock = libxl__lock_domain_userdata(gc, domid);
> +        if (!lock) {
> +            rc = ERROR_LOCK_FAIL;
> +            goto out;
> +        }
>  

What makes PCI devices special with regard to other devices? In other
words, do we need to make similar changes to other devices as well?

Wei.

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

Re: [PATCH 3/3] libxl: don't try to manipulate json config for stubdomain

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2157 bytes --]

On Wed, Oct 19, 2016 at 10:38:04AM +0100, Wei Liu wrote:
> On Tue, Oct 18, 2016 at 03:53:33AM +0200, Marek Marczykowski-Górecki wrote:
> > Stubdomain do not have it's own config file - its configuration is
> > derived from target domains. Do not try to manipulate it when attaching
> > PCI device.
> > 
> > This bug prevented starting HVM with stubdomain and PCI passthrough
> > device attached.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> >  tools/libxl/libxl_pci.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 3805d30..5ad70c5 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -151,14 +151,18 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
> >      GCNEW(device);
> >      libxl__device_from_pcidev(gc, domid, pcidev, device);
> >  
> > -    lock = libxl__lock_domain_userdata(gc, domid);
> > -    if (!lock) {
> > -        rc = ERROR_LOCK_FAIL;
> > -        goto out;
> > -    }
> > +    /* Stubdomain config is derived from its target domain, it doesn't have
> > +       its own file */
> > +    if (!libxl_is_stubdom(CTX, domid, NULL)) {
> > +        lock = libxl__lock_domain_userdata(gc, domid);
> > +        if (!lock) {
> > +            rc = ERROR_LOCK_FAIL;
> > +            goto out;
> > +        }
> >  
> 
> What makes PCI devices special with regard to other devices? In other
> words, do we need to make similar changes to other devices as well?

I think PCI is special because libxl doesn't bother to update stubdomain
configuration for other device types at all... If I try dynamic
attach of network device to a HVM (with stubdomain), it gets attached
_only_ to the target domain. So HVM without any PV drivers is out of
luck. Is it supposed to work with qemu in dom0?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 0/3] Fix PCI passthrough for HVM with stubdomain

[Samuel Thibault]

Hello,

Marek Marczykowski-Górecki, on Tue 18 Oct 2016 03:53:30 +0200, wrote:
> This series is follow up to previous attempts to fix this. Related threads:
>  - http://markmail.org/thread/dwjcdfk3y7s5c5kl "PCI passthrough with stubdomain"
>  - http://xen.markmail.org/thread/l7tvqcxbiyc2grvr "Json config and stubdomain"
> 
> With those patches applied, HVM finally have PCI devices working. Tested on
> Linux 4.4.14 and Window 7 pro guests (both 64bit).

The three changes make sense to me.  I don't know libxl any more, so I
wouldn't be able to review them, though :)

Thanks!
Samuel

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