diff mbox series

[v2,11/25] KVM: x86/mmu: remove kvm_calc_shadow_root_page_role_common

Message ID 20220221162243.683208-12-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series KVM MMU refactoring part 2: role changes | expand

Commit Message

Paolo Bonzini Feb. 21, 2022, 4:22 p.m. UTC
kvm_calc_shadow_root_page_role_common is the same as
kvm_calc_cpu_mode except for the level, which is overwritten
afterwards in kvm_calc_shadow_mmu_root_page_role
and kvm_calc_shadow_npt_root_page_role.

role.base.direct is already set correctly for the CPU mode,
and CR0.PG=1 is required for VMRUN so it will also be
correct for nested NPT.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu/mmu.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

Comments

Sean Christopherson March 8, 2022, 5:48 p.m. UTC | #1
On Mon, Feb 21, 2022, Paolo Bonzini wrote:
> kvm_calc_shadow_root_page_role_common is the same as
> kvm_calc_cpu_mode except for the level, which is overwritten
> afterwards in kvm_calc_shadow_mmu_root_page_role
> and kvm_calc_shadow_npt_root_page_role.
> 
> role.base.direct is already set correctly for the CPU mode,
> and CR0.PG=1 is required for VMRUN so it will also be
> correct for nested NPT.

Bzzzt, this is wrong, the nested NPT MMU is indirect but will be computed as direct.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 3ffa6f2bf991..31874fad12fb 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4796,27 +4796,11 @@ static void init_kvm_tdp_mmu(struct kvm_vcpu *vcpu,
>  	reset_tdp_shadow_zero_bits_mask(context);
>  }
>  
> -static union kvm_mmu_role
> -kvm_calc_shadow_root_page_role_common(struct kvm_vcpu *vcpu,
> -				      const struct kvm_mmu_role_regs *regs)
> -{
> -	union kvm_mmu_role role = kvm_calc_mmu_role_common(vcpu, regs);
> -
> -	role.base.smep_andnot_wp = role.ext.cr4_smep && !____is_cr0_wp(regs);
> -	role.base.smap_andnot_wp = role.ext.cr4_smap && !____is_cr0_wp(regs);
> -	role.base.has_4_byte_gpte = ____is_cr0_pg(regs) && !____is_cr4_pae(regs);
> -
> -	return role;
> -}
> -
>  static union kvm_mmu_role
>  kvm_calc_shadow_mmu_root_page_role(struct kvm_vcpu *vcpu,
>  				   const struct kvm_mmu_role_regs *regs)
>  {
> -	union kvm_mmu_role role =
> -		kvm_calc_shadow_root_page_role_common(vcpu, regs);
> -
> -	role.base.direct = !____is_cr0_pg(regs);
> +	union kvm_mmu_role role = kvm_calc_cpu_mode(vcpu, regs);
>  
>  	if (!____is_efer_lma(regs))
>  		role.base.level = PT32E_ROOT_LEVEL;
> @@ -4869,9 +4853,8 @@ kvm_calc_shadow_npt_root_page_role(struct kvm_vcpu *vcpu,
>  				   const struct kvm_mmu_role_regs *regs)
>  {
>  	union kvm_mmu_role role =
> -		kvm_calc_shadow_root_page_role_common(vcpu, regs);
> +               kvm_calc_cpu_mode(vcpu, regs);

No need to split this line with the less verbose name.

>  
> -	role.base.direct = false;

As above, this line needs to stay.

>  	role.base.level = kvm_mmu_get_tdp_level(vcpu);
>  
>  	return role;
> -- 
> 2.31.1
> 
>
Paolo Bonzini March 8, 2022, 5:50 p.m. UTC | #2
On 3/8/22 18:48, Sean Christopherson wrote:
> On Mon, Feb 21, 2022, Paolo Bonzini wrote:
>> kvm_calc_shadow_root_page_role_common is the same as
>> kvm_calc_cpu_mode except for the level, which is overwritten
>> afterwards in kvm_calc_shadow_mmu_root_page_role
>> and kvm_calc_shadow_npt_root_page_role.
>>
>> role.base.direct is already set correctly for the CPU mode,
>> and CR0.PG=1 is required for VMRUN so it will also be
>> correct for nested NPT.
> 
> Bzzzt, this is wrong, the nested NPT MMU is indirect but will be computed as direct.

CR0.PG=1 means it's *not* direct:

> +	role.base.direct = !____is_cr0_pg(regs);

Paolo
Sean Christopherson March 8, 2022, 6:17 p.m. UTC | #3
On Tue, Mar 08, 2022, Paolo Bonzini wrote:
> On 3/8/22 18:48, Sean Christopherson wrote:
> > On Mon, Feb 21, 2022, Paolo Bonzini wrote:
> > > kvm_calc_shadow_root_page_role_common is the same as
> > > kvm_calc_cpu_mode except for the level, which is overwritten
> > > afterwards in kvm_calc_shadow_mmu_root_page_role
> > > and kvm_calc_shadow_npt_root_page_role.
> > > 
> > > role.base.direct is already set correctly for the CPU mode,
> > > and CR0.PG=1 is required for VMRUN so it will also be
> > > correct for nested NPT.
> > 
> > Bzzzt, this is wrong, the nested NPT MMU is indirect but will be computed as direct.
> 
> CR0.PG=1 means it's *not* direct:
> 
> > +	role.base.direct = !____is_cr0_pg(regs);

Ha!  I was just cleverly making the case for checking ____is_cr0_pg() instead of
"direct" for computing the dependent flags, I swear...

On a serious note, can we add a WARN_ON_ONCE(role.base.direct)?  Not so much that
the WARN will be helpful, but to document the subtle dependency?  If the relevant
code goes away in the end, ignore this requrest.
Paolo Bonzini March 8, 2022, 6:18 p.m. UTC | #4
On 3/8/22 19:17, Sean Christopherson wrote:
>>> +	role.base.direct = !____is_cr0_pg(regs);
> 
> On a serious note, can we add a WARN_ON_ONCE(role.base.direct)?  Not so much that
> the WARN will be helpful, but to document the subtle dependency?  If the relevant
> code goes away in the end, ignore this requrest.

Ok, that can be done.  Either that or !is_cr0_pg().

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 3ffa6f2bf991..31874fad12fb 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4796,27 +4796,11 @@  static void init_kvm_tdp_mmu(struct kvm_vcpu *vcpu,
 	reset_tdp_shadow_zero_bits_mask(context);
 }
 
-static union kvm_mmu_role
-kvm_calc_shadow_root_page_role_common(struct kvm_vcpu *vcpu,
-				      const struct kvm_mmu_role_regs *regs)
-{
-	union kvm_mmu_role role = kvm_calc_mmu_role_common(vcpu, regs);
-
-	role.base.smep_andnot_wp = role.ext.cr4_smep && !____is_cr0_wp(regs);
-	role.base.smap_andnot_wp = role.ext.cr4_smap && !____is_cr0_wp(regs);
-	role.base.has_4_byte_gpte = ____is_cr0_pg(regs) && !____is_cr4_pae(regs);
-
-	return role;
-}
-
 static union kvm_mmu_role
 kvm_calc_shadow_mmu_root_page_role(struct kvm_vcpu *vcpu,
 				   const struct kvm_mmu_role_regs *regs)
 {
-	union kvm_mmu_role role =
-		kvm_calc_shadow_root_page_role_common(vcpu, regs);
-
-	role.base.direct = !____is_cr0_pg(regs);
+	union kvm_mmu_role role = kvm_calc_cpu_mode(vcpu, regs);
 
 	if (!____is_efer_lma(regs))
 		role.base.level = PT32E_ROOT_LEVEL;
@@ -4869,9 +4853,8 @@  kvm_calc_shadow_npt_root_page_role(struct kvm_vcpu *vcpu,
 				   const struct kvm_mmu_role_regs *regs)
 {
 	union kvm_mmu_role role =
-		kvm_calc_shadow_root_page_role_common(vcpu, regs);
+               kvm_calc_cpu_mode(vcpu, regs);
 
-	role.base.direct = false;
 	role.base.level = kvm_mmu_get_tdp_level(vcpu);
 
 	return role;