Re: [Xen-devel] [PATCH v2 1/4] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 16, 2019 at 06:00:33PM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Jan 16, 2019 at 05:47:19PM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 15, 2019 at 04:36:28PM +0100, 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.
> > 
> > You still need pciback in order to reset the device when it's
> > deassigned from the guest, so it's functionality is not only used by
> > PV guests.
> 
> Right, I'll update the commit message to match v2 code.
> 
> > > 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.
> > 
> > Right. When doing passthrough with a stubdomain you want the target
> > domain to have the memory and IO regions mapped, and the stubdomain to
> > handle the rest.
> > 
> > > Signed-off-by: Marek Marczykowski-Górecki 
> > > <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > Changes in v2:
> > >  - previously called "libxl: attach xen-pciback only to PV domains"
> > >  - instead of excluding all HVMs, change the condition to what actually
> > >    matters here - check if stubdomain is in use; this way xen-pciback is
> > >    always in use (either for the target domain, or it's stubdomain),
> > >    fixing PCI reset by xen-pciback concerns
> > > ---
> > >  tools/libxl/libxl_pci.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > > index 87afa03..3b6b23c 100644
> > > --- a/tools/libxl/libxl_pci.c
> > > +++ b/tools/libxl/libxl_pci.c
> > > @@ -1106,7 +1106,7 @@ out:
> > >          }
> > >      }
> > >  
> > > -    if (!starting)
> > > +    if (!starting && !libxl_get_stubdom_id(CTX, domid))
> > 
> > This change seems to assume that both libxl_domain_config for the
> > target and the stubdomain will have the assigned pci devices in the
> > pcidevs field. 
> 
> Not really. libxl__device_pci_add() calls do_pci_add() for both stubdomain
> (if applicable) and target domain.

OK. From an architectural PoV I think it would make more sense to copy
the list of pci devices to the stubdom config in libxl__spawn_stub_dm,
but I'm not that familiar with pci handling in libxl, so there might
be a reason why things are done like this currently.

The change LGTM, albeit I found the pci handling code quite hard to
follow. I'm also not sure whether certain parts of the code are
correct, for example the PCI INTx seems to be mapped to both the
stubdomain and the target domain, when it's only the target domain the
one that actually uses it.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel