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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 7 Jun 2024 10:10:28 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>
Cc: seanjc@...gle.com, kvm@...r.kernel.org, kai.huang@...el.com, 
	dmatlack@...gle.com, erdemaktas@...gle.com, isaku.yamahata@...il.com, 
	linux-kernel@...r.kernel.org, sagis@...gle.com, yan.y.zhao@...el.com, 
	Isaku Yamahata <isaku.yamahata@...el.com>
Subject: Re: [PATCH v2 08/15] KVM: x86/tdp_mmu: Introduce KVM MMU root types
 to specify page table type

On Thu, May 30, 2024 at 11:07 PM Rick Edgecombe
<rick.p.edgecombe@...el.com> 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
>