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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Nov 15, 2022 at 10:36:32AM +0100, Jan Beulich wrote:
> On 14.11.2022 20:20, Marek Marczykowski-Górecki wrote:
> > QEMU needs to know whether clearing maskbit of a vector is really
> > clearing, or was already cleared before. Currently Xen sends only
> > clearing that bit to the device model, but not setting it, so QEMU
> > cannot detect it.
> 
> Except for qword writes as it looks. Furthermore even clearing
> requests aren't sent if address/data are unchanged. If you agree,
> please add this here in some form for having a complete picture.

Ok.

> > Because of that, QEMU is working this around by
> > checking via /dev/mem, but that isn't the proper approach.
> > 
> > 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.

> > --- 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?


-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature