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

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

On Thu, Nov 17, 2022 at 09:04:40AM +0100, Jan Beulich wrote:
> On 17.11.2022 04:34, Marek Marczykowski-Górecki wrote:
> > 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<----
> > 
> > Jan, is the above safe? It should prevent clearing MASKALL if INTx isn't
> > disabled, unless I missed something? But also, it will allow enabling
> > MSI-X with MASKALL=1 together with MSI, which I'm not sure if should be
> > allowed.
> 
> While it would probably be okay with what we have now (after your earlier
> patch introducing allowed_bits), it's likely not going to be correct once
> further bits would be added to allowed_bits (which clearly is going to be
> wanted sooner or later, e.g. for multi-vector MSI). Hence I think ...
> 
> > Alternatively to the above patch, I could allow specifically setting
> > MSIX_FLAGS_ENABLE + MSIX_FLAGS_MASKALL while INTx isn't disabled as a
> > single exception,
> 
> ... this is the way to go, and ...

Ok, I'll go this way then.

> > but at this point I'm not sure if some other driver or
> > OS wouldn't approach this in yet another way.
> 
> ... I guess we need to further add exceptions as needed - the one further
> approach I could see is to keep all entry's mask bits set until setting
> "INTx disable", without using MASKALL.
> 
> I'd like to note though that the PCI spec has no such exception. It,
> however, also doesn't mandate setting "INTx disable" - that's merely a
> workaround for flawed hardware afaik. (In Xen we also only cross check
> MSI and MSI-X. IOW we rely on Dom0 anyway when it comes to driving
> "INTx disable".) Therefore in the end pciback looks to be going too far
> in enforcing such dependencies.
> 
> Thinking of this - what about making the change in
> xen_pcibk_get_interrupt_type() instead, setting INTERRUPT_TYPE_MSIX only
> when MASKALL is clear (alongside ENABLE being set)? This would then also
> cover command_write().

That may be a good idea, but wouldn't cover the case here:
xen_pcibk_get_interrupt_type() at this time returns INTERRUPT_TYPE_INTX
only, and setting MSIX_FLAGS_ENABLE is prevented.

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

Attachment: signature.asc
Description: PGP signature