From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls Date: Tue, 16 Jun 2015 15:19:59 +0100 Message-ID: <55804CAF0200007800085917__38173.339496325$1434464516$gmane$org@mail.emea.novell.com> References: <5571AA3B020000780008152E@mail.emea.novell.com> <5571ABBA0200007800081543@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z4rif-0004h3-65 for xen-devel@lists.xenproject.org; Tue, 16 Jun 2015 14:20:05 +0000 In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: xen-devel , qemu-devel@nongnu.org List-Id: xen-devel@lists.xenproject.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 = 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. >> + 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. >> --- 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. Jan