[PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Juergen Gross]

Get rid of the literal "1" used as general error return value in KVM
MSR emulation. It can easily be replaced by negative errno values
instead.

This is meant to avoid confusion with the literal "1" used as return
value for "return to guest".

Changes in V2:
- series carved out from initial "KVM: Avoid literal numbers as return
  values" series
- don't use new KVM_MSR_RET_* defines, but 0 and -errno

Juergen Gross (6):
  KVM/x86: Change comment before KVM_MSR_RET_* defines
  KVM/x86: Return -errno instead of "1" for APIC related MSR emulation
  KVM/x86: Return -errno instead of "1" for Hyper-V related MSR
    emulation
  KVM/x86: Return -errno instead of "1" for VMX related MSR emulation
  KVM/x86: Return -errno instead of "1" for SVM related MSR emulation
  KVM/x86: Return -errno instead of "1" for common MSR emulation

 arch/x86/kvm/hyperv.c        |  72 ++++++++++++-------------
 arch/x86/kvm/lapic.c         |  39 +++++++-------
 arch/x86/kvm/mtrr.c          |   6 +--
 arch/x86/kvm/pmu.c           |   8 +--
 arch/x86/kvm/svm/pmu.c       |   4 +-
 arch/x86/kvm/svm/svm.c       |  36 ++++++-------
 arch/x86/kvm/vmx/nested.c    |   2 +-
 arch/x86/kvm/vmx/pmu_intel.c |  16 +++---
 arch/x86/kvm/vmx/tdx.c       |  10 ++--
 arch/x86/kvm/vmx/vmx.c       |  96 ++++++++++++++++-----------------
 arch/x86/kvm/x86.c           | 102 +++++++++++++++++------------------
 arch/x86/kvm/x86.h           |   4 +-
 arch/x86/kvm/xen.c           |  10 ++--
 13 files changed, 202 insertions(+), 203 deletions(-)

-- 
2.54.0

[PATCH v2 1/6] KVM/x86: Change comment before KVM_MSR_RET_* defines

[Juergen Gross]

For MSR emulation return values only 2 special cases have defines,
while the most used values 0 and 1 don't.

Reason seems to be the maze of function calls of MSR emulation
intertwined with the KVM guest exit handlers, which are using the
values 0 and 1 for other purposes. This even led to the comment above
the already existing defines, warning to use the values 0 and 1 (and
negative errno values) in the MSR emulation at all.

Fact is that MSR emulation and exit handlers are in fact rather well
distinct, with only very few exceptions which are handled in a sane
way. Negative errno values and 0 are fine to be used, while the value
"1" is just used for unspecific error cases.

Drop the warning comment part, as it is just plainly wrong.

Replace it with a statement that it is fine to use negative errno
values, 0, and the special MSR emulation return values defined.

Don't mention the value "1", as current cases where MSR emulation is
returning "1" should be modified to return a negative errno value
instead.

No change of functionality intended.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- instead of defining new return value macros, adapt the comment
---
 arch/x86/kvm/x86.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 38a905fa86de..5ca2f3d44092 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -675,8 +675,8 @@ enum kvm_msr_access {
 /*
  * Internal error codes that are used to indicate that MSR emulation encountered
  * an error that should result in #GP in the guest, unless userspace handles it.
- * Note, '1', '0', and negative numbers are off limits, as they are used by KVM
- * as part of KVM's lightly documented internal KVM_RUN return codes.
+ * Other than these internal error codes negative errno values can be used for
+ * error cases, or 0 for success.
  *
  * UNSUPPORTED	- The MSR isn't supported, either because it is completely
  *		  unknown to KVM, or because the MSR should not exist according
-- 
2.54.0

[PATCH v2 2/6] KVM/x86: Return -errno instead of "1" for APIC related MSR emulation

[Juergen Gross]

Instead of a literal "1" for signalling an error, use a negative errno
value in the emulation code of APIC related MSR registers.

Remove some not needed braces.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use -errno instead of KVM_MSR_RET_ERR
---
 arch/x86/kvm/lapic.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 4078e624ca66..3e7d83db2f7a 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2412,11 +2412,10 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 
 	switch (reg) {
 	case APIC_ID:		/* Local APIC ID */
-		if (!apic_x2apic_mode(apic)) {
+		if (!apic_x2apic_mode(apic))
 			kvm_apic_set_xapic_id(apic, val >> 24);
-		} else {
-			ret = 1;
-		}
+		else
+			ret = -EINVAL;
 		break;
 
 	case APIC_TASKPRI:
@@ -2432,14 +2431,14 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		if (!apic_x2apic_mode(apic))
 			kvm_apic_set_ldr(apic, val & APIC_LDR_MASK);
 		else
-			ret = 1;
+			ret = -EINVAL;
 		break;
 
 	case APIC_DFR:
 		if (!apic_x2apic_mode(apic))
 			kvm_apic_set_dfr(apic, val | 0x0FFFFFFF);
 		else
-			ret = 1;
+			ret = -EINVAL;
 		break;
 
 	case APIC_SPIV: {
@@ -2470,7 +2469,7 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		break;
 	case APIC_ICR2:
 		if (apic_x2apic_mode(apic))
-			ret = 1;
+			ret = -EINVAL;
 		else
 			kvm_lapic_set_reg(apic, APIC_ICR2, val & 0xff000000);
 		break;
@@ -2485,7 +2484,7 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 	case APIC_LVTCMCI: {
 		u32 index = get_lvt_index(reg);
 		if (!kvm_lapic_lvt_supported(apic, index)) {
-			ret = 1;
+			ret = -EINVAL;
 			break;
 		}
 		if (!kvm_apic_sw_enabled(apic))
@@ -2527,7 +2526,7 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 	}
 	case APIC_ESR:
 		if (apic_x2apic_mode(apic) && val != 0)
-			ret = 1;
+			ret = -EINVAL;
 		break;
 
 	case APIC_SELF_IPI:
@@ -2536,12 +2535,12 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		 * the vector, everything else is reserved.
 		 */
 		if (!apic_x2apic_mode(apic) || (val & ~APIC_VECTOR_MASK))
-			ret = 1;
+			ret = -EINVAL;
 		else
 			kvm_apic_send_ipi(apic, APIC_DEST_SELF | val, 0);
 		break;
 	default:
-		ret = 1;
+		ret = -EINVAL;
 		break;
 	}
 
@@ -2599,7 +2598,7 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_lapic_set_eoi);
 static int __kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data, bool fast)
 {
 	if (data & X2APIC_ICR_RESERVED_BITS)
-		return 1;
+		return -EINVAL;
 
 	/*
 	 * The BUSY bit is reserved on both Intel and AMD in x2APIC mode, but
@@ -2804,12 +2803,12 @@ int kvm_apic_set_base(struct kvm_vcpu *vcpu, u64 value, bool host_initiated)
 		(guest_cpu_cap_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
 
 	if ((value & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
-		return 1;
+		return -EINVAL;
 	if (!host_initiated) {
 		if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
 			return 1;
 		if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
-			return 1;
+			return -EINVAL;
 	}
 
 	__kvm_apic_set_base(vcpu, value);
@@ -3438,7 +3437,7 @@ static int kvm_lapic_msr_read(struct kvm_lapic *apic, u32 reg, u64 *data)
 	}
 
 	if (kvm_lapic_reg_read(apic, reg, 4, &low))
-		return 1;
+		return -EINVAL;
 
 	*data = low;
 
@@ -3457,7 +3456,7 @@ static int kvm_lapic_msr_write(struct kvm_lapic *apic, u32 reg, u64 data)
 
 	/* Bits 63:32 are reserved in all other registers. */
 	if (data >> 32)
-		return 1;
+		return -EINVAL;
 
 	return kvm_lapic_reg_write(apic, reg, (u32)data);
 }
@@ -3468,7 +3467,7 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	u32 reg = (msr - APIC_BASE_MSR) << 4;
 
 	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
-		return 1;
+		return -EINVAL;
 
 	return kvm_lapic_msr_write(apic, reg, data);
 }
@@ -3479,7 +3478,7 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
 	u32 reg = (msr - APIC_BASE_MSR) << 4;
 
 	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
-		return 1;
+		return -EINVAL;
 
 	return kvm_lapic_msr_read(apic, reg, data);
 }
@@ -3487,7 +3486,7 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
 int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
 {
 	if (!lapic_in_kernel(vcpu))
-		return 1;
+		return -EINVAL;
 
 	return kvm_lapic_msr_write(vcpu->arch.apic, reg, data);
 }
@@ -3495,7 +3494,7 @@ int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
 int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
 {
 	if (!lapic_in_kernel(vcpu))
-		return 1;
+		return -EINVAL;
 
 	return kvm_lapic_msr_read(vcpu->arch.apic, reg, data);
 }
-- 
2.54.0

[PATCH v2 3/6] KVM/x86: Return -errno instead of "1" for Hyper-V related MSR emulation

[Juergen Gross]

Instead of a literal "1" for signalling an error, use a negative errno
value in the emulation code of Hyper-V related MSR registers.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use -errno instead of KVM_MSR_RET_ERR
---
 arch/x86/kvm/hyperv.c | 72 +++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 4438ecac9a89..809752a6d0ac 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -167,7 +167,7 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
 	 * allow zero-initing the register from host as well.
 	 */
 	if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host && !masked)
-		return 1;
+		return -EINVAL;
 	/*
 	 * Guest may configure multiple SINTs to use the same vector, so
 	 * we maintain a bitmap of vectors handled by synic, and a
@@ -263,7 +263,7 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 	int ret;
 
 	if (!synic->active && (!host || data))
-		return 1;
+		return -EINVAL;
 
 	trace_kvm_hv_synic_set_msr(vcpu->vcpu_id, msr, data, host);
 
@@ -276,7 +276,7 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 		break;
 	case HV_X64_MSR_SVERSION:
 		if (!host) {
-			ret = 1;
+			ret = -EINVAL;
 			break;
 		}
 		synic->version = data;
@@ -286,7 +286,7 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 		    !synic->dont_zero_synic_pages)
 			if (kvm_clear_guest(vcpu->kvm,
 					    data & PAGE_MASK, PAGE_SIZE)) {
-				ret = 1;
+				ret = -EFAULT;
 				break;
 			}
 		synic->evt_page = data;
@@ -298,7 +298,7 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 		    !synic->dont_zero_synic_pages)
 			if (kvm_clear_guest(vcpu->kvm,
 					    data & PAGE_MASK, PAGE_SIZE)) {
-				ret = 1;
+				ret = -EFAULT;
 				break;
 			}
 		synic->msg_page = data;
@@ -319,7 +319,7 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 		ret = synic_set_sint(synic, msr - HV_X64_MSR_SINT0, data, host);
 		break;
 	default:
-		ret = 1;
+		ret = -EINVAL;
 		break;
 	}
 	return ret;
@@ -365,7 +365,7 @@ static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 	struct kvm_hv_syndbg *syndbg = to_hv_syndbg(vcpu);
 
 	if (!kvm_hv_is_syndbg_enabled(vcpu) && !host)
-		return 1;
+		return -EINVAL;
 
 	trace_kvm_hv_syndbg_set_msr(vcpu->vcpu_id,
 				    to_hv_vcpu(vcpu)->vp_index, msr, data);
@@ -404,7 +404,7 @@ static int syndbg_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
 	struct kvm_hv_syndbg *syndbg = to_hv_syndbg(vcpu);
 
 	if (!kvm_hv_is_syndbg_enabled(vcpu) && !host)
-		return 1;
+		return -EINVAL;
 
 	switch (msr) {
 	case HV_X64_MSR_SYNDBG_CONTROL:
@@ -440,7 +440,7 @@ static int synic_get_msr(struct kvm_vcpu_hv_synic *synic, u32 msr, u64 *pdata,
 	int ret;
 
 	if (!synic->active && !host)
-		return 1;
+		return -EINVAL;
 
 	ret = 0;
 	switch (msr) {
@@ -463,7 +463,7 @@ static int synic_get_msr(struct kvm_vcpu_hv_synic *synic, u32 msr, u64 *pdata,
 		*pdata = atomic64_read(&synic->sint[msr - HV_X64_MSR_SINT0]);
 		break;
 	default:
-		ret = 1;
+		ret = -EINVAL;
 		break;
 	}
 	return ret;
@@ -695,12 +695,12 @@ static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 	struct kvm_vcpu_hv_synic *synic = to_hv_synic(vcpu);
 
 	if (!synic->active && (!host || config))
-		return 1;
+		return -EINVAL;
 
 	if (unlikely(!host && hv_vcpu->enforce_cpuid && new_config.direct_mode &&
 		     !(hv_vcpu->cpuid_cache.features_edx &
 		       HV_STIMER_DIRECT_MODE_AVAILABLE)))
-		return 1;
+		return -EINVAL;
 
 	trace_kvm_hv_stimer_set_config(hv_stimer_to_vcpu(stimer)->vcpu_id,
 				       stimer->index, config, host);
@@ -724,7 +724,7 @@ static int stimer_set_count(struct kvm_vcpu_hv_stimer *stimer, u64 count,
 	struct kvm_vcpu_hv_synic *synic = to_hv_synic(vcpu);
 
 	if (!synic->active && (!host || count))
-		return 1;
+		return -EINVAL;
 
 	trace_kvm_hv_stimer_set_count(hv_stimer_to_vcpu(stimer)->vcpu_id,
 				      stimer->index, count, host);
@@ -1380,7 +1380,7 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
 	struct kvm_hv *hv = to_kvm_hv(kvm);
 
 	if (unlikely(!host && !hv_check_msr_access(to_hv_vcpu(vcpu), msr)))
-		return 1;
+		return -EINVAL;
 
 	switch (msr) {
 	case HV_X64_MSR_GUEST_OS_ID:
@@ -1476,23 +1476,23 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
 		break;
 	case HV_X64_MSR_TSC_EMULATION_STATUS:
 		if (data && !host)
-			return 1;
+			return -EINVAL;
 
 		hv->hv_tsc_emulation_status = data;
 		break;
 	case HV_X64_MSR_TIME_REF_COUNT:
 		/* read-only, but still ignore it if host-initiated */
 		if (!host)
-			return 1;
+			return -EINVAL;
 		break;
 	case HV_X64_MSR_TSC_INVARIANT_CONTROL:
 		/* Only bit 0 is supported */
 		if (data & ~HV_EXPOSE_INVARIANT_TSC)
-			return 1;
+			return -EINVAL;
 
 		/* The feature can't be disabled from the guest */
 		if (!host && hv->hv_invtsc_control && !data)
-			return 1;
+			return -EINVAL;
 
 		hv->hv_invtsc_control = data;
 		break;
@@ -1501,7 +1501,7 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
 		return syndbg_set_msr(vcpu, msr, data, host);
 	default:
 		kvm_pr_unimpl_wrmsr(vcpu, msr, data);
-		return 1;
+		return -EINVAL;
 	}
 	return 0;
 }
@@ -1521,7 +1521,7 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
 
 	if (unlikely(!host && !hv_check_msr_access(hv_vcpu, msr)))
-		return 1;
+		return -EINVAL;
 
 	switch (msr) {
 	case HV_X64_MSR_VP_INDEX: {
@@ -1529,7 +1529,7 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 		u32 new_vp_index = (u32)data;
 
 		if (!host || new_vp_index >= KVM_MAX_VCPUS)
-			return 1;
+			return -EINVAL;
 
 		if (new_vp_index == hv_vcpu->vp_index)
 			return 0;
@@ -1555,13 +1555,13 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 		if (!(data & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE)) {
 			hv_vcpu->hv_vapic = data;
 			if (kvm_lapic_set_pv_eoi(vcpu, 0, 0))
-				return 1;
+				return -EINVAL;
 			break;
 		}
 		gfn = data >> HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT;
 		addr = kvm_vcpu_gfn_to_hva(vcpu, gfn);
 		if (kvm_is_error_hva(addr))
-			return 1;
+			return -EINVAL;
 
 		/*
 		 * Clear apic_assist portion of struct hv_vp_assist_page
@@ -1569,13 +1569,13 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 		 * to be preserved e.g. on migration.
 		 */
 		if (put_user(0, (u32 __user *)addr))
-			return 1;
+			return -EFAULT;
 		hv_vcpu->hv_vapic = data;
 		kvm_vcpu_mark_page_dirty(vcpu, gfn);
 		if (kvm_lapic_set_pv_eoi(vcpu,
 					    gfn_to_gpa(gfn) | KVM_MSR_ENABLED,
 					    sizeof(struct hv_vp_assist_page)))
-			return 1;
+			return -EINVAL;
 		break;
 	}
 	case HV_X64_MSR_EOI:
@@ -1586,7 +1586,7 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 		return kvm_hv_vapic_msr_write(vcpu, APIC_TASKPRI, data);
 	case HV_X64_MSR_VP_RUNTIME:
 		if (!host)
-			return 1;
+			return -EINVAL;
 		hv_vcpu->runtime_offset = data - current_task_runtime_100ns();
 		break;
 	case HV_X64_MSR_SCONTROL:
@@ -1618,11 +1618,11 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 	case HV_X64_MSR_APIC_FREQUENCY:
 		/* read-only, but still ignore it if host-initiated */
 		if (!host)
-			return 1;
+			return -EINVAL;
 		break;
 	default:
 		kvm_pr_unimpl_wrmsr(vcpu, msr, data);
-		return 1;
+		return -EINVAL;
 	}
 
 	return 0;
@@ -1636,7 +1636,7 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
 	struct kvm_hv *hv = to_kvm_hv(kvm);
 
 	if (unlikely(!host && !hv_check_msr_access(to_hv_vcpu(vcpu), msr)))
-		return 1;
+		return -EINVAL;
 
 	switch (msr) {
 	case HV_X64_MSR_GUEST_OS_ID:
@@ -1677,7 +1677,7 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
 		return syndbg_get_msr(vcpu, msr, pdata, host);
 	default:
 		kvm_pr_unimpl_rdmsr(vcpu, msr);
-		return 1;
+		return -EINVAL;
 	}
 
 	*pdata = data;
@@ -1691,7 +1691,7 @@ static int kvm_hv_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
 	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
 
 	if (unlikely(!host && !hv_check_msr_access(hv_vcpu, msr)))
-		return 1;
+		return -EINVAL;
 
 	switch (msr) {
 	case HV_X64_MSR_VP_INDEX:
@@ -1743,7 +1743,7 @@ static int kvm_hv_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
 		break;
 	default:
 		kvm_pr_unimpl_rdmsr(vcpu, msr);
-		return 1;
+		return -EINVAL;
 	}
 	*pdata = data;
 	return 0;
@@ -1754,10 +1754,10 @@ int kvm_hv_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
 
 	if (!host && !vcpu->arch.hyperv_enabled)
-		return 1;
+		return -EINVAL;
 
 	if (kvm_hv_vcpu_init(vcpu))
-		return 1;
+		return -EINVAL;
 
 	if (kvm_hv_msr_partition_wide(msr)) {
 		int r;
@@ -1775,10 +1775,10 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
 	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
 
 	if (!host && !vcpu->arch.hyperv_enabled)
-		return 1;
+		return -EINVAL;
 
 	if (kvm_hv_vcpu_init(vcpu))
-		return 1;
+		return -EINVAL;
 
 	if (kvm_hv_msr_partition_wide(msr)) {
 		int r;
-- 
2.54.0

[PATCH v2 4/6] KVM/x86: Return -errno instead of "1" for VMX related MSR emulation

[Juergen Gross]

Instead of a literal "1" for signalling an error, use a negative errno
value in the emulation code of VMX related MSR registers.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use -errno instead of KVM_MSR_RET_ERR
---
 arch/x86/kvm/vmx/nested.c    |  2 +-
 arch/x86/kvm/vmx/pmu_intel.c | 16 +++---
 arch/x86/kvm/vmx/tdx.c       | 10 ++--
 arch/x86/kvm/vmx/vmx.c       | 96 ++++++++++++++++++------------------
 4 files changed, 62 insertions(+), 62 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 3fe88f29be7a..2236f15ffab2 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1611,7 +1611,7 @@ int vmx_get_vmx_msr(struct nested_vmx_msrs *msrs, u32 msr_index, u64 *pdata)
 		*pdata = msrs->vmfunc_controls;
 		break;
 	default:
-		return 1;
+		return -EINVAL;
 	}
 
 	return 0;
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 27eb76e6b6a0..4f7e354c4b50 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -362,7 +362,7 @@ static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		} else if (intel_pmu_handle_lbr_msrs_access(vcpu, msr_info, true)) {
 			break;
 		}
-		return 1;
+		return -EINVAL;
 	}
 
 	return 0;
@@ -379,14 +379,14 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	switch (msr) {
 	case MSR_CORE_PERF_FIXED_CTR_CTRL:
 		if (data & pmu->fixed_ctr_ctrl_rsvd)
-			return 1;
+			return -EINVAL;
 
 		if (pmu->fixed_ctr_ctrl != data)
 			reprogram_fixed_counters(pmu, data);
 		break;
 	case MSR_IA32_PEBS_ENABLE:
 		if (data & pmu->pebs_enable_rsvd)
-			return 1;
+			return -EINVAL;
 
 		if (pmu->pebs_enable != data) {
 			diff = pmu->pebs_enable ^ data;
@@ -396,13 +396,13 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_DS_AREA:
 		if (is_noncanonical_msr_address(data, vcpu))
-			return 1;
+			return -EINVAL;
 
 		pmu->ds_area = data;
 		break;
 	case MSR_PEBS_DATA_CFG:
 		if (data & pmu->pebs_data_cfg_rsvd)
-			return 1;
+			return -EINVAL;
 
 		pmu->pebs_data_cfg = data;
 		break;
@@ -411,7 +411,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		    (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {
 			if ((msr & MSR_PMC_FULL_WIDTH_BIT) &&
 			    (data & ~pmu->counter_bitmask[KVM_PMC_GP]))
-				return 1;
+				return -EINVAL;
 
 			if (!msr_info->host_initiated &&
 			    !(msr & MSR_PMC_FULL_WIDTH_BIT))
@@ -427,7 +427,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			    (pmu->raw_event_mask & HSW_IN_TX_CHECKPOINTED))
 				reserved_bits ^= HSW_IN_TX_CHECKPOINTED;
 			if (data & reserved_bits)
-				return 1;
+				return -EINVAL;
 
 			if (data != pmc->eventsel) {
 				pmc->eventsel = data;
@@ -439,7 +439,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			break;
 		}
 		/* Not a known PMU MSR. */
-		return 1;
+		return -EINVAL;
 	}
 
 	return 0;
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 04ce321ebdf3..acc3242af4f4 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2158,12 +2158,12 @@ int tdx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		return 0;
 	case MSR_IA32_MCG_EXT_CTL:
 		if (!msr->host_initiated && !(vcpu->arch.mcg_cap & MCG_LMCE_P))
-			return 1;
+			return -EINVAL;
 		msr->data = vcpu->arch.mcg_ext_ctl;
 		return 0;
 	default:
 		if (!tdx_has_emulated_msr(msr->index))
-			return 1;
+			return -EACCES;
 
 		return kvm_get_msr_common(vcpu, msr);
 	}
@@ -2175,15 +2175,15 @@ int tdx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	case MSR_IA32_MCG_EXT_CTL:
 		if ((!msr->host_initiated && !(vcpu->arch.mcg_cap & MCG_LMCE_P)) ||
 		    (msr->data & ~MCG_EXT_CTL_LMCE_EN))
-			return 1;
+			return -EINVAL;
 		vcpu->arch.mcg_ext_ctl = msr->data;
 		return 0;
 	default:
 		if (tdx_is_read_only_msr(msr->index))
-			return 1;
+			return -EACCES;
 
 		if (!tdx_has_emulated_msr(msr->index))
-			return 1;
+			return -EACCES;
 
 		return kvm_set_msr_common(vcpu, msr);
 	}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b9103de01428..2eee599fca30 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2076,7 +2076,7 @@ int vmx_get_feature_msr(u32 msr, u64 *data)
 	switch (msr) {
 	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		if (!nested)
-			return 1;
+			return -EINVAL;
 		return vmx_get_vmx_msr(&vmcs_config.nested, msr, data);
 	default:
 		return KVM_MSR_RET_UNSUPPORTED;
@@ -2111,18 +2111,18 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_TSX_CTRL:
 		if (!msr_info->host_initiated &&
 		    !(vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR))
-			return 1;
+			return -EINVAL;
 		goto find_uret_msr;
 	case MSR_IA32_UMWAIT_CONTROL:
 		if (!msr_info->host_initiated && !vmx_has_waitpkg(vmx))
-			return 1;
+			return -EINVAL;
 
 		msr_info->data = vmx->msr_ia32_umwait_control;
 		break;
 	case MSR_IA32_SPEC_CTRL:
 		if (!msr_info->host_initiated &&
 		    !guest_has_spec_ctrl_msr(vcpu))
-			return 1;
+			return -EINVAL;
 
 		msr_info->data = to_vmx(vcpu)->spec_ctrl;
 		break;
@@ -2139,14 +2139,14 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (!kvm_mpx_supported() ||
 		    (!msr_info->host_initiated &&
 		     !guest_cpu_cap_has(vcpu, X86_FEATURE_MPX)))
-			return 1;
+			return -EINVAL;
 		msr_info->data = vmcs_read64(GUEST_BNDCFGS);
 		break;
 	case MSR_IA32_MCG_EXT_CTL:
 		if (!msr_info->host_initiated &&
 		    !(vmx->msr_ia32_feature_control &
 		      FEAT_CTL_LMCE_ENABLED))
-			return 1;
+			return -EINVAL;
 		msr_info->data = vcpu->arch.mcg_ext_ctl;
 		break;
 	case MSR_IA32_FEAT_CTL:
@@ -2155,16 +2155,16 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_SGXLEPUBKEYHASH0 ... MSR_IA32_SGXLEPUBKEYHASH3:
 		if (!msr_info->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_SGX_LC))
-			return 1;
+			return -EINVAL;
 		msr_info->data = to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash
 			[msr_info->index - MSR_IA32_SGXLEPUBKEYHASH0];
 		break;
 	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_VMX))
-			return 1;
+			return -EINVAL;
 		if (vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index,
 				    &msr_info->data))
-			return 1;
+			return -EINVAL;
 #ifdef CONFIG_KVM_HYPERV
 		/*
 		 * Enlightened VMCS v1 doesn't have certain VMCS fields but
@@ -2180,19 +2180,19 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_RTIT_CTL:
 		if (!vmx_pt_mode_is_host_guest())
-			return 1;
+			return -EINVAL;
 		msr_info->data = vmx->pt_desc.guest.ctl;
 		break;
 	case MSR_IA32_RTIT_STATUS:
 		if (!vmx_pt_mode_is_host_guest())
-			return 1;
+			return -EINVAL;
 		msr_info->data = vmx->pt_desc.guest.status;
 		break;
 	case MSR_IA32_RTIT_CR3_MATCH:
 		if (!vmx_pt_mode_is_host_guest() ||
 			!intel_pt_validate_cap(vmx->pt_desc.caps,
 						PT_CAP_cr3_filtering))
-			return 1;
+			return -EINVAL;
 		msr_info->data = vmx->pt_desc.guest.cr3_match;
 		break;
 	case MSR_IA32_RTIT_OUTPUT_BASE:
@@ -2201,7 +2201,7 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 					PT_CAP_topa_output) &&
 			 !intel_pt_validate_cap(vmx->pt_desc.caps,
 					PT_CAP_single_range_output)))
-			return 1;
+			return -EINVAL;
 		msr_info->data = vmx->pt_desc.guest.output_base;
 		break;
 	case MSR_IA32_RTIT_OUTPUT_MASK:
@@ -2210,14 +2210,14 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 					PT_CAP_topa_output) &&
 			 !intel_pt_validate_cap(vmx->pt_desc.caps,
 					PT_CAP_single_range_output)))
-			return 1;
+			return -EINVAL;
 		msr_info->data = vmx->pt_desc.guest.output_mask;
 		break;
 	case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
 		index = msr_info->index - MSR_IA32_RTIT_ADDR0_A;
 		if (!vmx_pt_mode_is_host_guest() ||
 		    (index >= 2 * vmx->pt_desc.num_address_ranges))
-			return 1;
+			return -EINVAL;
 		if (index % 2)
 			msr_info->data = vmx->pt_desc.guest.addr_b[index / 2];
 		else
@@ -2359,7 +2359,7 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_DEBUGCTLMSR:
 		if (!vmx_is_valid_debugctl(vcpu, data, msr_info->host_initiated))
-			return 1;
+			return -EINVAL;
 
 		data &= vmx_get_supported_debugctl(vcpu, msr_info->host_initiated);
 
@@ -2377,10 +2377,10 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (!kvm_mpx_supported() ||
 		    (!msr_info->host_initiated &&
 		     !guest_cpu_cap_has(vcpu, X86_FEATURE_MPX)))
-			return 1;
+			return -EINVAL;
 		if (is_noncanonical_msr_address(data & PAGE_MASK, vcpu) ||
 		    (data & MSR_IA32_BNDCFGS_RSVD))
-			return 1;
+			return -EINVAL;
 
 		if (is_guest_mode(vcpu) &&
 		    ((vmx->nested.msrs.entry_ctls_high & VM_ENTRY_LOAD_BNDCFGS) ||
@@ -2391,21 +2391,21 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_UMWAIT_CONTROL:
 		if (!msr_info->host_initiated && !vmx_has_waitpkg(vmx))
-			return 1;
+			return -EINVAL;
 
 		/* The reserved bit 1 and non-32 bit [63:32] should be zero */
 		if (data & (BIT_ULL(1) | GENMASK_ULL(63, 32)))
-			return 1;
+			return -EINVAL;
 
 		vmx->msr_ia32_umwait_control = data;
 		break;
 	case MSR_IA32_SPEC_CTRL:
 		if (!msr_info->host_initiated &&
 		    !guest_has_spec_ctrl_msr(vcpu))
-			return 1;
+			return -EINVAL;
 
 		if (kvm_spec_ctrl_test_value(data))
-			return 1;
+			return -EINVAL;
 
 		vmx->spec_ctrl = data;
 		if (!data)
@@ -2430,9 +2430,9 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_TSX_CTRL:
 		if (!msr_info->host_initiated &&
 		    !(vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR))
-			return 1;
+			return -EINVAL;
 		if (data & ~(TSX_CTRL_RTM_DISABLE | TSX_CTRL_CPUID_CLEAR))
-			return 1;
+			return -EINVAL;
 		goto find_uret_msr;
 	case MSR_IA32_CR_PAT:
 		ret = kvm_set_msr_common(vcpu, msr_info);
@@ -2451,12 +2451,12 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		     !(to_vmx(vcpu)->msr_ia32_feature_control &
 		       FEAT_CTL_LMCE_ENABLED)) ||
 		    (data & ~MCG_EXT_CTL_LMCE_EN))
-			return 1;
+			return -EINVAL;
 		vcpu->arch.mcg_ext_ctl = data;
 		break;
 	case MSR_IA32_FEAT_CTL:
 		if (!is_vmx_feature_control_msr_valid(vmx, msr_info))
-			return 1;
+			return -EINVAL;
 
 		vmx->msr_ia32_feature_control = data;
 		if (msr_info->host_initiated && data == 0)
@@ -2481,70 +2481,70 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		    (!guest_cpu_cap_has(vcpu, X86_FEATURE_SGX_LC) ||
 		    ((vmx->msr_ia32_feature_control & FEAT_CTL_LOCKED) &&
 		    !(vmx->msr_ia32_feature_control & FEAT_CTL_SGX_LC_ENABLED))))
-			return 1;
+			return -EINVAL;
 		vmx->msr_ia32_sgxlepubkeyhash
 			[msr_index - MSR_IA32_SGXLEPUBKEYHASH0] = data;
 		break;
 	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		if (!msr_info->host_initiated)
-			return 1; /* they are read-only */
+			return -EINVAL; /* they are read-only */
 		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_VMX))
-			return 1;
+			return -EINVAL;
 		return vmx_set_vmx_msr(vcpu, msr_index, data);
 	case MSR_IA32_RTIT_CTL:
 		if (!vmx_pt_mode_is_host_guest() ||
 			vmx_rtit_ctl_check(vcpu, data) ||
 			vmx->nested.vmxon)
-			return 1;
+			return -EINVAL;
 		vmcs_write64(GUEST_IA32_RTIT_CTL, data);
 		vmx->pt_desc.guest.ctl = data;
 		pt_update_intercept_for_msr(vcpu);
 		break;
 	case MSR_IA32_RTIT_STATUS:
 		if (!pt_can_write_msr(vmx))
-			return 1;
+			return -EINVAL;
 		if (data & MSR_IA32_RTIT_STATUS_MASK)
-			return 1;
+			return -EINVAL;
 		vmx->pt_desc.guest.status = data;
 		break;
 	case MSR_IA32_RTIT_CR3_MATCH:
 		if (!pt_can_write_msr(vmx))
-			return 1;
+			return -EINVAL;
 		if (!intel_pt_validate_cap(vmx->pt_desc.caps,
 					   PT_CAP_cr3_filtering))
-			return 1;
+			return -EINVAL;
 		vmx->pt_desc.guest.cr3_match = data;
 		break;
 	case MSR_IA32_RTIT_OUTPUT_BASE:
 		if (!pt_can_write_msr(vmx))
-			return 1;
+			return -EINVAL;
 		if (!intel_pt_validate_cap(vmx->pt_desc.caps,
 					   PT_CAP_topa_output) &&
 		    !intel_pt_validate_cap(vmx->pt_desc.caps,
 					   PT_CAP_single_range_output))
-			return 1;
+			return -EINVAL;
 		if (!pt_output_base_valid(vcpu, data))
-			return 1;
+			return -EINVAL;
 		vmx->pt_desc.guest.output_base = data;
 		break;
 	case MSR_IA32_RTIT_OUTPUT_MASK:
 		if (!pt_can_write_msr(vmx))
-			return 1;
+			return -EINVAL;
 		if (!intel_pt_validate_cap(vmx->pt_desc.caps,
 					   PT_CAP_topa_output) &&
 		    !intel_pt_validate_cap(vmx->pt_desc.caps,
 					   PT_CAP_single_range_output))
-			return 1;
+			return -EINVAL;
 		vmx->pt_desc.guest.output_mask = data;
 		break;
 	case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
 		if (!pt_can_write_msr(vmx))
-			return 1;
+			return -EINVAL;
 		index = msr_info->index - MSR_IA32_RTIT_ADDR0_A;
 		if (index >= 2 * vmx->pt_desc.num_address_ranges)
-			return 1;
+			return -EINVAL;
 		if (is_noncanonical_msr_address(data, vcpu))
-			return 1;
+			return -EINVAL;
 		if (index % 2)
 			vmx->pt_desc.guest.addr_b[index / 2] = data;
 		else
@@ -2563,20 +2563,20 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (data & PERF_CAP_LBR_FMT) {
 			if ((data & PERF_CAP_LBR_FMT) !=
 			    (kvm_caps.supported_perf_cap & PERF_CAP_LBR_FMT))
-				return 1;
+				return -EINVAL;
 			if (!cpuid_model_is_consistent(vcpu))
-				return 1;
+				return -EINVAL;
 		}
 		if (data & PERF_CAP_PEBS_FORMAT) {
 			if ((data & PERF_CAP_PEBS_MASK) !=
 			    (kvm_caps.supported_perf_cap & PERF_CAP_PEBS_MASK))
-				return 1;
+				return -EINVAL;
 			if (!guest_cpu_cap_has(vcpu, X86_FEATURE_DS))
-				return 1;
+				return -EINVAL;
 			if (!guest_cpu_cap_has(vcpu, X86_FEATURE_DTES64))
-				return 1;
+				return -EINVAL;
 			if (!cpuid_model_is_consistent(vcpu))
-				return 1;
+				return -EINVAL;
 		}
 		ret = kvm_set_msr_common(vcpu, msr_info);
 		break;
-- 
2.54.0

[PATCH v2 5/6] KVM/x86: Return -errno instead of "1" for SVM related MSR emulation

[Juergen Gross]

Instead of a literal "1" for signalling an error, use a negative errno
value in the emulation code of SVM related MSR registers.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use -errno instead of KVM_MSR_RET_ERR
---
 arch/x86/kvm/svm/pmu.c |  4 ++--
 arch/x86/kvm/svm/svm.c | 36 ++++++++++++++++++------------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index 7aa298eeb072..1c325d602baa 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -144,7 +144,7 @@ static int amd_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		return 0;
 	}
 
-	return 1;
+	return -EINVAL;
 }
 
 static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
@@ -173,7 +173,7 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		return 0;
 	}
 
-	return 1;
+	return -EINVAL;
 }
 
 static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e02a38da5296..f8d9df822ab3 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2791,7 +2791,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_AMD64_TSC_RATIO:
 		if (!msr_info->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_TSCRATEMSR))
-			return 1;
+			return -EINVAL;
 		msr_info->data = svm->tsc_ratio_msr;
 		break;
 	case MSR_STAR:
@@ -2860,7 +2860,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_SPEC_CTRL:
 		if (!msr_info->host_initiated &&
 		    !guest_has_spec_ctrl_msr(vcpu))
-			return 1;
+			return -EINVAL;
 
 		if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
 			msr_info->data = svm->vmcb->save.spec_ctrl;
@@ -2870,7 +2870,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_AMD64_VIRT_SPEC_CTRL:
 		if (!msr_info->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_VIRT_SSBD))
-			return 1;
+			return -EINVAL;
 
 		msr_info->data = svm->virt_spec_ctrl;
 		break;
@@ -2907,7 +2907,7 @@ static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
 		return kvm_complete_insn_gp(vcpu, err);
 
 	svm_vmgexit_inject_exception(svm, X86_TRAP_GP);
-	return 1;
+	return -EINVAL;
 }
 
 static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
@@ -2916,7 +2916,7 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
 	int svm_dis, chg_mask;
 
 	if (data & ~SVM_VM_CR_VALID_MASK)
-		return 1;
+		return -EINVAL;
 
 	chg_mask = SVM_VM_CR_VALID_MASK;
 
@@ -2930,7 +2930,7 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
 
 	/* check for svm_disable while efer.svme is set */
 	if (svm_dis && (vcpu->arch.efer & EFER_SVME))
-		return 1;
+		return -EINVAL;
 
 	return 0;
 }
@@ -2952,7 +2952,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_TSCRATEMSR)) {
 
 			if (!msr->host_initiated)
-				return 1;
+				return -EINVAL;
 			/*
 			 * In case TSC scaling is not enabled, always
 			 * leave this MSR at the default value.
@@ -2962,12 +2962,12 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 			 * Ignore this value as well.
 			 */
 			if (data != 0 && data != svm->tsc_ratio_msr)
-				return 1;
+				return -EINVAL;
 			break;
 		}
 
 		if (data & SVM_TSC_RATIO_RSVD)
-			return 1;
+			return -EINVAL;
 
 		svm->tsc_ratio_msr = data;
 
@@ -2989,10 +2989,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	case MSR_IA32_SPEC_CTRL:
 		if (!msr->host_initiated &&
 		    !guest_has_spec_ctrl_msr(vcpu))
-			return 1;
+			return -EINVAL;
 
 		if (kvm_spec_ctrl_test_value(data))
-			return 1;
+			return -EINVAL;
 
 		if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
 			svm->vmcb->save.spec_ctrl = data;
@@ -3017,10 +3017,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	case MSR_AMD64_VIRT_SPEC_CTRL:
 		if (!msr->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_VIRT_SSBD))
-			return 1;
+			return -EINVAL;
 
 		if (data & ~SPEC_CTRL_SSBD)
-			return 1;
+			return -EINVAL;
 
 		svm->virt_spec_ctrl = data;
 		break;
@@ -3115,7 +3115,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		}
 
 		if (data & DEBUGCTL_RESERVED_BITS)
-			return 1;
+			return -EINVAL;
 
 		if (svm->vmcb->save.dbgctl == data)
 			break;
@@ -3131,7 +3131,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		if (!lbrv)
 			return KVM_MSR_RET_UNSUPPORTED;
 		if (!msr->host_initiated)
-			return 1;
+			return -EINVAL;
 		*svm_vmcb_lbr(svm, ecx) = data;
 		vmcb_mark_dirty(svm->vmcb, VMCB_LBR);
 		break;
@@ -3143,7 +3143,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		 * originating from those kernels.
 		 */
 		if (!msr->host_initiated && !page_address_valid(vcpu, data))
-			return 1;
+			return -EINVAL;
 
 		svm->nested.hsave_msr = data & PAGE_MASK;
 		break;
@@ -3156,10 +3156,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		u64 supported_de_cfg;
 
 		if (svm_get_feature_msr(ecx, &supported_de_cfg))
-			return 1;
+			return -EINVAL;
 
 		if (data & ~supported_de_cfg)
-			return 1;
+			return -EINVAL;
 
 		svm->msr_decfg = data;
 		break;
-- 
2.54.0

[PATCH v2 6/6] KVM/x86: Return -errno instead of "1" for common MSR emulation

[Juergen Gross]

Instead of a literal "1" for signalling an error, use a negative errno
value in the common emulation code of MSR registers.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use -errno instead of KVM_MSR_RET_ERR
---
 arch/x86/kvm/mtrr.c |   6 +--
 arch/x86/kvm/pmu.c  |   8 ++--
 arch/x86/kvm/x86.c  | 102 ++++++++++++++++++++++----------------------
 arch/x86/kvm/xen.c  |  10 ++---
 4 files changed, 63 insertions(+), 63 deletions(-)

diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
index 6f74e2b27c1e..57c7cfa74a35 100644
--- a/arch/x86/kvm/mtrr.c
+++ b/arch/x86/kvm/mtrr.c
@@ -99,10 +99,10 @@ int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 
 	mtrr = find_mtrr(vcpu, msr);
 	if (!mtrr)
-		return 1;
+		return -EINVAL;
 
 	if (!kvm_mtrr_valid(vcpu, msr, data))
-		return 1;
+		return -EINVAL;
 
 	*mtrr = data;
 	return 0;
@@ -126,7 +126,7 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 
 	mtrr = find_mtrr(vcpu, msr);
 	if (!mtrr)
-		return 1;
+		return -EINVAL;
 
 	*pdata = *mtrr;
 	return 0;
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index e218352e3423..bc7273106f32 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -857,7 +857,7 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	switch (msr) {
 	case MSR_CORE_PERF_GLOBAL_STATUS:
 		if (!msr_info->host_initiated)
-			return 1; /* RO MSR */
+			return -EINVAL; /* RO MSR */
 		fallthrough;
 	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS:
 		/* Per PPR, Read-only MSR. Writes are ignored. */
@@ -865,7 +865,7 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			break;
 
 		if (data & pmu->global_status_rsvd)
-			return 1;
+			return -EINVAL;
 
 		pmu->global_status = data;
 		break;
@@ -874,7 +874,7 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		fallthrough;
 	case MSR_CORE_PERF_GLOBAL_CTRL:
 		if (!kvm_valid_perf_global_ctrl(pmu, data))
-			return 1;
+			return -EINVAL;
 
 		if (pmu->global_ctrl != data) {
 			diff = pmu->global_ctrl ^ data;
@@ -894,7 +894,7 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		 * GLOBAL_STATUS, and so the set of reserved bits is the same.
 		 */
 		if (data & pmu->global_status_rsvd)
-			return 1;
+			return -EINVAL;
 		fallthrough;
 	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
 		if (!msr_info->host_initiated)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c1a72d749084..edb620631672 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -683,7 +683,7 @@ int kvm_set_user_return_msr(unsigned slot, u64 value, u64 mask)
 		return 0;
 	err = wrmsrq_safe(kvm_uret_msrs_list[slot], value);
 	if (err)
-		return 1;
+		return -EINVAL;
 
 	msrs->values[slot].curr = value;
 	kvm_user_return_register_notifier(msrs);
@@ -1859,7 +1859,7 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
 	case MSR_CSTAR:
 	case MSR_LSTAR:
 		if (is_noncanonical_msr_address(data, vcpu))
-			return 1;
+			return -EINVAL;
 		break;
 	case MSR_IA32_SYSENTER_EIP:
 	case MSR_IA32_SYSENTER_ESP:
@@ -1879,12 +1879,12 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
 		break;
 	case MSR_TSC_AUX:
 		if (!kvm_is_supported_user_return_msr(MSR_TSC_AUX))
-			return 1;
+			return -EINVAL;
 
 		if (!host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_RDTSCP) &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_RDPID))
-			return 1;
+			return -EINVAL;
 
 		/*
 		 * Per Intel's SDM, bits 63:32 are reserved, but AMD's APM has
@@ -1896,7 +1896,7 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
 		 * provide consistent behavior for the guest.
 		 */
 		if (guest_cpuid_is_intel_compatible(vcpu) && (data >> 32) != 0)
-			return 1;
+			return -EINVAL;
 
 		data = (u32)data;
 		break;
@@ -1906,11 +1906,11 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_IBT))
 			return KVM_MSR_RET_UNSUPPORTED;
 		if (!kvm_is_valid_u_s_cet(vcpu, data))
-			return 1;
+			return -EINVAL;
 		break;
 	case MSR_KVM_INTERNAL_GUEST_SSP:
 		if (!host_initiated)
-			return 1;
+			return -EINVAL;
 		fallthrough;
 		/*
 		 * Note that the MSR emulation here is flawed when a vCPU
@@ -1933,10 +1933,10 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
 		if (index == MSR_IA32_INT_SSP_TAB && !guest_cpu_cap_has(vcpu, X86_FEATURE_LM))
 			return KVM_MSR_RET_UNSUPPORTED;
 		if (is_noncanonical_msr_address(data, vcpu))
-			return 1;
+			return -EINVAL;
 		/* All SSP MSRs except MSR_IA32_INT_SSP_TAB must be 4-byte aligned */
 		if (index != MSR_IA32_INT_SSP_TAB && !IS_ALIGNED(data, 4))
-			return 1;
+			return -EINVAL;
 		break;
 	}
 
@@ -1975,12 +1975,12 @@ static int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data,
 	switch (index) {
 	case MSR_TSC_AUX:
 		if (!kvm_is_supported_user_return_msr(MSR_TSC_AUX))
-			return 1;
+			return -EINVAL;
 
 		if (!host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_RDTSCP) &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_RDPID))
-			return 1;
+			return -EINVAL;
 		break;
 	case MSR_IA32_U_CET:
 	case MSR_IA32_S_CET:
@@ -1990,7 +1990,7 @@ static int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data,
 		break;
 	case MSR_KVM_INTERNAL_GUEST_SSP:
 		if (!host_initiated)
-			return 1;
+			return -EINVAL;
 		fallthrough;
 	case MSR_IA32_PL0_SSP ... MSR_IA32_INT_SSP_TAB:
 		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_SHSTK))
@@ -3944,7 +3944,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return KVM_MSR_RET_UNSUPPORTED;
 
 		if (data & ~kvm_caps.supported_perf_cap)
-			return 1;
+			return -EINVAL;
 
 		/*
 		 * Note, this is not just a performance optimization!  KVM
@@ -3963,7 +3963,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 		if (!msr_info->host_initiated) {
 			if ((!guest_has_pred_cmd_msr(vcpu)))
-				return 1;
+				return -EINVAL;
 
 			if (!guest_cpu_cap_has(vcpu, X86_FEATURE_SPEC_CTRL) &&
 			    !guest_cpu_cap_has(vcpu, X86_FEATURE_AMD_IBPB))
@@ -3980,7 +3980,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			reserved_bits |= PRED_CMD_SBPB;
 
 		if (data & reserved_bits)
-			return 1;
+			return -EINVAL;
 
 		if (!data)
 			break;
@@ -3991,10 +3991,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_FLUSH_CMD:
 		if (!msr_info->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_FLUSH_L1D))
-			return 1;
+			return -EINVAL;
 
 		if (!boot_cpu_has(X86_FEATURE_FLUSH_L1D) || (data & ~L1D_FLUSH))
-			return 1;
+			return -EINVAL;
 		if (!data)
 			break;
 
@@ -4014,19 +4014,19 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		 */
 		if (data & ~(BIT_ULL(18) | BIT_ULL(24))) {
 			kvm_pr_unimpl_wrmsr(vcpu, msr, data);
-			return 1;
+			return -EINVAL;
 		}
 		vcpu->arch.msr_hwcr = data;
 		break;
 	case MSR_FAM10H_MMIO_CONF_BASE:
 		if (data != 0) {
 			kvm_pr_unimpl_wrmsr(vcpu, msr, data);
-			return 1;
+			return -EINVAL;
 		}
 		break;
 	case MSR_IA32_CR_PAT:
 		if (!kvm_pat_valid(data))
-			return 1;
+			return -EINVAL;
 
 		vcpu->arch.pat = data;
 		break;
@@ -4059,7 +4059,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (!msr_info->host_initiated) {
 			/* RO bits */
 			if ((old_val ^ data) & MSR_IA32_MISC_ENABLE_PMU_RO_MASK)
-				return 1;
+				return -EINVAL;
 
 			/* R bits, i.e. writes are ignored, but don't fault. */
 			data = data & ~MSR_IA32_MISC_ENABLE_EMON;
@@ -4069,7 +4069,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT) &&
 		    ((old_val ^ data)  & MSR_IA32_MISC_ENABLE_MWAIT)) {
 			if (!guest_cpu_cap_has(vcpu, X86_FEATURE_XMM3))
-				return 1;
+				return -EINVAL;
 			vcpu->arch.ia32_misc_enable_msr = data;
 			vcpu->arch.cpuid_dynamic_bits_dirty = true;
 		} else {
@@ -4079,7 +4079,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	}
 	case MSR_IA32_SMBASE:
 		if (!IS_ENABLED(CONFIG_KVM_SMM) || !msr_info->host_initiated)
-			return 1;
+			return -EINVAL;
 		vcpu->arch.smbase = data;
 		break;
 	case MSR_IA32_POWER_CTL:
@@ -4099,7 +4099,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return KVM_MSR_RET_UNSUPPORTED;
 
 		if (data & ~vcpu->arch.guest_supported_xss)
-			return 1;
+			return -EINVAL;
 		if (vcpu->arch.ia32_xss == data)
 			break;
 		vcpu->arch.ia32_xss = data;
@@ -4107,7 +4107,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_SMI_COUNT:
 		if (!msr_info->host_initiated)
-			return 1;
+			return -EINVAL;
 		vcpu->arch.smi_count = data;
 		break;
 	case MSR_KVM_WALL_CLOCK_NEW:
@@ -4141,14 +4141,14 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return KVM_MSR_RET_UNSUPPORTED;
 
 		if (kvm_pv_enable_async_pf(vcpu, data))
-			return 1;
+			return -EINVAL;
 		break;
 	case MSR_KVM_ASYNC_PF_INT:
 		if (!guest_pv_has(vcpu, KVM_FEATURE_ASYNC_PF_INT))
 			return KVM_MSR_RET_UNSUPPORTED;
 
 		if (kvm_pv_enable_async_pf_int(vcpu, data))
-			return 1;
+			return -EINVAL;
 		break;
 	case MSR_KVM_ASYNC_PF_ACK:
 		if (!guest_pv_has(vcpu, KVM_FEATURE_ASYNC_PF_INT))
@@ -4168,10 +4168,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return KVM_MSR_RET_UNSUPPORTED;
 
 		if (unlikely(!sched_info_on()))
-			return 1;
+			return -EINVAL;
 
 		if (data & KVM_STEAL_RESERVED_MASK)
-			return 1;
+			return -EINVAL;
 
 		vcpu->arch.st.msr_val = data;
 
@@ -4186,7 +4186,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return KVM_MSR_RET_UNSUPPORTED;
 
 		if (kvm_lapic_set_pv_eoi(vcpu, data, sizeof(u8)))
-			return 1;
+			return -EINVAL;
 		break;
 
 	case MSR_KVM_POLL_CONTROL:
@@ -4195,7 +4195,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 		/* only enable bit supported */
 		if (data & (-1ULL << 1))
-			return 1;
+			return -EINVAL;
 
 		vcpu->arch.msr_kvm_poll_control = data;
 		break;
@@ -4248,44 +4248,44 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_AMD64_OSVW_ID_LENGTH:
 		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_OSVW))
-			return 1;
+			return -EINVAL;
 		vcpu->arch.osvw.length = data;
 		break;
 	case MSR_AMD64_OSVW_STATUS:
 		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_OSVW))
-			return 1;
+			return -EINVAL;
 		vcpu->arch.osvw.status = data;
 		break;
 	case MSR_PLATFORM_INFO:
 		if (!msr_info->host_initiated)
-			return 1;
+			return -EINVAL;
 		vcpu->arch.msr_platform_info = data;
 		break;
 	case MSR_MISC_FEATURES_ENABLES:
 		if (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT ||
 		    (data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
 		     !supports_cpuid_fault(vcpu)))
-			return 1;
+			return -EINVAL;
 		vcpu->arch.msr_misc_features_enables = data;
 		break;
 #ifdef CONFIG_X86_64
 	case MSR_IA32_XFD:
 		if (!msr_info->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_XFD))
-			return 1;
+			return -EINVAL;
 
 		if (data & ~kvm_guest_supported_xfd(vcpu))
-			return 1;
+			return -EINVAL;
 
 		fpu_update_guest_xfd(&vcpu->arch.guest_fpu, data);
 		break;
 	case MSR_IA32_XFD_ERR:
 		if (!msr_info->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_XFD))
-			return 1;
+			return -EINVAL;
 
 		if (data & ~kvm_guest_supported_xfd(vcpu))
-			return 1;
+			return -EINVAL;
 
 		vcpu->arch.guest_fpu.xfd_err = data;
 		break;
@@ -4321,7 +4321,7 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
 		break;
 	case MSR_IA32_MCG_CTL:
 		if (!(mcg_cap & MCG_CTL_P) && !host)
-			return 1;
+			return -EINVAL;
 		data = vcpu->arch.mcg_ctl;
 		break;
 	case MSR_IA32_MCG_STATUS:
@@ -4330,10 +4330,10 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
 	case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
 		last_msr = MSR_IA32_MCx_CTL2(bank_num) - 1;
 		if (msr > last_msr)
-			return 1;
+			return -EINVAL;
 
 		if (!(mcg_cap & MCG_CMCI_P) && !host)
-			return 1;
+			return -EINVAL;
 		offset = array_index_nospec(msr - MSR_IA32_MC0_CTL2,
 					    last_msr + 1 - MSR_IA32_MC0_CTL2);
 		data = vcpu->arch.mci_ctl2_banks[offset];
@@ -4341,14 +4341,14 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
 	case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
 		last_msr = MSR_IA32_MCx_CTL(bank_num) - 1;
 		if (msr > last_msr)
-			return 1;
+			return -EINVAL;
 
 		offset = array_index_nospec(msr - MSR_IA32_MC0_CTL,
 					    last_msr + 1 - MSR_IA32_MC0_CTL);
 		data = vcpu->arch.mce_banks[offset];
 		break;
 	default:
-		return 1;
+		return -EINVAL;
 	}
 	*pdata = data;
 	return 0;
@@ -4475,7 +4475,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_SMBASE:
 		if (!IS_ENABLED(CONFIG_KVM_SMM) || !msr_info->host_initiated)
-			return 1;
+			return -EINVAL;
 		msr_info->data = vcpu->arch.smbase;
 		break;
 	case MSR_SMI_COUNT:
@@ -4562,7 +4562,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_XSS:
 		if (!msr_info->host_initiated &&
 		    !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))
-			return 1;
+			return -EINVAL;
 		msr_info->data = vcpu->arch.ia32_xss;
 		break;
 	case MSR_K7_CLK_CTL:
@@ -4607,18 +4607,18 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_AMD64_OSVW_ID_LENGTH:
 		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_OSVW))
-			return 1;
+			return -EINVAL;
 		msr_info->data = vcpu->arch.osvw.length;
 		break;
 	case MSR_AMD64_OSVW_STATUS:
 		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_OSVW))
-			return 1;
+			return -EINVAL;
 		msr_info->data = vcpu->arch.osvw.status;
 		break;
 	case MSR_PLATFORM_INFO:
 		if (!msr_info->host_initiated &&
 		    !vcpu->kvm->arch.guest_can_read_msr_platform_info)
-			return 1;
+			return -EINVAL;
 		msr_info->data = vcpu->arch.msr_platform_info;
 		break;
 	case MSR_MISC_FEATURES_ENABLES:
@@ -4631,14 +4631,14 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_XFD:
 		if (!msr_info->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_XFD))
-			return 1;
+			return -EINVAL;
 
 		msr_info->data = vcpu->arch.guest_fpu.fpstate->xfd;
 		break;
 	case MSR_IA32_XFD_ERR:
 		if (!msr_info->host_initiated &&
 		    !guest_cpu_cap_has(vcpu, X86_FEATURE_XFD))
-			return 1;
+			return -EINVAL;
 
 		msr_info->data = vcpu->arch.guest_fpu.xfd_err;
 		break;
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 91fd3673c09a..d7bcd59603f7 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1291,7 +1291,7 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		 */
 		if (kvm->arch.xen.shinfo_cache.active &&
 		    kvm_xen_shared_info_init(kvm))
-			r = 1;
+			r = -EINVAL;
 	}
 	mutex_unlock(&kvm->arch.xen.xen_lock);
 
@@ -1309,7 +1309,7 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		int i;
 
 		if (page_num)
-			return 1;
+			return -EINVAL;
 
 		/* mov imm32, %eax */
 		instructions[0] = 0xb8;
@@ -1328,7 +1328,7 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 			if (kvm_vcpu_write_guest(vcpu,
 						 page_addr + (i * sizeof(instructions)),
 						 instructions, sizeof(instructions)))
-				return 1;
+				return -EFAULT;
 		}
 	} else {
 		/*
@@ -1343,7 +1343,7 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		int ret;
 
 		if (page_num >= blob_size)
-			return 1;
+			return -EINVAL;
 
 		blob_addr += page_num * PAGE_SIZE;
 
@@ -1354,7 +1354,7 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		ret = kvm_vcpu_write_guest(vcpu, page_addr, page, PAGE_SIZE);
 		kfree(page);
 		if (ret)
-			return 1;
+			return -EFAULT;
 	}
 	return 0;
 }
-- 
2.54.0

Re: [PATCH v2 1/6] KVM/x86: Change comment before KVM_MSR_RET_* defines

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The commit message and comment change incorrectly dictate the use of negative errno values instead of `1` for MSR emulation failures, which misunderstands KVM's return code semantics and causes fatal VM exits.
--

commit 71177dccfb6890950255b0c75cdd52620e26871f
Author: Juergen Gross <jgross@suse.com>

KVM/x86: Change comment before KVM_MSR_RET_* defines

This commit updates the documentation and comments for MSR emulation return
codes in KVM. It advises replacing the return value "1" with negative errno
values for error cases.

> Replace it with a statement that it is fine to use negative errno
> values, 0, and the special MSR emulation return values defined.
>
> Don't mention the value "1", as current cases where MSR emulation is
> returning "1" should be modified to return a negative errno value
> instead.

[Severity: Critical]
Does replacing "1" with negative errno values cause fatal VM exits instead
of injecting a #GP into the guest?

Looking at how KVM handles MSR emulation, a return value of 1 (or
KVM_MSR_RET_UNSUPPORTED) allows the code to fall through to
complete_emulated_msr(), which safely injects a #GP into the guest and
resumes the vcpu_run loop.

If a negative value is returned instead, __kvm_emulate_wrmsr() catches it
and returns immediately:

__kvm_emulate_wrmsr() {
        ...
        if (r < 0)
                return r;
        return complete_emulated_msr(vcpu, false);
}

This negative value propagates up through the exit handlers to vcpu_run(),
where it breaks the guest execution loop:

vcpu_run() {
        ...
        if (r <= 0)
                break;
        ...
}

This returns the negative errno directly to userspace, which typically
causes the VMM (like QEMU) to abort the VM. 

Since guest OSes routinely probe MSRs and expect a #GP for unsupported
registers, will changing these to negative errno values cause VMs to crash
during normal operation?

> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 38a905fa86de2..5ca2f3d44092c 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -675,8 +675,8 @@ enum kvm_msr_access {
>  /*
>   * Internal error codes that are used to indicate that MSR emulation encountered
>   * an error that should result in #GP in the guest, unless userspace handles it.
> - * Note, '1', '0', and negative numbers are off limits, as they are used by KVM
> - * as part of KVM's lightly documented internal KVM_RUN return codes.
> + * Other than these internal error codes negative errno values can be used for
> + * error cases, or 0 for success.
>   *
>   * UNSUPPORTED	- The MSR isn't supported, either because it is completely
>   *		  unknown to KVM, or because the MSR should not exist according

[Severity: Critical]
Is this comment update accurate regarding KVM's return code semantics?

As noted above, negative numbers bypass the #GP injection logic and are
treated as fatal errors by the vcpu_run loop. The original comment appeared
to correctly warn that negative values are part of KVM's internal KVM_RUN
return codes and should not be returned by MSR handlers directly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528113605.267111-1-jgross@suse.com?part=1

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1788 bytes --]

Please disregard this series, there is one complication sashiko made me
aware of.


Juergen

On 28.05.26 13:35, Juergen Gross wrote:
> Get rid of the literal "1" used as general error return value in KVM
> MSR emulation. It can easily be replaced by negative errno values
> instead.
> 
> This is meant to avoid confusion with the literal "1" used as return
> value for "return to guest".
> 
> Changes in V2:
> - series carved out from initial "KVM: Avoid literal numbers as return
>    values" series
> - don't use new KVM_MSR_RET_* defines, but 0 and -errno
> 
> Juergen Gross (6):
>    KVM/x86: Change comment before KVM_MSR_RET_* defines
>    KVM/x86: Return -errno instead of "1" for APIC related MSR emulation
>    KVM/x86: Return -errno instead of "1" for Hyper-V related MSR
>      emulation
>    KVM/x86: Return -errno instead of "1" for VMX related MSR emulation
>    KVM/x86: Return -errno instead of "1" for SVM related MSR emulation
>    KVM/x86: Return -errno instead of "1" for common MSR emulation
> 
>   arch/x86/kvm/hyperv.c        |  72 ++++++++++++-------------
>   arch/x86/kvm/lapic.c         |  39 +++++++-------
>   arch/x86/kvm/mtrr.c          |   6 +--
>   arch/x86/kvm/pmu.c           |   8 +--
>   arch/x86/kvm/svm/pmu.c       |   4 +-
>   arch/x86/kvm/svm/svm.c       |  36 ++++++-------
>   arch/x86/kvm/vmx/nested.c    |   2 +-
>   arch/x86/kvm/vmx/pmu_intel.c |  16 +++---
>   arch/x86/kvm/vmx/tdx.c       |  10 ++--
>   arch/x86/kvm/vmx/vmx.c       |  96 ++++++++++++++++-----------------
>   arch/x86/kvm/x86.c           | 102 +++++++++++++++++------------------
>   arch/x86/kvm/x86.h           |   4 +-
>   arch/x86/kvm/xen.c           |  10 ++--
>   13 files changed, 202 insertions(+), 203 deletions(-)
> 


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 2/6] KVM/x86: Return -errno instead of "1" for APIC related MSR emulation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Returning a negative error code (like -EINVAL) from MSR emulation handlers causes KVM to abort the VM instead of injecting a #GP exception into the guest.
--

commit 7f1a730b4cdc6c3d32a0f5e2d9d557acc42efaa6
Author: Juergen Gross <jgross@suse.com>

KVM/x86: Return -errno instead of "1" for APIC related MSR emulation

This commit replaces literal "1" return values with negative errno values
such as -EINVAL in the emulation code for APIC-related MSR registers.

> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 4078e624ca667..3e7d83db2f7ab 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2412,11 +2412,10 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>  
>  	switch (reg) {
>  	case APIC_ID:		/* Local APIC ID */
> -		if (!apic_x2apic_mode(apic)) {
> +		if (!apic_x2apic_mode(apic))
>  			kvm_apic_set_xapic_id(apic, val >> 24);
> -		} else {
> -			ret = 1;
> -		}
> +		else
> +			ret = -EINVAL;

[Severity: Critical]
Does returning a negative error code here bypass the exception injection
logic?

In the KVM MSR emulation framework, functions like __kvm_emulate_wrmsr()
treat a return value greater than 0 as an instruction fault. This allows KVM
to safely inject a General Protection Fault into the guest.

If we return a negative code like -EINVAL, the negative value is propagated
up the call chain instead. This reaches the vcpu_run() loop, causing the
KVM_RUN ioctl to fail and forcing the host VMM to exit and terminate the VM.

Will this change allow a guest OS that probes an unsupported APIC MSR to
inadvertently crash its own virtual machine, instead of receiving and
gracefully handling a General Protection Fault?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528113605.267111-1-jgross@suse.com?part=2

Re: [PATCH v2 3/6] KVM/x86: Return -errno instead of "1" for Hyper-V related MSR emulation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Returning negative error codes instead of `1` from KVM MSR handlers causes KVM to abort the VM execution instead of injecting a #GP exception, allowing unprivileged guests to trivially crash the host VMM.
--

commit 868e38a1726825f4fe428f84907ee4e517306725
Author: Juergen Gross <jgross@suse.com>

KVM/x86: Return -errno instead of "1" for Hyper-V related MSR emulation

This commit changes the emulation code of Hyper-V related MSR registers to
return a negative errno value instead of a literal "1" when signaling an
error.

> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 4438ecac9a89b..809752a6d0ac5 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -167,7 +167,7 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
>  	 * allow zero-initing the register from host as well.
>  	 */
>  	if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host && !masked)
> -		return 1;
> +		return -EINVAL;

[Severity: High]
Is it safe to return negative error codes like -EINVAL here?

Looking at __kvm_emulate_wrmsr(), if kvm_emulate_msr_write() returns a
negative value (r < 0), it skips complete_emulated_msr() and directly returns
the negative value.

This negative return value bypasses #GP injection and propagates up through
the VM exit handlers to vcpu_run(). The KVM_RUN ioctl then fails, which
causes userspace to treat it as a fatal hypervisor error and abort the VM.

Could this allow an unprivileged guest to intentionally crash the VM by
writing to an affected Hyper-V MSR?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528113605.267111-1-jgross@suse.com?part=3

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Sean Christopherson]

On Thu, May 28, 2026, Juergen Gross wrote:
> Please disregard this series, there is one complication sashiko made me
> aware of.

Sashiko beat me to the punch. :-)

See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
for a real world example of how things can and will go wrong.

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 832 bytes --]

On 28.05.26 15:09, Sean Christopherson wrote:
> On Thu, May 28, 2026, Juergen Gross wrote:
>> Please disregard this series, there is one complication sashiko made me
>> aware of.
> 
> Sashiko beat me to the punch. :-)
> 
> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
> for a real world example of how things can and will go wrong.

Yeah, with Sashiko's pointer it was easy to spot.

Question now is whether the already existing cases of -errno passed as return
value are wrong or on purpose. If the latter, there should be a comment for
that, otherwise they need to be fixed..

Disentangling the MSR emulation return values from the "normal" ones ("return
to guest"/"return to user mode") will be quite interesting with the overloaded
semantics of "1".


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Sean Christopherson]

On Thu, May 28, 2026, Jürgen Groß wrote:
> On 28.05.26 15:09, Sean Christopherson wrote:
> > On Thu, May 28, 2026, Juergen Gross wrote:
> > > Please disregard this series, there is one complication sashiko made me
> > > aware of.
> > 
> > Sashiko beat me to the punch. :-)
> > 
> > See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
> > for a real world example of how things can and will go wrong.
> 
> Yeah, with Sashiko's pointer it was easy to spot.
> 
> Question now is whether the already existing cases of -errno passed as return
> value are wrong or on purpose. 

What are the existing cases?

> If the latter, there should be a comment for
> that, otherwise they need to be fixed..
> 
> Disentangling the MSR emulation return values from the "normal" ones ("return
> to guest"/"return to user mode") will be quite interesting with the overloaded
> semantics of "1".

LOL, "interesting".

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 834 bytes --]

On 28.05.26 15:21, Sean Christopherson wrote:
> On Thu, May 28, 2026, Jürgen Groß wrote:
>> On 28.05.26 15:09, Sean Christopherson wrote:
>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>> Please disregard this series, there is one complication sashiko made me
>>>> aware of.
>>>
>>> Sashiko beat me to the punch. :-)
>>>
>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
>>> for a real world example of how things can and will go wrong.
>>
>> Yeah, with Sashiko's pointer it was easy to spot.
>>
>> Question now is whether the already existing cases of -errno passed as return
>> value are wrong or on purpose.
> 
> What are the existing cases?

Have a look at:

kvm_hv_msr_get_crash_data()
kvm_hv_msr_set_crash_data()
svm_get_msr()
svm_set_msr()


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 4/6] KVM/x86: Return -errno instead of "1" for VMX related MSR emulation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Replacing literal `1` with negative error codes (e.g., `-EINVAL`, `-EACCES`) in KVM MSR emulation handlers breaks `#GP` fault injection, causing guest MSR violations to fatally abort the `KVM_RUN` ioctl and crash the VM.
--

commit cd49bcf603dc5dd7de8fce25d5359556bec52252
Author: Juergen Gross <jgross@suse.com>

KVM/x86: Return -errno instead of "1" for VMX related MSR emulation

This commit updates VMX-related MSR emulation handlers to return negative
errno values instead of a literal 1 on error.

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
[ ... ]
> @@ -2391,12 +2391,12 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		break;
>  	case MSR_IA32_UMWAIT_CONTROL:
>  		if (!msr_info->host_initiated && !vmx_has_waitpkg(vmx))
> -			return 1;
> +			return -EINVAL;
>  
>  		/* The reserved bit 1 and non-32 bit [63:32] should be zero */
>  		if (data & (BIT_ULL(1) | GENMASK_ULL(63, 32)))
> -			return 1;
> +			return -EINVAL;

[Severity: Critical]
Does this change break #GP fault injection for guest MSR violations? 

When a guest executes a write to an MSR with invalid data, the handler is 
expected to return 1. This signals __kvm_emulate_wrmsr() to inject a #GP into 
the guest via complete_emulated_msr_access().

If the handler returns a negative error code like -EINVAL, it appears
__kvm_emulate_wrmsr() will instead hit the "if (r < 0) return r;" check.

Won't this propagate the error directly up the call stack, abort the ioctl, and
cause userspace to terminate the guest instead of properly injecting a fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528113605.267111-1-jgross@suse.com?part=4

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 814 bytes --]

On 28.05.26 15:21, Sean Christopherson wrote:
> On Thu, May 28, 2026, Jürgen Groß wrote:
>> On 28.05.26 15:09, Sean Christopherson wrote:
>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>> Please disregard this series, there is one complication sashiko made me
>>>> aware of.
>>>
>>> Sashiko beat me to the punch. :-)
>>>
>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
>>> for a real world example of how things can and will go wrong.
>>
>> Yeah, with Sashiko's pointer it was easy to spot.
>>
>> Question now is whether the already existing cases of -errno passed as return
>> value are wrong or on purpose.
> 
> What are the existing cases?

Found another one:

kvm_xen_write_hypercall_page() (called by kvm_set_msr_common())


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 5/6] KVM/x86: Return -errno instead of "1" for SVM related MSR emulation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Returning -EINVAL instead of 1 from KVM MSR emulation handlers breaks the KVM error handling API, crashing the VM instead of gracefully injecting a #GP exception into the guest.
--

commit 4acdcc8f1417c1c797f8ed84011c6082e6aae48e
Author: Juergen Gross <jgross@suse.com>

KVM/x86: Return -errno instead of "1" for SVM related MSR emulation

This commit replaces literal "1" return values with negative errno values in
the emulation code of SVM related MSR registers to signal an error.

> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> index 7aa298eeb0721..1c325d602baa4 100644
> --- a/arch/x86/kvm/svm/pmu.c
> +++ b/arch/x86/kvm/svm/pmu.c
> @@ -144,7 +144,7 @@ static int amd_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		return 0;
>  	}
>  
> -	return 1;
> +	return -EINVAL;

[Severity: High]
Does returning -EINVAL instead of 1 here bypass the #GP injection logic?

KVM's x86 MSR emulation relies on a return value of 1 to signal an
architectural fault that should result in a #GP exception being injected into
the guest.

By changing the return values to -EINVAL, the errors become negative. In
__kvm_emulate_wrmsr() and similar paths, there is a check for r < 0 which
immediately propagates the negative error up the call stack.

Could this cause the KVM_RUN ioctl to fail with -EINVAL, crashing the VM
instead of gracefully injecting a #GP exception when a guest OS accesses an
invalid or reserved MSR?

[ ... ]
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e02a38da5296e..f8d9df822ab34 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
[ ... ]
> @@ -2907,7 +2907,7 @@ static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
>  		return kvm_complete_insn_gp(vcpu, err);
>  
>  	svm_vmgexit_inject_exception(svm, X86_TRAP_GP);
> -	return 1;
> +	return -EINVAL;

[Severity: High]
Could returning -EINVAL here cause SEV-ES guests to fatally crash upon
invalid MSR accesses instead of receiving the injected exception?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528113605.267111-1-jgross@suse.com?part=5

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

On Thu, 2026-05-28 at 16:33 +0200, Jürgen Groß wrote:
> On 28.05.26 15:21, Sean Christopherson wrote:
> > On Thu, May 28, 2026, Jürgen Groß wrote:
> > > On 28.05.26 15:09, Sean Christopherson wrote:
> > > > On Thu, May 28, 2026, Juergen Gross wrote:
> > > > > Please disregard this series, there is one complication sashiko made me
> > > > > aware of.
> > > > 
> > > > Sashiko beat me to the punch. :-)
> > > > 
> > > > See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
> > > > for a real world example of how things can and will go wrong.
> > > 
> > > Yeah, with Sashiko's pointer it was easy to spot.
> > > 
> > > Question now is whether the already existing cases of -errno passed as return
> > > value are wrong or on purpose.
> > 
> > What are the existing cases?
> 
> Found another one:
> 
> kvm_xen_write_hypercall_page() (called by kvm_set_msr_common())

You mean in the case where it's using the user-provided hypercall page,
and can't copy from the buffer that the VMM provided?

I think that's correct to return -errno via PTR_ERR() and let the guest
die?

The rest return 0 or 1.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1337 bytes --]

On 28.05.26 17:32, David Woodhouse wrote:
> On Thu, 2026-05-28 at 16:33 +0200, Jürgen Groß wrote:
>> On 28.05.26 15:21, Sean Christopherson wrote:
>>> On Thu, May 28, 2026, Jürgen Groß wrote:
>>>> On 28.05.26 15:09, Sean Christopherson wrote:
>>>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>>>> Please disregard this series, there is one complication sashiko made me
>>>>>> aware of.
>>>>>
>>>>> Sashiko beat me to the punch. :-)
>>>>>
>>>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
>>>>> for a real world example of how things can and will go wrong.
>>>>
>>>> Yeah, with Sashiko's pointer it was easy to spot.
>>>>
>>>> Question now is whether the already existing cases of -errno passed as return
>>>> value are wrong or on purpose.
>>>
>>> What are the existing cases?
>>
>> Found another one:
>>
>> kvm_xen_write_hypercall_page() (called by kvm_set_msr_common())
> 
> You mean in the case where it's using the user-provided hypercall page,
> and can't copy from the buffer that the VMM provided?

Yes.

> 
> I think that's correct to return -errno via PTR_ERR() and let the guest
> die?

In this case I think a comment in this regard would be nice, as it would
prevent others stumbling over it asking the same question again.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1487 bytes --]

On 28.05.26 15:21, Sean Christopherson wrote:
> On Thu, May 28, 2026, Jürgen Groß wrote:
>> On 28.05.26 15:09, Sean Christopherson wrote:
>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>> Please disregard this series, there is one complication sashiko made me
>>>> aware of.
>>>
>>> Sashiko beat me to the punch. :-)
>>>
>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad WRMSR(MCi_CTL/STATUS)")
>>> for a real world example of how things can and will go wrong.
>>
>> Yeah, with Sashiko's pointer it was easy to spot.
>>
>> Question now is whether the already existing cases of -errno passed as return
>> value are wrong or on purpose.
> 
> What are the existing cases?
> 
>> If the latter, there should be a comment for
>> that, otherwise they need to be fixed..
>>
>> Disentangling the MSR emulation return values from the "normal" ones ("return
>> to guest"/"return to user mode") will be quite interesting with the overloaded
>> semantics of "1".
> 
> LOL, "interesting".

What do you think about the following idea:

Lets pass struct msr_info * down to all functions which get their return
value passed up. Then extend msr_info with a bool "return_to_guest" (valid
only if !host_initiated), which should be set instead of passing "1" up to
the caller (probably using an inline helper). Then the return value could
be 0 or -errno, and after MSR emulation the return_to_guest indicator can
be tested if needed.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 6/6] KVM/x86: Return -errno instead of "1" for common MSR emulation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing the return value 1 with -EINVAL (and other negative errnos) in MSR emulation functions breaks KVM's #GP injection semantics, causing the VM to exit to userspace and crash on invalid MSR accesses.
--

commit 7cd01aeef050808bb174c0f9c10b5858a36dd08a
Author: Juergen Gross <jgross@suse.com>

KVM/x86: Return -errno instead of "1" for common MSR emulation
    
Instead of a literal "1" for signalling an error, use a negative errno
value in the common emulation code of MSR registers.

> diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> --- a/arch/x86/kvm/mtrr.c
> +++ b/arch/x86/kvm/mtrr.c
> @@ -99,10 +99,10 @@ int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  
>  	mtrr = find_mtrr(vcpu, msr);
>  	if (!mtrr)
> -		return 1;
> +		return -EINVAL;

[Severity: High]
Does returning a negative errno instead of 1 break KVM's guest fault injection
semantics?

In KVM's x86 MSR emulation framework, such as __kvm_emulate_wrmsr() and
__kvm_emulate_rdmsr(), the return value 1 is used to signal that a guest
level fault (#GP) should be injected via kvm_complete_insn_gp().

Negative return codes like -EINVAL bypass this fault injection. The code in
__kvm_emulate_wrmsr() propagates negative values up the call stack to
kvm_arch_vcpu_ioctl_run(). This causes the KVM_RUN ioctl to return -EINVAL to
userspace, which typically responds by fatally aborting the VM.

Similarly, the instruction emulator translates negative returns into
X86EMUL_UNHANDLEABLE, leading to handle_emulation_failure() which also forces
a userspace exit instead of propagating a graceful guest fault.

Can this allow a guest OS to trivially crash its VM by reading or writing an
unsupported or reserved MSR?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528113605.267111-1-jgross@suse.com?part=6

Re: [PATCH v2 0/6] KVM/x86: Drop "1" as MSR emulation return value

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1823 bytes --]

On 28.05.26 17:50, Jürgen Groß wrote:
> On 28.05.26 15:21, Sean Christopherson wrote:
>> On Thu, May 28, 2026, Jürgen Groß wrote:
>>> On 28.05.26 15:09, Sean Christopherson wrote:
>>>> On Thu, May 28, 2026, Juergen Gross wrote:
>>>>> Please disregard this series, there is one complication sashiko made me
>>>>> aware of.
>>>>
>>>> Sashiko beat me to the punch. :-)
>>>>
>>>> See commit 2368048bf5c2 ("KVM: x86: Signal #GP, not -EPERM, on bad 
>>>> WRMSR(MCi_CTL/STATUS)")
>>>> for a real world example of how things can and will go wrong.
>>>
>>> Yeah, with Sashiko's pointer it was easy to spot.
>>>
>>> Question now is whether the already existing cases of -errno passed as return
>>> value are wrong or on purpose.
>>
>> What are the existing cases?
>>
>>> If the latter, there should be a comment for
>>> that, otherwise they need to be fixed..
>>>
>>> Disentangling the MSR emulation return values from the "normal" ones ("return
>>> to guest"/"return to user mode") will be quite interesting with the overloaded
>>> semantics of "1".
>>
>> LOL, "interesting".
> 
> What do you think about the following idea:
> 
> Lets pass struct msr_info * down to all functions which get their return
> value passed up. Then extend msr_info with a bool "return_to_guest" (valid
> only if !host_initiated), which should be set instead of passing "1" up to
> the caller (probably using an inline helper). Then the return value could
> be 0 or -errno, and after MSR emulation the return_to_guest indicator can
> be tested if needed.

In the end I think it is less error prone to define a struct kvm_msr_ret_t
used as return type, consisting of an int and a bool with the same semantics
as above. Helpers will still be a good idea IMHO.

I'll have a try how it looks like.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]