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 <[email protected]> > Signed-off-by: Marek Marczykowski-Górecki <[email protected]> > --- > 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? Thanks, Roger.
