Re: [Qemu-arm] [PATCH v5 3/3] target: arm: Add support for VCPU event states

[gengdongjiu]

On 2018/8/17 22:50, Peter Maydell wrote:
> On 21 July 2018 at 18:57, Dongjiu Geng <address@hidden> wrote:
>> This patch extends the qemu-kvm state sync logic with support for
>> KVM_GET/SET_VCPU_EVENTS, giving access to yet missing SError exception.
>> And also it can support the exception state migration.
> 
>> Signed-off-by: Dongjiu Geng <address@hidden>
>> ---
>> change since v4:
>> 1. Rebase the code to latest
>>
>> change since v3:
>> 1. Add a new new subsection with a suitable 'ras_needed' function
>>    controlling whether it is present
>> 2. Add a ARM_FEATURE_RAS feature bit for CPUARMState
>>
>> Signed-off-by: Dongjiu Geng <address@hidden>
> 
> Hi; sorry it's taken me so long to get to this patchset.
> I think it's basically the right shape, but I have some
> changes to suggest below.

You are welcome, thanks for your time that give below  precious suggestion and 
review comments.

> 
>> ---
>>  target/arm/cpu.h     |  6 ++++++
>>  target/arm/kvm64.c   | 59 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  target/arm/machine.c | 22 ++++++++++++++++++++
>>  3 files changed, 87 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index e310ffc..f00f0b6 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -645,6 +645,11 @@ typedef struct CPUARMState {
>>      const struct arm_boot_info *boot_info;
>>      /* Store GICv3CPUState to access from this struct */
>>      void *gicv3state;
>> +    struct {
>> +        uint32_t pending;
>> +        uint32_t has_esr;
>> +        uint64_t esr;
>> +    } serror;
> 
> I recommend putting this earlier in the CPUARMState struct.
> Specifically, you want to put it somewhere before the
> "end_reset_fields" marker. All fields before that are cleared
> to zero on CPU reset. If you put the struct after that then
> you would need manual code in the cpu reset function to
> reset it appropriately.
Ok, I will move it to the right place. thanks for the pointing out.

> 
>>  } CPUARMState;
>>
>>  /**
>> @@ -1486,6 +1491,7 @@ enum arm_features {
>>      ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
>>      ARM_FEATURE_V8_FCMA, /* has complex number part of v8.3 extensions.  */
>>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>> +    ARM_FEATURE_RAS_EXT, /* has RAS Extension */
>>  };
>>
>>  static inline int arm_feature(CPUARMState *env, int feature)
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index e0b8246..ebf7a00 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -527,6 +527,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>          unset_feature(&env->features, ARM_FEATURE_PMU);
>>      }
>>
>> +    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_INJECT_SERROR_ESR)) {
>> +        set_feature(&env->features, ARM_FEATURE_RAS_EXT);
>> +    }
> 
> I think this is confusing two things. Whether the host kernel
> has support for injecting SError ESRs is not the same as
> whether the guest CPU has the RAS extensions. So you don't want
> to use an ARM_FEATURE_* bit to track whether we need to migrate
> the SError ESR state.
> 
> Instead you should just use a bool variable local to this file
> to track whether the kernel has state we need to migrate.
> Compare the way we use "have_guest_debug" to track whether
> KVM_CAP_SET_GUEST_DEBUG is set. You can call this
> have_inject_serror_esr.

Ok, I will use your suggested method to do it in this file.

> 
>> +
>>      /* Do KVM_ARM_VCPU_INIT ioctl */
>>      ret = kvm_arm_vcpu_init(cs);
>>      if (ret) {
>> @@ -600,6 +604,50 @@ int kvm_arm_cpreg_level(uint64_t regidx)
>>  #define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
>>                   KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>>
>> +static int kvm_put_vcpu_events(ARMCPU *cpu)
>> +{
>> +    CPUARMState *env = &cpu->env;
>> +    struct kvm_vcpu_events events = {};
>> +
>> +    if (!kvm_has_vcpu_events()) {
>> +        return 0;
>> +    }
>> +
>> +    memset(&events, 0, sizeof(events));
>> +    events.exception.serror_pending = env->serror.pending;
>> +
>> +    if (arm_feature(env, ARM_FEATURE_RAS_EXT)) {
> 
> Here you can check have_inject_serror_esr.

Got it, will change it.

> 
>> +        events.exception.serror_has_esr = env->serror.has_esr;
>> +        events.exception.serror_esr = env->serror.esr;
>> +    }
>> +
>> +    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
>> +}
>> +
>> +static int kvm_get_vcpu_events(ARMCPU *cpu)
>> +{
>> +    CPUARMState *env = &cpu->env;
>> +    struct kvm_vcpu_events events;
>> +    int ret;
>> +
>> +    if (!kvm_has_vcpu_events()) {
>> +        return 0;
>> +    }
>> +
>> +    memset(&events, 0, sizeof(events));
>> +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_VCPU_EVENTS, &events);
>> +
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    env->serror.pending = events.exception.serror_pending;
>> +    env->serror.has_esr = events.exception.serror_has_esr;
>> +    env->serror.esr = events.exception.serror_esr;
>> +
>> +    return 0;
>> +}
>> +
>>  int kvm_arch_put_registers(CPUState *cs, int level)
>>  {
>>      struct kvm_one_reg reg;
>> @@ -727,6 +775,12 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>          return ret;
>>      }
>>
>> +    ret = kvm_put_vcpu_events(cpu);
>> +    if (ret) {
>> +        printf("return error kvm_put_vcpu_events: %d\n", ret);
> 
> Stray debug printf() left in?
I will remove this printf().

> 
>> +        return ret;
>> +    }
>> +
>>      if (!write_list_to_kvmstate(cpu, level)) {
>>          return EINVAL;
>>      }
>> @@ -863,6 +917,11 @@ int kvm_arch_get_registers(CPUState *cs)
>>      }
>>      vfp_set_fpcr(env, fpr);
>>
>> +    ret = kvm_get_vcpu_events(cpu);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +
>>      if (!write_kvmstate_to_list(cpu)) {
>>          return EINVAL;
>>      }
>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>> index 2e28d08..ead8b2a 100644
>> --- a/target/arm/machine.c
>> +++ b/target/arm/machine.c
>> @@ -172,6 +172,27 @@ static const VMStateDescription vmstate_sve = {
>>  };
>>  #endif /* AARCH64 */
>>
>> +static bool ras_needed(void *opaque)
>> +{
>> +    ARMCPU *cpu = opaque;
>> +    CPUARMState *env = &cpu->env;
>> +
>> +    return arm_feature(env, ARM_FEATURE_RAS_EXT);
> 
> I think the best check to use here is
> "return env.serror.pending != 0". If no SError is
> pending then there's no new state to migrate.

Ok, it looks reasonable to check "return env.serror.pending != 0",
I will change it.

> 
> We should also call this VMState something other than
> "ras", because it doesn't contain RAS-specific data.
> "serror" will do (so "vmstate_serror", "cpu/serror", etc).

OK, I will rename it to "vmstate_serror" from vmstate_ras.

> 
>> +}
>> +
>> +static const VMStateDescription vmstate_ras = {
>> +    .name = "cpu/ras",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = ras_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(env.serror.pending, ARMCPU),
>> +        VMSTATE_UINT32(env.serror.has_esr, ARMCPU),
>> +        VMSTATE_UINT64(env.serror.esr, ARMCPU),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>  static bool m_needed(void *opaque)
>>  {
>>      ARMCPU *cpu = opaque;
>> @@ -723,6 +744,7 @@ const VMStateDescription vmstate_arm_cpu = {
>>  #ifdef TARGET_AARCH64
>>          &vmstate_sve,
>>  #endif
>> +        &vmstate_ras,
>>          NULL
>>      }
>>  };
> 
> thanks
> -- PMM
> 
> .
>