Re: [PATCH 1/4] KVM: x86: always initialize system_event.ndata

[Marc Zyngier <maz@kernel.org>]

On Thu, 21 Apr 2022 19:04:40 +0100,
Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> The KVM_SYSTEM_EVENT_NDATA_VALID mechanism that was introduced
> contextually with KVM_SYSTEM_EVENT_SEV_TERM is not a good match
> for ARM and RISC-V, which want to communicate information even
> for existing KVM_SYSTEM_EVENT_* constants.  Userspace is not ready
> to filter out bit 31 of type, and fails to process the
> KVM_EXIT_SYSTEM_EVENT exit.
> 
> Therefore, tie the availability of ndata to a system capability
> (which will be added once all architectures are on board).
> Userspace written for released versions of Linux has no reason to
> check flags, since it was never written, so it is okay to replace
> it with ndata and data[0] (on 32-bit kernels) or with data[0]
> (on 64-bit kernels).

How is it going to work for new userspace on old kernels, for which
the ndata field is left uninitialised?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/sev.c   | 3 +--
>  arch/x86/kvm/x86.c       | 2 ++
>  include/uapi/linux/kvm.h | 1 -
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index a93f0d01bb90..51b963ec122b 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2739,8 +2739,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>  			reason_set, reason_code);
>  
>  		vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
> -		vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SEV_TERM |
> -					       KVM_SYSTEM_EVENT_NDATA_VALID;
> +		vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SEV_TERM;
>  		vcpu->run->system_event.ndata = 1;
>  		vcpu->run->system_event.data[1] = control->ghcb_gpa;

Isn't this really odd? ndata = 1, and yet you populate data[1]?

>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4e7f3a8da16a..517c0228881c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10056,12 +10056,14 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		if (kvm_check_request(KVM_REQ_HV_CRASH, vcpu)) {
>  			vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>  			vcpu->run->system_event.type = KVM_SYSTEM_EVENT_CRASH;
> +			vcpu->run->system_event.ndata = 0;
>  			r = 0;
>  			goto out;
>  		}
>  		if (kvm_check_request(KVM_REQ_HV_RESET, vcpu)) {
>  			vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
>  			vcpu->run->system_event.type = KVM_SYSTEM_EVENT_RESET;
> +			vcpu->run->system_event.ndata = 0;
>  			r = 0;
>  			goto out;
>  		}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index dd1d8167e71f..5a57f74b4903 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -445,7 +445,6 @@ struct kvm_run {
>  #define KVM_SYSTEM_EVENT_RESET          2
>  #define KVM_SYSTEM_EVENT_CRASH          3
>  #define KVM_SYSTEM_EVENT_SEV_TERM       4
> -#define KVM_SYSTEM_EVENT_NDATA_VALID    (1u << 31)
>  			__u32 type;
>  			__u32 ndata;
>  			__u64 data[16];

Cat we please get a #define that aliases data[0] to flags? At the next
merge of the KVM headers into their respective trees, all the existing
VMM are going to break if they have a reference to this field (CrosVM
definitely does today -- yes, we're ahead of time).

Also, getting a bisectable series would be good.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.