linux-kernel - Re: [RFC PATCH v2 57/69] KVM: VMX: Move register caching logic to common code

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 6 Jul 2021 16:44:46 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     isaku.yamahata@...el.com, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, erdemaktas@...gle.com,
        Connor Kuehl <ckuehl@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc:     isaku.yamahata@...il.com,
        Sean Christopherson <sean.j.christopherson@...el.com>
Subject: Re: [RFC PATCH v2 57/69] KVM: VMX: Move register caching logic to
 common code

On 03/07/21 00:05, isaku.yamahata@...el.com wrote:
> From: Sean Christopherson <sean.j.christopherson@...el.com>
> 
> Move the guts of vmx_cache_reg() to vt_cache_reg() in preparation for
> reusing the bulk of the code for TDX, which can access guest state for
> debug TDs.
> 
> Use kvm_x86_ops.cache_reg() in ept_update_paging_mode_cr0() rather than
> trying to expose vt_cache_reg() to vmx.c, even though it means taking a
> retpoline.  The code runs if and only if EPT is enabled but unrestricted
> guest.  Only one generation of CPU, Nehalem, supports EPT but not
> unrestricted guest, and disabling unrestricted guest without also
> disabling EPT is, to put it bluntly, dumb.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---
>   arch/x86/kvm/vmx/main.c | 37 +++++++++++++++++++++++++++++++++++-
>   arch/x86/kvm/vmx/vmx.c  | 42 +----------------------------------------
>   2 files changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 0d8d2a0a2979..b619615f77de 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -341,7 +341,42 @@ static void vt_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
>   
>   static void vt_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
>   {
> -	vmx_cache_reg(vcpu, reg);
> +	unsigned long guest_owned_bits;
> +
> +	kvm_register_mark_available(vcpu, reg);
> +
> +	switch (reg) {
> +	case VCPU_REGS_RSP:
> +		vcpu->arch.regs[VCPU_REGS_RSP] = vmcs_readl(GUEST_RSP);
> +		break;
> +	case VCPU_REGS_RIP:
> +		vcpu->arch.regs[VCPU_REGS_RIP] = vmcs_readl(GUEST_RIP);
> +		break;
> +	case VCPU_EXREG_PDPTR:
> +		if (enable_ept)
> +			ept_save_pdptrs(vcpu);
> +		break;
> +	case VCPU_EXREG_CR0:
> +		guest_owned_bits = vcpu->arch.cr0_guest_owned_bits;
> +
> +		vcpu->arch.cr0 &= ~guest_owned_bits;
> +		vcpu->arch.cr0 |= vmcs_readl(GUEST_CR0) & guest_owned_bits;
> +		break;
> +	case VCPU_EXREG_CR3:
> +		if (is_unrestricted_guest(vcpu) ||
> +		    (enable_ept && is_paging(vcpu)))
> +			vcpu->arch.cr3 = vmcs_readl(GUEST_CR3);
> +		break;
> +	case VCPU_EXREG_CR4:
> +		guest_owned_bits = vcpu->arch.cr4_guest_owned_bits;
> +
> +		vcpu->arch.cr4 &= ~guest_owned_bits;
> +		vcpu->arch.cr4 |= vmcs_readl(GUEST_CR4) & guest_owned_bits;
> +		break;
> +	default:
> +		KVM_BUG_ON(1, vcpu->kvm);
> +		break;
> +	}
>   }
>   
>   static unsigned long vt_get_rflags(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index e315a46d1566..3c3bfc80d2bb 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2326,46 +2326,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   	return ret;
>   }
>   
> -static void vmx_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
> -{
> -	unsigned long guest_owned_bits;
> -
> -	kvm_register_mark_available(vcpu, reg);
> -
> -	switch (reg) {
> -	case VCPU_REGS_RSP:
> -		vcpu->arch.regs[VCPU_REGS_RSP] = vmcs_readl(GUEST_RSP);
> -		break;
> -	case VCPU_REGS_RIP:
> -		vcpu->arch.regs[VCPU_REGS_RIP] = vmcs_readl(GUEST_RIP);
> -		break;
> -	case VCPU_EXREG_PDPTR:
> -		if (enable_ept)
> -			ept_save_pdptrs(vcpu);
> -		break;
> -	case VCPU_EXREG_CR0:
> -		guest_owned_bits = vcpu->arch.cr0_guest_owned_bits;
> -
> -		vcpu->arch.cr0 &= ~guest_owned_bits;
> -		vcpu->arch.cr0 |= vmcs_readl(GUEST_CR0) & guest_owned_bits;
> -		break;
> -	case VCPU_EXREG_CR3:
> -		if (is_unrestricted_guest(vcpu) ||
> -		    (enable_ept && is_paging(vcpu)))
> -			vcpu->arch.cr3 = vmcs_readl(GUEST_CR3);
> -		break;
> -	case VCPU_EXREG_CR4:
> -		guest_owned_bits = vcpu->arch.cr4_guest_owned_bits;
> -
> -		vcpu->arch.cr4 &= ~guest_owned_bits;
> -		vcpu->arch.cr4 |= vmcs_readl(GUEST_CR4) & guest_owned_bits;
> -		break;
> -	default:
> -		KVM_BUG_ON(1, vcpu->kvm);
> -		break;
> -	}
> -}
> -
>   static __init int vmx_disabled_by_bios(void)
>   {
>   	return !boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) ||
> @@ -3066,7 +3026,7 @@ static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>   
>   	if (!kvm_register_is_available(vcpu, VCPU_EXREG_CR3))
> -		vmx_cache_reg(vcpu, VCPU_EXREG_CR3);
> +		kvm_x86_ops.cache_reg(vcpu, VCPU_EXREG_CR3);
>   	if (!(cr0 & X86_CR0_PG)) {
>   		/* From paging/starting to nonpaging */
>   		exec_controls_setbit(vmx, CPU_BASED_CR3_LOAD_EXITING |
> 

This shows the problem with #including vmx.c.  You should have a .h file 
for both vmx.h and main.h (e.g. kvm_intel.h), so that here you can just 
use vt_cache_reg.

Paolo