From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH 1/5] qemu-kvm: msix: Don't fire notifier spuriously on set/unset Date: Tue, 18 Oct 2011 13:06:07 +0200 Message-ID: <20111018110607.GB28776@redhat.com> References: <0cc8d4ef4adb06b4ee6370488609352dae6fa3df.1318924251.git.jan.kiszka@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , Marcelo Tosatti , kvm@vger.kernel.org To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:28843 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754599Ab1JRLFD (ORCPT ); Tue, 18 Oct 2011 07:05:03 -0400 Content-Disposition: inline In-Reply-To: <0cc8d4ef4adb06b4ee6370488609352dae6fa3df.1318924251.git.jan.kiszka@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Oct 18, 2011 at 09:50:50AM +0200, Jan Kiszka wrote: > If MSI-X is disabled or the global mask is set, don't fire the notifier > during registration or removal, reporting a wrong state. > > Signed-off-by: Jan Kiszka Hmm. Could you please describe how does one trigger the incorrect behaviour? Looking at the upstream code, I notice that msix_notify_if_unmasked already calls msix_is_masked which checks the function mask. And it also checks msix_entry_used which, ATM, is only set when msix is enabled. > --- > hw/msix.c | 22 ++++++++++++++-------- > 1 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/hw/msix.c b/hw/msix.c > index 60d6d1e..6493937 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -563,10 +563,13 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f) > int r, n; > assert(!dev->msix_mask_notifier); > dev->msix_mask_notifier = f; > - for (n = 0; n < dev->msix_entries_nr; ++n) { > - r = msix_set_mask_notifier_for_vector(dev, n); > - if (r < 0) { > - goto undo; > + if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & > + (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) { > + for (n = 0; n < dev->msix_entries_nr; ++n) { > + r = msix_set_mask_notifier_for_vector(dev, n); > + if (r < 0) { > + goto undo; > + } > } > } > return 0; > @@ -583,10 +586,13 @@ int msix_unset_mask_notifier(PCIDevice *dev) > { > int r, n; > assert(dev->msix_mask_notifier); > - for (n = 0; n < dev->msix_entries_nr; ++n) { > - r = msix_unset_mask_notifier_for_vector(dev, n); > - if (r < 0) { > - goto undo; > + if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & > + (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) { > + for (n = 0; n < dev->msix_entries_nr; ++n) { > + r = msix_unset_mask_notifier_for_vector(dev, n); > + if (r < 0) { > + goto undo; > + } > } > } > dev->msix_mask_notifier = NULL; > -- > 1.7.3.4