On 2019/9/27 21:33, Peter Maydell wrote:
> 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.
Thanks, I will merge this patch with the next patch.
>
>> +{
>> + 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:
Yes, I will remove this comment.
>
>> + 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.
I will remove it too.
>
>> + 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.
OK, I will remove this stray '*'.
>
>
>> + * 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.
OK, I will add the comment at the top of the function.
>
>> + */
>> + 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
>
> .
>
--
Thanks,
Xiang
