On Tue, Nov 08, 2022 at 01:29:46AM +0000,
"Huang, Kai" <kai.huang@xxxxxxxxx> wrote:
>
> > +
> > +#define TDX_MAX_NR_CPUID_CONFIGS \
> > + ((sizeof(struct tdsysinfo_struct) - \
> > + offsetof(struct tdsysinfo_struct, cpuid_configs)) \
> > + / sizeof(struct tdx_cpuid_config))
> > +
> > +struct tdx_capabilities {
> > + u8 tdcs_nr_pages;
> > + u8 tdvpx_nr_pages;
> > +
> > + u64 attrs_fixed0;
> > + u64 attrs_fixed1;
> > + u64 xfam_fixed0;
> > + u64 xfam_fixed1;
> > +
> > + u32 nr_cpuid_configs;
> > + struct tdx_cpuid_config cpuid_configs[TDX_MAX_NR_CPUID_CONFIGS];
> > +};
> > +
> > +/* Capabilities of KVM + the TDX module. */
> > +static struct tdx_capabilities tdx_caps;
>
> I think you can introduce this tdx_capabilities in another patch.
>
> As claimed this patch can just focus on initializing the TDX module. Whether
> you need this tdx_capabilities or tdx_sysinfo is enough can be done in the patch
> when they are really needed. It makes review easier otherwise people won't be
> able to tell why tdx_capabilities is needed here.
Ok, the previous patch ("x86/virt/tdx: Add a helper function to return
system wide info about TDX module ") and this part will be moved right before the
first use of tdx_caps. "KVM: TDX: create/destroy VM structure"
> > +
> > +static int __init tdx_module_setup(void)
> > +{
> > + const struct tdsysinfo_struct *tdsysinfo;
> > + int ret = 0;
> > +
> > + BUILD_BUG_ON(sizeof(*tdsysinfo) != 1024);
> > + BUILD_BUG_ON(TDX_MAX_NR_CPUID_CONFIGS != 37);
> > +
> > + ret = tdx_enable();
> > + if (ret) {
> > + pr_info("Failed to initialize TDX module.\n");
> > + return ret;
> > + }
> > +
> > + tdsysinfo = tdx_get_sysinfo();
> > + if (tdsysinfo->num_cpuid_config > TDX_MAX_NR_CPUID_CONFIGS)
> > + return -EIO;
> > +
> > + tdx_caps = (struct tdx_capabilities) {
> > + .tdcs_nr_pages = tdsysinfo->tdcs_base_size / PAGE_SIZE,
> > + /*
> > + * TDVPS = TDVPR(4K page) + TDVPX(multiple 4K pages).
> > + * -1 for TDVPR.
> > + */
> > + .tdvpx_nr_pages = tdsysinfo->tdvps_base_size / PAGE_SIZE - 1,
> > + .attrs_fixed0 = tdsysinfo->attributes_fixed0,
> > + .attrs_fixed1 = tdsysinfo->attributes_fixed1,
> > + .xfam_fixed0 = tdsysinfo->xfam_fixed0,
> > + .xfam_fixed1 = tdsysinfo->xfam_fixed1,
> > + .nr_cpuid_configs = tdsysinfo->num_cpuid_config,
> > + };
> > + if (!memcpy(tdx_caps.cpuid_configs, tdsysinfo->cpuid_configs,
> > + tdsysinfo->num_cpuid_config *
> > + sizeof(struct tdx_cpuid_config)))
> > + return -EIO;
> > +
> > + pr_info("kvm: TDX is supported. x86 phys bits %d\n",
> > + boot_cpu_data.x86_phys_bits);
>
> What''s the benefit of print out x86_phys_bits? Looks a little bit weird here.
>
> TDX host code will print out TDX private KeyID range. I think that is useful
> enough?
Ok, please make TDX host code print it. I will remove key id rane.
> > +
> > + return 0;
> > +}
> > +
> > +int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
> > +{
> > + int r;
> > +
> > + if (!enable_ept) {
> > + pr_warn("Cannot enable TDX with EPT disabled\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* MOVDIR64B instruction is needed. */
> > + if (!static_cpu_has(X86_FEATURE_MOVDIR64B)) {
> > + pr_warn("Cannot enable TDX with MOVDIR64B supported ");
> ^
> without
> > + return -ENODEV;
> > + }
>
> I think you should explain why MOVDIR64B is required, otherwise this just comes
> out of blue.
>
> Btw, is this absolutely required? TDX also supports Li-mode, which doesn't have
> integrity check. So theoretically with Li-mode, normal zeroing is also OK but
> doesn't need to use MOVDIR64B.
>
> That being said, do we have a way to tell whether TDX works in Ci or Li mode?
As long as I don't know. When clearing page, we can use
if (featuremovdir64b) movdir64b else memset(0).
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
