Re: [PATCH v2 08/15] KVM: x86/tdp_mmu: Introduce KVM MMU root types to specify page table type

[Edgecombe, Rick P]

On Fri, 2024-06-07 at 10:10 +0200, Paolo Bonzini wrote:
> On Thu, May 30, 2024 at 11:07 PM Rick Edgecombe
> <rick.p.edgecombe@xxxxxxxxx> wrote:
> > +enum kvm_tdp_mmu_root_types {
> > +       KVM_VALID_ROOTS = BIT(0),
> > +
> > +       KVM_ANY_ROOTS = 0,
> > +       KVM_ANY_VALID_ROOTS = KVM_VALID_ROOTS,
> 
> I would instead define it as
> 
>     KVM_INVALID_ROOTS = BIT(0),
>     KVM_VALID_ROOTS = BIT(1),
>     KVM_ALL_ROOTS = KVM_VALID_ROOTS | KVM_INVALID_ROOTS,
> 
> and then
> 
>   if (root->role.invalid)
>     return types & KVM_INVALID_ROOTS;
>   else
>     return types & KVM_VALID_ROOTS;
> 
> Then in the next patch you can do
> 
>      KVM_INVALID_ROOTS = BIT(0),
> -    KVM_VALID_ROOTS = BIT(1),
> +   KVM_DIRECT_ROOTS = BIT(1),
> +   KVM_MIRROR_ROOTS = BIT(2),
> +   KVM_VALID_ROOTS = KVM_DIRECT_ROOTS | KVM_MIRROR_ROOTS,
>      KVM_ALL_ROOTS = KVM_VALID_ROOTS | KVM_INVALID_ROOTS,
> 
> and likewise
> 
>   if (root->role.invalid)
>     return types & KVM_INVALID_ROOTS;
>   else if (likely(!is_mirror_sp(root)))
>     return types & KVM_DIRECT_ROOTS;
>   else
>     return types & KVM_MIRROR_ROOTS;
> 
> This removes the need for KVM_ANY_VALID_ROOTS (btw I don't know if
> it's me, but ALL sounds more grammatical than ANY in this context). So
> the resulting code should be a bit clearer.
> 
> Apart from this small tweak, the overall idea is really good.

Yes, this makes more sense. Thanks.