On Thu, 21 Apr 2022 19:04:40 +0100,
Paolo Bonzini <pbonzini@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
