Re: [PATCH v3 4/4] x86/msi: clear initial MSI-X state on boot

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

On Mon, Apr 24, 2023 at 11:25:01AM -0400, Jason Andryuk wrote:
> On Mon, Apr 24, 2023 at 10:19 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >
> > On 06.04.2023 05:57, Marek Marczykowski-Górecki wrote:
> > > Some firmware/devices are found to not reset MSI-X properly, leaving
> > > MASKALL set. Jason reports on his machine MASKALL persists through a
> > > warm reboot, but is cleared on cold boot. Xen relies on initial state
> > > being MASKALL clear. Especially, pci_reset_msix_state() assumes if
> > > MASKALL is set, it was Xen setting it due to msix->host_maskall or
> > > msix->guest_maskall. Clearing just MASKALL might be unsafe if ENABLE is
> > > set, so clear them both.
> > >
> > > Reported-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> > > Signed-off-by: Marek Marczykowski-Górecki 
> > > <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> >
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > albeit with a couple of nits (which I'd be happy to address while
> > committing, so long as you agree). 

Yes, thanks!

> > First one being on the last
> > sentence above: It's surely not just "might"; if resetting already
> > doesn't work right, nothing says that the individual mask bit all
> > end up set. Clearing ENABLE as well is only natural imo, if we
> > already need to fix up after firmware. So maybe "Even if so far not
> > observed to be left set, clear ENABLE as well"?
> >
> > > --- a/xen/drivers/passthrough/msi.c
> > > +++ b/xen/drivers/passthrough/msi.c
> > > @@ -46,6 +46,23 @@ int pdev_msi_init(struct pci_dev *pdev)
> > >          spin_lock_init(&msix->table_lock);
> > >
> > >          ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> > > +
> > > +        if ( ctrl & (PCI_MSIX_FLAGS_MASKALL|PCI_MSIX_FLAGS_ENABLE) )
> >
> > Style (missing blanks around |; once more below).
> >
> > > +        {
> > > +            /*
> > > +             * pci_reset_msix_state() relies on MASKALL not being set
> > > +             * initially, clear it (and ENABLE too - for safety), to 
> > > meet that
> > > +             * expectation.
> > > +             */
> > > +            printk(XENLOG_WARNING
> > > +                   "%pp: unexpected initial MSI-X state (MASKALL=%d, 
> > > ENABLE=%d), fixing\n",
> > > +                   &pdev->sbdf,
> > > +                   (ctrl & PCI_MSIX_FLAGS_MASKALL) ? 1 : 0,
> > > +                   (ctrl & PCI_MSIX_FLAGS_ENABLE) ? 1 : 0);
> >
> > Our "canonical" way of dealing with this is !!(x & y).
> >
> > > +            ctrl &= ~(PCI_MSIX_FLAGS_ENABLE|PCI_MSIX_FLAGS_MASKALL);
> > > +            pci_conf_write16(pdev->sbdf, msix_control_reg(pos), ctrl);
> > > +        }
> > > +
> > >          msix->nr_entries = msix_table_size(ctrl);
> > >
> > >          pdev->msix = msix;
> >
> >
> > Aiui there's no dependency here on the earlier patches in the series;
> > please confirm (or otherwise).

Indeed. An earlier patch uncovered a firmware (or such) issue on some
systems and this patch deals with it, but it doesn't depend on earlier
patches.

> > Jason - any chance of getting a Tested-by: from you?
> 
> I'm building v3 now.  v2  worked for clearing MASKALL on initial boot.
> 
> I posted in these two messages - a summary is below.
> https://lore.kernel.org/xen-devel/CAKf6xpto87QRSKT2qc1yApNfaw2SrLLxPoytYJv_jEbYTAbjCg@xxxxxxxxxxxxxx/
> https://lore.kernel.org/xen-devel/CAKf6xptHALLR-Qjf=p5y0o9Ud2V7eFMJuB8Ap-PLjv-N7PAJVQ@xxxxxxxxxxxxxx/
> 
> OpenXT has a patch that performs an extra reset after domain shutdown,
> and that causes Xen to set MASKALL.  I confirmed by removing it.  So
> this patch helps with clearing MASKALL on host boot, but with the
> OpenXT patch, rebooting a domain fails.  MASKALL gets set on VM
> shutdown and then the subsequent boot can't assign the device.
> 
> So this patch is helpful in some scenarios, but it was also an issue
> caused by the OpenXT patch.  Does that make it unsuitable for
> inclusion?  I assume the OpenXT patch wasn't an issue previously since
> MSI-X was never enabled.

Upstream Xen IMO should deal with the state it gets on boot, regardless
of what was running previously (the actual issue is likely in firmware
or device itself, that it doesn't clear that bit, but well...). So,
rebooting from OpenXT, into vanilla upstream Xen should result in fully
functional system. That's why I included this patch, but haven't dealt
with an issue caused by OpenXT patch on subsequent domain startups (as
it doesn't apply to the upstream code base).

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

Attachment: signature.asc
Description: PGP signature