From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60915) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4sn4-0007oi-F6 for qemu-devel@nongnu.org; Tue, 16 Jun 2015 11:28:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4smu-00060F-TQ for qemu-devel@nongnu.org; Tue, 16 Jun 2015 11:28:42 -0400 Received: from smtp.citrix.com ([66.165.176.89]:56906) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4smu-0005zx-P4 for qemu-devel@nongnu.org; Tue, 16 Jun 2015 11:28:32 -0400 Date: Tue, 16 Jun 2015 15:56:57 +0100 From: Stefano Stabellini In-Reply-To: <55804CAF0200007800085917@mail.emea.novell.com> Message-ID: References: <5571AA3B020000780008152E@mail.emea.novell.com> <5571ABBA0200007800081543@mail.emea.novell.com> <55804CAF0200007800085917@mail.emea.novell.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Subject: Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Beulich Cc: xen-devel , qemu-devel@nongnu.org, Stefano Stabellini On Tue, 16 Jun 2015, Jan Beulich wrote: > >>> On 16.06.15 at 16:03, 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?