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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.03.2023 15:04, Marek Marczykowski-Górecki wrote:
> On Tue, Mar 28, 2023 at 02:54:38PM +0200, Jan Beulich wrote:
>> On 25.03.2023 03:49, 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.
>>> 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.
>>
>> But pci_reset_msix_state() comes into play only when assigning a device
>> to a DomU. If the tool stack doing a reset doesn't properly clear the
>> bit, how would it be cleared the next time round (i.e. after the guest
>> stopped and then possibly was started again)? It feels like the issue
>> wants dealing with elsewhere, possibly in the tool stack.
> 
> I may be misremembering some details, but AFAIR Xen intercepts
> toolstack's (or more generally: accesses from dom0) attempt to clean
> this up and once it enters an inconsistent state (or rather: starts with
> such at the start of the day), there was no way to clean it up.

Iirc Roger and you already discussed that there needs to be an
indication of device reset having happened, so that Xen can resync
from this "behind its back" operation. That would look to be the
point/place where such inconsistencies should be eliminated.

> I have considered changing pci_reset_msix_state() to not choke on
> MASKALL being set, but I'm a bit afraid of doing it, as there it seems
> there is a lot of assumptions all over the place and I may miss some.

Well, the comment there alone is enough justification to leave that
check as is, I would say. To drop it there, something equivalent
would likely want adding in its stead.

But you haven't really answered my earlier question: If reset leaves
MASKALL set, how would the same issue be taken care of the 2nd time
round? (If, otoh, firmware sets the bit for some reason, but reset
clears it along with ENABLE, I could see how a one-time action might
suffice. But the first sentence in the description doesn't make clear
which situation it is that we have to work around.)

Jan