Re: [PATCH v18 5/6] target-arm: kvm64: inject synchronous External Abort

[Xiang Zheng]

On 2019/9/27 21:33, Peter Maydell wrote:
> On Fri, 6 Sep 2019 at 09:33, Xiang Zheng <address@hidden> wrote:
>>
>> From: Dongjiu Geng <address@hidden>
>>
>> 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 <address@hidden>
>> Signed-off-by: Xiang Zheng <address@hidden>
> 
>> +/* 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