Re: [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

On Fri, Sep 18, 2015 at 08:57:28AM +0200, Thomas Huth wrote:
> Access to the kvm->buses (like with the kvm_io_bus_read() and -write()
> functions) has to be protected via the kvm->srcu lock.
> The kvmppc_h_logical_ci_load() and -store() functions are missing
> this lock so far, so let's add it there, too.
> This fixes the problem that the kernel reports "suspicious RCU usage"
> when lock debugging is enabled.
> 
> Fixes: 99342cf8044420eebdf9297ca03a14cb6a7085a1
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Nice catch.  Looks like I missed this because the places
kvm_io_bus_{read,write}() are called on x86 are buried about 5 layers
below where the srcu lock is taken :/.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  arch/powerpc/kvm/book3s.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index d75bf32..096e5eb 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -828,12 +828,15 @@ int kvmppc_h_logical_ci_load(struct kvm_vcpu *vcpu)
>  	unsigned long size = kvmppc_get_gpr(vcpu, 4);
>  	unsigned long addr = kvmppc_get_gpr(vcpu, 5);
>  	u64 buf;
> +	int srcu_idx;
>  	int ret;
>  
>  	if (!is_power_of_2(size) || (size > sizeof(buf)))
>  		return H_TOO_HARD;
>  
> +	srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>  	ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, size, &buf);
> +	srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx);
>  	if (ret != 0)
>  		return H_TOO_HARD;
>  
> @@ -868,6 +871,7 @@ int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu)
>  	unsigned long addr = kvmppc_get_gpr(vcpu, 5);
>  	unsigned long val = kvmppc_get_gpr(vcpu, 6);
>  	u64 buf;
> +	int srcu_idx;
>  	int ret;
>  
>  	switch (size) {
> @@ -891,7 +895,9 @@ int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu)
>  		return H_TOO_HARD;
>  	}
>  
> +	srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>  	ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, size, &buf);
> +	srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx);
>  	if (ret != 0)
>  		return H_TOO_HARD;
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]