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

[Paolo Bonzini]

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.

Paolo

> +};
> +
>  bool kvm_tdp_mmu_zap_leafs(struct kvm *kvm, gfn_t start, gfn_t end, bool flush);
>  bool kvm_tdp_mmu_zap_sp(struct kvm *kvm, struct kvm_mmu_page *sp);
>  void kvm_tdp_mmu_zap_all(struct kvm *kvm);
> --
> 2.34.1
>