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: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?


> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux