Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 19 May 2022 14:25:04 +0800 | From | Chenyi Qiang <> | Subject | Re: [PATCH v6 1/3] KVM: X86: Save&restore the triple fault request |
| |
Thanks Sean for your review!
On 5/19/2022 2:42 AM, Sean Christopherson wrote: > Nits on the shortlog... > > Please don't capitalize x86, spell out "and" instead of using an ampersand (though > I think it can be omitted entirely), and since there are plenty of chars left, call > out that this is adding/extending KVM's ABI, e.g. it's not obvious from the shortlog > where/when the save+restore happens. > > KVM: x86: Extend KVM_{G,S}ET_VCPU_EVENTS to support pending triple fault >
Will fix it.
> On Thu, Apr 21, 2022, Chenyi Qiang wrote: >> For the triple fault sythesized by KVM, e.g. the RSM path or >> nested_vmx_abort(), if KVM exits to userspace before the request is >> serviced, userspace could migrate the VM and lose the triple fault. >> >> Add the support to save and restore the triple fault event from >> userspace. Introduce a new event KVM_VCPUEVENT_VALID_TRIPLE_FAULT in >> get/set_vcpu_events to track the triple fault request. >> >> Note that in the set_vcpu_events path, userspace is able to set/clear >> the triple fault request through triple_fault_pending field. >> >> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> >> --- >> Documentation/virt/kvm/api.rst | 7 +++++++ >> arch/x86/include/uapi/asm/kvm.h | 4 +++- >> arch/x86/kvm/x86.c | 15 +++++++++++++-- >> 3 files changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst >> index 72183ae628f7..e09ce3cb49c5 100644 >> --- a/Documentation/virt/kvm/api.rst >> +++ b/Documentation/virt/kvm/api.rst >> @@ -1150,6 +1150,9 @@ The following bits are defined in the flags field: >> fields contain a valid state. This bit will be set whenever >> KVM_CAP_EXCEPTION_PAYLOAD is enabled. >> >> +- KVM_VCPUEVENT_VALID_TRIPLE_FAULT may be set to signal that the >> + triple_fault_pending field contains a valid state. >> + >> ARM64: >> ^^^^^^ >> >> @@ -1245,6 +1248,10 @@ can be set in the flags field to signal that the >> exception_has_payload, exception_payload, and exception.pending fields >> contain a valid state and shall be written into the VCPU. >> >> +KVM_VCPUEVENT_VALID_TRIPLE_FAULT can be set in flags field to signal that >> +the triple_fault_pending field contains a valid state and shall be written >> +into the VCPU. >> + >> ARM64: >> ^^^^^^ >> >> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h >> index 21614807a2cb..fd083f6337af 100644 >> --- a/arch/x86/include/uapi/asm/kvm.h >> +++ b/arch/x86/include/uapi/asm/kvm.h >> @@ -325,6 +325,7 @@ struct kvm_reinject_control { >> #define KVM_VCPUEVENT_VALID_SHADOW 0x00000004 >> #define KVM_VCPUEVENT_VALID_SMM 0x00000008 >> #define KVM_VCPUEVENT_VALID_PAYLOAD 0x00000010 >> +#define KVM_VCPUEVENT_VALID_TRIPLE_FAULT 0x00000020 >> >> /* Interrupt shadow states */ >> #define KVM_X86_SHADOW_INT_MOV_SS 0x01 >> @@ -359,7 +360,8 @@ struct kvm_vcpu_events { >> __u8 smm_inside_nmi; >> __u8 latched_init; >> } smi; >> - __u8 reserved[27]; >> + __u8 triple_fault_pending; > > What about writing this as > > struct { > __u8 pending; > } triple_fault; > > to match the other events? It's kinda silly, but I find it easier to visually > identify the various events that are handled by kvm_vcpu_events. >
Sure, will change in this format.
>> + __u8 reserved[26]; >> __u8 exception_has_payload; >> __u64 exception_payload; >> }; >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index ab336f7c82e4..c8b9b0bc42aa 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -4911,9 +4911,12 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu, >> !!(vcpu->arch.hflags & HF_SMM_INSIDE_NMI_MASK); >> events->smi.latched_init = kvm_lapic_latched_init(vcpu); >> >> + events->triple_fault_pending = kvm_test_request(KVM_REQ_TRIPLE_FAULT, vcpu); >> + >> events->flags = (KVM_VCPUEVENT_VALID_NMI_PENDING >> | KVM_VCPUEVENT_VALID_SHADOW >> - | KVM_VCPUEVENT_VALID_SMM); >> + | KVM_VCPUEVENT_VALID_SMM >> + | KVM_VCPUEVENT_VALID_TRIPLE_FAULT); > > Does setting KVM_VCPUEVENT_VALID_TRIPLE_FAULT need to be guarded with a capability, > a la KVM_CAP_EXCEPTION_PAYLOAD, so that migrating from a new KVM to an old KVM doesn't > fail? Seems rather pointless since the VM is likely hosed either way... >
Indeed, at least adding a capability makes it more compatible. Will add it in next version.
>> if (vcpu->kvm->arch.exception_payload_enabled) >> events->flags |= KVM_VCPUEVENT_VALID_PAYLOAD;
| ![\](/images/icornerr.gif) |