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@xxxxxxxxxxx> > >>> > >>> 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html