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

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

On Tue, Mar 28, 2023 at 02:54:38PM +0200, Jan Beulich wrote:
> On 25.03.2023 03:49, Marek Marczykowski-Górecki wrote:
> > Some firmware/devices are found to not reset MSI-X properly, leaving
> > MASKALL set. Xen relies on initial state being both disabled.
> > 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.
> 
> But pci_reset_msix_state() comes into play only when assigning a device
> to a DomU. If the tool stack doing a reset doesn't properly clear the
> bit, how would it be cleared the next time round (i.e. after the guest
> stopped and then possibly was started again)? It feels like the issue
> wants dealing with elsewhere, possibly in the tool stack.

I may be misremembering some details, but AFAIR Xen intercepts
toolstack's (or more generally: accesses from dom0) attempt to clean
this up and once it enters an inconsistent state (or rather: starts with
such at the start of the day), there was no way to clean it up.

I have considered changing pci_reset_msix_state() to not choke on
MASKALL being set, but I'm a bit afraid of doing it, as there it seems
there is a lot of assumptions all over the place and I may miss some.

> 
> > --- a/xen/drivers/passthrough/msi.c
> > +++ b/xen/drivers/passthrough/msi.c
> > @@ -48,6 +48,13 @@ int pdev_msi_init(struct pci_dev *pdev)
> >          ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> >          msix->nr_entries = msix_table_size(ctrl);
> >  
> > +        /*
> > +         * Clear both ENABLE and MASKALL, pci_reset_msix_state() relies on 
> > this
> > +         * initial state.
> > +         */
> 
> Please can comments be accurate at least at the time of writing?
> pci_reset_msix_state() doesn't care about ENABLE at all.
> 
> Jan

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

Attachment: signature.asc
Description: PGP signature