[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on rese

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset
Date: Wed, 22 Nov 2017 15:22:50 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

On 22/11/2017 14:32, Ladi Prosek wrote:
On Wed, Nov 22, 2017 at 11:46 AM, Marcel Apfelbaum <address@hidden> wrote:
Hi Ladi,

On 20/11/2017 16:22, Ladi Prosek wrote:

msix_mask_all() is supposed to invoke the release vector notifier if the
state of the
respective vector changed from unmasked or masked.

You mean from unmasked "to" masked right?

Yes, that's a typo.

The way it's currently called from

msix_reset(), though, may result in calling the release notifier even if
the vector
is already masked.

1) msix_reset() clears out the msix_cap field and the msix_table.
2) msix_mask_all() runs with was_masked=false for all vectors because of
1), which
     results in calling the release notifier on all vectors.
3) if msix_reset() is subsequently called again, it goes through the same
steps and
     calls the release notifier on all vectors again.

As far as I can see in the code you are right.(very reset will trigger the
release notifiers

This commit moves msix_mask_all() up so it runs before the device state is


it adds a call to msix_update_function_masked() so that the device
remembers that
MSI-X is masked.

msix_update_function_masked checks the msix is enabled or masked-off.
You are building on the fact the msix will not be enabled to set
"msix_function_masked" to "true", right?
(I just want to be sure I understand the patch)

Correct. msix_enabled() will return false because we've just reset

   dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET]

I guess we could also simply assign true to it:

   dev->msix_function_masked = true;

just like msix_init() does.

Yes, is preferable - I think.
If you intend to send V2, please wait first for Alex's remarks if he has any.


This is likely a low impact issue, found while debugging an already broken
device. It
is however easy to fix and the expectation that the use and release
notifier invocations
are always balanced is very natural.

I would leave it (maybe) out of 2.11 because it may expose other bugs
and we are after rc2 already.

Adding Alex Williamson to see it does not affect device assignment,
other than that the patch looks OK to me.


Signed-off-by: Ladi Prosek <address@hidden>
   hw/pci/msix.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index c944c02135..34656de9b0 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -500,11 +500,12 @@ void msix_reset(PCIDevice *dev)
+    msix_mask_all(dev, dev->msix_entries_nr);
       dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
             ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
       memset(dev->msix_table, 0, dev->msix_entries_nr *
       memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) /
-    msix_mask_all(dev, dev->msix_entries_nr);
+    msix_update_function_masked(dev);
     /* PCI spec suggests that devices make it possible for software to

reply via email to

[Prev in Thread] Current Thread [Next in Thread]