linux-kernel - Re: [PATCH v2 15/25] KVM: x86/mmu: remove extended bits from mmu

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: <YieoXYBFyo9pZhhX@google.com>
Date:   Tue, 8 Mar 2022 19:02:53 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        dmatlack@...gle.com
Subject: Re: [PATCH v2 15/25] KVM: x86/mmu: remove extended bits from
 mmu_role, rename field

On Mon, Feb 21, 2022, Paolo Bonzini wrote:
> mmu_role represents the role of the root of the page tables.
> It does not need any extended bits, as those govern only KVM's
> page table walking; the is_* functions used for page table
> walking always use the CPU mode.
> 
> ext.valid is not present anymore in the MMU role, but an
> all-zero MMU role is impossible because the level field is
> never zero in the MMU role.

Probably worth adding a blurb to clarify that the level is zero in the extended
(guest) role if paging is disabled in the guest?

> So just zap the whole mmu_role
> in order to force invalidation after CPUID is updated.
> 
> While making this change, which requires touching almost every
> occurrence of "mmu_role", rename it to "root_role".

Heh, can you please wrap closer to 75 chars?  Your d20 rolls came up low again.

> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
> @@ -4764,7 +4763,7 @@ static void init_kvm_tdp_mmu(struct kvm_vcpu *vcpu,
>  	reset_tdp_shadow_zero_bits_mask(context);
>  }
>  
> -static union kvm_mmu_role
> +static union kvm_mmu_page_role
>  kvm_calc_shadow_mmu_root_page_role(struct kvm_vcpu *vcpu,
>  				   union kvm_mmu_role role)
>  {
> @@ -4785,19 +4784,19 @@ kvm_calc_shadow_mmu_root_page_role(struct kvm_vcpu *vcpu,
>  	 * MMU contexts.
>  	 */
>  	role.base.efer_nx = true;
> -	return role;
> +	return role.base;

For both cases where the root role is derived from the cpu_role, can we make
an explicit copy of the "base" as root_role, and then do mods on that?  Doing
modification on the incoming role is unnecessarily hard to read IMO, I doubt the
compiler even generates different code.

For me, this makes it more obvious that the root_role is derived from the cpu_role.

static union kvm_mmu_page_role
kvm_calc_shadow_mmu_root_page_role(struct kvm_vcpu *vcpu,
				   const union kvm_mmu_role cpu_role)
{
	union kvm_mmu_page_role root_role = cpu_role.base;

	if (!cpu_role.ext.efer_lma)
		root_role.level = PT32E_ROOT_LEVEL;
	else if (cpu_role.ext.cr4_la57)
		root_role.level = PT64_ROOT_5LEVEL;
	else
		root_role.level = PT64_ROOT_4LEVEL;

	/*
	 * KVM forces EFER.NX=1 when TDP is disabled, reflect it in the MMU role.
	 * KVM uses NX when TDP is disabled to handle a variety of scenarios,
	 * notably for huge SPTEs if iTLB multi-hit mitigation is enabled and
	 * to generate correct permissions for CR0.WP=0/CR4.SMEP=1/EFER.NX=0.
	 * The iTLB multi-hit workaround can be toggled at any time, so assume
	 * NX can be used by any non-nested shadow MMU to avoid having to reset
	 * MMU contexts.
	 */
	root_role.efer_nx = true;
	return root_role;
}

static union kvm_mmu_page_role
kvm_calc_shadow_npt_root_page_role(struct kvm_vcpu *vcpu,
				   const union kvm_mmu_role cpu_role)
{
	union kvm_mmu_page_role root_role = cpu_role.base;

	WARN_ON_ONCE(root_role.direct);
	root_role.level = kvm_mmu_get_tdp_level(vcpu);
	return root_role;
}