linux-kernel - Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6a831f1b429ab6d1dd3e60a947cab7ab0b3f7149.camel@intel.com>
Date: Wed, 29 May 2024 02:13:24 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Yamahata, Isaku" <isaku.yamahata@...el.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "sagis@...gle.com"
	<sagis@...gle.com>, "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"Zhao, Yan Y" <yan.y.zhao@...el.com>, "Aktas, Erdem" <erdemaktas@...gle.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "dmatlack@...gle.com" <dmatlack@...gle.com>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU

On Tue, 2024-05-28 at 18:57 -0700, Isaku Yamahata wrote:
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -438,6 +438,9 @@ static void handle_removed_pt(struct kvm *kvm,
> > tdp_ptep_t
> > pt, bool shared)
> >                           */
> >                          old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte,
> >                                                            REMOVED_SPTE,
> > level);
> > +
> > +                       if (is_mirror_sp(sp))
> > +                               reflect_removed_spte(kvm, gfn, old_spte,
> > REMOVED_SPTE, level);
> 
> The callback before handling lower level will result in error.

Hmm, yea the order is changed. It didn't result in an error for some reason
though. Can you elaborate?

> 
> 
> >                  }
> >                  handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), gfn,
> >                                      old_spte, REMOVED_SPTE, sp->role,
> > shared);
> 
> 
> We should call it here after processing lower level.
> 
> 
> 
> > @@ -667,9 +670,6 @@ static void handle_changed_spte(struct kvm *kvm, int
> > as_id,
> > gfn_t gfn,
> >                  handle_removed_pt(kvm, spte_to_child_pt(old_spte, level),
> > shared);
> >          }
> >   
> > -       if (is_mirror && !is_present)
> > -               reflect_removed_spte(kvm, gfn, old_spte, new_spte,
> > role.level);
> > -
> >          if (was_leaf && is_accessed_spte(old_spte) &&
> >              (!is_present || !is_accessed_spte(new_spte) || pfn_changed))
> >                  kvm_set_pfn_accessed(spte_to_pfn(old_spte));
> > @@ -839,6 +839,9 @@ static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_id,
> > tdp_ptep_t sptep,
> >                                                        new_spte, level),
> > kvm);
> >          }
> >   
> > +       if (is_mirror_sptep(sptep))
> > +               reflect_removed_spte(kvm, gfn, old_spte, REMOVED_SPTE,
> > level);
> > +
> 
> Ditto.
> 
> 
> >          role = sptep_to_sp(sptep)->role;
> >          role.level = level;
> >          handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, role,
> > false);
> 
> The callback should be here.  It should be after handling the lower level.

Ok, let me try.

> 
> 
> 
> > Otherwise, we could move the "set present" mirroring operations into
> > handle_changed_spte(), and have some earlier conditional logic do the
> > REMOVED_SPTE parts. It starts to become more scattered.
> > Anyway, it's just a code clarity thing arising from having hard time
> > explaining
> > the design in the log. Any opinions?
> 
> Originally I tried to consolidate the callbacks by following TDP MMU using
> handle_changed_spte().

How did it handle the REMOVED_SPTE part of the set_present() path?

>   Anyway we can pick from two outcomes based on which is
> easy to understand/maintain.

I guess I can try to generate a diff of the other one and we can compare. It's a
matter of opinion, but I think splitting it between the two methods is the most
confusing.