Re: [PATCH 1/3] kvm/ppc/booke: Hold srcu lock when calling gfn functions

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Apr 26, 2013 at 07:53:38PM -0500, Scott Wood wrote:
> KVM core expects arch code to acquire the srcu lock when calling
> gfn_to_memslot and similar functions.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
>  arch/powerpc/kvm/44x_tlb.c  |    5 +++++
>  arch/powerpc/kvm/booke.c    |   19 +++++++++++++++++++
>  arch/powerpc/kvm/e500_mmu.c |    5 +++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/arch/powerpc/kvm/44x_tlb.c b/arch/powerpc/kvm/44x_tlb.c
> index 5dd3ab4..ed03854 100644
> --- a/arch/powerpc/kvm/44x_tlb.c
> +++ b/arch/powerpc/kvm/44x_tlb.c
> @@ -441,6 +441,7 @@ int kvmppc_44x_emul_tlbwe(struct kvm_vcpu *vcpu, u8 ra, u8 rs, u8 ws)
>  	struct kvmppc_vcpu_44x *vcpu_44x = to_44x(vcpu);
>  	struct kvmppc_44x_tlbe *tlbe;
>  	unsigned int gtlb_index;
> +	int idx;
>  
>  	gtlb_index = kvmppc_get_gpr(vcpu, ra);
>  	if (gtlb_index >= KVM44x_GUEST_TLB_SIZE) {
> @@ -473,6 +474,8 @@ int kvmppc_44x_emul_tlbwe(struct kvm_vcpu *vcpu, u8 ra, u8 rs, u8 ws)
>  		return EMULATE_FAIL;
>  	}
>  
> +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> +
>  	if (tlbe_is_host_safe(vcpu, tlbe)) {
>  		gva_t eaddr;
>  		gpa_t gpaddr;
> @@ -489,6 +492,8 @@ int kvmppc_44x_emul_tlbwe(struct kvm_vcpu *vcpu, u8 ra, u8 rs, u8 ws)
>  		kvmppc_mmu_map(vcpu, eaddr, gpaddr, gtlb_index);
>  	}
>  
> +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +
>  	trace_kvm_gtlb_write(gtlb_index, tlbe->tid, tlbe->word0, tlbe->word1,
>  			     tlbe->word2);
>  
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 1020119..506c87d 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -832,6 +832,8 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  {
>  	int r = RESUME_HOST;
>  	int s;
> +	int idx = 0; /* silence bogus uninitialized warning */
> +	bool need_srcu = false;
>  
>  	/* update before a new last_exit_type is rewritten */
>  	kvmppc_update_timing_stats(vcpu);
> @@ -847,6 +849,20 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  	run->exit_reason = KVM_EXIT_UNKNOWN;
>  	run->ready_for_interrupt_injection = 1;
>  
> +	/*
> +	 * Don't get the srcu lock unconditionally, because kvm_ppc_pv()
> +	 * can call kvm_vcpu_block(), and kvm_ppc_pv() is shared with
> +	 * book3s, so dropping the srcu lock there would be awkward.
> +	 */
> +	switch (exit_nr) {
> +	case BOOKE_INTERRUPT_ITLB_MISS:
> +	case BOOKE_INTERRUPT_DTLB_MISS:
> +		need_srcu = true;
> +	}

This is not good practice (codepaths should either hold srcu or not hold
it, unconditionally).

Can you give more details of the issue? (not obvious)