Hi James,
On 2018/6/29 23:59, James Morse wrote:
> Hi Dongjiu Geng,
>
> On 25/06/18 21:58, Dongjiu Geng wrote:
>> For the migrating VMs, user space may need to know the exception
>> state. For example, in the machine A, KVM make an SError pending,
>> when migrate to B, KVM also needs to pend an SError.
>>
>> This new IOCTL exports user-invisible states related to SError.
>> Together with appropriate user space changes, user space can get/set
>> the SError exception state to do migrate/snapshot/suspend.
>
>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 469de8a..357304a 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -335,6 +335,11 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
>> int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
>> int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
>> int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
>> +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
>> + struct kvm_vcpu_events *events);
>> +
>> +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
>> + struct kvm_vcpu_events *events);
>>
>
> (Nit: funny indentation)
The indentation is in order to not over 80 characters, I will change it as below to make it more reasonable
+int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events);
+
+int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events);
>
>
>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
>> index 56a0260..8be14cc 100644
>> --- a/arch/arm64/kvm/guest.c
>> +++ b/arch/arm64/kvm/guest.c
>> @@ -289,6 +289,49 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>
>> +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;
>> +
>
>> + kvm_set_sei_esr(vcpu, events->exception.serror_esr);
>
> This silently discards all but the bottom 24 bits of serror_esr.
>
> It makes sense that this field is 64 bit, because the register is 64 bit, and it
> would let us use this API to migrate any new state that appears in the higher
> bits... But those bits will come with an ID/feature field, we shouldn't accept
> an attempt to restore them on a CPU that doesn't support the feature. If that
> happens here, it silently succeeds, but the kernel just threw the extra bits away.
>
> You added documentation that only the bottom 24bits can be set, can we add
> checks to enforce this, so the bits can be used later.
yes, sure! I will check it. thanks for the suggestion.
>
>
>> + } else if (serror_pending) {
>> + kvm_inject_vabt(vcpu);
>> + }
>> +
>> + return 0;
>> +}
>
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index a4c1b76..4e6f366 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -1107,6 +1107,27 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>> r = kvm_arm_vcpu_has_attr(vcpu, &attr);
>> break;
>> }
>> +#ifdef __KVM_HAVE_VCPU_EVENTS
>
> So its this #ifdef, or a uapi struct for a feature 32bit doesn't support.
> I think the right thing to do is wire this up for 32bit, it also calls
> kvm_inject_vabt() in handle_exit.c, so must have the same migration problems.
>
> I'll post a patch to do this as I've got something I can test it on.
Ok, so here I will temporarily use "#ifdef __KVM_HAVE_VCPU_EVENTS" to avoid the 32bit platform build errors,
when you post a new patch, you can remove the "#ifdef". thanks.
>
>
>> + case KVM_GET_VCPU_EVENTS: {
>> + struct kvm_vcpu_events events;
>> +
>> + if (kvm_arm_vcpu_get_events(vcpu, &events))
>> + return -EINVAL;
>> +
>> + if (copy_to_user(argp, &events, sizeof(events)))
>> + return -EFAULT;
>> +
>> + return 0;
>> + }
>> + case KVM_SET_VCPU_EVENTS: {
>> + struct kvm_vcpu_events events;
>> +
>> + if (copy_from_user(&events, argp, sizeof(events)))
>> + return -EFAULT;
>> +
>> + return kvm_arm_vcpu_set_events(vcpu, &events);
>> + }
>> +#endif
>
> (It bugs me that the architecture has some rules about merging multiple
> architected ESR values, that we neither enforce, nor document as user-space's
> problem. It doesn't matter for RAS, but might for any future ESR encodings. But
> I guess user-space wouldn't be aware of them anyway, and it can already put
> bogus values in SPSR/ESR/ELR etc.)
>
>
> With a check against the top bits of ESR:
> Reviewed-by: James Morse <james.morse@xxxxxxx>
Thanks for this "Reviewed-by", I will check against the top bits of ESR
>
>
> Thanks,
>
> James
>
> .
>
