On Fri, 6 Sep 2019 at 09:33, Xiang Zheng <zhengxiang9@xxxxxxxxxx> wrote:
>
> From: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
>
> Introduce kvm_inject_arm_sea() function in which we will setup the type
> of exception and the syndrome information in order to inject a virtual
> synchronous external abort. When switching to guest, it will jump to the
> synchronous external abort vector table entry.
>
> The ESR_ELx.DFSC is set to synchronous external abort(0x10), and
> ESR_ELx.FnV is set to not valid(0x1), which will tell guest that FAR is
> not valid and hold an UNKNOWN value. These values will be set to KVM
> register structures through KVM_SET_ONE_REG IOCTL.
>
> Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
> Signed-off-by: Xiang Zheng <zhengxiang9@xxxxxxxxxx>
> +/* Inject synchronous external abort */
> +static void kvm_inject_arm_sea(CPUState *c)
This will cause a compilation failure at this point in
the patch series, because the compiler will complain about
a static function which is defined but never used.
To avoid breaking bisection, we need to put the definition
of the function in the same patch where it's used.
> +{
> + ARMCPU *cpu = ARM_CPU(c);
> + CPUARMState *env = &cpu->env;
> + CPUClass *cc = CPU_GET_CLASS(c);
> + uint32_t esr;
> + bool same_el;
> +
> + /**
> + * Set the exception type to synchronous data abort
> + * and the target exception Level to EL1.
> + */
This comment doesn't really tell us anything that's not obvious
from the two lines of code that it's commenting on:
> + c->exception_index = EXCP_DATA_ABORT;
> + env->exception.target_el = 1;
> +
> + /*
> + * Set the DFSC to synchronous external abort and set FnV to not valid,
> + * this will tell guest the FAR_ELx is UNKNOWN for this abort.
> + */
> +
> + /* This exception comes from lower or current exception level. */
This comment too is stating the obvious I think.
> + same_el = arm_current_el(env) == env->exception.target_el;
> + esr = syn_data_abort_no_iss(same_el, 1, 0, 0, 0, 0, 0x10);
> +
> + env->exception.syndrome = esr;
> +
> + /**
There's a stray second '*' in this comment-start.
> + * The vcpu thread already hold BQL, so no need hold again when
> + * calling do_interrupt
I think this requirement would be better placed as a
comment at the top of the function noting that callers
must hold the iothread lock.
> + */
> + cc->do_interrupt(c);
> +}
> +
> #define AARCH64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 5feb312941..499672ebbc 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -33,7 +33,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
> * ISV field.
> */
> if (!(template_syn & ARM_EL_ISV) || target_el != 2 || s1ptw) {
> - syn = syn_data_abort_no_iss(same_el,
> + syn = syn_data_abort_no_iss(same_el, 0,
> ea, 0, s1ptw, is_write, fsc);
> } else {
> /*
> --
> 2.19.1
thanks
-- PMM
