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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

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). 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).
>
> 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.

Regards,
Jason