Date:   Mon, 27 Feb 2023 15:30:56 -0800
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     "Huang, Kai" <kai.huang@intel.com>
Cc:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "Shahar, Sagi" <sagis@google.com>,
        "Aktas, Erdem" <erdemaktas@google.com>,
        "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
        "dmatlack@google.com" <dmatlack@google.com>,
        "Christopherson,, Sean" <seanjc@google.com>
Subject: Re: [PATCH v11 058/113] KVM: TDX: MTRR: implement get_mt_mask() for
 TDX
Message-ID: <20230227233056.GT4175971@ls.amr.corp.intel.com>
References: <cover.1673539699.git.isaku.yamahata@intel.com>
 <21e0d94ccf2fd3d766d6aa7b45441791c04e5e4f.1673539699.git.isaku.yamahata@intel.com>
 <1a32f2570f13fa0125ddbbceeabcd36fc4beb9db.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1a32f2570f13fa0125ddbbceeabcd36fc4beb9db.camel@intel.com>
Precedence: bulk

On Tue, Jan 17, 2023 at 03:11:46AM +0000,
"Huang, Kai" <kai.huang@intel.com> wrote:

> On Thu, 2023-01-12 at 08:32 -0800, isaku.yamahata@intel.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > Although TDX supports only WB for private GPA, MTRR/PAT for shared GPA
> > should be supported. Implement get_mt_mask() following vmx case.
> 
> By far this is the first patch to handle MTRR/PAT.  There's absolutely no
> background have been explained.
> 
> So what about MTRR/PAT related MSRs handling?  No code needed to handle?
> 
> I was expecting there should be at least some words here to explain how TDX
> handles them, and if no handling is required in KVM, why.
> 
> W/o those, I don't think this patch is reviewable.  

I've updated the commit message.


> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  arch/x86/kvm/vmx/main.c    | 10 +++++++++-
> >  arch/x86/kvm/vmx/tdx.c     | 19 +++++++++++++++++++
> >  arch/x86/kvm/vmx/x86_ops.h |  2 ++
> >  3 files changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > index 770d1b29d1c3..4319f6d7a4da 100644
> > --- a/arch/x86/kvm/vmx/main.c
> > +++ b/arch/x86/kvm/vmx/main.c
> > @@ -158,6 +158,14 @@ static void vt_load_mmu_pgd(struct kvm_vcpu *vcpu, hpa_t root_hpa,
> >  	vmx_load_mmu_pgd(vcpu, root_hpa, pgd_level);
> >  }
> >  
> > +static u8 vt_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> > +{
> > +	if (is_td_vcpu(vcpu))
> > +		return tdx_get_mt_mask(vcpu, gfn, is_mmio);
> > +
> > +	return vmx_get_mt_mask(vcpu, gfn, is_mmio);
> > +}
> > +
> >  static int vt_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> >  {
> >  	if (!is_td(kvm))
> > @@ -267,7 +275,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
> >  
> >  	.set_tss_addr = vmx_set_tss_addr,
> >  	.set_identity_map_addr = vmx_set_identity_map_addr,
> > -	.get_mt_mask = vmx_get_mt_mask,
> > +	.get_mt_mask = vt_get_mt_mask,
> >  
> >  	.get_exit_info = vmx_get_exit_info,
> >  
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index e68816999387..c4c5a8f786c1 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -309,6 +309,25 @@ int tdx_vm_init(struct kvm *kvm)
> >  	return 0;
> >  }
> >  
> > +u8 tdx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> > +{
> > +	/* TDX private GPA is always WB. */
> > +	if (gfn & kvm_gfn_shared_mask(vcpu->kvm)) {
> 
> First of all, private GPA doesn't have 'shared bit' set, so comment  doesn't
> reflect code.
> 
> Secondly (and again), IIUC the shared bit of the gfn has been stripped out long
> time ago, so this is incorrect.
> 
> Please don't sliently ignore other people's comment:
> 
> https://lore.kernel.org/lkml/Y19NzlQcwhV%2F2wl3@debian.me/T/#mf319d5b718519709362f9f094bfc5b53fd870241

Make the logic common for vmx and tdx. the difference is to check cr0.cd.

Thanks,
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>