Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

From: Sathyanarayanan Kuppuswamy  <sathyanarayanan.kuppuswamy@linux.intel.com>
To: Kai Huang <kai.huang@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <mgross@linux.intel.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
	Tony Luck <tony.luck@intel.com>, Andi Kleen <ak@linux.intel.com>,
	linux-kernel@vger.kernel.org,
	platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver
Date: Tue, 19 Apr 2022 07:00:03 -0700	[thread overview]
Message-ID: <07ef65c4-708e-1bcf-9a7e-f804acefcc7c@linux.intel.com> (raw)
In-Reply-To: <0d532b0ce1155bf7778366b14c5d1311c45fef01.camel@intel.com>

On 4/19/22 1:16 AM, Kai Huang wrote:
> In fact after slightly thinking more, I think you can split TDREPORT TDCALL
> support with GetQuote/SetupEventNotifyInterrupt support.  The reason is as I
> said, GetQuote isn't mandatory to support attestation.  TD attestation agent can
> use i.e. vsock, tcp/ip, to communicate to QE directly.  Whether kernel needs to
> support GetQuote is actually arguable.

IMO, we should not use a usage model to categorize "GetQuote" support
as a mandatory or non-mandatory requirement.

For customers who use VSOCK, they can get away without GetQuote
TDVMCALL support. But for customers who do not want to use
VSOCK model, this is a required support. AFAIK, our current customer
requirement is to use TDVMCALL approach for attestation support.

If your suggestion is to split GetQuote support as separate
patch to make it easier for review, I am fine with such
suggestion.

Maintainers, any opinion? Would you prefer to split the
driver into two patches?

> 
> So IMHO you can split this attestation driver into two parts:
> 
> 1) A "basic" driver which supports reporting TDREPORT to userspace
> 2) Additional support of GetQuote/SetupEventNotifyInterrupt.
> 
> The 1) can even be in a single patch (I guess it won't be complicated).  It is
> easy to review (and i.e. can be merged separately), and with it, you will
> immediately have one way to support attestation.
> 
> 2) can be reviewed separately, perhaps with one additional Kconfig option (i.e.
> CONFIG_INTEL_TDX_ATTESTATION_GET_QUOTE).  I think this part has most of the

GetQuote IOCTL support is a very simple feature support, so, IMO, we
don't need to complicate it with additional config.

> complexity things in terms of review.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer