Re: [PATCH 2/3] x86/msi: remove return value from msi_set_mask_bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.11.2022 15:41, David Vrabel wrote:
> On 11/11/2022 09:44, Jan Beulich wrote:
>>
>> The idea of the WARN() / BUG_ON() is to
>> - not leave failed unmasking unrecorded,
>> - not continue after failure to mask an entry.
> 
> Then lets make msi_set_mask_bit() unable to fail with something like 
> this (untested) patch. Would this be acceptable?
> 
> David

Hmm, that's quite a bit of code churn for something which, for now at
least, I'm not convinced needs changing. Yes, ...

>  From 837649a70d44455f4fd98e2eaa46dcf35a56d00a Mon Sep 17 00:00:00 2001
> From: David Vrabel <dvrabel@xxxxxxxxxxxx>
> Date: Fri, 11 Nov 2022 14:30:16 +0000
> Subject: [PATCH] x86: Always enable memory space decodes when using MSI-X
> 
> Instead of the numerous (racy) checks for memory space accesses being
> enabled before writing the the MSI-X table, force Memory Space Enable
> to be set in the Command register if MSI-X is used.

... there is some raciness there, but we assume a well-behaved Dom0
(and DomU don't have direct access to the decode enable bits), so the
checks are more to be on the safe side (the original change attempted
to merely deal with the specific pciback behavior, without impacting
other [odd/unexpected] things Dom0 may be doing, as long as what it's
doing isn't plain wrong/buggy).

> This allows the memory_decoded() function and the associated error
> paths to be removed (since it will always return true). In particular,
> msi_set_mask_bit() can no longer fail and its return value is removed.
> 
> Note that if the PCI device is a virtual function, the relevant
> command register is in the corresponding physical function.
> 
> Signed-off-by: David Vrabel <dvrabel@xxxxxxxxxxxx>

What I'm missing in the description is a discussion of the safety of
the change, in particular the taking away of the control of the
memory decode bit from Dom0 (over certain periods of time). Without
that I'm afraid I can't answer your question (and wouldn't want to
review the change in detail).

Jan