linux-kernel - Re: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

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]

Date:   Wed, 3 May 2023 23:00:53 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "guoke@...ontech.com" <guoke@...ontech.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "haiwenyao@...ontech.com" <haiwenyao@...ontech.com>
Subject: Re: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR
 handler

On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> From: Wenyao Hai <haiwenyao@...ontech.com>
> 
> Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing
> through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr().

What's the value of doing so, besides saving a function of kvm_set_msr_common()?

PAT change shouldn't be something frequent so shouldn't in a performance
critical path.  Given the PAT logic on Intel and AMD are basically the same ,
isn't it better to do in kvm_set_msr_common()?

For instance, given mtrr code is also in common x86, if we ever want to add some
additional logic to, i.e. calculate effective memtype, isn't better to do handle
pat in common code too?

> 
> Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by
> 
> 	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
> 
> in kvm_set_msr_common().
> 
> Signed-off-by: Wenyao Hai <haiwenyao@...ontech.com>
> [sean: massage changelog]
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 44fb619803b8..53e249109483 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		    get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
>  			get_vmcs12(vcpu)->guest_ia32_pat = data;
>  
> -		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> +		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT)
>  			vmcs_write64(GUEST_IA32_PAT, data);
> -			vcpu->arch.pat = data;
> -			break;
> -		}
> -		ret = kvm_set_msr_common(vcpu, msr_info);
> +
> +		vcpu->arch.pat = data;
>  		break;
>  	case MSR_IA32_MCG_EXT_CTL:
>  		if ((!msr_info->host_initiated &&