On 2015/12/22 17:55, Marc Zyngier wrote:
> Assuming we trap a system register, and decide that the access is
> illegal, we will inject an exception in the guest. In this
> case, we shouldn't increment the PC, or the vcpu will miss the
> first instruction of the handler, leading to a mildly confused
> guest.
>
> Solve this by snapshoting PC before the access is performed,
> and checking if it has moved or not before incrementing it.
>
Thanks a lot. This solves the problem of guest PMU failing to inject EL1
fault to guest.
Tested-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
Reviewed-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> Reported-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------
> 1 file changed, 36 insertions(+), 37 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d2650e8..9c87e0c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -966,6 +966,39 @@ static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params,
> return NULL;
> }
>
> +/* Perform the sysreg access, returns 0 on success */
> +static int access_sys_reg(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *params,
> + const struct sys_reg_desc *r)
> +{
> + u64 pc = *vcpu_pc(vcpu);
> +
> + if (unlikely(!r))
> + return -1;
> +
> + /*
> + * Not having an accessor means that we have configured a trap
> + * that we don't know how to handle. This certainly qualifies
> + * as a gross bug that should be fixed right away.
> + */
> + BUG_ON(!r->access);
> +
> + if (likely(r->access(vcpu, params, r))) {
> + /*
> + * Skip the instruction if it was emulated without PC
> + * having changed. This allows us to detect a fault
> + * being injected (incrementing the PC here would
> + * cause the vcpu to skip the first instruction of its
> + * fault handler).
> + */
> + if (pc == *vcpu_pc(vcpu))
> + kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> + return 0;
> + }
> +
> + return -1;
> +}
> +
> int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> kvm_inject_undefined(vcpu);
> @@ -994,26 +1027,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu,
>
> r = find_reg(params, table, num);
>
> - if (r) {
> - /*
> - * Not having an accessor means that we have
> - * configured a trap that we don't know how to
> - * handle. This certainly qualifies as a gross bug
> - * that should be fixed right away.
> - */
> - BUG_ON(!r->access);
> -
> - if (likely(r->access(vcpu, params, r))) {
> - /* Skip instruction, since it was emulated */
> - kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> - }
> -
> - /* Handled */
> - return 0;
> - }
> -
> - /* Not handled */
> - return -1;
> + return access_sys_reg(vcpu, params, r);
> }
>
> static void unhandled_cp_access(struct kvm_vcpu *vcpu,
> @@ -1178,27 +1192,12 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
> if (!r)
> r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
>
> - if (likely(r)) {
> - /*
> - * Not having an accessor means that we have
> - * configured a trap that we don't know how to
> - * handle. This certainly qualifies as a gross bug
> - * that should be fixed right away.
> - */
> - BUG_ON(!r->access);
> -
> - if (likely(r->access(vcpu, params, r))) {
> - /* Skip instruction, since it was emulated */
> - kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> - return 1;
> - }
> - /* If access function fails, it should complain. */
> - } else {
> + if (access_sys_reg(vcpu, params, r)) {
> kvm_err("Unsupported guest sys_reg access at: %lx\n",
> *vcpu_pc(vcpu));
> print_sys_reg_instr(params);
> + kvm_inject_undefined(vcpu);
> }
> - kvm_inject_undefined(vcpu);
> return 1;
> }
>
>
--
Shannon
--
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
