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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.03.2023 14:07, Marek Marczykowski-Górecki wrote:
> 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:
>>> --- 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?

Warning I would say, as this isn't liable to repeat frequently for the
same device. And it's helpful to see even in release build runs using
default log levels.

Jan