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.
> OTOH, there will only be one user of msix_write_config remaining:
> pci_default_write_config. So there are not that many users to check for
> correct API use.
>
> Jan
I'm looking to fix bugs first, add features second.
> --
> 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
