Re: [PATCH v9 02/16] vpci: use per-domain PCI lock to protect vpci structure

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 9/20/23 04:09, Roger Pau Monné wrote:
> On Tue, Sep 19, 2023 at 12:20:39PM -0400, Stewart Hildebrand wrote:
>> On 9/19/23 11:39, Roger Pau Monné wrote:
>>> On Tue, Aug 29, 2023 at 11:19:42PM +0000, Volodymyr Babchuk wrote:
>>>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
>>>> index 8f2b59e61a..a0733bb2cb 100644
>>>> --- a/xen/drivers/vpci/msi.c
>>>> +++ b/xen/drivers/vpci/msi.c
>>>> @@ -318,15 +321,28 @@ void vpci_dump_msi(void)
>>>>                       * holding the lock.
>>>>                       */
>>>>                      printk("unable to print all MSI-X entries: %d\n", rc);
>>>> -                    process_pending_softirqs();
>>>> -                    continue;
>>>> +                    goto pdev_done;
>>>>                  }
>>>>              }
>>>>
>>>>              spin_unlock(&pdev->vpci->lock);
>>>> + pdev_done:
>>>> +            /*
>>>> +             * Unlock lock to process pending softirqs. This is
>>>> +             * potentially unsafe, as d->pdev_list can be changed in
>>>> +             * meantime.
>>>> +             */
>>>> +            read_unlock(&d->pci_lock);
>>>>              process_pending_softirqs();
>>>> +            if ( !read_trylock(&d->pci_lock) )
>>>> +            {
>>>> +                printk("unable to access other devices for the domain\n");
>>>> +                goto domain_done;
>>>
>>> Shouldn't the domain_done label be after the read_unlock(), so that we
>>> can proceed to try to dump the devices for the next domain?  With the
>>> proposed code a failure to acquire one of the domains pci_lock
>>> terminates the dump.
>>>
>>>> +            }
>>>>          }
>>>> +        read_unlock(&d->pci_lock);
>>>>      }
>>>> + domain_done:
>>>>      rcu_read_unlock(&domlist_read_lock);
>>>>  }
>>>>
>>
>> With the label moved, a no-op expression after the label is needed to make 
>> the compiler happy:
>>
>>             }
>>         }
>>         read_unlock(&d->pci_lock);
>>  domain_done:
>>         (void)0;
>>     }
>>     rcu_read_unlock(&domlist_read_lock);
>> }
>>
>>
>> If the no-op is omitted, the compiler may complain (gcc 9.4.0):
>>
>> drivers/vpci/msi.c: In function ‘vpci_dump_msi’:
>> drivers/vpci/msi.c:351:2: error: label at end of compound statement
>>   351 |  domain_done:
>>       |  ^~~~~~~~~~~
> 
> 
> Might be better to place the label at the start of the loop, and
> likely rename to next_domain.

That would bypass the loop condition and increment statements.