Received-SPF: pass (google.com: domain of linux-kernel+bounces-109441-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 20 Mar 2024 14:36:00 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
	Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
	Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, chen.bo@intel.com,
	hang.yuan@intel.com, tina.zhang@intel.com,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Binbin Wu <binbin.wu@linux.intel.com>,
	Yuan Yao <yuan.yao@intel.com>, isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for
 SEAMCALLs to the TDX module
Message-ID: <20240320213600.GI1994522@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <7cfd33d896fce7b49bcf4b7179d0ded22c06b8c2.1708933498.git.isaku.yamahata@intel.com>
 <579cb765-8a5e-4058-bc1d-9de7ac4b95d1@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <579cb765-8a5e-4058-bc1d-9de7ac4b95d1@intel.com>

On Wed, Mar 20, 2024 at 01:03:21PM +1300,
"Huang, Kai" <kai.huang@intel.com> wrote:

> > +static inline u64 tdx_seamcall(u64 op, struct tdx_module_args *in,
> > +			       struct tdx_module_args *out)
> > +{
> > +	u64 ret;
> > +
> > +	if (out) {
> > +		*out = *in;
> > +		ret = seamcall_ret(op, out);
> > +	} else
> > +		ret = seamcall(op, in);
> 
> I think it's silly to have the @out argument in this way.
> 
> What is the main reason to still have it?
> 
> Yeah we used to have the @out in __seamcall() assembly function.  The
> assembly code checks the @out and skips copying registers to @out when it is
> NULL.
> 
> But it got removed when we tried to unify the assembly for TDCALL/TDVMCALL
> and SEAMCALL to have a *SINGLE* assembly macro.
> 
> https://lore.kernel.org/lkml/cover.1692096753.git.kai.huang@intel.com/
> 
> To me that means we should just accept the fact we will always have a valid
> @out.
> 
> But there might be some case that you _obviously_ need the @out and I
> missed?

As I replied at [1], those four wrappers need to return values.
The first three on error, the last one on success.

  [1] https://lore.kernel.org/kvm/20240320202040.GH1994522@ls.amr.corp.intel.com/

  tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
  tdh_mem_page_aug(kvm_tdx, gpa, hpa, &entry, &level_state);
  tdh_mem_page_remove(kvm_tdx, gpa, tdx_level, &entry, &level_state);
  u64 tdh_vp_rd(struct vcpu_tdx *tdx, u64 field, u64 *value)

We can delete out from other wrappers.
Because only TDH.MNG.CREATE() and TDH.MNG.ADDCX() can return TDX_RND_NO_ENTROPY,
we can use __seamcall().  The TDX spec doesn't guarantee such error code
convention.  It's very unlikely, though.


> > +static inline u64 tdh_sys_lp_shutdown(void)
> > +{
> > +	struct tdx_module_args in = {
> > +	};
> > +
> > +	return tdx_seamcall(TDH_SYS_LP_SHUTDOWN, &in, NULL);
> > +}
> 
> As Sean already pointed out, I am sure it's/should not used in this series.
> 
> That being said, I found it's not easy to determine whether one wrapper will
> be used by this series or not.  The other option is we introduce the
> wrapper(s) when they get actally used, but I can see (especially at this
> stage) it's also a apple vs orange question that people may have different
> preference.
> 
> Perhaps we can say something like below in changelog ...
> 
> "
> Note, not all VM-managing related SEAMCALLs have a wrapper here, but only
> provide wrappers that are essential to the run the TDX guest with basic
> feature set.
> "
> 
> ... so that people will at least to pay attention to this during the review?

Makes sense. We can split this patch into other patches that first use the
wrappers.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>