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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan