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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.09.2023 15:56, Stewart Hildebrand wrote:
> 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.

Right, such a label would be bogus even without that; instead of "goto"
the use site then simply should use "continue".

Jan