| Message ID | 20220421072958.16375-2-chenyi.qiang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce Notify VM exit | expand |
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
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.
> + __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...
> if (vcpu->kvm->arch.exception_payload_enabled)
> events->flags |= KVM_VCPUEVENT_VALID_PAYLOAD;
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;
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; + __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); if (vcpu->kvm->arch.exception_payload_enabled) events->flags |= KVM_VCPUEVENT_VALID_PAYLOAD; @@ -4929,7 +4932,8 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, | KVM_VCPUEVENT_VALID_SIPI_VECTOR | KVM_VCPUEVENT_VALID_SHADOW | KVM_VCPUEVENT_VALID_SMM - | KVM_VCPUEVENT_VALID_PAYLOAD)) + | KVM_VCPUEVENT_VALID_PAYLOAD + | KVM_VCPUEVENT_VALID_TRIPLE_FAULT)) return -EINVAL; if (events->flags & KVM_VCPUEVENT_VALID_PAYLOAD) { @@ -5002,6 +5006,13 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, } } + if (events->flags & KVM_VCPUEVENT_VALID_TRIPLE_FAULT) { + if (events->triple_fault_pending) + kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + else + kvm_clear_request(KVM_REQ_TRIPLE_FAULT, vcpu); + } + kvm_make_request(KVM_REQ_EVENT, vcpu); return 0;
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(-)