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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel

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