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
