On Mon, Feb 26, 2024, isaku.yamahata@xxxxxxxxx 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.
