arch/powerpc/kvm: Avoid extra checks when emulating HFSCR bits

[PATCH] arch/powerpc/kvm: Avoid extra checks when emulating HFSCR bits

Posted by Gautam Menghani 1 year, 5 months ago

When a KVM guest tries to use a feature disabled by HFSCR, it exits to
the host for emulation support, and the code checks for all bits which
are emulated. Avoid checking all the bits by using a switch case.

Signed-off-by: Gautam Menghani <gautam@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 99c7ce825..a72fd2543 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1922,14 +1922,22 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu,
 
 		r = EMULATE_FAIL;
 		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
-			if (cause == FSCR_MSGP_LG)
+			switch (cause) {
+			case FSCR_MSGP_LG:
 				r = kvmppc_emulate_doorbell_instr(vcpu);
-			if (cause == FSCR_PM_LG)
+				break;
+			case FSCR_PM_LG:
 				r = kvmppc_pmu_unavailable(vcpu);
-			if (cause == FSCR_EBB_LG)
+				break;
+			case FSCR_EBB_LG:
 				r = kvmppc_ebb_unavailable(vcpu);
-			if (cause == FSCR_TM_LG)
+				break;
+			case FSCR_TM_LG:
 				r = kvmppc_tm_unavailable(vcpu);
+				break;
+			default:
+				break;
+			}
 		}
 		if (r == EMULATE_FAIL) {
 			kvmppc_core_queue_program(vcpu, SRR1_PROGILL |
-- 
2.45.2

Re: [PATCH] arch/powerpc/kvm: Avoid extra checks when emulating HFSCR bits

Posted by Michael Ellerman 1 year, 5 months ago

Gautam Menghani <gautam@linux.ibm.com> writes:
> When a KVM guest tries to use a feature disabled by HFSCR, it exits to
> the host for emulation support, and the code checks for all bits which
> are emulated. Avoid checking all the bits by using a switch case.

The patch looks fine, but I don't know what you mean by "avoid checking
all the bits".

The existing code checks 4 cases, the case statement checks the same 4
cases (plus the default case).

There are other cause values (not bits), but the new and old code don't
check them all anyway. (Which is OK because the default return value is
EMULATE_FAIL)

AFAICS it generates almost identical code.

So I think the change log should just say something like "all the FSCR
cause values are exclusive so use a case statement which better
expresses that" ?

Also please try to copy the existing subject style for the KVM code, for
this file it would be "KVM: PPC: Book3S HV: ...". I agree it's verbose,
and wouldn't be my choice, but thats what's always been used so let's
stick to it.

cheers

> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 99c7ce825..a72fd2543 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1922,14 +1922,22 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu,
>  
>  		r = EMULATE_FAIL;
>  		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> -			if (cause == FSCR_MSGP_LG)
> +			switch (cause) {
> +			case FSCR_MSGP_LG:
>  				r = kvmppc_emulate_doorbell_instr(vcpu);
> -			if (cause == FSCR_PM_LG)
> +				break;
> +			case FSCR_PM_LG:
>  				r = kvmppc_pmu_unavailable(vcpu);
> -			if (cause == FSCR_EBB_LG)
> +				break;
> +			case FSCR_EBB_LG:
>  				r = kvmppc_ebb_unavailable(vcpu);
> -			if (cause == FSCR_TM_LG)
> +				break;
> +			case FSCR_TM_LG:
>  				r = kvmppc_tm_unavailable(vcpu);
> +				break;
> +			default:
> +				break;
> +			}
>  		}
>  		if (r == EMULATE_FAIL) {
>  			kvmppc_core_queue_program(vcpu, SRR1_PROGILL |
> -- 
> 2.45.2

Re: [PATCH] arch/powerpc/kvm: Avoid extra checks when emulating HFSCR bits

Posted by Gautam Menghani 1 year, 5 months ago

On Tue, Jul 16, 2024 at 11:17:13AM GMT, Michael Ellerman wrote:
> Gautam Menghani <gautam@linux.ibm.com> writes:
> > When a KVM guest tries to use a feature disabled by HFSCR, it exits to
> > the host for emulation support, and the code checks for all bits which
> > are emulated. Avoid checking all the bits by using a switch case.
> 
> The patch looks fine, but I don't know what you mean by "avoid checking
> all the bits".
> 
> The existing code checks 4 cases, the case statement checks the same 4
> cases (plus the default case).
> 
> There are other cause values (not bits), but the new and old code don't
> check them all anyway. (Which is OK because the default return value is
> EMULATE_FAIL)
> 
> AFAICS it generates almost identical code.
> 
> So I think the change log should just say something like "all the FSCR
> cause values are exclusive so use a case statement which better
> expresses that" ?

Yes agreed, will send a v2 with suggested changes.

> 
> Also please try to copy the existing subject style for the KVM code, for
> this file it would be "KVM: PPC: Book3S HV: ...". I agree it's verbose,
> and wouldn't be my choice, but thats what's always been used so let's
> stick to it.
> 

Ack. 

Thanks,
Gautam

> cheers
> 
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index 99c7ce825..a72fd2543 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -1922,14 +1922,22 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu,
> >  
> >  		r = EMULATE_FAIL;
> >  		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> > -			if (cause == FSCR_MSGP_LG)
> > +			switch (cause) {
> > +			case FSCR_MSGP_LG:
> >  				r = kvmppc_emulate_doorbell_instr(vcpu);
> > -			if (cause == FSCR_PM_LG)
> > +				break;
> > +			case FSCR_PM_LG:
> >  				r = kvmppc_pmu_unavailable(vcpu);
> > -			if (cause == FSCR_EBB_LG)
> > +				break;
> > +			case FSCR_EBB_LG:
> >  				r = kvmppc_ebb_unavailable(vcpu);
> > -			if (cause == FSCR_TM_LG)
> > +				break;
> > +			case FSCR_TM_LG:
> >  				r = kvmppc_tm_unavailable(vcpu);
> > +				break;
> > +			default:
> > +				break;
> > +			}
> >  		}
> >  		if (r == EMULATE_FAIL) {
> >  			kvmppc_core_queue_program(vcpu, SRR1_PROGILL |
> > -- 
> > 2.45.2

Re: [PATCH] arch/powerpc/kvm: Avoid extra checks when emulating HFSCR bits

Posted by Gautam Menghani 1 year, 5 months ago

Hello,

Please review this patch and let me know if any changes are needed.

Thanks,
Gautam