Re: [PATCH v3 16/41] KVM: PPC: Book3S HV P9: Move radix MMU switching instructions together

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 22/03/2021 16:25, Nicholas Piggin wrote:
> Excerpts from Alexey Kardashevskiy's message of March 22, 2021 2:24 pm:
>>
>>
>> On 06/03/2021 02:06, Nicholas Piggin wrote:
>>> Switching the MMU from radix<->radix mode is tricky particularly as the
>>> MMU can remain enabled and requires a certain sequence of SPR updates.
>>> Move these together into their own functions.
>>>
>>> This also includes the radix TLB check / flush because it's tied in to
>>> MMU switching due to tlbiel getting LPID from LPIDR.
>>>
>>> (XXX: isync / hwsync synchronisation TBD)
>>
>>
>> Looks alright but what is this comment about? Is something missing or
>> just sub optimal?
> 
> Ah, yeah the architecture says for example a CSI is required before +
> after each, but the fine print is that you only need those to separate
> between previous or subsequent accesses that may use those contexts
> being switched from/to.
> 
> Then there is the question of CSI between the instructions so e.g., you
> don't get the TLB prefetch bug if the mtPIDR could go out of order ahead
> of the mtLPIDR, but those instructions are serialized so they wouldn't.
> 
> There's possibly a few clarifications coming to the architecture around
> this as well.
> 
> I think things are relatively okay but probably need a bit more
> commenting to justify where the isyncs() aren't. It's possible we might
> be able to even remove the isyncs that are there.
> 
> Making a like-for-like conversion is a bit tricky because there are
> possible context synchronising instructions between them already.
> 
> Maybe for the first series, I'll just put an isync between all of them,
> and then a later patch can replace some of them with comments.
> 
>>
>>
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>
>>
>>> ---
>>>    arch/powerpc/kvm/book3s_hv.c | 55 +++++++++++++++++++++---------------
>>>    1 file changed, 32 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>>> index f1230f9d98ba..b9cae42b9cd5 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -3449,12 +3449,38 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
>>>    	trace_kvmppc_run_core(vc, 1);
>>>    }
>>>    
>>> +static void switch_mmu_to_guest_radix(struct kvm *kvm, struct kvm_vcpu *vcpu, u64 lpcr)
>>> +{
>>> +	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>>> +	struct kvm_nested_guest *nested = vcpu->arch.nested;
>>> +	u32 lpid;
>>> +
>>> +	lpid = nested ? nested->shadow_lpid : kvm->arch.lpid;
>>> +
>>> +	mtspr(SPRN_LPID, lpid);
>>> +	mtspr(SPRN_LPCR, lpcr);
>>> +	mtspr(SPRN_PID, vcpu->arch.pid);
>>> +	isync();
>>> +
>>> +	/* TLBIEL must have LPIDR set, so set guest LPID before flushing. */
>>> +	kvmppc_check_need_tlb_flush(kvm, vc->pcpu, nested);
>>> +}
>>> +
>>> +static void switch_mmu_to_host_radix(struct kvm *kvm, u32 pid)
>>> +{
>>> +	mtspr(SPRN_PID, pid);
>>> +	mtspr(SPRN_LPID, kvm->arch.host_lpid);
>>> +	mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
>>> +	isync();
>>> +}
>>> +
>>>    /*
>>>     * Load up hypervisor-mode registers on P9.
>>>     */
>>>    static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    				     unsigned long lpcr)
>>>    {
>>> +	struct kvm *kvm = vcpu->kvm;
>>>    	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>>>    	s64 hdec;
>>>    	u64 tb, purr, spurr;
>>> @@ -3477,12 +3503,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    	 * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
>>>    	 * so set HDICE before writing HDEC.
>>>    	 */
>>> -	mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr | LPCR_HDICE);
>>> +	mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
>>>    	isync();
>>>    
>>>    	hdec = time_limit - mftb();
>>>    	if (hdec < 0) {
>>> -		mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr);
>>> +		mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
>>>    		isync();
>>>    		return BOOK3S_INTERRUPT_HV_DECREMENTER;
>>>    	}
>>> @@ -3517,7 +3543,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    	}
>>>    	mtspr(SPRN_CIABR, vcpu->arch.ciabr);
>>>    	mtspr(SPRN_IC, vcpu->arch.ic);
>>> -	mtspr(SPRN_PID, vcpu->arch.pid);
>>>    
>>>    	mtspr(SPRN_PSSCR, vcpu->arch.psscr | PSSCR_EC |
>>>    	      (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
>>> @@ -3531,8 +3556,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    
>>>    	mtspr(SPRN_AMOR, ~0UL);
>>>    
>>> -	mtspr(SPRN_LPCR, lpcr);
>>> -	isync();
>>> +	switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>>>    
>>>    	kvmppc_xive_push_vcpu(vcpu);
>>>    
>>> @@ -3571,7 +3595,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    		mtspr(SPRN_DAWR1, host_dawr1);
>>>    		mtspr(SPRN_DAWRX1, host_dawrx1);
>>>    	}
>>> -	mtspr(SPRN_PID, host_pidr);
>>>    
>>>    	/*
>>>    	 * Since this is radix, do a eieio; tlbsync; ptesync sequence in
>>> @@ -3586,9 +3609,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    	if (cpu_has_feature(CPU_FTR_ARCH_31))
>>>    		asm volatile(PPC_CP_ABORT);
>>>    
>>> -	mtspr(SPRN_LPID, vcpu->kvm->arch.host_lpid);	/* restore host LPID */
>>> -	isync();
>>> -
>>>    	vc->dpdes = mfspr(SPRN_DPDES);
>>>    	vc->vtb = mfspr(SPRN_VTB);
>>>    	mtspr(SPRN_DPDES, 0);
>>> @@ -3605,7 +3625,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    	}
>>>    
>>>    	mtspr(SPRN_HDEC, 0x7fffffff);
>>> -	mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr);
>>> +
>>> +	switch_mmu_to_host_radix(kvm, host_pidr);
>>>    
>>>    	return trap;
>>>    }
>>> @@ -4138,7 +4159,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    {
>>>    	struct kvm_run *run = vcpu->run;
>>>    	int trap, r, pcpu;
>>> -	int srcu_idx, lpid;
>>> +	int srcu_idx;
>>>    	struct kvmppc_vcore *vc;
>>>    	struct kvm *kvm = vcpu->kvm;
>>>    	struct kvm_nested_guest *nested = vcpu->arch.nested;
>>> @@ -4212,13 +4233,6 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>>>    	vc->vcore_state = VCORE_RUNNING;
>>>    	trace_kvmppc_run_core(vc, 0);
>>>    
>>> -	if (cpu_has_feature(CPU_FTR_HVMODE)) {
>>
>>
>> The new location of mtspr(SPRN_LPID, lpid) does not check for
>> CPU_FTR_HVMODE anymore, is this going to work with HV KVM on pseries?
> 
> Yes, these are moved to HVMODE specific code now.

ah right, kvmhv_on_pseries() is !cpu_has_feature(CPU_FTR_HVMODE).


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


-- 
Alexey