On 2018/8/24 0:52, Peter Maydell wrote:
> On 23 August 2018 at 16:45, Dongjiu Geng <gengdong...@huawei.com> 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 <gengdong...@huawei.com>
>
> Did you forget to send a cover letter for this patchset?
> I didn't see one. Our tooling prefers cover letters for
> any patchset with more than one patch in it.
Ok, I will add a cover letter.
>
>> ---
>> Change since v5:
>> address Peter's comments:
>> 1. Move the "struct serror" before the "end_reset_fields" in CPUARMState
>> 2. Remove ARM_FEATURE_RAS_EXT and add a variable have_inject_serror_esr
>> 3. Use the variable have_inject_serror_esr to track whether the kernel has
>> state we need to migrate
>> 4. Remove printf() in kvm_arch_put_registers()
>> 5. ras_needed/vmstate_ras to serror_needed/vmstate_serror
>> 6. Check to use "return env.serror.pending != 0" instead of
>> "arm_feature(env, ARM_FEATURE_RAS_EXT)" in the ras_needed()
>>
>> 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
>> ---
>> target/arm/cpu.h | 7 +++++++
>> target/arm/kvm64.c | 59
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> target/arm/machine.c | 22 ++++++++++++++++++++
>> 3 files changed, 88 insertions(+)
>
>
>> +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 (have_inject_serror_esr) {
>> + events.exception.serror_has_esr = env->serror.has_esr;
>> + events.exception.serror_esr = env->serror.esr;
>> + }
>
> I realised that the effect of this condition is that
> if we migrate a VM from a machine which supports specifying the
> SError ESR to one which does not, and at the point of migration
> there is a pending SError with an ESR value, then we will
> silently drop the specified ESR value. The other alternative
> would be to fail the migration (by dropping the if() check,
> and letting the host kernel fail the ioctl if that meant that
> we asked it to set an SError ESR it couldn't manage.)
>
> I guess that's OK? It's all hypothetical currently since
> we don't support migration between different host CPU types.
Peter,
there are two status needed to migrate, one is serror_pending, another is
SError ESR value.
If A migrates to B, A can set an SError ESR, but B does not support to set.
when A is pending a SError and need to migrate to B, I think it should support
to migrate the serror_pending status without the ESR value(the ESR value is 0).
That is to say, if A is pending a SError, when migrate to B, B should also
pend a SError.
or do you think we should refused this migration?
currently kernel can support to pend a SError without the ESR value.
As shown the kernel code in [1].
when has_esr is true the ioctl can return failure, then Qemu can check the
return value to decide whether do this migration.
but when the has_esr is false, without setting the ESR, QEMU can not check the
return value because it always return true.
[1]:
+int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events)
+{
+ int i;
+ bool serror_pending = events->exception.serror_pending;
+ bool has_esr = events->exception.serror_has_esr;
+
+ /* check whether the reserved field is zero */
+ for (i = 0; i < ARRAY_SIZE(events->reserved); i++)
+ if (events->reserved[i])
+ return -EINVAL;
+
+ /* check whether the pad field is zero */
+ for (i = 0; i < ARRAY_SIZE(events->exception.pad); i++)
+ if (events->exception.pad[i])
+ return -EINVAL;
+
+ if (serror_pending && has_esr) {
+ if (!cpus_have_const_cap(ARM64_HAS_RAS_EXTN))
+ return -EINVAL;
+
+ if (!((events->exception.serror_esr) & ~ESR_ELx_ISS_MASK))
+ kvm_set_sei_esr(vcpu, events->exception.serror_esr);
+ else
+ return -EINVAL;
+ } else if (serror_pending) {
+ kvm_inject_vabt(vcpu);
+ }
+
+ return 0;
+}
>
>> +
>> + return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
>> +}
>> +
>
>> +static const VMStateDescription vmstate_serror = {
>> + .name = "cpu/ras",
>
> You forgot to update the name here.
Thanks for this pointing out, will change it
>
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = serror_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;
>> @@ -726,6 +747,7 @@ const VMStateDescription vmstate_arm_cpu = {
>> #ifdef TARGET_AARCH64
>> &vmstate_sve,
>> #endif
>> + &vmstate_serror,
>> NULL
>> }
>> };
>> --
>
> thanks
> -- PMM
>
> .
>
