On Tue, Jul 16, 2024 at 11:17:13AM GMT, Michael Ellerman wrote:
> Gautam Menghani <gautam@xxxxxxxxxxxxx> 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
