Message-ID: <ZS005ahNvQ/drB8I@mail-itl>
Date:   Mon, 16 Oct 2023 15:04:36 +0200
From:   Marek Marczykowski-Górecki 
        <marmarek@...isiblethingslab.com>
To:     Roger Pau Monné <roger.pau@...rix.com>
Cc:     linux-kernel@...r.kernel.org, Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Jan Beulich <jbeulich@...e.com>,
        "moderated list:XEN HYPERVISOR INTERFACE" 
        <xen-devel@...ts.xenproject.org>
Subject: Re: [PATCH v3] xen-pciback: Consider INTx disabled when MSI/MSI-X is
 enabled

On Mon, Oct 16, 2023 at 11:05:10AM +0200, Roger Pau Monné wrote:
> On Fri, Nov 18, 2022 at 04:49:23PM +0100, Marek Marczykowski-Górecki wrote:
> > Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until
> > the table is filled. Then it disables INTx just before clearing MASKALL
> > bit. Currently this approach is rejected by xen-pciback.
> > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
> > enabled (in other words: enabling MSI/MSI-X implicitly disables INTx).
> > 
> > Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This
> > applies to three places:
> >  - checking currently enabled interrupts type,
> >  - transition to MSI/MSI-X - where INTx would be implicitly disabled,
> >  - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is
> >    enabled, as device should consider INTx disabled anyway in that case
> 
> Is this last point strictly needed?  From the description above it
> seems Linux only cares about enabling MSI(-X) without the disable INTx
> bit set.

I'm not sure, but it seems logical to have it symmetric.

> 
> > 
> > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too")
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>
> > ---
> > Changes in v3:
> >  - allow clearing INTx regardless of MSI/MSI-X state, to be consistent
> >    with enabling MSI/MSI-X
> > Changes in v2:
> >  - restructure the patch to consider not only MASKALL bit, but enabling
> >    MSI/MSI-X generally, without explicitly disabling INTx first
> > ---
> >  drivers/xen/xen-pciback/conf_space.c          | 19 +++++++++++------
> >  .../xen/xen-pciback/conf_space_capability.c   |  3 ++-
> >  drivers/xen/xen-pciback/conf_space_header.c   | 21 +++----------------
> >  3 files changed, 18 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/xen/xen-pciback/conf_space.c b/drivers/xen/xen-pciback/conf_space.c
> > index 059de92aea7d..d47eee6c5143 100644
> > --- a/drivers/xen/xen-pciback/conf_space.c
> > +++ b/drivers/xen/xen-pciback/conf_space.c
> > @@ -288,12 +288,6 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
> >  	u16 val;
> >  	int ret = 0;
> >  
> > -	err = pci_read_config_word(dev, PCI_COMMAND, &val);
> > -	if (err)
> > -		return err;
> > -	if (!(val & PCI_COMMAND_INTX_DISABLE))
> > -		ret |= INTERRUPT_TYPE_INTX;
> > -
> >  	/*
> >  	 * Do not trust dev->msi(x)_enabled here, as enabling could be done
> >  	 * bypassing the pci_*msi* functions, by the qemu.
> > @@ -316,6 +310,19 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
> >  		if (val & PCI_MSIX_FLAGS_ENABLE)
> >  			ret |= INTERRUPT_TYPE_MSIX;
> >  	}
> 
> Since we are explicitly hiding INTx now, should we also do something
> about MSI(X) being both enabled at the same time?  The spec states:
> 
> "System configuration software sets one of these bits to enable either
> MSI or MSI-X, but never both simultaneously. Behavior is undefined if
> both MSI and MSI-X are enabled simultaneously."
> 
> So finding both MSI and MSI-X enabled likely means something has gone
> very wrong?  Likely to be done in a separate change, just realized
> while looking at the patch context.

Pciback try to prevent such situation (that's exactly the point of
checking the current interrupt type). But if you get into such situation
somehow anyway (likely bypassing pciback), then pciback will still allow
to disable one of them, so you can fix the situation (the enforcement of
"only one type at the time" is done setting the enable bit, but you can still
clear it).

If both MSI and MSI-X are enabled xen_pcibk_get_interrupt_type() will
return both bits set.

> > +
> > +	/*
> > +	 * PCIe spec says device cannot use INTx if MSI/MSI-X is enabled,
> > +	 * so check for INTx only when both are disabled.
> > +	 */
> > +	if (!ret) {
> > +		err = pci_read_config_word(dev, PCI_COMMAND, &val);
> > +		if (err)
> > +			return err;
> > +		if (!(val & PCI_COMMAND_INTX_DISABLE))
> > +			ret |= INTERRUPT_TYPE_INTX;
> > +	}
> > +
> >  	return ret ?: INTERRUPT_TYPE_NONE;
> >  }
> >  
> > diff --git a/drivers/xen/xen-pciback/conf_space_capability.c b/drivers/xen/xen-pciback/conf_space_capability.c
> > index 097316a74126..eb4c1af44f5c 100644
> > --- a/drivers/xen/xen-pciback/conf_space_capability.c
> > +++ b/drivers/xen/xen-pciback/conf_space_capability.c
> > @@ -236,10 +236,11 @@ static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value,
> >  		return PCIBIOS_SET_FAILED;
> >  
> >  	if (new_value & field_config->enable_bit) {
> > -		/* don't allow enabling together with other interrupt types */
> > +		/* don't allow enabling together with other interrupt type */
> 
> This comment needs to be adjusted to note that we allow enabling while
> INTx is not disabled in the command register, in order to please
> Linuxes MSI(-X) startup sequence.

Ok.

> FWIW, another option would be to simply disable INTX here once MSI(-X)
> is attempted to be enabled, won't that avoid having to modify
> xen_pcibk_get_interrupt_type()?

I would rather avoid implicit changes to other bits, it may lead to hard
to debug corner cases (in this case, for example, if domU decides to
disable MSI-X later on, it would be left with INTx disabled too, so no
interrupts at all).

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists