[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zg1v4wSgPWiY1Tok@google.com>
Date: Wed, 3 Apr 2024 08:04:03 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: isaku.yamahata@...el.com
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
Sagi Shahar <sagis@...gle.com>, Kai Huang <kai.huang@...el.com>, chen.bo@...el.com,
hang.yuan@...el.com, tina.zhang@...el.com,
Sean Christopherson <sean.j.christopherson@...el.com>, Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural definitions
On Mon, Feb 26, 2024, isaku.yamahata@...el.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.
Powered by blists - more mailing lists