linux-kernel - Re: [PATCH v19 111/130] KVM: TDX: Implement callbacks for MSR operations for TDX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zg1yPIV6cVJrwGxX@google.com>
Date: Wed, 3 Apr 2024 08:14:04 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: isaku.yamahata@...el.com
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com, 
	Sagi Shahar <sagis@...gle.com>, Kai Huang <kai.huang@...el.com>, chen.bo@...el.com, 
	hang.yuan@...el.com, tina.zhang@...el.com
Subject: Re: [PATCH v19 111/130] KVM: TDX: Implement callbacks for MSR
 operations for TDX

On Mon, Feb 26, 2024, isaku.yamahata@...el.com wrote:
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 389bb95d2af0..c8f991b69720 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1877,6 +1877,76 @@ void tdx_get_exit_info(struct kvm_vcpu *vcpu, u32 *reason,
>  	*error_code = 0;
>  }
>  
> +static bool tdx_is_emulated_kvm_msr(u32 index, bool write)
> +{
> +	switch (index) {
> +	case MSR_KVM_POLL_CONTROL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +bool tdx_has_emulated_msr(u32 index, bool write)
> +{
> +	switch (index) {
> +	case MSR_IA32_UCODE_REV:
> +	case MSR_IA32_ARCH_CAPABILITIES:
> +	case MSR_IA32_POWER_CTL:
> +	case MSR_IA32_CR_PAT:
> +	case MSR_IA32_TSC_DEADLINE:
> +	case MSR_IA32_MISC_ENABLE:
> +	case MSR_PLATFORM_INFO:
> +	case MSR_MISC_FEATURES_ENABLES:
> +	case MSR_IA32_MCG_CAP:
> +	case MSR_IA32_MCG_STATUS:
> +	case MSR_IA32_MCG_CTL:
> +	case MSR_IA32_MCG_EXT_CTL:
> +	case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
> +	case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
> +		/* MSR_IA32_MCx_{CTL, STATUS, ADDR, MISC, CTL2} */
> +		return true;
> +	case APIC_BASE_MSR ... APIC_BASE_MSR + 0xff:
> +		/*
> +		 * x2APIC registers that are virtualized by the CPU can't be
> +		 * emulated, KVM doesn't have access to the virtual APIC page.
> +		 */
> +		switch (index) {
> +		case X2APIC_MSR(APIC_TASKPRI):
> +		case X2APIC_MSR(APIC_PROCPRI):
> +		case X2APIC_MSR(APIC_EOI):
> +		case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR + APIC_ISR_NR):
> +		case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR + APIC_ISR_NR):
> +		case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR + APIC_ISR_NR):
> +			return false;
> +		default:
> +			return true;
> +		}
> +	case MSR_IA32_APICBASE:
> +	case MSR_EFER:
> +		return !write;

Meh, for literally two MSRs, just open code them in tdx_set_msr() and drop the
@write param.  Or alternatively add:

static bool tdx_is_read_only_msr(u32 msr){
{
	return msr == MSR_IA32_APICBASE || msr == MSR_EFER;
}

> +	case 0x4b564d00 ... 0x4b564dff:

This is silly, just do

	case MSR_KVM_POLL_CONTROL:
		return false;

and let everything else go through the default statement, no?

> +		/* KVM custom MSRs */
> +		return tdx_is_emulated_kvm_msr(index, write);
> +	default:
> +		return false;
> +	}
> +}
> +
> +int tdx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> +{
> +	if (tdx_has_emulated_msr(msr->index, false))
> +		return kvm_get_msr_common(vcpu, msr);
> +	return 1;

Please invert these and make the happy path the not-taken path, i.e.

	if (!tdx_has_emulated_msr(msr->index))
		return 1;

	return kvm_get_msr_common(vcpu, msr);

The standard kernel pattern is

	if (error)
		return <error thingie>

	return <happy thingie>

> +}
> +
> +int tdx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> +{
> +	if (tdx_has_emulated_msr(msr->index, true))

As above:

	if (tdx_is_read_only_msr(msr->index))
		return 1;

	if (!tdx_has_emulated_msr(msr->index))
		return 1;

	return kvm_set_msr_common(vcpu, msr);

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d5b18cad9dcd..0e1d3853eeb4 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -90,7 +90,6 @@
>  #include "trace.h"
>  
>  #define MAX_IO_MSRS 256
> -#define KVM_MAX_MCE_BANKS 32
>  
>  struct kvm_caps kvm_caps __read_mostly = {
>  	.supported_mce_cap = MCG_CTL_P | MCG_SER_P,
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 4e40c23d66ed..c87b7a777b67 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -9,6 +9,8 @@
>  #include "kvm_cache_regs.h"
>  #include "kvm_emulate.h"
>  
> +#define KVM_MAX_MCE_BANKS 32

Split this to a separate.  Yes, it's trivial, but that's _exactly_ why it should
be in a separate patch.  The more trivial refactoring you split out, the more we
can apply _now_ and take off your hands.