Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 18, 2011 at 02:44:12PM +0200, Jan Kiszka wrote:
> On 2011-10-18 14:37, Michael S. Tsirkin wrote:
> > On Tue, Oct 18, 2011 at 02:22:38PM +0200, Jan Kiszka wrote:
> >> On 2011-10-18 14:11, Michael S. Tsirkin wrote:
> >>> On Tue, Oct 18, 2011 at 01:54:23PM +0200, Jan Kiszka wrote:
> >>>> On 2011-10-18 13:43, Michael S. Tsirkin wrote:
> >>>>> On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote:
> >>>>>> Reorganize msix_mmio_writel so that msix_handle_mask_update is only
> >>>>>> called on mask changes. Pass previous config space value to
> >>>>>> msix_write_config so that it can check if a mask change took place.
> >>>>>>
> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>
> >>>>> What we did in other cases is track the old value in device state.
> >>>>> This makes the API easier to use correctly.
> >>>>> I'm testing the following as a replacement - any comments?
> >>>>
> >>>> No concerns about caching the mask state, but...
> >>>>
> >>>>>
> >>>>> diff --git a/hw/msix.c b/hw/msix.c
> >>>>> index b15bafc..655a600 100644
> >>>>> --- a/hw/msix.c
> >>>>> +++ b/hw/msix.c
> >>>>> @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> >>>>>      /* Make flags bit writable. */
> >>>>>      pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> >>>>>  	    MSIX_MASKALL_MASK;
> >>>>> +    pdev->msix_function_masked = false;
> >>>>>      return 0;
> >>>>>  }
> >>>>>  
> >>>>> @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> >>>>>      *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
> >>>>>  }
> >>>>>  
> >>>>> -static int msix_function_masked(PCIDevice *dev)
> >>>>> -{
> >>>>> -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> >>>>> -}
> >>>>> -
> >>>>>  static int msix_is_masked(PCIDevice *dev, int vector)
> >>>>>  {
> >>>>>      unsigned offset =
> >>>>>          vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> >>>>> -    return msix_function_masked(dev) ||
> >>>>> +    return dev->msix_function_masked ||
> >>>>>  	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >>>>>  }
> >>>>>  
> >>>>> @@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >>>>>  {
> >>>>>      unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> >>>>>      int vector;
> >>>>> +    bool fmsk;
> >>>>>  
> >>>>>      if (!range_covers_byte(addr, len, enable_pos)) {
> >>>>>          return;
> >>>>> @@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >>>>>  
> >>>>>      pci_device_deassert_intx(dev);
> >>>>>  
> >>>>> -    if (msix_function_masked(dev)) {
> >>>>> +    fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> >>>>> +    if (dev->msix_function_masked == fmsk) {
> >>>>
> >>>> ...this misses MSIX_MASKALL_MASK (but !MSIX_ENABLE_MASK) ->
> >>>> MSIX_ENABLE_MASK.
> >>>>
> >>>
> >>> Could you clarify please?
> >>>     if (!msix_enabled(dev)) {
> >>>         return;
> >>>     }
> >>> is at start of this function so nothing happens if
> >>> MSIX is disabled.
> >>
> >> OK, missed that. But let's have a look again:
> >>
> >> ENABLE  -> 0 => msix_function_masked := false
> >> 0 -> ENABLE  => msix_function_masked remains false, thus no firing
> >> (while we did fire in the past and should do so in the future)
> >>
> >> Jan
> > 
> > I'm not sure I get it. You mean mask was set when we enabled?
> > Then we can't send any interrupts so handlers do not fire.
> > What's wrong?
> 
> I mean mask was NOT set when disabling, and will remain unset when
> enabling. Then we should fire the notifier although the mask bit was not
> changed physically.
> There should be no vectors pending at this point as
> they are supposed to be cleared when we disable MSI-X (as far as I
> understand the spec).
> 
> Jan
> 

Ah. Right. Good catch. It's probably best to just make that
state be !enabled || masked. Like the below? (compiled only):

--->

msix: track function masked in pci device state

Only go over the table when function is masked.
This optimization is not really important for qemu.git but helps
qemu-kvm.git.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/msix.c b/hw/msix.c
index b15bafc..63b41b9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
     /* Make flags bit writable. */
     pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
 	    MSIX_MASKALL_MASK;
+    pdev->msix_function_masked = true;
     return 0;
 }
 
@@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
 }
 
-static int msix_function_masked(PCIDevice *dev)
-{
-    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
-}
-
 static int msix_is_masked(PCIDevice *dev, int vector)
 {
     unsigned offset =
         vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
-    return msix_function_masked(dev) ||
+    return dev->msix_function_masked ||
 	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
 }
 
@@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
     }
 }
 
+static void msix_update_function_masked(PCIDevice *dev)
+{
+    dev->msix_function_masked = !msix_enabled(dev) ||
+        (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK);
+}
+
 /* Handle MSI-X capability config write. */
 void msix_write_config(PCIDevice *dev, uint32_t addr,
                        uint32_t val, int len)
 {
     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
     int vector;
+    bool was_masked;
 
     if (!range_covers_byte(addr, len, enable_pos)) {
         return;
     }
 
+    was_masked = dev->msix_function_masked;
+    msix_update_function_masked(dev);
+
     if (!msix_enabled(dev)) {
         return;
     }
 
     pci_device_deassert_intx(dev);
 
-    if (msix_function_masked(dev)) {
+    if (dev->msix_function_masked == was_masked) {
         return;
     }
 
@@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
     msix_free_irq_entries(dev);
     qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
     qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
+    msix_update_function_masked(dev);
 }
 
 /* Does device support MSI-X? */
diff --git a/hw/pci.h b/hw/pci.h
index 86a81c8..da4c9c3 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -178,6 +178,8 @@ struct PCIDevice {
     unsigned *msix_entry_used;
     /* Region including the MSI-X table */
     uint32_t msix_bar_size;
+    /* MSIX function mask set or MSIX disabled */
+    bool msix_function_masked;
     /* Version id needed for VMState */
     int32_t version_id;
 

-- 
MST