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

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