Hi Marc,
On Wed, Apr 06, 2022 at 04:07:28PM +0100, Marc Zyngier wrote:
> > + /*
> > + * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> > + * Avoid conflicting with future expansion of AArch64 feature registers
> > + * and simply treat them as RAZ here.
> > + */
> > + if (params->CRm > 3)
> > + params->regval = 0;
> > + else
> > + ret = emulate_sys_reg(vcpu, params);
> > +
> > + vcpu_set_reg(vcpu, Rt, params->regval);
>
> It feels odd to update Rt without checking whether the read has
> succeeded. In your case, this is harmless, but would break with the
> approach I'm outlining below.
>
A total kludge to avoid yet another level of indentation :) I'll go
about this the right way next spin.
> > + return ret;
> > +}
> > +
> > +/**
> > + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> > + * AArch32 ID register.
> > + * @params: the system register access parameters
> > + *
> > + * Note that CP15 ID registers where CRm=0 are excluded from this check. The
> > + * only register trapped in the CRm=0 range is CTR, which is already handled in
> > + * the cp15 register table.
>
> There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
> which prevents it from fitting in your scheme.
>
> > + */
> > +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> > +{
> > + return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> > +}
> > +
> > /**
> > * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
> > * @vcpu: The VCPU pointer
> > @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> > params.Op1 = (esr >> 14) & 0x7;
> > params.Op2 = (esr >> 17) & 0x7;
> >
> > + /*
> > + * Certain AArch32 ID registers are handled by rerouting to the AArch64
> > + * system register table.
> > + */
> > + if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(¶ms))
> > + return kvm_emulate_cp15_id_reg(vcpu, ¶ms);
>
> I think this is a bit ugly. We reach this point from a function that
> was cp15-specific, and now we are reconstructing the context. I'd
> rather this is moved to kvm_handle_cp15_32(), and treated there
> (untested):
>
Completely agree, hoisting this would be much more elegant.
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7b45c040cc27..a071d89ace92 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
> * @run: The kvm_run struct
> */
> static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *params,
> const struct sys_reg_desc *global,
> size_t nr_global)
> {
> - struct sys_reg_params params;
> - u32 esr = kvm_vcpu_get_esr(vcpu);
> int Rt = kvm_vcpu_sys_get_rt(vcpu);
>
> - params.CRm = (esr >> 1) & 0xf;
> - params.regval = vcpu_get_reg(vcpu, Rt);
> - params.is_write = ((esr & 1) == 0);
> - params.CRn = (esr >> 10) & 0xf;
> - params.Op0 = 0;
> - params.Op1 = (esr >> 14) & 0x7;
> - params.Op2 = (esr >> 17) & 0x7;
> + params->regval = vcpu_get_reg(vcpu, Rt);
>
> - if (!emulate_cp(vcpu, ¶ms, global, nr_global)) {
> - if (!params.is_write)
> - vcpu_set_reg(vcpu, Rt, params.regval);
> + if (!emulate_cp(vcpu, params, global, nr_global)) {
> + if (!params->is_write)
> + vcpu_set_reg(vcpu, Rt, params->regval);
> return 1;
> }
>
> - unhandled_cp_access(vcpu, ¶ms);
> + unhandled_cp_access(vcpu, params);
> return 1;
> }
>
> @@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
>
> int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
> {
> - return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
> + struct sys_reg_params params;
> +
> + params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> + if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
> + return kvm_emulate_cp15_id_reg(vcpu, ¶ms);
> +
> + return kvm_handle_cp_32(vcpu, ¶ms, cp15_regs, ARRAY_SIZE(cp15_regs));
> }
>
> int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
> @@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
>
> int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
> {
> - return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
> + struct sys_reg_params params;
> +
> + params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> + return kvm_handle_cp_32(vcpu, ¶ms, cp14_regs, ARRAY_SIZE(cp14_regs));
> }
>
> static bool is_imp_def_sys_reg(struct sys_reg_params *params)
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index cc0cc95a0280..fd4b2bb8c782 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -35,6 +35,13 @@ struct sys_reg_params {
> .Op2 = ((esr) >> 17) & 0x7, \
> .is_write = !((esr) & 1) })
>
> +#define esr_cp1x_32_to_params(esr) \
> + ((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7, \
> + .CRn = ((esr) >> 10) & 0xf, \
> + .CRm = ((esr) >> 1) & 0xf, \
> + .Op2 = ((esr) >> 17) & 0x7, \
> + .is_write = !((esr) & 1) })
> +
> struct sys_reg_desc {
> /* Sysreg string for debug */
> const char *name;
>
>
> What do you think?
Way better. Your suggested patch looks correct, I'll fold all of this
together and test it out. Thanks for the suggestions :)
--
Best,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
