Re: [RFC PATCH v2 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration

[Shenming Lu <lushenming@huawei.com>]

On 2021/1/27 22:21, Alex Williamson wrote:
> On Wed, 27 Jan 2021 19:27:35 +0800
> Shenming Lu <lushenming@huawei.com> wrote:
> 
>> On 2021/1/27 5:36, Alex Williamson wrote:
>>> On Wed, 9 Dec 2020 16:09:19 +0800
>>> Shenming Lu <lushenming@huawei.com> wrote:
>>>   
>>>> Different from the normal situation when the guest starts, we can
>>>> know the max unmasked vetctor (at the beginning) after msix_load()
>>>> in VFIO migration. So in order to avoid ineffectively disabling and  
>>>
>>> s/ineffectively/inefficiently/?  It's "effective" either way I think.  
>>
>> Yeah, I should say "inefficiently". :-)
>>
>>>   
>>>> enabling vectors repeatedly, let's allocate all needed vectors first
>>>> and then enable these unmasked vectors one by one without disabling.
>>>>
>>>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>>>> ---
>>>>  hw/pci/msix.c         | 17 +++++++++++++++++
>>>>  hw/vfio/pci.c         | 10 ++++++++--
>>>>  include/hw/pci/msix.h |  2 ++
>>>>  3 files changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>>>> index 67e34f34d6..bf291d3ff8 100644
>>>> --- a/hw/pci/msix.c
>>>> +++ b/hw/pci/msix.c
>>>> @@ -557,6 +557,23 @@ unsigned int msix_nr_vectors_allocated(const PCIDevice *dev)
>>>>      return dev->msix_entries_nr;
>>>>  }
>>>>  
>>>> +int msix_get_max_unmasked_vector(PCIDevice *dev)
>>>> +{
>>>> +    int max_unmasked_vector = -1;
>>>> +    int vector;
>>>> +
>>>> +    if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
>>>> +        (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
>>>> +        for (vector = 0; vector < dev->msix_entries_nr; vector++) {
>>>> +            if (!msix_is_masked(dev, vector)) {
>>>> +                max_unmasked_vector = vector;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return max_unmasked_vector;
>>>> +}  
>>>
>>> Comments from QEMU PCI folks?
>>>   
>>>> +
>>>>  static int msix_set_notifier_for_vector(PCIDevice *dev, unsigned int vector)
>>>>  {
>>>>      MSIMessage msg;
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index 51dc373695..e755ed2514 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -568,6 +568,9 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>>>>  
>>>>  static void vfio_msix_enable(VFIOPCIDevice *vdev)
>>>>  {
>>>> +    int max_unmasked_vector = msix_get_max_unmasked_vector(&vdev->pdev);
>>>> +    unsigned int used_vector = MAX(max_unmasked_vector, 0);
>>>> +  
>>>
>>> The above PCI function could also be done inline here pretty easily too:
>>>
>>> unsigned int nr, max_vec = 0;
>>>
>>> if (!msix_masked(&vdev->pdev))
>>>     for (nr = 0; nr < msix_nr_vectors_allocated(&vdev->pdev); nr++) {
>>>         if (!msix_is_masked(&vdev->pdev, nr)) {
>>>             max_vec = nr;
>>>         }
>>>     }
>>> }
>>>
>>> It's a bit cleaner than the msix utility function, imo.  
>>
>> Yeah, it makes sense.
>>
>>>   
>>>>      vfio_disable_interrupts(vdev);
>>>>  
>>>>      vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
>>>> @@ -586,9 +589,12 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
>>>>       * triggering to userspace, then immediately release the vector, leaving
>>>>       * the physical device with no vectors enabled, but MSI-X enabled, just
>>>>       * like the guest view.
>>>> +     * If there are unmasked vectors (such as in migration) which will be
>>>> +     * enabled soon, we can allocate them here to avoid ineffectively disabling
>>>> +     * and enabling vectors repeatedly later.  
>>>
>>> It just happens that migration triggers this usage model where the
>>> MSI-X enable bit is set with vectors unmasked in the vector table, but
>>> this is not unique to migration, guests can follow this pattern as well.
>>> Has this been tested with a variety of guests?  Logically it seems
>>> correct, but always good to prove so.  Thanks,  
>>
>> I have tested it in migration and normal guest startup (only the latest Linux).
>> And I can try to test with some other kernels, could you be more specific about this?
> 
> Minimally also Windows, ideally a BSD as well.  Thanks,
> 

Hi Alex,

I have tested this patch with a Windows guest (Windows Server 2012 R2 Datacenter, Intel
X722 Ethernet controller (passthrough)) and nothing went wrong. And I found that it does
trigger our usage model in the normal guest startup: has all needed vectors already unmasked
in the vector table when calling vfio_msix_enable()...

Thanks,
Shenming