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.
