On 3/15/24 12:38, Sean Christopherson wrote:
>> tdh_mem_page_remove() _should_ just be logically:
>>
>> * initialize tdx_module_args. Move a few things into place on
>> the stack and zero the rest.
> The "zero the rest" is what generates the fugly code. The underlying problem is
> that the SEAMCALL assembly functions unpack _all_ registers from tdx_module_args.
> As a result, tdx_module_args needs to be zeroed to avoid loading registers with
> unitialized stack data.
It's the "zero the rest" and also the copy:
> + if (out) {
> + *out = *in;
> + ret = seamcall_ret(op, out);
> + } else
> + ret = seamcall(op, in);
Things get a wee bit nicer if you do an out-of-line mempcy() instead of
the structure copy. But the really fun part is that 'out' is NULL and
the compiler *SHOULD* know it. I'm not actually sure what trips it up.
In any case, I think it ends up generating code for both sides of the
if/else including the entirely superfluous copy.
The two nested while loops (one for TDX_RND_NO_ENTROPY and the other for
TDX_ERROR_SEPT_BUSY) also don't make for great code generation.
So, sure, the generated code here could be a better. But there's a lot
more going on here than just shuffling gunk in and out of the 'struct
tdx_module_args', and there's a _lot_ more work to do for one of these
than for a plain old kvm_hypercall*().
It might make sense to separate out the "out" functionality into and
maybe to uninline _some_ of the helper levels. But after that, there's
not a lot of low hanging fruit.
