Re: [Qemu-devel] [PATCH v9 2/2] target: arm: Add support for VCPU event states

[gengdongjiu]

Hi Andrew,
   Thanks for the review and comments.

[...]
> > @@ -600,6 +605,60 @@ 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 = {};
> > +    int ret;
> > +
> > +    if (!kvm_has_vcpu_events()) {
> > +        return 0;
> > +    }
> > +
> > +    memset(&events, 0, sizeof(events));
> 
> nit: this memset is redundant with the '= {}' above. I'd probably keep it and 
> remove the '= {}' though.
  
Will remove the '= {}', thanks

> 
> > +    events.exception.serror_pending = env->serror.pending;
> > +
> > +    /* Inject SError to guest with specified syndrome if host kernel
> > +     * supports it, otherwise inject SError without syndrome.
> > +     */
> > +    if (have_inject_serror_esr) {
> > +        events.exception.serror_has_esr = env->serror.has_esr;
> > +        events.exception.serror_esr = env->serror.esr;
> > +    }
> > +
> > +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
> > +    if (ret) {
> > +        error_report("failed to put vcpu events");
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +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);
> > +
> 
> nit: no need for this blank line

Will remove this blank line

> 
> > +    if (ret) {
> > +        error_report("failed to get vcpu events");
> > +        return ret;
> > +    }
[...]
> > +
> >  static bool m_needed(void *opaque)
> >  {
> >      ARMCPU *cpu = opaque;
> > @@ -726,6 +747,7 @@ const VMStateDescription vmstate_arm_cpu = {
> > #ifdef TARGET_AARCH64
> >          &vmstate_sve,
> >  #endif
> > +        &vmstate_serror,
> >          NULL
> >      }
> >  };
> > --
> > 1.8.3.1
> >
> >
> 
> Hmm. kvm_vcpu_events is defined for both 32-bit and 64-bit and the vmstate 
> isn't in the '#ifdef TARGET_AARCH64', like vmstate_sve, yet
> only the target/arm/kvm64.c file is getting updated. Shouldn't we put 
> kvm_get/put_vcpu_events() in target/arm/kvm.c and then also
> update target/arm/kvm32.c?

I will also update the target/arm/kvm32.c, thanks a lot for the suggestion.

> 
> Thanks,
> drew