Re: [PATCH 07/13] x86/virt/tdx: Add SEAMCALL wrapper tdh_mem_sept_add() to add SEPT pages

[Yan Zhao]

On Tue, Jan 07, 2025 at 11:48:12AM -0800, Dave Hansen wrote:
> On 1/2/25 13:59, Edgecombe, Rick P wrote:
> > union tdx_sept_gpa_mapping_info {
> > 	struct {
> > 		u64 level	: 3;
> > 		u64 reserved1	: 9;
> > 		u64 gfn		: 40;
> > 		u64 reserved2	: 12;
> > 	};
> > 	u64 full;
> > };
> 
> This is functionally OK, but seeing bitfields on a value that's probably
> going to get shifted around makes me nervous because of:
This is defined according to the TDX spec.
e.g. in TDH.MEM.SEPT.ADD:

RCX | EPT mapping information:
----|---------------------------------------------------------------------------
    | Bits |  Name    | Description
    |------|----------|---------------------------------------------------------
    | 2:0  |  Level   | Level of the non-leaf Secure EPT entry that will map the
    |      |          | new Secure EPT page - see 3.6.1
    |      |          | Level must between 1 and 3 for a 4-level EPT or between
    |      |          | 1 and 4 for a 5-level EPT.
    |------|----------|---------------------------------------------------------
    | 11:3 | Reserved | Reserved: must be 0
    |------|----------|---------------------------------------------------------
    | 51:12|   GPA    | Bits 51:12 of the guest physical address of to be mapped
    |      |          | for the new Secure EPT page Depending on the level, the
    |      |          | following least significant bits must be 0:
    |      |          | Level 1 (EPT): Bits 20:12
    |      |          | Level 2 (EPD): Bits 29:12
    |      |          | Level 3 (EPDPT): Bits 38:12
    |      |          | Level 4 (EPML4): Bits 47:12
    |------|----------|---------------------------------------------------------
    | 63:52| Reserved | Reserved: must be 0


So, why does this bitfields definition make things worse?

> > https://lore.kernel.org/lkml/20231111020019.553664-1-michael.roth@xxxxxxx/
> I wouldn't NAK it just for this, but it's also not how I would code it up.