On 25/05/14 16:34, Christoffer Dall wrote:
> On Tue, May 20, 2014 at 05:55:40PM +0100, Marc Zyngier wrote:
>> As we're about to trap a bunch of CP14 registers, let's rework
>> the CP15 handling so it can be generalized and work with multiple
>> tables.
>>
>> Reviewed-by: Anup Patel <anup.patel@xxxxxxxxxx>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm64/include/asm/kvm_asm.h | 2 +-
>> arch/arm64/include/asm/kvm_coproc.h | 3 +-
>> arch/arm64/include/asm/kvm_host.h | 9 ++-
>> arch/arm64/kvm/handle_exit.c | 4 +-
>> arch/arm64/kvm/sys_regs.c | 121 +++++++++++++++++++++++++++++-------
>> 5 files changed, 111 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
>> index e6b159a..12f9dd7 100644
>> --- a/arch/arm64/include/asm/kvm_asm.h
>> +++ b/arch/arm64/include/asm/kvm_asm.h
>> @@ -93,7 +93,7 @@
>> #define c10_AMAIR0 (AMAIR_EL1 * 2) /* Aux Memory Attr Indirection Reg */
>> #define c10_AMAIR1 (c10_AMAIR0 + 1)/* Aux Memory Attr Indirection Reg */
>> #define c14_CNTKCTL (CNTKCTL_EL1 * 2) /* Timer Control Register (PL1) */
>> -#define NR_CP15_REGS (NR_SYS_REGS * 2)
>> +#define NR_COPRO_REGS (NR_SYS_REGS * 2)
>>
>> #define ARM_EXCEPTION_IRQ 0
>> #define ARM_EXCEPTION_TRAP 1
>> diff --git a/arch/arm64/include/asm/kvm_coproc.h b/arch/arm64/include/asm/kvm_coproc.h
>> index 9a59301..0b52377 100644
>> --- a/arch/arm64/include/asm/kvm_coproc.h
>> +++ b/arch/arm64/include/asm/kvm_coproc.h
>> @@ -39,7 +39,8 @@ void kvm_register_target_sys_reg_table(unsigned int target,
>> struct kvm_sys_reg_target_table *table);
>>
>> int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 4737961..31cff7a 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -86,7 +86,7 @@ struct kvm_cpu_context {
>> struct kvm_regs gp_regs;
>> union {
>> u64 sys_regs[NR_SYS_REGS];
>> - u32 cp15[NR_CP15_REGS];
>> + u32 copro[NR_COPRO_REGS];
>> };
>> };
>>
>> @@ -141,7 +141,12 @@ struct kvm_vcpu_arch {
>>
>> #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs)
>> #define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)])
>> -#define vcpu_cp15(v,r) ((v)->arch.ctxt.cp15[(r)])
>> +/*
>> + * CP14 and CP15 live in the same array, as they are backed by the
>> + * same system registers.
>> + */
>> +#define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r)])
>> +#define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r)])
>>
>> struct kvm_vm_stat {
>> u32 remote_tlb_flush;
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 7bc41ea..f0ca49f 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -69,9 +69,9 @@ static exit_handle_fn arm_exit_handlers[] = {
>> [ESR_EL2_EC_WFI] = kvm_handle_wfx,
>> [ESR_EL2_EC_CP15_32] = kvm_handle_cp15_32,
>> [ESR_EL2_EC_CP15_64] = kvm_handle_cp15_64,
>> - [ESR_EL2_EC_CP14_MR] = kvm_handle_cp14_access,
>> + [ESR_EL2_EC_CP14_MR] = kvm_handle_cp14_32,
>> [ESR_EL2_EC_CP14_LS] = kvm_handle_cp14_load_store,
>> - [ESR_EL2_EC_CP14_64] = kvm_handle_cp14_access,
>> + [ESR_EL2_EC_CP14_64] = kvm_handle_cp14_64,
>> [ESR_EL2_EC_HVC32] = handle_hvc,
>> [ESR_EL2_EC_SMC32] = handle_smc,
>> [ESR_EL2_EC_HVC64] = handle_hvc,
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index d46a965..429e38c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -474,6 +474,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>> NULL, reset_val, FPEXC32_EL2, 0x70 },
>> };
>>
>> +/* Trapped cp14 registers */
>> +static const struct sys_reg_desc cp14_regs[] = {
>> +};
>> +
>> /*
>> * Trapped cp15 registers. TTBR0/TTBR1 get a double encoding,
>> * depending on the way they are accessed (as a 32bit or a 64bit
>> @@ -581,26 +585,19 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> return 1;
>> }
>>
>> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> -{
>> - kvm_inject_undefined(vcpu);
>> - return 1;
>> -}
>> -
>> -static void emulate_cp15(struct kvm_vcpu *vcpu,
>> - const struct sys_reg_params *params)
>
> document the return value please?
Sure.
>> +static int emulate_cp(struct kvm_vcpu *vcpu,
>> + const struct sys_reg_params *params,
>> + const struct sys_reg_desc *table,
>> + size_t num)
>> {
>> - size_t num;
>> - const struct sys_reg_desc *table, *r;
>> + const struct sys_reg_desc *r;
>>
>> - table = get_target_table(vcpu->arch.target, false, &num);
>> + if (!table)
>> + return -1; /* Not handled */
>>
>> - /* Search target-specific then generic table. */
>> r = find_reg(params, table, num);
>> - if (!r)
>> - r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs));
>>
>> - if (likely(r)) {
>> + if (r) {
>> /*
>> * Not having an accessor means that we have
>> * configured a trap that we don't know how to
>
> This is making me think: Do we really want that BUG_ON? It's certainly
> a KVM bug, but not something that compromises the host kernel state is
> it? We can safely just kill the VM and exit with an internal error
> could we not? Do we care?
Fair enough. Should be pretty easy to fix indeed.
> Anyway, something we can fix independently.
OK.
>> @@ -612,12 +609,37 @@ static void emulate_cp15(struct kvm_vcpu *vcpu,
>> if (likely(r->access(vcpu, params, r))) {
>> /* Skip instruction, since it was emulated */
>> kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
>> - return;
>> }
>> - /* If access function fails, it should complain. */
>> +
>> + /* Handled */
>> + return 0;
>> }
>>
>> - kvm_err("Unsupported guest CP15 access at: %08lx\n", *vcpu_pc(vcpu));
>> + /* Not handled */
>> + return -1;
>> +}
>> +
>> +static void unhandled_cp_access(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *params)
>> +{
>> + u8 hsr_ec = kvm_vcpu_trap_get_class(vcpu);
>> + int cp;
>> +
>> + switch(hsr_ec) {
>> + case ESR_EL2_EC_CP15_32:
>> + case ESR_EL2_EC_CP15_64:
>> + cp = 15;
>> + break;
>> + case ESR_EL2_EC_CP14_MR:
>> + case ESR_EL2_EC_CP14_64:
>> + cp = 14;
>> + break;
>> + default:
>> + WARN_ON((cp = -1));
>> + }
>> +
>> + kvm_err("Unsupported guest CP%d access at: %08lx\n",
>> + cp, *vcpu_pc(vcpu));
>> print_sys_reg_instr(params);
>> kvm_inject_undefined(vcpu);
>> }
>> @@ -627,7 +649,11 @@ static void emulate_cp15(struct kvm_vcpu *vcpu,
>> * @vcpu: The VCPU pointer
>> * @run: The kvm_run struct
>> */
>> -int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>> + const struct sys_reg_desc *global,
>> + size_t nr_global,
>> + const struct sys_reg_desc *target_specific,
>> + size_t nr_specific)
>> {
>> struct sys_reg_params params;
>> u32 hsr = kvm_vcpu_get_hsr(vcpu);
>> @@ -656,8 +682,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> *vcpu_reg(vcpu, params.Rt) = val;
>> }
>>
>> - emulate_cp15(vcpu, ¶ms);
>> + if (!emulate_cp(vcpu, ¶ms, target_specific, nr_specific))
>> + goto out;
>> + if (!emulate_cp(vcpu, ¶ms, global, nr_global))
>> + goto out;
>> +
>> + unhandled_cp_access(vcpu, ¶ms);
>>
>> +out:
>> /* Do the opposite hack for the read side */
>> if (!params.is_write) {
>> u64 val = *vcpu_reg(vcpu, params.Rt);
>> @@ -673,7 +705,11 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> * @vcpu: The VCPU pointer
>> * @run: The kvm_run struct
>> */
>
> I think you forgot to modify the kdcos function name
Indeed.
>> -int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
>> + const struct sys_reg_desc *global,
>> + size_t nr_global,
>> + const struct sys_reg_desc *target_specific,
>> + size_t nr_specific)
>> {
>> struct sys_reg_params params;
>> u32 hsr = kvm_vcpu_get_hsr(vcpu);
>> @@ -688,10 +724,51 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> params.Op1 = (hsr >> 14) & 0x7;
>> params.Op2 = (hsr >> 17) & 0x7;
>>
>> - emulate_cp15(vcpu, ¶ms);
>> + if (!emulate_cp(vcpu, ¶ms, target_specific, nr_specific))
>> + return 1;
>> + if (!emulate_cp(vcpu, ¶ms, global, nr_global))
>> + return 1;
>> +
>> + unhandled_cp_access(vcpu, ¶ms);
>> return 1;
>> }
>>
>> +int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> + const struct sys_reg_desc *target_specific;
>> + size_t num;
>> +
>> + target_specific = get_target_table(vcpu->arch.target, false, &num);
>> + return kvm_handle_cp_64(vcpu,
>> + cp15_regs, ARRAY_SIZE(cp15_regs),
>> + target_specific, num);
>> +}
>> +
>> +int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> + const struct sys_reg_desc *target_specific;
>> + size_t num;
>> +
>> + target_specific = get_target_table(vcpu->arch.target, false, &num);
>> + return kvm_handle_cp_32(vcpu,
>> + cp15_regs, ARRAY_SIZE(cp15_regs),
>> + target_specific, num);
>> +}
>> +
>> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> + return kvm_handle_cp_64(vcpu,
>> + cp14_regs, ARRAY_SIZE(cp14_regs),
>> + NULL, 0);
>> +}
>> +
>> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> + return kvm_handle_cp_32(vcpu,
>> + cp14_regs, ARRAY_SIZE(cp14_regs),
>> + NULL, 0);
>> +}
>> +
>> static int emulate_sys_reg(struct kvm_vcpu *vcpu,
>> const struct sys_reg_params *params)
>> {
>> --
>> 1.8.3.4
>>
>
> Besides the nit:
>
> Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
