Re: [PATCH 07/12] libxl: Allow stubdomain to control interupts of PCI device

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Jul 25, 2024 at 02:06:04PM +0000, Anthony PERARD wrote:
> On Thu, May 16, 2024 at 03:58:28PM +0200, Marek Marczykowski-Górecki wrote:
> > Especially allow it to control MSI/MSI-X enabling bits. This part only
> > writes a flag to a sysfs, the actual implementation is on the kernel
> > side.
> >
> > This requires Linux >= 5.10 in dom0 (or relevant patch backported).
> 
> Does it not work before 5.10? Because the
> Documentation/ABI/testing/sysfs-driver-pciback in linux tree say that
> allow_interrupt_control is in 5.6.

For MSI-X to work at least with Linux it needs a fixup
2c269f42d0f382743ab230308b836ffe5ae9b2ae, which was backported to
5.10.201, but not further. 

> > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> > index 96cb4da0794e..6f357b70b815 100644
> > --- a/tools/libs/light/libxl_pci.c
> > +++ b/tools/libs/light/libxl_pci.c
> > @@ -1513,6 +1513,14 @@ static void pci_add_dm_done(libxl__egc *egc,
> >              rc = ERROR_FAIL;
> >              goto out;
> >          }
> > +    } else if (libxl_is_stubdom(ctx, domid, NULL)) {
> > +        /* Allow acces to MSI enable flag in PCI config space for the 
> > stubdom */
> 
> s/acces/access/
> 
> > +        if ( sysfs_write_bdf(gc, 
> > SYSFS_PCIBACK_DRIVER"/allow_interrupt_control",
> > +                             pci) < 0 ) {
> > +            LOGD(ERROR, domainid, "Setting allow_interrupt_control for 
> > device");
> > +            rc = ERROR_FAIL;
> > +            goto out;
> 
> Is it possible to make this non-fatal for cases where the kernel is
> older than the introduction of the new setting? Or does pci passthrough
> doesn't work at all with a stubdom before the change in the kernel?

MSI/MSI-X will not work. And if QEMU wouldn't hide MSI/MSI-X (upstream
one doesn't), Linux won't fallback to INTx, so the device won't work at
all.

> If making this new setting conditional is an option, you could
> potentially improve the error code returned by sysfs_write_bdf() to
> distinguish between an open() failure and write() failure, to avoid
> checking the existance of the path ahead of the call. But maybe that
> pointless because it doesn't appear possible to distinguish between
> permission denied and not found.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature