Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stewart Hildebrand <stewart.hildebrand@amd.com>,
	Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Kevin Tian <kevin.tian@intel.com>, Paul Durrant <paul@xen.org>,
	Volodymyr Babchuk <volodymyr_babchuk@epam.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure
Date: Wed, 24 Jan 2024 18:51:15 +0100	[thread overview]
Message-ID: <ZbFOE6kV7NXFrNjb@macbook> (raw)
In-Reply-To: <a3233487-965b-49ae-bebe-e5476fcda29c@suse.com>

On Wed, Jan 24, 2024 at 12:34:10PM +0100, Jan Beulich wrote:
> On 24.01.2024 10:24, Roger Pau Monné wrote:
> > On Wed, Jan 24, 2024 at 09:48:35AM +0100, Jan Beulich wrote:
> >> On 23.01.2024 16:07, Roger Pau Monné wrote:
> >>> On Tue, Jan 23, 2024 at 03:32:12PM +0100, Jan Beulich wrote:
> >>>> On 15.01.2024 20:43, Stewart Hildebrand wrote:
> >>>>> @@ -2888,6 +2888,8 @@ int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
> >>>>>  {
> >>>>>      int irq, pirq, ret;
> >>>>>  
> >>>>> +    ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock));
> >>>>
> >>>> If either lock is sufficient to hold here, ...
> >>>>
> >>>>> --- a/xen/arch/x86/physdev.c
> >>>>> +++ b/xen/arch/x86/physdev.c
> >>>>> @@ -123,7 +123,9 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
> >>>>>  
> >>>>>      case MAP_PIRQ_TYPE_MSI:
> >>>>>      case MAP_PIRQ_TYPE_MULTI_MSI:
> >>>>> +        pcidevs_lock();
> >>>>>          ret = allocate_and_map_msi_pirq(d, *index, pirq_p, type, msi);
> >>>>> +        pcidevs_unlock();
> >>>>>          break;
> >>>>
> >>>
> >>> IIRC (Stewart can further comment) this is done holding the pcidevs
> >>> lock to keep the path unmodified, as there's no need to hold the
> >>> per-domain rwlock.
> >>
> >> Yet why would we prefer to acquire a global lock when a per-domain one
> >> suffices?
> > 
> > I was hoping to introduce less changes, specially if they are not
> > strictly required, as it's less risk.  I'm always quite worry of
> > locking changes.
> 
> In which case more description / code commenting is needed. The pattern
> of the assertions looks dangerous.

Is such dangerousness perception because you fear some of the pcidevs
lock usage might be there not just for preventing the pdev from going
away, but also to guarantee exclusive access to certain state?

> Even if (as you say in a later reply)
> this is only temporary, we all know how long "temporary" can be. It
> might even be advisable to introduce a helper construct.

The aim here was to modify as little as possible, in order to avoid
having to analyze all possible users of pcidevs lock, and thus not
block the vPCI work on the probably lengthy and difficult analysis.

Not sure adding a construct makes is much better, as I didn't want to
give the impression all checks for the pcidevs lock can merely be
replaced by the new construct.

Thanks, Roger.