Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> +    } else if (s->msix->enabled) {
> >> +        if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
> >> +            xen_pt_msix_disable(s);
> >> +            s->msix->enabled = false;
> >> +        } else if (!s->msix->maskall) {
> > 
> > Why are you changing the state of s->msix->maskall here?
> > This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
> > maskall, right?
> 
> We're at an else if inside an else if here. The only case left
> after the if() still seen above is that value has
> PCI_MSIX_FLAGS_MASKALL set.

You are right.

Maybe just add

/* (value & PCI_MSIX_FLAGS_MASKALL) at this point */


> >> +            s->msix->maskall = true;
> >> +            xen_pt_msix_maskall(s, true);
> >> +        }
> >>      }
> >>  
> >> -    debug_msix_enabled_old = s->msix->enabled;
> >> -    s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
> >>      if (s->msix->enabled != debug_msix_enabled_old) {
> >>          XEN_PT_LOG(&s->dev, "%s MSI-X\n",
> >>                     s->msix->enabled ? "enable" : "disable");
> >>      }
> >>  
> >> +    xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
> > 
> > I have to say that I don't like the asymmetry between reading and
> > writing PCI config registers. If writes go via hypercalls, reads should
> > go via hypercalls too.
> 
> We're not doing any cfg register write via hypercalls (not here,
> and not elsewhere). What is being replaced by the patch are
> write to two bits which happen to live in PCI config space. Plus,
> reading directly, and doing writes via hypercall only when really
> needed would still be the right thing from a performance pov.

OK. It would be nice to document somewhere that QEMU is not supposed to
touch those two bits directly, but I wouldn't know where. Maybe a new
doc under docs/misc?


> >> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
> >> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> >> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
> >>          return -1;
> >>      }
> >>  
> >> -    return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
> >> -                           enabled);
> > 
> > Would it make sense to remove msi_msix_enable completely to avoid any
> > further mistakes?
> 
> Perhaps, yes. I think I actually had suggested so quite a while back.
> But I don't see myself wasting much more time on this, ehm, code.

Isn't it just a matter of removing msi_msix_enable?