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

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

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