Re: [PATCH v2 15/25] KVM: x86/mmu: remove extended bits from mmu_role, rename field

Sean Christopherson
2022-03-08T19:03:02Z

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@redhat.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;
}