Re: [PATCH 3/9] x86/virt/tdx: Support global metadata read for all element sizes

Huang, Kai
2024-06-19T09:51:59Z

> >
> > -static int read_sys_metadata_field16(u64 field_id,
> > - int offset,
> > - void *stbuf)
> > +/*
> > + * Read one global metadata field and store the data to a location of a
> > + * given buffer specified by the offset and size (in bytes).
> > + */
> > +static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
> > + int bytes)
>
> Actually I think opencoding read_sys_metadata_field in
> stbuf_read_sysmd_field and leaving the function named as
> read_sys_metadata_field would be best. The new function is still very
> short and linear.

I am not sure why it is better. IMHO having a wrapper function of a
SEAMCALL leaf, read_sys_metadata_field() for TDH.SYS.RD in this case,
isn't a bad idea. To me it is even better as it is clearer to me.

I can see you don't like the "stbuf" prefix, so we can consider to remove
it, but for now I'd wait for some more time to see whether other people
also have comments.

>
> Another point - why do proliferate the offset calculations in the lower
> layers of TDX. Simply pass in a buffer and a size, calculate the exact
> location in the callers of the read functions.

The current upstream code takes the 'offset' as argument. Here I just
extend it to take the size. I don't feel I have strong justification to
do additional change. Also to me between the two it's hard to say which
is definitely better than the other.