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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.