Received-SPF: pass (google.com: domain of linux-kernel+bounces-130026-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, 3 Apr 2024 08:04:03 -0700
In-Reply-To: <522cbfe6e5a351f88480790fe3c3be36c82ca4b1.1708933498.git.isaku.yamahata@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <cover.1708933498.git.isaku.yamahata@intel.com> <522cbfe6e5a351f88480790fe3c3be36c82ca4b1.1708933498.git.isaku.yamahata@intel.com>
Message-ID: <Zg1v4wSgPWiY1Tok@google.com>
Subject: Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural definitions
From: Sean Christopherson <seanjc@google.com>
To: isaku.yamahata@intel.com
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com, 
	Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>, chen.bo@intel.com, 
	hang.yuan@intel.com, tina.zhang@intel.com, 
	Sean Christopherson <sean.j.christopherson@intel.com>, Xiaoyao Li <xiaoyao.li@intel.com>
Content-Type: text/plain; charset="us-ascii"

On Mon, Feb 26, 2024, isaku.yamahata@intel.com wrote:
> +union tdx_vcpu_state_details {
> +	struct {
> +		u64 vmxip	: 1;
> +		u64 reserved	: 63;
> +	};
> +	u64 full;
> +};

No unions please.  KVM uses unions in a few places where they are the lesser of
all evils, but in general, unions are frowned upon.  Bitfields in particular are
strongly discourage, as they are a nightmare to read/review and tend to generate
bad code.

E.g. for this one, something like (names aren't great)

static inline bool tdx_has_pending_virtual_interrupt(struct kvm_vcpu *vcpu)
{
	return <get "non arch field"> & TDX_VCPU_STATE_VMXIP;
}

> +union tdx_sept_entry {
> +	struct {
> +		u64 r		:  1;
> +		u64 w		:  1;
> +		u64 x		:  1;
> +		u64 mt		:  3;
> +		u64 ipat	:  1;
> +		u64 leaf	:  1;
> +		u64 a		:  1;
> +		u64 d		:  1;
> +		u64 xu		:  1;
> +		u64 ignored0	:  1;
> +		u64 pfn		: 40;
> +		u64 reserved	:  5;
> +		u64 vgp		:  1;
> +		u64 pwa		:  1;
> +		u64 ignored1	:  1;
> +		u64 sss		:  1;
> +		u64 spp		:  1;
> +		u64 ignored2	:  1;
> +		u64 sve		:  1;

Yeah, NAK to these unions.  They are crappy duplicates of existing definitions,
e.g. it took me a few seconds to realize SVE is SUPPRESS_VE, which is far too
long.

> +	};
> +	u64 raw;
> +};
> +enum tdx_sept_entry_state {
> +	TDX_SEPT_FREE = 0,
> +	TDX_SEPT_BLOCKED = 1,
> +	TDX_SEPT_PENDING = 2,
> +	TDX_SEPT_PENDING_BLOCKED = 3,
> +	TDX_SEPT_PRESENT = 4,
> +};
> +
> +union tdx_sept_level_state {
> +	struct {
> +		u64 level	:  3;
> +		u64 reserved0	:  5;
> +		u64 state	:  8;
> +		u64 reserved1	: 48;
> +	};
> +	u64 raw;
> +};

Similar thing here.  Depending on what happens with the SEAMCALL argument mess,
the code can look somethign like:

static u8 tdx_get_sept_level(struct tdx_module_args *out)
{
	return out->rdx & TDX_SEPT_LEVEL_MASK;
}

static u8 tdx_get_sept_state(struct tdx_module_args *out)
{
	return (out->rdx & TDX_SEPT_STATE_MASK) >> TDX_SEPT_STATE_SHIFT;
}

> +union tdx_md_field_id {
> +	struct {
> +		u64 field                       : 24;
> +		u64 reserved0                   : 8;
> +		u64 element_size_code           : 2;
> +		u64 last_element_in_field       : 4;
> +		u64 reserved1                   : 3;
> +		u64 inc_size                    : 1;
> +		u64 write_mask_valid            : 1;
> +		u64 context                     : 3;
> +		u64 reserved2                   : 1;
> +		u64 class                       : 6;
> +		u64 reserved3                   : 1;
> +		u64 non_arch                    : 1;
> +	};
> +	u64 raw;
> +};
> +
> +#define TDX_MD_ELEMENT_SIZE_CODE(_field_id)			\
> +	({ union tdx_md_field_id _fid = { .raw = (_field_id)};  \
> +		_fid.element_size_code; })

Yeah, no thanks.  MASK + SHIFT will do just fine.