Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 31 Mar 2023 15:44:32 +0300
From:   Zhi Wang <zhi.wang.linux@gmail.com>
To:     Isaku Yamahata <isaku.yamahata@gmail.com>
Cc:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        erdemaktas@google.com, Sean Christopherson <seanjc@google.com>,
        Sagi Shahar <sagis@google.com>,
        David Matlack <dmatlack@google.com>,
        Kai Huang <kai.huang@intel.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>
Subject: Re: [PATCH v13 016/113] KVM: TDX: x86: Add ioctl to get TDX
 systemwide parameters
Message-ID: <20230331154432.00001373@gmail.com>
In-Reply-To: <20230331001803.GE1112017@ls.amr.corp.intel.com>
References: <cover.1678643051.git.isaku.yamahata@intel.com>
        <cb0ae8b4941aaa45e1e5856dde644f9b2f53d9a6.1678643052.git.isaku.yamahata@intel.com>
        <20230325104306.00004585@gmail.com>
        <20230329231722.GA1108448@ls.amr.corp.intel.com>
        <20230331001803.GE1112017@ls.amr.corp.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 30 Mar 2023 17:18:03 -0700
Isaku Yamahata <isaku.yamahata@gmail.com> wrote:

> On Wed, Mar 29, 2023 at 04:17:22PM -0700,
> Isaku Yamahata <isaku.yamahata@gmail.com> wrote:
> 
> > On Sat, Mar 25, 2023 at 10:43:06AM +0200,
> > Zhi Wang <zhi.wang.linux@gmail.com> wrote:
> > 
> > > On Sun, 12 Mar 2023 10:55:40 -0700
> > > isaku.yamahata@intel.com wrote:
> > > 
> > > Does this have to be a new generic ioctl with a dedicated new x86_ops? SNP
> > > does not use it at all and all the system-scoped ioctl of SNP going through
> > > the CCP driver. So getting system-scope information of TDX/SNP will end up
> > > differently.
> > > 
> > > Any thought, Sean? Moving getting SNP system-wide information to
> > > KVM dev ioctl seems not ideal and TDX does not have a dedicated driver like
> > > CCP. Maybe make this ioctl TDX-specific? KVM_TDX_DEV_OP?
> > 
> > We only need global parameters of the TDX module, and we don't interact with TDX
> > module at this point.  One alternative is to export those parameters via sysfs.
> > Also the existence of the sysfs node indicates that the TDX module is
> > loaded(initialized?) or not in addition to boot log.  Thus we can drop system
> > scope one.
> > What do you think?
> >

I like this idea and the patch below, it feels right for me now. It would be nice
if more folks can chime in and comment.
 
> > Regarding to other TDX KVM specific ioctls (KVM_TDX_INIT_VM, KVM_TDX_INIT_VCPU,
> > KVM_TDX_INIT_MEM_REGION, and KVM_TDX_FINALIZE_VM), they are specific to KVM.  So
> > I don't think it can be split out to independent driver.
> 

They can stay in KVM as they are KVM-specific. SNP also has KVM-specific ioctls
which wraps the SEV driver calls. At this level, both TDX and SNP go their specific
implementation without more abstraction other than KVM_ENCRYPT_MEMORY_OP. Their
strategies are aligned.

The problem of the previous approach was the abstraction that no other implementation
is using it. It is like, TDX wants a higher abstraction to cover both TDX and SNP,
but SNP is not using it, which makes the abstraction looks strange.

> Here is the patch to export those info via sysfs.
> 
> From e0744e506eb92e47d8317e489945a3ba804edfa7 Mon Sep 17 00:00:00 2001
> Message-Id: <e0744e506eb92e47d8317e489945a3ba804edfa7.1680221730.git.isaku.yamahata@intel.com>
> In-Reply-To: <8e0bc0e8e5d435f54f10c7642a862629ef2acb89.1680221729.git.isaku.yamahata@intel.com>
> References: <8e0bc0e8e5d435f54f10c7642a862629ef2acb89.1680221729.git.isaku.yamahata@intel.com>
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> Date: Thu, 30 Mar 2023 00:05:03 -0700
> Subject: [PATCH] x86/virt/tdx: Export TD config params of TDX module via sysfs
> 
> TDX module has parameters for VMM to configure TD.  User space VMM, e.g.
> qemu, needs to know it. Export them to user space via sysfs.
> 
> TDX 1.0 provides TDH.SYS.INFO to provide system information in
> TDSYSINFO_STRUCT.  Its future extensibility is limited because of its
> struct.  From TDX 1.5, TDH.SYS.RD(metadata field_id) to read the info
> specified by field id.  So instead of exporting TDSYSINFO_STRUCT, adapt
> metadata way to export those system information.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-firmware-tdx |  23 +++
>  arch/x86/include/asm/tdx.h                   |  33 ++++
>  arch/x86/virt/vmx/tdx/tdx.c                  | 164 +++++++++++++++++++
>  arch/x86/virt/vmx/tdx/tdx.h                  |  18 ++
>  4 files changed, 238 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-tdx
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-tdx b/Documentation/ABI/testing/sysfs-firmware-tdx
> new file mode 100644
> index 000000000000..1f26fb178144
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-tdx
> @@ -0,0 +1,23 @@
> +What:           /sys/firmware/tdx/tdx_module/metadata
> +Date:           March 2023
> +KernelVersion:  6.3
> +Contact:        Isaku Yamahata <isaku.yamahata@intel.com>, kvm@vger.kernel.org
> +Users:          qemu, libvirt
> +Description:
> +                The TDX feature requires a firmware that is known as the TDX
> +                module.  The TDX module exposes its metadata in the following
> +                read-only files.  The information corresponds to the TDX global
> +                metadata specified by 64bit field id.  The file name is hex
> +                string in lower case.  The value is binary.
> +                User space VMM like qemu needs refer to them to determine what
> +                parameters are needed or allowed to configure guest TDs.
> +
> +                ================== ============================================
> +                1900000300000000   ATTRIBUTES_FIXED0
> +                1900000300000001   ATTRIBUTES_FIXED1
> +                1900000300000002   XFAM_FIXED0
> +                1900000300000003   XFAM_FIXED1
> +                9900000100000004   NUM_CPUID_CONFIG
> +                9900000300000400   CPUID_LEAVES
> +                9900000300000500   CPUID_VALUES
> +                ================== ============================================
> \ No newline at end of file
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 05870e5ed131..c650ac22a916 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -110,6 +110,39 @@ struct tdx_cpuid_config {
>  	u32	edx;
>  } __packed;
>  
> +struct tdx_cpuid_config_leaf {
> +	u32	leaf;
> +	u32	sub_leaf;
> +} __packed;
> +static_assert(offsetof(struct tdx_cpuid_config, leaf) ==
> +	      offsetof(struct tdx_cpuid_config_leaf, leaf));
> +static_assert(offsetof(struct tdx_cpuid_config, sub_leaf) ==
> +	      offsetof(struct tdx_cpuid_config_leaf, sub_leaf));
> +static_assert(offsetofend(struct tdx_cpuid_config, sub_leaf) ==
> +	      sizeof(struct tdx_cpuid_config_leaf));
> +
> +struct tdx_cpuid_config_value {
> +	u32	eax;
> +	u32	ebx;
> +	u32	ecx;
> +	u32	edx;
> +} __packed;
> +static_assert(offsetof(struct tdx_cpuid_config, eax) -
> +	      offsetof(struct tdx_cpuid_config, eax) ==
> +	      offsetof(struct tdx_cpuid_config_value, eax));
> +static_assert(offsetof(struct tdx_cpuid_config, ebx) -
> +	      offsetof(struct tdx_cpuid_config, eax) ==
> +	      offsetof(struct tdx_cpuid_config_value, ebx));
> +static_assert(offsetof(struct tdx_cpuid_config, ecx) -
> +	      offsetof(struct tdx_cpuid_config, eax) ==
> +	      offsetof(struct tdx_cpuid_config_value, ecx));
> +static_assert(offsetof(struct tdx_cpuid_config, edx) -
> +	      offsetof(struct tdx_cpuid_config, eax) ==
> +	      offsetof(struct tdx_cpuid_config_value, edx));
> +static_assert(offsetofend(struct tdx_cpuid_config, edx) -
> +	      offsetof(struct tdx_cpuid_config, eax) ==
> +	      sizeof(struct tdx_cpuid_config_value));
> +
>  #define TDSYSINFO_STRUCT_SIZE		1024
>  #define TDSYSINFO_STRUCT_ALIGNMENT	1024
>  
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index f9f9c1b76501..56ca520d67d6 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -33,6 +33,12 @@
>  #include <asm/tdx.h>
>  #include "tdx.h"
>  
> +#ifdef CONFIG_SYSFS
> +static int tdx_sysfs_init(void);
> +#else
> +static inline int tdx_sysfs_init(void) { return 0;}
> +#endif
> +
>  u32 tdx_global_keyid __ro_after_init;
>  EXPORT_SYMBOL_GPL(tdx_global_keyid);
>  static u32 tdx_guest_keyid_start __ro_after_init;
> @@ -399,6 +405,10 @@ static int __tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
>  	if (ret)
>  		return ret;
>  
> +	ret = tdx_sysfs_init();
> +	if (ret)
> +		return ret;
> +
>  	pr_info("TDX module: atributes 0x%x, vendor_id 0x%x, major_version %u, minor_version %u, build_date %u, build_num %u",
>  		sysinfo->attributes,	sysinfo->vendor_id,
>  		sysinfo->major_version, sysinfo->minor_version,
> @@ -1367,3 +1377,157 @@ int tdx_enable(void)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(tdx_enable);
> +
> +#ifdef CONFIG_SYSFS
> +
> +static struct kobject *tdx_kobj;
> +static struct kobject *tdx_module_kobj;
> +static struct kobject *tdx_metadata_kobj;
> +
> +#define TDX_METADATA_ATTR(_name, field_id_name, _size)		\
> +static struct bin_attribute tdx_metadata_ ## _name = {		\
> +	.attr = {						\
> +		.name = field_id_name,				\
> +		.mode = 0444,					\
> +	},							\
> +	.size = _size,						\
> +	.read = tdx_metadata_ ## _name ## _show,		\
> +}
> +
> +#define TDX_METADATA_ATTR_SHOW(_name, field_id_name)					\
> +static ssize_t tdx_metadata_ ## _name ## _show(struct file *filp, struct kobject *kobj,	\
> +					       struct bin_attribute *bin_attr,		\
> +					       char *buf, loff_t offset, size_t count)	\
> +{											\
> +	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);			\
> +											\
> +	return memory_read_from_buffer(buf, count, &offset,				\
> +				       &sysinfo->_name,					\
> +				       sizeof(sysinfo->_name));				\
> +}											\
> +TDX_METADATA_ATTR(_name, field_id_name, sizeof_field(struct tdsysinfo_struct, _name))
> +
> +TDX_METADATA_ATTR_SHOW(attributes_fixed0, TDX_METADATA_ATTRIBUTES_FIXED0_NAME);
> +TDX_METADATA_ATTR_SHOW(attributes_fixed1, TDX_METADATA_ATTRIBUTES_FIXED1_NAME);
> +TDX_METADATA_ATTR_SHOW(xfam_fixed0, TDX_METADATA_XFAM_FIXED0_NAME);
> +TDX_METADATA_ATTR_SHOW(xfam_fixed1, TDX_METADATA_XFAM_FIXED1_NAME);
> +
> +static ssize_t tdx_metadata_num_cpuid_config_show(struct file *filp, struct kobject *kobj,
> +						  struct bin_attribute *bin_attr,
> +						  char *buf, loff_t offset, size_t count)
> +{
> +	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
> +	/*
> +	 * Although tdsysinfo_struct.num_cpuid_config is defined as u32 for
> +	 * alignment, TDX 1.5 defines metadata NUM_CONFIG_CPUID as u16.
> +	 */
> +	u16 tmp = (u16)sysinfo->num_cpuid_config;
> +
> +	WARN_ON_ONCE(tmp != sysinfo->num_cpuid_config);
> +	return memory_read_from_buffer(buf, count, &offset, &tmp, sizeof(tmp));
> +}
> +TDX_METADATA_ATTR(num_cpuid_config, TDX_METADATA_NUM_CPUID_CONFIG_NAME, sizeof(u16));
> +
> +static ssize_t tdx_metadata_cpuid_leaves_show(struct file *filp, struct kobject *kobj,
> +					      struct bin_attribute *bin_attr, char *buf,
> +					      loff_t offset, size_t count)
> +{
> +	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
> +	ssize_t r;
> +	struct tdx_cpuid_config_leaf *tmp;
> +	u32 i;
> +
> +	tmp = kmalloc(bin_attr->size, GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < sysinfo->num_cpuid_config; i++) {
> +		struct tdx_cpuid_config *c = &sysinfo->cpuid_configs[i];
> +		struct tdx_cpuid_config_leaf *leaf = (struct tdx_cpuid_config_leaf *)c;
> +
> +		memcpy(tmp + i, leaf, sizeof(*leaf));
> +	}
> +
> +	r = memory_read_from_buffer(buf, count, &offset, tmp, bin_attr->size);
> +	kfree(tmp);
> +	return r;
> +}
> +
> +TDX_METADATA_ATTR(cpuid_leaves, TDX_METADATA_CPUID_LEAVES_NAME, 0);
> +
> +static ssize_t tdx_metadata_cpuid_values_show(struct file *filp, struct kobject *kobj,
> +					      struct bin_attribute *bin_attr, char *buf,
> +					      loff_t offset, size_t count)
> +{
> +	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
> +	struct tdx_cpuid_config_value *tmp;
> +	ssize_t r;
> +	u32 i;
> +
> +	tmp = kmalloc(bin_attr->size, GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < sysinfo->num_cpuid_config; i++) {
> +		struct tdx_cpuid_config *c = &sysinfo->cpuid_configs[i];
> +		struct tdx_cpuid_config_value *value = (struct tdx_cpuid_config_value *)&c->eax;
> +
> +		memcpy(tmp + i, value, sizeof(*value));
> +	}
> +
> +	r = memory_read_from_buffer(buf, count, &offset, tmp, bin_attr->size);
> +	kfree(tmp);
> +	return r;
> +}
> +
> +TDX_METADATA_ATTR(cpuid_values, TDX_METADATA_CPUID_VALUES_NAME, 0);
> +
> +static struct bin_attribute *tdx_metadata_attrs[] = {
> +	&tdx_metadata_attributes_fixed0,
> +	&tdx_metadata_attributes_fixed1,
> +	&tdx_metadata_xfam_fixed0,
> +	&tdx_metadata_xfam_fixed1,
> +	&tdx_metadata_num_cpuid_config,
> +	&tdx_metadata_cpuid_leaves,
> +	&tdx_metadata_cpuid_values,
> +	NULL,
> +};
> +
> +static const struct attribute_group tdx_metadata_attr_group = {
> +	.bin_attrs = tdx_metadata_attrs,
> +};
> +
> +static int tdx_sysfs_init(void)
> +{
> +	struct tdsysinfo_struct *sysinfo;
> +	int ret;
> +
> +	tdx_kobj = kobject_create_and_add("tdx", firmware_kobj);
> +	if (!tdx_kobj) {
> +		pr_err("kobject_create_and_add tdx failed\n");
> +		return -EINVAL;
> +	}
> +
> +	tdx_module_kobj = kobject_create_and_add("tdx_module", tdx_kobj);
> +	if (!tdx_module_kobj) {
> +		pr_err("kobject_create_and_add tdx_module failed\n");
> +		return -EINVAL;
> +	}
> +	tdx_metadata_kobj = kobject_create_and_add("metadata", tdx_module_kobj);
> +	if (!tdx_metadata_kobj) {
> +		pr_err("Sysfs exporting tdx global metadata failed %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	sysinfo = &PADDED_STRUCT(tdsysinfo);
> +	tdx_metadata_cpuid_leaves.size = sysinfo->num_cpuid_config *
> +		sizeof(struct tdx_cpuid_config_leaf);
> +	tdx_metadata_cpuid_values.size = sysinfo->num_cpuid_config *
> +		sizeof(struct tdx_cpuid_config_value);
> +	ret = sysfs_create_group(tdx_metadata_kobj, &tdx_metadata_attr_group);
> +	if (ret)
> +		pr_err("Sysfs exporting tdx module attributes failed %d\n", ret);
> +
> +	return ret;
> +}
> +#endif
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index db0cbcceb5b3..a48f38fe6cc4 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -93,4 +93,22 @@ struct tdmr_info_list {
>  	int max_tdmrs;	/* How many 'tdmr_info's are allocated */
>  };
>  
> +/* TDX metadata base field id. */
> +#define TDX_METADATA_ATTRIBUTES_FIXED0		0x1900000300000000ULL
> +#define TDX_METADATA_ATTRIBUTES_FIXED1		0x1900000300000001ULL
> +#define TDX_METADATA_XFAM_FIXED0		0x1900000300000002ULL
> +#define TDX_METADATA_XFAM_FIXED1		0x1900000300000003ULL
> +#define TDX_METADATA_NUM_CPUID_CONFIG		0x9900000100000004ULL
> +#define TDX_METADATA_CPUID_LEAVES		0x9900000300000400ULL
> +#define TDX_METADATA_CPUID_VALUES		0x9900000300000500ULL
> +
> +/* File name for sysfs: hex with lower case. */
> +#define TDX_METADATA_ATTRIBUTES_FIXED0_NAME	"1900000300000000"
> +#define TDX_METADATA_ATTRIBUTES_FIXED1_NAME	"1900000300000001"
> +#define TDX_METADATA_XFAM_FIXED0_NAME		"1900000300000002"
> +#define TDX_METADATA_XFAM_FIXED1_NAME		"1900000300000003"
> +#define TDX_METADATA_NUM_CPUID_CONFIG_NAME	"9900000100000004"
> +#define TDX_METADATA_CPUID_LEAVES_NAME		"9900000300000400"
> +#define TDX_METADATA_CPUID_VALUES_NAME		"9900000300000500"
> +
>  #endif