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

[Jan Kiszka <jan.kiszka@siemens.com>]

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.

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

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