Re: [PATCH v3] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Sean Christopherson <seanjc@google.com>]

+Paolo and Vitaly

In the future, I highly recommend using scripts/get_maintainers.pl.

On Wed, Mar 30, 2022, Peter Gonda wrote:
> SEV-ES guests can request termination using the GHCB's MSR protocol. See
> AMD's GHCB spec section '4.1.13 Termination Request'. Currently when a
> guest does this the userspace VMM sees an KVM_EXIT_UNKNOWN (-EVINAL)
> return code from KVM_RUN. By adding a KVM_EXIT_SHUTDOWN_ENTRY to kvm_run
> struct the userspace VMM can clearly see the guest has requested a SEV-ES
> termination including the termination reason code set and reason code.
> 
> Signed-off-by: Peter Gonda <pgonda@google.com>
> 
> ---
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 75fa6dd268f0..5f9d37dd3f6f 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2735,8 +2735,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>  		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
>  			reason_set, reason_code);

This pr_info() should be removed.  A malicious usersepace could spam the kernel
by constantly running a vCPU that requests termination.

> -		ret = -EINVAL;
> -		break;
> +		vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
> +		vcpu->run->shutdown.reason = KVM_SHUTDOWN_SEV_TERM;
> +		vcpu->run->shutdown.ndata = 2;
> +		vcpu->run->shutdown.data[0] = reason_set;
> +		vcpu->run->shutdown.data[1] = reason_code;

Does KVM really need to split the reason_set and reason_code?  Without reading
the spec, it's not even clear what "set" means.  Assuming it's something like
"the reason code is valid", then I don't see any reason (lol) to split the two.
If we do split them, then arguably the reason_code should be filled if and only
if reason_set is true, and that's just extra work.

Dumping the entire raw "MSR" would simplify this code and the helper to fill the
termination request.

Though I'm not actually sure KVM_EXIT_SHUTDOWN is the best exit reason.  The
exit reason used for Hyper-V's paravirt stuff is arguably more appropriate,
because in this case the guest hasn't actually encountered shutdown, rather it
has requested termination.

		/* KVM_EXIT_SYSTEM_EVENT */
		struct {
#define KVM_SYSTEM_EVENT_SHUTDOWN       1
#define KVM_SYSTEM_EVENT_RESET          2
#define KVM_SYSTEM_EVENT_CRASH          3
			__u32 type;
			__u64 flags;
		} system_event;

Though looking at system_event, isn't that missing padding after type?  Ah, KVM
doesn't actually populate flags, wonderful.  Maybe we can get away with tweaking
it to:

		struct {
			__u32 type;
			__u32 ndata;
			__u64 data[16];
		} system_event;

> +
> +		return 0;
>  	}
>  	default:
>  		/* Error, keep GHCB MSR value as-is */
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 6535adee3e9c..c2cc10776517 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1953,6 +1953,8 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
>  	kvm_vcpu_reset(vcpu, true);
>  
>  	kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
> +	vcpu->run->shutdown.reason = KVM_SHUTDOWN_REQ;
> +	vcpu->run->shutdown.ndata = 0;
>  	return 0;
>  }
>  
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 84a7500cd80c..85b21fc490e4 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4988,6 +4988,8 @@ static __always_inline int handle_external_interrupt(struct kvm_vcpu *vcpu)
>  static int handle_triple_fault(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
> +	vcpu->run->shutdown.reason = KVM_SHUTDOWN_REQ;
> +	vcpu->run->shutdown.ndata = 0;

If we do piggyback KVM_EXIT_SHUTDOWN, a helper to fill the shutdown reason is
warranted, similar to prepare_emulation_failure_exit().  If the raw GHCB MSR is
dumped, the helper can be:

  void kvm_prepare_shutdown_exit(struct kvm_vcpu *vcpu, u64 *data);

where a NULL @data means ndata=0, and a non-NULL @data means ndata=1.

>  	vcpu->mmio_needed = 0;
>  	return 0;
>  }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d3a9ce07a565..f7cd224a4c32 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9999,6 +9999,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  				kvm_x86_ops.nested_ops->triple_fault(vcpu);
>  			} else {
>  				vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
> +				vcpu->run->shutdown.reason = KVM_SHUTDOWN_REQ;
> +				vcpu->run->shutdown.ndata = 0;
>  				vcpu->mmio_needed = 0;
>  				r = 0;
>  				goto out;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 8616af85dc5d..017c03421c48 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -271,6 +271,12 @@ struct kvm_xen_exit {
>  #define KVM_EXIT_XEN              34
>  #define KVM_EXIT_RISCV_SBI        35
>  
> +/* For KVM_EXIT_SHUTDOWN */
> +/* Standard VM shutdown request. No additional metadata provided. */
> +#define KVM_SHUTDOWN_REQ	0

I dislike the "REQ" part.  In a triple fault scenario, the guest isn't requesting
anything.  KVM does "request" shutdown, but that's not a request from the guest's
perspective and is purely a KVM implementation detail.  I'm having trouble coming
up with a decent alternative, but that's probably another indicator that piggybacking
KVM_EXIT_SHUTDOWN may not be appropriate.

> +/* SEV-ES termination request */
> +#define KVM_SHUTDOWN_SEV_TERM	1
> +
>  /* For KVM_EXIT_INTERNAL_ERROR */
>  /* Emulate instruction failed. */
>  #define KVM_INTERNAL_ERROR_EMULATION	1
> @@ -311,6 +317,12 @@ struct kvm_run {
>  		struct {
>  			__u64 hardware_exit_reason;
>  		} hw;
> +		/* KVM_EXIT_SHUTDOWN */
> +		struct {
> +			__u64 reason;
> +			__u32 ndata;

This needs 

			__u32 pad;

to ensure data[16] is aligned regardless of compiler.  Though it's simpler to
just do:

		struct {
			__u32 reason;
			__u32 ndata;
			__u64 data[16];
		}

because 4 billion reasons is probably enough :-)

> +			__u64 data[16];
> +		} shutdown;
>  		/* KVM_EXIT_FAIL_ENTRY */
>  		struct {
>  			__u64 hardware_entry_failure_reason;
> @@ -1145,6 +1157,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_PMU_CAPABILITY 212
>  #define KVM_CAP_DISABLE_QUIRKS2 213
>  #define KVM_CAP_VM_TSC_CONTROL 214
> +#define KVM_CAP_EXIT_SHUTDOWN_REASON 215
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 70e05af5ebea..03b6e472f32c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4299,6 +4299,7 @@ static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
>  	case KVM_CAP_CHECK_EXTENSION_VM:
>  	case KVM_CAP_ENABLE_CAP_VM:
>  	case KVM_CAP_HALT_POLL:
> +	case KVM_CAP_EXIT_SHUTDOWN_REASON:
>  		return 1;
>  #ifdef CONFIG_KVM_MMIO
>  	case KVM_CAP_COALESCED_MMIO:
> -- 
> 2.35.1.1094.g7c7d902a7c-goog
>