Re: [PATCH 01/20] KVM: Treat the device list as an rculist

[Oliver Upton <oliver.upton@linux.dev>]

On Wed, Feb 28, 2024 at 09:23:38PM +0100, Paolo Bonzini wrote:
> On Wed, Feb 28, 2024 at 8:31 PM Oliver Upton <oliver.upton@linux.dev> wrote:
> >
> > On Wed, Feb 28, 2024 at 11:13:52AM -0800, Sean Christopherson wrote:
> > > I'm taking exception to the "Prepare by converting to an rculist." statement.
> > > This is not an RCU-protected list, it's a list that abuses rcu_list_add() and
> > > list_for_each_rcu() to allow readers to run concurrently with insertion.  E.g.
> > > IIUC, if it weren't for PROVE_RCU, the rcu_read_(un)lock() in the reader could
> > > be omitted and everything would work just fine.
> >
> > Fair, if we wanted to be exact about it this change relies on the
> > pointer assignment properties of RCU, but not necessarily full-blown RCU
> > protection. But...
> >
> > > Ah, but it's a moot point, because kvm_device_release() does delete from the list,
> > > and does not do so in an RCU-safe manner.  So that needs to be fixed, and then
> > > this is indeed an RCU-protected list.
> >
> > Shows how much thought I threw at the problem :D Nice catch, I'll fold
> > in a fix next time around.
> 
> You also need to adjust the error case of kvm_ioctl_create_device()
> (not a thorough review, I did).
> 
> Also double check that it's okay for your concurrent reader to find
> the device while ops->init runs (or, better, ensure that it only does
> what it has to do after ops->init is finished, with the appropriate
> store-release/load-acquire operations on the two sides).

The reader on the GIC side of things is protected against this by
construction (no ->init() op), but I'd expect the reader & device to
establish their own ordering / synchronization for the relevant data.
Feel like the generic code is only responsible for protecting the
structure of the liest.

TBH, the issue I was working around isn't lock serialization
but rather lock ordering (I need to get at this while holding the
vcpu->mutex). Zero performance expectations for the reader I added over
in the vgic side of things.

So, with that in mind, I'm equally satisfied doing the less-complicated
thing of protecting the structure of the list with its own lock.

-- 
Thanks,
Oliver