Re: [PATCH v6 1/3] KVM: X86: Save&restore the triple fault request

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
---
  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;



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux