Re: [PATCH 1/2] x86/msi: passthrough all MSI-X vector ctrl writes to device model

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2022 12:37, Marek Marczykowski-Górecki wrote:
> On Tue, Nov 15, 2022 at 10:36:32AM +0100, Jan Beulich wrote:
>> On 14.11.2022 20:20, Marek Marczykowski-Górecki wrote:
>>> Give all necessary information to QEMU by passing all ctrl writes,
>>> including masking a vector.
>>
>> Can we perhaps still avoid sending dword writes which don't change
>> the mask bit?
> 
> Is it worth it? I don't think such writes are common (which I confirm
> observing debug log - every single write to maskbit Linux did was
> changing the value). The old value isn't readily available here.

For one a 2nd aspect would be Windows behavior. As you've seen in
the hypervisor code somebody back at the time even thought
accelerating reads was useful. I'm going from that rather than
knowing for sure that such an optimization would help anywhere.

>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -271,7 +271,8 @@ out:
>>>  }
>>>  
>>>  static int msixtbl_write(struct vcpu *v, unsigned long address,
>>> -                         unsigned int len, unsigned long val)
>>> +                         unsigned int len, unsigned long val,
>>> +                         bool completion)
>>>  {
>>
>> I'd like to propose an alternative approach without an extra parameter:
>> Have msix_write_completion() pass 0 for "len" and move the initial
>> check
>>
>>     if ( (len != 4 && len != 8) || (address & (len - 1)) )
>>         return r;
>>
>> into _msixtbl_write(). Then ...
>>
>>> @@ -343,7 +344,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long 
>>> address,
>>>  
>>>  unlock:
>>>      spin_unlock_irqrestore(&desc->lock, flags);
>>> -    if ( len == 4 )
>>> +    if ( len == 4 && completion )
>>>          r = X86EMUL_OKAY;
>>
>> ... this could simply be "if ( !len )", seeing that even with your
>> approach it could simply be "if ( completion )".
> 
> I find such usage of magic len=0 confusing. It would change the meaning
> of "len" from "write length" to "write length, unless it's 0 - then
> write length is 4 and it's called from msix_write_completion.
> Is there any real value from avoiding extra parameter?

Perhaps a matter of taste, but to me redundant parameters are odd
at times as well - often I end up wondering in such cases why an
extra parameter was introduced when things could easily be done
with what was there. In the specific case here there's also the
further aspect of you moving the function across the boundary of
all arguments fitting in registers available for parameter passing
(which of course only matters if the compiler decides to not
inline the function at all call sites).

Jan