diff mbox series

[v3,20/41] KVM: PPC: Book3S HV P9: Move setting HDEC after switching to guest LPCR

Message ID 20210305150638.2675513-21-npiggin@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series KVM: PPC: Book3S: C-ify the P9 entry/exit code | expand
Related show

Checks

Context Check Description
snowpatch_ozlabs/apply_patch warning Failed to apply on branch powerpc/merge (91966823812efbd175f904599e5cf2a854b39809)
snowpatch_ozlabs/apply_patch warning Failed to apply on branch powerpc/next (fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8)
snowpatch_ozlabs/apply_patch warning Failed to apply on branch linus/master (280d542f6ffac0e6d65dc267f92191d509b13b64)
snowpatch_ozlabs/apply_patch warning Failed to apply on branch powerpc/fixes (5c88a17e15795226b56d83f579cbb9b7a4864f79)
snowpatch_ozlabs/apply_patch warning Failed to apply on branch linux-next (7a7fd0de4a9804299793e564a555a49c1fc924cb)
snowpatch_ozlabs/apply_patch fail Failed to apply to any branch

Commit Message

Nicholas Piggin March 5, 2021, 3:06 p.m. UTC 
LPCR[HDICE]=0 suppresses hypervisor decrementer exceptions on some
processors, so it must be enabled before HDEC is set.

Rather than set it in the host LPCR then setting HDEC, move the HDEC
update to after the guest MMU context (including LPCR) is loaded.
There shouldn't be much concern with delaying HDEC by some 10s or 100s
of nanoseconds by setting it a bit later.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kvm/book3s_hv.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

Comments

Fabiano Rosas March 8, 2021, 5:52 p.m. UTC | #1 
Nicholas Piggin <npiggin@gmail.com> writes:

> LPCR[HDICE]=0 suppresses hypervisor decrementer exceptions on some
> processors, so it must be enabled before HDEC is set.
>
> Rather than set it in the host LPCR then setting HDEC, move the HDEC
> update to after the guest MMU context (including LPCR) is loaded.
> There shouldn't be much concern with delaying HDEC by some 10s or 100s
> of nanoseconds by setting it a bit later.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

> ---
>  arch/powerpc/kvm/book3s_hv.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 1f2ba8955c6a..ffde1917ab68 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3505,20 +3505,9 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>  		host_dawrx1 = mfspr(SPRN_DAWRX1);
>  	}
>
> -	/*
> -	 * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
> -	 * so set HDICE before writing HDEC.
> -	 */
> -	mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
> -	isync();
> -
>  	hdec = time_limit - mftb();
> -	if (hdec < 0) {
> -		mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
> -		isync();
> +	if (hdec < 0)
>  		return BOOK3S_INTERRUPT_HV_DECREMENTER;
> -	}
> -	mtspr(SPRN_HDEC, hdec);
>
>  	if (vc->tb_offset) {
>  		u64 new_tb = mftb() + vc->tb_offset;
> @@ -3564,6 +3553,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>
>  	switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>
> +	/*
> +	 * P9 suppresses the HDEC exception when LPCR[HDICE] = 0,
> +	 * so set guest LPCR (with HDICE) before writing HDEC.
> +	 */
> +	mtspr(SPRN_HDEC, hdec);
> +
>  	mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
>  	mtspr(SPRN_SRR1, vcpu->arch.shregs.srr1);
Alexey Kardashevskiy March 22, 2021, 8:39 a.m. UTC | #2 
On 06/03/2021 02:06, Nicholas Piggin wrote:
> LPCR[HDICE]=0 suppresses hypervisor decrementer exceptions on some
> processors, so it must be enabled before HDEC is set.

Educating myself - is not it a processor bug when it does not suppress 
hdec exceptions with HDICE=0?

Also, why do we want to enable interrupts before writing HDEC? Enabling 
it may cause an interrupt right away a

Anyway whatever the answers are, this is not changed by this patch and 
the change makes sense so

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


> Rather than set it in the host LPCR then setting HDEC, move the HDEC
> update to after the guest MMU context (including LPCR) is loaded.
> There shouldn't be much concern with delaying HDEC by some 10s or 100s
> of nanoseconds by setting it a bit later.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/kvm/book3s_hv.c | 19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 1f2ba8955c6a..ffde1917ab68 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3505,20 +3505,9 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>   		host_dawrx1 = mfspr(SPRN_DAWRX1);
>   	}
>   
> -	/*
> -	 * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
> -	 * so set HDICE before writing HDEC.
> -	 */
> -	mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
> -	isync();
> -
>   	hdec = time_limit - mftb();
> -	if (hdec < 0) {
> -		mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
> -		isync();
> +	if (hdec < 0)
>   		return BOOK3S_INTERRUPT_HV_DECREMENTER;
> -	}
> -	mtspr(SPRN_HDEC, hdec);
>   
>   	if (vc->tb_offset) {
>   		u64 new_tb = mftb() + vc->tb_offset;
> @@ -3564,6 +3553,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>   
>   	switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>   
> +	/*
> +	 * P9 suppresses the HDEC exception when LPCR[HDICE] = 0,
> +	 * so set guest LPCR (with HDICE) before writing HDEC.
> +	 */
> +	mtspr(SPRN_HDEC, hdec);
> +
>   	mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
>   	mtspr(SPRN_SRR1, vcpu->arch.shregs.srr1);
>   
>
Nicholas Piggin March 22, 2021, 1:24 p.m. UTC | #3 
Excerpts from Alexey Kardashevskiy's message of March 22, 2021 6:39 pm:
> 
> 
> On 06/03/2021 02:06, Nicholas Piggin wrote:
>> LPCR[HDICE]=0 suppresses hypervisor decrementer exceptions on some
>> processors, so it must be enabled before HDEC is set.
> 
> Educating myself - is not it a processor bug when it does not suppress 
> hdec exceptions with HDICE=0?

It seems to be contrary to the architecture if it does suppress them.
The HDEC exception is supposed to come into existence when the top bit
of HDEC SPR, regardless of HDICE AFAIKS.

Interrupt being taken in response to an exception existing is different.
The interrupt is suppressed when HDICE=0, which is also a requirement
of the architecture.

> Also, why do we want to enable interrupts before writing HDEC? Enabling 
> it may cause an interrupt right away a

HDICE does not enable HDEC interrupts when HV=1 unless EE is 1.

In other words, HDEC interrupts are masked with EE just like DEC 
interrupts when you are the hypervisor, but they ignore EE when the 
guest is running (so guest can't delay them).

> 
> Anyway whatever the answers are, this is not changed by this patch and 
> the change makes sense so
> 
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Thanks,
Nick

> 
> 
>> Rather than set it in the host LPCR then setting HDEC, move the HDEC
>> update to after the guest MMU context (including LPCR) is loaded.
>> There shouldn't be much concern with delaying HDEC by some 10s or 100s
>> of nanoseconds by setting it a bit later.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   arch/powerpc/kvm/book3s_hv.c | 19 +++++++------------
>>   1 file changed, 7 insertions(+), 12 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 1f2ba8955c6a..ffde1917ab68 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -3505,20 +3505,9 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>   		host_dawrx1 = mfspr(SPRN_DAWRX1);
>>   	}
>>   
>> -	/*
>> -	 * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
>> -	 * so set HDICE before writing HDEC.
>> -	 */
>> -	mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
>> -	isync();
>> -
>>   	hdec = time_limit - mftb();
>> -	if (hdec < 0) {
>> -		mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
>> -		isync();
>> +	if (hdec < 0)
>>   		return BOOK3S_INTERRUPT_HV_DECREMENTER;
>> -	}
>> -	mtspr(SPRN_HDEC, hdec);
>>   
>>   	if (vc->tb_offset) {
>>   		u64 new_tb = mftb() + vc->tb_offset;
>> @@ -3564,6 +3553,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>   
>>   	switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>>   
>> +	/*
>> +	 * P9 suppresses the HDEC exception when LPCR[HDICE] = 0,
>> +	 * so set guest LPCR (with HDICE) before writing HDEC.
>> +	 */
>> +	mtspr(SPRN_HDEC, hdec);
>> +
>>   	mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
>>   	mtspr(SPRN_SRR1, vcpu->arch.shregs.srr1);
>>   
>> 
> 
> -- 
> Alexey
>
diff mbox series

Patch

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 1f2ba8955c6a..ffde1917ab68 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -3505,20 +3505,9 @@  static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
 		host_dawrx1 = mfspr(SPRN_DAWRX1);
 	}
 
-	/*
-	 * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
-	 * so set HDICE before writing HDEC.
-	 */
-	mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
-	isync();
-
 	hdec = time_limit - mftb();
-	if (hdec < 0) {
-		mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
-		isync();
+	if (hdec < 0)
 		return BOOK3S_INTERRUPT_HV_DECREMENTER;
-	}
-	mtspr(SPRN_HDEC, hdec);
 
 	if (vc->tb_offset) {
 		u64 new_tb = mftb() + vc->tb_offset;
@@ -3564,6 +3553,12 @@  static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
 
 	switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
 
+	/*
+	 * P9 suppresses the HDEC exception when LPCR[HDICE] = 0,
+	 * so set guest LPCR (with HDICE) before writing HDEC.
+	 */
+	mtspr(SPRN_HDEC, hdec);
+
 	mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
 	mtspr(SPRN_SRR1, vcpu->arch.shregs.srr1);