Re: [PATCH 09/15] KVM: SVM: Drop "always" flag from list of possible passthrough MSRs

[Tom Lendacky <thomas.lendacky@amd.com>]

On 11/27/24 14:19, Aaron Lewis wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/svm.c | 134 ++++++++++++++++++++---------------------
>  1 file changed, 67 insertions(+), 67 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 25d41709a0eaa..3813258497e49 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -81,51 +81,48 @@ static DEFINE_PER_CPU(u64, current_tsc_ratio);
>  
>  #define X2APIC_MSR(x)	(APIC_BASE_MSR + (x >> 4))
>  
> -static const struct svm_direct_access_msrs {
> -	u32 index;   /* Index of the MSR */
> -	bool always; /* True if intercept is initially cleared */
> -} direct_access_msrs[MAX_DIRECT_ACCESS_MSRS] = {
> -	{ .index = MSR_STAR,				.always = true  },
> -	{ .index = MSR_IA32_SYSENTER_CS,		.always = true  },
> -	{ .index = MSR_IA32_SYSENTER_EIP,		.always = false },
> -	{ .index = MSR_IA32_SYSENTER_ESP,		.always = false },
> +static const u32 direct_access_msrs[MAX_DIRECT_ACCESS_MSRS] = {
> +	MSR_STAR,
> +	MSR_IA32_SYSENTER_CS,
> +	MSR_IA32_SYSENTER_EIP,
> +	MSR_IA32_SYSENTER_ESP,
>  #ifdef CONFIG_X86_64
> -	{ .index = MSR_GS_BASE,				.always = true  },
> -	{ .index = MSR_FS_BASE,				.always = true  },
> -	{ .index = MSR_KERNEL_GS_BASE,			.always = true  },
> -	{ .index = MSR_LSTAR,				.always = true  },
> -	{ .index = MSR_CSTAR,				.always = true  },
> -	{ .index = MSR_SYSCALL_MASK,			.always = true  },
> +	MSR_GS_BASE,
> +	MSR_FS_BASE,
> +	MSR_KERNEL_GS_BASE,
> +	MSR_LSTAR,
> +	MSR_CSTAR,
> +	MSR_SYSCALL_MASK,
>  #endif
> -	{ .index = MSR_IA32_SPEC_CTRL,			.always = false },
> -	{ .index = MSR_IA32_PRED_CMD,			.always = false },
> -	{ .index = MSR_IA32_FLUSH_CMD,			.always = false },
> -	{ .index = MSR_IA32_DEBUGCTLMSR,		.always = false },
> -	{ .index = MSR_IA32_LASTBRANCHFROMIP,		.always = false },
> -	{ .index = MSR_IA32_LASTBRANCHTOIP,		.always = false },
> -	{ .index = MSR_IA32_LASTINTFROMIP,		.always = false },
> -	{ .index = MSR_IA32_LASTINTTOIP,		.always = false },
> -	{ .index = MSR_IA32_XSS,			.always = false },
> -	{ .index = MSR_EFER,				.always = false },
> -	{ .index = MSR_IA32_CR_PAT,			.always = false },
> -	{ .index = MSR_AMD64_SEV_ES_GHCB,		.always = false },
> -	{ .index = MSR_TSC_AUX,				.always = false },
> -	{ .index = X2APIC_MSR(APIC_ID),			.always = false },
> -	{ .index = X2APIC_MSR(APIC_LVR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_TASKPRI),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_ARBPRI),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_PROCPRI),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_EOI),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_RRR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_LDR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_DFR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_SPIV),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_ISR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_TMR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_IRR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_ESR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_ICR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_ICR2),		.always = false },
> +	MSR_IA32_SPEC_CTRL,
> +	MSR_IA32_PRED_CMD,
> +	MSR_IA32_FLUSH_CMD,
> +	MSR_IA32_DEBUGCTLMSR,
> +	MSR_IA32_LASTBRANCHFROMIP,
> +	MSR_IA32_LASTBRANCHTOIP,
> +	MSR_IA32_LASTINTFROMIP,
> +	MSR_IA32_LASTINTTOIP,
> +	MSR_IA32_XSS,
> +	MSR_EFER,
> +	MSR_IA32_CR_PAT,
> +	MSR_AMD64_SEV_ES_GHCB,
> +	MSR_TSC_AUX,
> +	X2APIC_MSR(APIC_ID),
> +	X2APIC_MSR(APIC_LVR),
> +	X2APIC_MSR(APIC_TASKPRI),
> +	X2APIC_MSR(APIC_ARBPRI),
> +	X2APIC_MSR(APIC_PROCPRI),
> +	X2APIC_MSR(APIC_EOI),
> +	X2APIC_MSR(APIC_RRR),
> +	X2APIC_MSR(APIC_LDR),
> +	X2APIC_MSR(APIC_DFR),
> +	X2APIC_MSR(APIC_SPIV),
> +	X2APIC_MSR(APIC_ISR),
> +	X2APIC_MSR(APIC_TMR),
> +	X2APIC_MSR(APIC_IRR),
> +	X2APIC_MSR(APIC_ESR),
> +	X2APIC_MSR(APIC_ICR),
> +	X2APIC_MSR(APIC_ICR2),
>  
>  	/*
>  	 * Note:
> @@ -134,15 +131,15 @@ static const struct svm_direct_access_msrs {
>  	 * the AVIC hardware would generate GP fault. Therefore, always
>  	 * intercept the MSR 0x832, and do not setup direct_access_msr.
>  	 */
> -	{ .index = X2APIC_MSR(APIC_LVTTHMR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_LVTPC),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_LVT0),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_LVT1),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_LVTERR),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_TMICT),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_TMCCT),		.always = false },
> -	{ .index = X2APIC_MSR(APIC_TDCR),		.always = false },
> -	{ .index = MSR_INVALID,				.always = false },
> +	X2APIC_MSR(APIC_LVTTHMR),
> +	X2APIC_MSR(APIC_LVTPC),
> +	X2APIC_MSR(APIC_LVT0),
> +	X2APIC_MSR(APIC_LVT1),
> +	X2APIC_MSR(APIC_LVTERR),
> +	X2APIC_MSR(APIC_TMICT),
> +	X2APIC_MSR(APIC_TMCCT),
> +	X2APIC_MSR(APIC_TDCR),
> +	MSR_INVALID,

By adding this there are two things being done in this patch. I think it
would be easier to see the changes related specifically to the "always"
flag being removed if the MSR_INVALID addition was a separate patch.

Thanks,
Tom

>  };
>  
>  /*
> @@ -763,9 +760,10 @@ static int direct_access_msr_slot(u32 msr)
>  {
>  	u32 i;
>  
> -	for (i = 0; direct_access_msrs[i].index != MSR_INVALID; i++)
> -		if (direct_access_msrs[i].index == msr)
> +	for (i = 0; direct_access_msrs[i] != MSR_INVALID; i++) {
> +		if (direct_access_msrs[i] == msr)
>  			return i;
> +	}
>  
>  	return -ENOENT;
>  }
> @@ -911,15 +909,17 @@ unsigned long *svm_vcpu_alloc_msrpm(void)
>  
>  void svm_vcpu_init_msrpm(struct kvm_vcpu *vcpu, unsigned long *msrpm)
>  {
> -	int i;
> -
> -	for (i = 0; direct_access_msrs[i].index != MSR_INVALID; i++) {
> -		if (!direct_access_msrs[i].always)
> -			continue;
> -		svm_disable_intercept_for_msr(vcpu, direct_access_msrs[i].index,
> -					      MSR_TYPE_RW);
> -	}
> +	svm_disable_intercept_for_msr(vcpu, MSR_STAR, MSR_TYPE_RW);
> +	svm_disable_intercept_for_msr(vcpu, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
>  
> +#ifdef CONFIG_X86_64
> +	svm_disable_intercept_for_msr(vcpu, MSR_GS_BASE, MSR_TYPE_RW);
> +	svm_disable_intercept_for_msr(vcpu, MSR_FS_BASE, MSR_TYPE_RW);
> +	svm_disable_intercept_for_msr(vcpu, MSR_KERNEL_GS_BASE, MSR_TYPE_RW);
> +	svm_disable_intercept_for_msr(vcpu, MSR_LSTAR, MSR_TYPE_RW);
> +	svm_disable_intercept_for_msr(vcpu, MSR_CSTAR, MSR_TYPE_RW);
> +	svm_disable_intercept_for_msr(vcpu, MSR_SYSCALL_MASK, MSR_TYPE_RW);
> +#endif
>  	if (sev_es_guest(vcpu->kvm))
>  		svm_disable_intercept_for_msr(vcpu, MSR_AMD64_SEV_ES_GHCB, MSR_TYPE_RW);
>  }
> @@ -935,7 +935,7 @@ void svm_set_x2apic_msr_interception(struct vcpu_svm *svm, bool intercept)
>  		return;
>  
>  	for (i = 0; i < MAX_DIRECT_ACCESS_MSRS; i++) {
> -		int index = direct_access_msrs[i].index;
> +		int index = direct_access_msrs[i];
>  
>  		if ((index < APIC_BASE_MSR) ||
>  		    (index > APIC_BASE_MSR + 0xff))
> @@ -965,8 +965,8 @@ static void svm_msr_filter_changed(struct kvm_vcpu *vcpu)
>  	 * refreshed since KVM is going to intercept them regardless of what
>  	 * userspace wants.
>  	 */
> -	for (i = 0; direct_access_msrs[i].index != MSR_INVALID; i++) {
> -		u32 msr = direct_access_msrs[i].index;
> +	for (i = 0; direct_access_msrs[i] != MSR_INVALID; i++) {
> +		u32 msr = direct_access_msrs[i];
>  
>  		if (!test_bit(i, svm->shadow_msr_intercept.read))
>  			svm_disable_intercept_for_msr(vcpu, msr, MSR_TYPE_R);
> @@ -1009,10 +1009,10 @@ static void init_msrpm_offsets(void)
>  
>  	memset(msrpm_offsets, 0xff, sizeof(msrpm_offsets));
>  
> -	for (i = 0; direct_access_msrs[i].index != MSR_INVALID; i++) {
> +	for (i = 0; direct_access_msrs[i] != MSR_INVALID; i++) {
>  		u32 offset;
>  
> -		offset = svm_msrpm_offset(direct_access_msrs[i].index);
> +		offset = svm_msrpm_offset(direct_access_msrs[i]);
>  		BUG_ON(offset == MSR_INVALID);
>  
>  		add_msr_offset(offset);