From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock Date: Tue, 9 Aug 2016 16:49:37 +0200 Message-ID: <20160809144937.GJ9175@cbox> References: <20160809122035.16196-1-christoffer.dall@linaro.org> <20160809122035.16196-3-christoffer.dall@linaro.org> <27023cbf-294f-b316-97ef-8da4e726cc98@redhat.com> <20160809125519.GG9175@cbox> <44eacdbf-5ec2-6b8c-d2cb-4fe09dbf001d@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Andre Przywara , paulus@ozlabs.org, borntraeger@de.ibm.com, kvmarm@lists.cs.columbia.edu To: Paolo Bonzini Return-path: Content-Disposition: inline In-Reply-To: <44eacdbf-5ec2-6b8c-d2cb-4fe09dbf001d@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org On Tue, Aug 09, 2016 at 03:16:26PM +0200, Paolo Bonzini wrote: > > > On 09/08/2016 14:55, Christoffer Dall wrote: > > On Tue, Aug 09, 2016 at 02:37:43PM +0200, Paolo Bonzini wrote: > >> > >> > >> On 09/08/2016 14:20, Christoffer Dall wrote: > >>> KVM devices were manipulating list data structures without any form of > >>> synchronization, and some implementations of the create operations also > >>> suffered from a lack of synchronization. > >>> > >>> Now when we've split the xics create operation into create and init, we > >>> can hold the kvm->lock mutex while calling the create operation and when > >>> manipulating the devices list. > >>> > >>> The error path in the generic code gets slightly ugly because we have to > >>> take the mutex again and delete the device from the list, but holding > >>> the mutex during anon_inode_getfd or releasing/locking the mutex in the > >>> common non-error path seemed wrong. > >>> > >>> Signed-off-by: Christoffer Dall > >> > >> Very nice (and small), but please add a comment to the create member in > >> kvm_device_ops. > > > > Like this?: > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index d3c9b82..9c28b4d 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -1113,6 +1113,12 @@ struct kvm_device { > > /* create, destroy, and name are mandatory */ > > struct kvm_device_ops { > > const char *name; > > + > > + /* > > + * create is called holding kvm->lock and any operations not suitable > > + * to do while holding the lock should be deferred to init (see > > + * below). > > + */ > > int (*create)(struct kvm_device *dev, u32 type); > > > > /* > > > > That's okay, series > > Reviewed-by: Paolo Bonzini Thanks, I'll send a v2. Will you just apply the patches to kvm/master or would you like me to include it in my pull request for -rc2 ? Also, do you want to wait for a tested-by from the other arch maintainers? -Christoffer