Hi Marc, > > On 31/05/18 14:08, 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. > > > > Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx> > > -- > > this series patch is separated from > > https://www.spinics.net/lists/kvm/msg168917.html > > change since V12: > > 1. change (vcpu->arch.hcr_el2 & HCR_VSE) to !!(vcpu->arch.hcr_el2 & > > HCR_VSE) in kvm_arm_vcpu_get_events() > > > > Change since V11: > > Address James's comments, thanks James 1. Align the struct of > > kvm_vcpu_events to 64 bytes 2. Avoid exposing the stale ESR value in > > the kvm_arm_vcpu_get_events() 3. Change variables 'injected' name to > > 'serror_pending' in the kvm_arm_vcpu_set_events() 4. Change to > > sizeof(events) from sizeof(struct kvm_vcpu_events) in > > kvm_arch_vcpu_ioctl() > > > > Change since V10: > > Address James's comments, thanks James 1. Merge the helper function > > with the user. > > 2. Move the ISS_MASK into pend_guest_serror() to clear top bits 3. > > Make kvm_vcpu_events struct align to 4 bytes 4. Add something check in > > the kvm_arm_vcpu_set_events() 5. Check kvm_arm_vcpu_get/set_events()'s > > return value. > > 6. Initialise kvm_vcpu_events to 0 so that padding transferred to user-space doesn't > > contain kernel stack. > > --- > > Documentation/virtual/kvm/api.txt | 31 ++++++++++++++++++++++++++++--- > > arch/arm/include/asm/kvm_host.h | 6 ++++++ > > arch/arm/kvm/guest.c | 12 ++++++++++++ > > arch/arm64/include/asm/kvm_emulate.h | 5 +++++ > > arch/arm64/include/asm/kvm_host.h | 7 +++++++ > > arch/arm64/include/uapi/asm/kvm.h | 13 +++++++++++++ > > arch/arm64/kvm/guest.c | 36 ++++++++++++++++++++++++++++++++++++ > > arch/arm64/kvm/inject_fault.c | 7 ++++++- > > arch/arm64/kvm/reset.c | 1 + > > virt/kvm/arm/arm.c | 21 +++++++++++++++++++++ > > 10 files changed, 135 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/virtual/kvm/api.txt > > b/Documentation/virtual/kvm/api.txt > > index fdac969..8896737 100644 > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -835,11 +835,13 @@ struct kvm_clock_data { > > > > Capability: KVM_CAP_VCPU_EVENTS > > Extended by: KVM_CAP_INTR_SHADOW > > -Architectures: x86 > > +Architectures: x86, arm, arm64 > > Type: vm ioctl > > Parameters: struct kvm_vcpu_event (out) > > Returns: 0 on success, -1 on error > > > > +X86: > > + > > Gets currently pending exceptions, interrupts, and NMIs as well as > > related states of the vcpu. > > > > @@ -881,15 +883,32 @@ Only two fields are defined in the flags field: > > - KVM_VCPUEVENT_VALID_SMM may be set in the flags field to signal that > > smi contains a valid state. > > > > +ARM, ARM64: > > + > > +Gets currently pending SError exceptions as well as related states of the vcpu. > > + > > +struct kvm_vcpu_events { > > + struct { > > + __u8 serror_pending; > > + __u8 serror_has_esr; > > + /* Align it to 8 bytes */ > > + __u8 pad[6]; > > + __u64 serror_esr; > > + } exception; > > + __u32 reserved[12]; > > +}; > > + > > 4.32 KVM_SET_VCPU_EVENTS > > > > -Capability: KVM_CAP_VCPU_EVENTS > > +Capebility: KVM_CAP_VCPU_EVENTS > > Extended by: KVM_CAP_INTR_SHADOW > > -Architectures: x86 > > +Architectures: x86, arm, arm64 > > Type: vm ioctl > > Parameters: struct kvm_vcpu_event (in) > > Returns: 0 on success, -1 on error > > > > +X86: > > + > > Set pending exceptions, interrupts, and NMIs as well as related > > states of the vcpu. > > > > @@ -910,6 +929,12 @@ shall be written into the VCPU. > > > > KVM_VCPUEVENT_VALID_SMM can only be set if KVM_CAP_X86_SMM is available. > > > > +ARM, ARM64: > > + > > +Set pending SError exceptions as well as related states of the vcpu. > > + > > +See KVM_GET_VCPU_EVENTS for the data structure. > > + > > > > 4.33 KVM_GET_DEBUGREGS > > > > diff --git a/arch/arm/include/asm/kvm_host.h > > b/arch/arm/include/asm/kvm_host.h index c7c28c8..39f9901 100644 > > --- a/arch/arm/include/asm/kvm_host.h > > +++ b/arch/arm/include/asm/kvm_host.h > > @@ -213,6 +213,12 @@ 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); > > + > > unsigned long kvm_call_hyp(void *hypfn, ...); void > > force_vm_exit(const cpumask_t *mask); > > > > diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c index > > a18f33e..c685f0e 100644 > > --- a/arch/arm/kvm/guest.c > > +++ b/arch/arm/kvm/guest.c > > @@ -261,6 +261,18 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > > return -EINVAL; > > } > > > > +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu, > > + struct kvm_vcpu_events *events) > > +{ > > + return -EINVAL; > > +} > > + > > +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu, > > + struct kvm_vcpu_events *events) > > +{ > > + return -EINVAL; > > +} > > + > > int __attribute_const__ kvm_target_cpu(void) { > > switch (read_cpuid_part()) { > > diff --git a/arch/arm64/include/asm/kvm_emulate.h > > b/arch/arm64/include/asm/kvm_emulate.h > > index 1dab3a9..18f61ff 100644 > > --- a/arch/arm64/include/asm/kvm_emulate.h > > +++ b/arch/arm64/include/asm/kvm_emulate.h > > @@ -81,6 +81,11 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu) > > return (unsigned long *)&vcpu->arch.hcr_el2; } > > > > +static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu) { > > + return vcpu->arch.vsesr_el2; > > +} > > + > > static inline void vcpu_set_vsesr(struct kvm_vcpu *vcpu, u64 vsesr) > > { > > vcpu->arch.vsesr_el2 = vsesr; > > 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); > > > > #define KVM_ARCH_WANT_MMU_NOTIFIER > > int kvm_unmap_hva(struct kvm *kvm, unsigned long hva); @@ -363,6 > > +368,8 @@ void handle_exit_early(struct kvm_vcpu *vcpu, struct kvm_run > > *run, int kvm_perf_init(void); int kvm_perf_teardown(void); > > > > +void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome); > > + > > struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long > > mpidr); > > > > void __kvm_set_tpidr_el2(u64 tpidr_el2); diff --git > > a/arch/arm64/include/uapi/asm/kvm.h > > b/arch/arm64/include/uapi/asm/kvm.h > > index 04b3256..df4faee 100644 > > --- a/arch/arm64/include/uapi/asm/kvm.h > > +++ b/arch/arm64/include/uapi/asm/kvm.h > > @@ -39,6 +39,7 @@ > > #define __KVM_HAVE_GUEST_DEBUG > > #define __KVM_HAVE_IRQ_LINE > > #define __KVM_HAVE_READONLY_MEM > > +#define __KVM_HAVE_VCPU_EVENTS > > > > #define KVM_COALESCED_MMIO_PAGE_OFFSET 1 > > > > @@ -153,6 +154,18 @@ struct kvm_sync_regs { struct > > kvm_arch_memory_slot { }; > > > > +/* for KVM_GET/SET_VCPU_EVENTS */ > > +struct kvm_vcpu_events { > > + struct { > > + __u8 serror_pending; > > + __u8 serror_has_esr; > > + /* Align it to 8 bytes */ > > + __u8 pad[6]; > > + __u64 serror_esr; > > + } exception; > > + __u32 reserved[12]; > > +}; > > + > > /* If you need to interpret the index values, here is the key: */ > > #define KVM_REG_ARM_COPROC_MASK 0x000000000FFF0000 > > #define KVM_REG_ARM_COPROC_SHIFT 16 > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index > > 56a0260..71d3841 100644 > > --- a/arch/arm64/kvm/guest.c > > +++ b/arch/arm64/kvm/guest.c > > @@ -289,6 +289,42 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > > return -EINVAL; > > } > > > > +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu, > > + struct kvm_vcpu_events *events) > > +{ > > + events->exception.serror_pending = !!(vcpu->arch.hcr_el2 & HCR_VSE); > > + events->exception.serror_has_esr = > > + cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && > > + (!!vcpu_get_vsesr(vcpu)); > > This is odd. Isn't VSESR==0 a valid value? And isn't serror_has_esr always true when ARM64_HAS_RAS_EXTN is set? An all-zero SError ESR now means: 'RAS error: Uncategorized' instead of 'no valid ISS'. yes, I think the VSESR can be 0. Thanks for the pointing out. So it is better to write as shown blow: events->exception.serror_has_esr = cpus_have_const_cap(ARM64_HAS_RAS_EXTN); > > + > > + if (events->exception.serror_pending && > > + events->exception.serror_has_esr) > > + events->exception.serror_esr = vcpu_get_vsesr(vcpu); > > + else > > + events->exception.serror_esr = 0; > > + > > + return 0; > > +} > > + > > +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu, > > + struct kvm_vcpu_events *events) > > +{ > > + bool serror_pending = events->exception.serror_pending; > > + bool has_esr = events->exception.serror_has_esr; > > + > > + 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); > > + > > Spurious blank line I will remove this blank line > > > + } else if (serror_pending) { > > + kvm_inject_vabt(vcpu); > > + } > > + > > + return 0; > > +} > > + > > int __attribute_const__ kvm_target_cpu(void) { > > unsigned long implementor = read_cpuid_implementor(); diff --git > > a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c index > > d8e7165..9e0ca56 100644 > > --- a/arch/arm64/kvm/inject_fault.c > > +++ b/arch/arm64/kvm/inject_fault.c > > @@ -166,7 +166,7 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu) > > > > static void pend_guest_serror(struct kvm_vcpu *vcpu, u64 esr) { > > - vcpu_set_vsesr(vcpu, esr); > > + vcpu_set_vsesr(vcpu, esr & ESR_ELx_ISS_MASK); > > *vcpu_hcr(vcpu) |= HCR_VSE; > > } > > > > @@ -186,3 +186,8 @@ void kvm_inject_vabt(struct kvm_vcpu *vcpu) { > > pend_guest_serror(vcpu, ESR_ELx_ISV); } > > + > > +void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome) { > > + pend_guest_serror(vcpu, syndrome); > > +} > > I think it'd make more sense to rename pend_guest_serror to kvm_set_sei_esr and be done with it. Ok, got it. > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c index > > 38c8a64..20e919a 100644 > > --- a/arch/arm64/kvm/reset.c > > +++ b/arch/arm64/kvm/reset.c > > @@ -82,6 +82,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext) > > break; > > case KVM_CAP_SET_GUEST_DEBUG: > > case KVM_CAP_VCPU_ATTRIBUTES: > > + case KVM_CAP_VCPU_EVENTS: > > r = 1; > > break; > > default: > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index > > a4c1b76..8b43968 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; > > } > > + case KVM_GET_VCPU_EVENTS: { > > + struct kvm_vcpu_events events; > > + > > + memset(&events, 0, sizeof(events)); > > You could write this as > > struct kvm_cpu_events events = { }; > > but it'd make more sense if kvm_arm_vcpu_get_events() did all the work rather than having this split responsibility. Ok, thanks for the good suggestion. > > > + 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(struct kvm_vcpu_events))) > > Prefer using sizeof(events) instead. Yes, it should, It is my careless. Thanks for the pointing out. > > > + return -EFAULT; > > + > > + return kvm_arm_vcpu_set_events(vcpu, &events); > > + } > > default: > > r = -EINVAL; > > } > > > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
