Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 4 Aug 2021 23:13:42 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Erdem Aktas <erdemaktas@google.com>
Cc:     Xiaoyao Li <xiaoyao.li@intel.com>,
        "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Connor Kuehl <ckuehl@redhat.com>, x86 <x86@kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:KERNEL VIRTUAL MACHINE (KVM)" <kvm@vger.kernel.org>,
        isaku.yamahata@gmail.com,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Kai Huang <kai.huang@linux.intel.com>,
        Chao Gao <chao.gao@intel.com>
Subject: Re: [RFC PATCH v2 05/69] KVM: TDX: Add architectural definitions for
 structures and values
Message-ID: <YQsfJsZskiI2968+@google.com>
References: <cover.1625186503.git.isaku.yamahata@intel.com>
 <d29b2ac2090f20e8de96888742feb413f597f1dc.1625186503.git.isaku.yamahata@intel.com>
 <CAAYXXYy=fn9dUMjY6b6wgCHSTLewnTZLKb00NMupDXSWbNC9OQ@mail.gmail.com>
 <1057bbfe-c73e-a182-7696-afc59a4786d8@intel.com>
 <CAAYXXYwDuRMQ16X3mshkGcBQXhvgoxPTCu8UGggYgfCzHOWwtQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAYXXYwDuRMQ16X3mshkGcBQXhvgoxPTCu8UGggYgfCzHOWwtQ@mail.gmail.com>
Precedence: bulk

On Wed, Aug 04, 2021, Erdem Aktas wrote:
> On Mon, Aug 2, 2021 at 6:25 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> > > Is this information correct and is this included in the spec? I tried
> > > to find it but somehow I do not see it clearly defined.
> > >
> > >> +#define TDX1_NR_TDCX_PAGES             4
> > >> +#define TDX1_NR_TDVPX_PAGES            5
> > >> +
> > >> +#define TDX1_MAX_NR_CPUID_CONFIGS      6
> > > Why is this just 6? I am looking at the CPUID table in the spec and
> > > there are already more than 6 CPUID leaves there.
> >
> > This is the number of CPUID config reported by TDH.SYS.INFO. Current KVM
> > only reports 6 leaves.
> 
> I, personally, still think that it should be enumerated, rather than
> hardcoded. It is not clear to me why it is 6 and nothing in the spec
> says it will not change.

It's both hardcoded and enumerated.  KVM's hardcoded value is specifically the
maximum value expected for TDX-Module modules supporting the so called "1.0" spec.
It's certainly possible a spec change could bump the maximum, but KVM will refuse
to use a module with higher maximums until Linux/KVM is updated to play nice with
the new module.

Having hardcoded maximum allows for simpler and more efficient code, as loops and
arrays can be statically defined instead of having to pass around the enumerated
values.

And we'd want sanity checking anyways, e.g. if the TDX-module pulled a stupid and
reported that it needs 4000 TDCX pages.  This approach gives KVM documented values
to sanity check, e.g. instead of arbitrary magic numbers.

The downside of this approach is that KVM will need to be updated to play nice
with a new module if any of these maximums are raised.  But, IMO that's acceptable
because I can't imagine a scenario where anyone would want to load a TDX module
without first testing the daylights out of the specific kernel+TDX combination,
especially a TDX-module that by definition includes new features.

> > >> +#define TDX1_MAX_NR_CMRS               32
> > >> +#define TDX1_MAX_NR_TDMRS              64
> > >> +#define TDX1_MAX_NR_RSVD_AREAS         16
> > >> +#define TDX1_PAMT_ENTRY_SIZE           16
> > >> +#define TDX1_EXTENDMR_CHUNKSIZE                256
> > >
> > > I believe all of the defined variables above need to be enumerated
> > > with TDH.SYS.INFO.

And they are, though I believe the code for doing the actual SEAMCALL wasn't
posted in this series.  The output is sanity checked by tdx_hardware_enable():

+	tdx_caps.tdcs_nr_pages = tdsysinfo->tdcs_base_size / PAGE_SIZE;
+	if (tdx_caps.tdcs_nr_pages != TDX1_NR_TDCX_PAGES)
+		return -EIO;
+
+	tdx_caps.tdvpx_nr_pages = tdsysinfo->tdvps_base_size / PAGE_SIZE - 1;
+	if (tdx_caps.tdvpx_nr_pages != TDX1_NR_TDVPX_PAGES)
+		return -EIO;
+
+	tdx_caps.attrs_fixed0 = tdsysinfo->attributes_fixed0;
+	tdx_caps.attrs_fixed1 = tdsysinfo->attributes_fixed1;
+	tdx_caps.xfam_fixed0 =	tdsysinfo->xfam_fixed0;
+	tdx_caps.xfam_fixed1 = tdsysinfo->xfam_fixed1;
+
+	tdx_caps.nr_cpuid_configs = tdsysinfo->num_cpuid_config;
+	if (tdx_caps.nr_cpuid_configs > TDX1_MAX_NR_CPUID_CONFIGS)
+		return -EIO;
+