From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55101) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4rik-0005VW-Bi for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:20:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4rie-0001hC-NG for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:20:10 -0400 Received: from mail.emea.novell.com ([130.57.118.101]:57605) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4rie-0001gQ-6X for qemu-devel@nongnu.org; Tue, 16 Jun 2015 10:20:04 -0400 Message-Id: <55804CAF0200007800085917@mail.emea.novell.com> Date: Tue, 16 Jun 2015 15:19:59 +0100 From: "Jan Beulich" References: <5571AA3B020000780008152E@mail.emea.novell.com> <5571ABBA0200007800081543@mail.emea.novell.com> In-Reply-To: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Disposition: inline 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: Stefano Stabellini Cc: xen-devel , qemu-devel@nongnu.org >>> 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 =3D false; >> + } else if (!s->msix->maskall) { >=20 > 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. >> + s->msix->maskall =3D true; >> + xen_pt_msix_maskall(s, true); >> + } >> } >> =20 >> - debug_msix_enabled_old =3D s->msix->enabled; >> - s->msix->enabled =3D !!(*val & PCI_MSIX_FLAGS_ENABLE); >> if (s->msix->enabled !=3D debug_msix_enabled_old) { >> XEN_PT_LOG(&s->dev, "%s MSI-X\n", >> s->msix->enabled ? "enable" : "disable"); >> } >> =20 >> + xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, = &dev_value); >=20 > 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. >> --- 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; >> } >> =20 >> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENAB= LE, >> - enabled); >=20 > 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. Jan