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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]

On Tue, Mar 28, 2023 at 01:37:14PM +0200, Roger Pau Monné wrote:
> On Sat, Mar 25, 2023 at 03:49:24AM +0100, 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.
> 
> The 'both' in this sentence seems out of context, as you just mention
> MASKALL in the previous sentence.
> 
> > 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@gmail.com>
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > v2:
> >  - new patch
> > ---
> >  xen/drivers/passthrough/msi.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c
> > index ce1a450f6f4a..60adad47e379 100644
> > --- 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.
> > +         */
> > +        ctrl &= ~(PCI_MSIX_FLAGS_ENABLE|PCI_MSIX_FLAGS_MASKALL);
> > +        pci_conf_write16(pdev->sbdf, msix_control_reg(pos), ctrl);
> 
> Should you make this conditional to them being set in the first place:
> 
> if ( ctrl & (PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL) )
> {
>     ...
> 
> We should avoid the extra access if possible (specially if it's to
> write the same value that has been read).
> 
> I would even move this before the msix_table_size() call, so the
> handling of ctrl is closer together.
> 
> Would it also be worth to print a debug message that the initial
> device state seems inconsistent?

That may make sense. XENLOG_WARNING? XENLOG_DEBUG?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]