Re: [PATCH] vpci: introduce per-domain lock to protect vpci structure

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 14.02.22 10:47, Roger Pau Monné wrote:
> On Mon, Feb 14, 2022 at 06:33:07AM +0000, Oleksandr Andrushchenko wrote:
>>
>> On 11.02.22 17:44, Roger Pau Monné wrote:
>>> On Fri, Feb 11, 2022 at 12:13:38PM +0000, Oleksandr Andrushchenko wrote:
>>>> On 11.02.22 13:40, Roger Pau Monné wrote:
>>>>> On Fri, Feb 11, 2022 at 07:27:39AM +0000, Oleksandr Andrushchenko wrote:
>>>>>> Hi, Roger!
>>>>>>
>>>>>> On 10.02.22 18:16, Roger Pau Monné wrote:
>>>>>>> On Wed, Feb 09, 2022 at 03:36:27PM +0200, Oleksandr Andrushchenko wrote:
>>>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>>>>
>>>>>>>> Introduce a per-domain read/write lock to check whether vpci is 
>>>>>>>> present,
>>>>>>>> so we are sure there are no accesses to the contents of the vpci struct
>>>>>>>> if not. This lock can be used (and in a few cases is used right away)
>>>>>>>> so that vpci removal can be performed while holding the lock in write
>>>>>>>> mode. Previously such removal could race with vpci_read for example.
>>>>>>> Sadly there's still a race in the usage of pci_get_pdev_by_domain wrt
>>>>>>> pci_remove_device, and likely when vPCI gets also used in
>>>>>>> {de}assign_device I think.
>>>>>> Yes, this is indeed an issue, but I was not trying to solve it in
>>>>>> context of vPCI locking yet. I think we should discuss how do
>>>>>> we approach pdev locking, so I can create a patch for that.
>>>>>> that being said, I would like not to solve pdev in  this patch yet
>>>>>>
>>>>>> ...I do understand we do want to avoid that, but at the moment
>>>>>> a single reliable way for making sure pdev is alive seems to
>>>>>> be pcidevs_lock....
>>>>> I think we will need to make pcidevs_lock a rwlock and take it in read
>>>>> mode for pci_get_pdev_by_domain.
>>>>>
>>>>> We didn't have this scenario before where PCI emulation is done in the
>>>>> hypervisor, and hence the locking around those data structures has not
>>>>> been designed for those use-cases.
>>>> Yes, I do understand that.
>>>> I hope pcidevs lock move to rwlock can be done as a separate
>>>> patch. While this is not done, do you think we can proceed with
>>>> vPCI series and pcidevs locking re-work being done after?
>>> Ideally we would like to sort out the locking once and for all. I
>>> would like to be sure that what we introduce now doesn't turn out to
>>> interact badly when we decide to look at the pcidevs locking issue.
>> Ok, so I'll start converting pcidevs into rwlock then
> Sorry, maybe I didn't express myself correctly, since the current
> series doesn't lead to a functional implementation of vPCI for domUs I
> would be fine with postponing the locking work, as long as the
> currently introduced code doesn't make it any worse or extend the
> locking scheme into new paths, but maybe that's not very helpful.
Indeed, I misunderstood you probably. Great, so we can continue
working on the vPCI series which when accepted will unblock
MSI/MSI-X work which depends on vPCI. Then, in parallel with MSIs,
we can start re-working pcidevs locking.
>
> Thanks, Roger.
Thank you,
Oleksandr