[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits th
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls |
Date: |
Tue, 16 Jun 2015 15:56:57 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 16:03, <address@hidden> 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?