Re: [PATCH 2/2] Do not access /dev/mem in MSI-X PCI passthrough on Xen

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Nov 16, 2022 at 10:34 PM Marek Marczykowski-Górecki
<marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Nov 16, 2022 at 10:40:02PM +0100, Marek Marczykowski-Górecki wrote:
> > On Wed, Nov 16, 2022 at 02:15:22PM -0500, Jason Andryuk wrote:
> > > On Mon, Nov 14, 2022 at 2:21 PM Marek Marczykowski-Górecki
> > > <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > The /dev/mem is used for two purposes:
> > > >  - reading PCI_MSIX_ENTRY_CTRL_MASKBIT
> > > >  - reading Pending Bit Array (PBA)
> > > >
> > > > The first one was originally done because when Xen did not send all
> > > > vector ctrl writes to the device model, so QEMU might have outdated old
> > > > register value. This has been changed in Xen, so QEMU can now use its
> > > > cached value of the register instead.
> > > >
> > > > The Pending Bit Array (PBA) handling is for the case where it lives on
> > > > the same page as the MSI-X table itself. Xen has been extended to handle
> > > > this case too (as well as other registers that may live on those pages),
> > > > so QEMU handling is not necessary anymore.
> > > >
> > > > Removing /dev/mem access is useful to work within stubdomain, and
> > > > necessary when dom0 kernel runs in lockdown mode.
> > > >
> > > > Signed-off-by: Marek Marczykowski-Górecki 
> > > > <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > >
> > > I put the Xen, QEMU, and xen-pciback patches into OpenXT and gave a
> > > little test.  When pci_permissive=0, iwlwifi fails to load its
> > > firmware.  With pci_permissive=1, it looks like MSI-X is enabled. (I
> > > previously included your libxl allow_interrupt_control patch - that
> > > seemed to get regular MSIs working prior to the MSI-X patches.)  I
> > > also removed the OpenXT equivalent of 0005-Disable-MSI-X-caps.patch.
> > > I am testing with Linux 5.4.y, so that could be another factor.
> >
> > Can you confirm the allow_interrupt_control is set by libxl? Also,
> > vanilla 5.4 doesn't have the allow_interrupt_control patch at all, and you
> > may have an earlier version that had "allow_msi_enable" as the sysfs
> > file name.

I backported allow_interrupt_control to 5.4 and that is set properly.

> Ok, I found what is wrong. Enabling MSI-X is refused, because INTx isn't
> disabled at this point yet. And apparently I was testing this with
> permissive=1...
>
> Linux does this:
> https://github.com/torvalds/linux/blob/master/drivers/pci/msi/msi.c#L611
> In short:
> 1. Enable MSI-X with MASKALL=1
> 2. Setup MSI-X table
> 3. Disable INTx
> 4. Set MASKALL=0
>
> This patch on top should fix this:
> ----8<----
> diff --git a/drivers/xen/xen-pciback/conf_space_capability.c 
> b/drivers/xen/xen-pciback/conf_space_capability.c
> index 097316a74126..f4c4381de76e 100644
> --- a/drivers/xen/xen-pciback/conf_space_capability.c
> +++ b/drivers/xen/xen-pciback/conf_space_capability.c
> @@ -235,7 +235,7 @@ static int msi_msix_flags_write(struct pci_dev *dev, int 
> offset, u16 new_value,
>             (new_value ^ old_value) & ~field_config->allowed_bits)
>                 return PCIBIOS_SET_FAILED;
>
> -       if (new_value & field_config->enable_bit) {
> +       if ((new_value & field_config->allowed_bits) == 
> field_config->enable_bit) {
>                 /* don't allow enabling together with other interrupt types */
>                 int int_type = xen_pcibk_get_interrupt_type(dev);
>
> ----8<----

FWIW, I can confirm this allows enabling MSI-X with permissive=0 for me.

Thanks,
Jason