Re: [PATCH v2 16/25] KVM: x86/mmu: rename kvm_mmu_role union

From: Sean Christopherson
Date: Tue Mar 08 2022 - 14:15:48 EST


On Mon, Feb 21, 2022, Paolo Bonzini wrote:
> It is quite confusing that the "full" union is called kvm_mmu_role
> but is used for the "cpu_mode" field of struct kvm_mmu. Rename it
> to kvm_mmu_paging_mode.

My official vote is to use

union kvm_mmu_cpu_role cpu_role

instead of

union kvm_mmu_paging_mode cpu_mode

The latter has too many inconsistencies, e.g. helpers using "role" instead of "mode",
the union using "paging" but the field/params using "cpu". The cpu instead of paging
thing isn't a coincidence as having the param be "paging_mode" would be really, really
confusing (ditto for kvm_calc_paging_mode()).

IMO this is consistent, if imperfect.

static union kvm_mmu_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu,
const struct kvm_mmu_role_regs *regs)
{
union kvm_mmu_cpu_role role = {0};

role.base.access = ACC_ALL;
role.base.smm = is_smm(vcpu);
role.base.guest_mode = is_guest_mode(vcpu);
role.base.direct = !____is_cr0_pg(regs);

role.ext.valid = 1;

if (!____is_cr0_pg(regs))
return role;

...

return role;
}

Whereas this is inconsistent, and also imperfect.

static union kvm_mmu_paging_mode kvm_calc_cpu_mode(struct kvm_vcpu *vcpu,
const struct kvm_mmu_role_regs *regs)
{
union kvm_mmu_paging_mode role = {0};

role.base.access = ACC_ALL;
role.base.smm = is_smm(vcpu);
role.base.guest_mode = is_guest_mode(vcpu);
role.base.direct = !____is_cr0_pg(regs);

role.ext.valid = 1;

if (!____is_cr0_pg(regs))
return role;

...

return role;
}