Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci

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

On Mon, Feb 07, 2022 at 04:26:56PM +0100, Jan Beulich wrote:
> On 07.02.2022 16:11, Oleksandr Andrushchenko wrote:
> > 
> > 
> > On 07.02.22 16:35, Oleksandr Andrushchenko wrote:
> >>
> >> On 07.02.22 16:27, Roger Pau Monné wrote:
> >>> On Mon, Feb 07, 2022 at 03:11:03PM +0100, Jan Beulich wrote:
> >>>> On 07.02.2022 14:53, Oleksandr Andrushchenko wrote:
> >>>>> On 07.02.22 14:46, Roger Pau Monné wrote:
> >>>>>> I think the per-domain rwlock seems like a good option. I would do
> >>>>>> that as a pre-patch.
> >>>>> It is. But it seems it won't solve the thing we started this adventure 
> >>>>> for:
> >>>>>
> >>>>> With per-domain read lock and still ABBA in modify_bars (hope the below
> >>>>> is correctly seen with a monospace font):
> >>>>>
> >>>>> cpu0: vpci_write-> d->RLock -> pdev1->lock ->                           
> >>>>>                        rom_write -> modify_bars: tmp (pdev2) ->lock
> >>>>> cpu1:        vpci_write-> d->RLock pdev2->lock -> cmd_write -> 
> >>>>> modify_bars: tmp (pdev1) ->lock
> >>>>>
> >>>>> There is no API to upgrade read lock to write lock in modify_bars which 
> >>>>> could help,
> >>>>> so in both cases vpci_write should take write lock.
> >>>> Hmm, yes, I think you're right: It's not modify_bars() itself which needs
> >>>> to acquire the write lock, but its (perhaps indirect) caller. Effectively
> >>>> vpci_write() would need to take the write lock if the range written
> >>>> overlaps the BARs or the command register.
> >>> I'm confused. If we use a per-domain rwlock approach there would be no
> >>> need to lock tmp again in modify_bars, because we should hold the
> >>> rwlock in write mode, so there's no ABBA?
> >> this is only possible with what you wrote below:
> >>> We will have however to drop the per domain read and vpci locks and
> >>> pick the per-domain lock in write mode.
> >> I think this is going to be unreliable. We need a reliable way to
> >> upgrade read lock to write lock.
> >> Then, we can drop pdev->vpci_lock at all, because we are always
> >> protected with d->rwlock and those who want to free pdev->vpci
> >> will use write lock.
> >>
> >> So, per-domain rwlock with write upgrade implemented minus pdev->vpci
> >> should do the trick
> > Linux doesn't implement write upgrade and it seems for a reason [1]:
> > "Also, you cannot “upgrade” a read-lock to a write-lock, so if you at _any_ 
> > time
> > need to do any changes (even if you don’t do it every time), you have to get
> > the write-lock at the very beginning."
> > 
> > So, I am not sure we can have the same for Xen...
> > 
> > At the moment I see at least two possible ways to solve the issue:
> > 1. Make vpci_write use write lock, thus make all write accesses synchronized
> > for the given domain, read are fully parallel
> 
> 1b. Make vpci_write use write lock for writes to command register and BARs
> only; keep using the read lock for all other writes.

We do not support writing to the BARs with memory decoding enabled
currently for dom0, so we would only need to pick the lock in write
mode for the command register and ROM BAR write handler AFAICT.

Thanks, Roger.