Re: [PATCH 5/9] x86emul: re-use new stub_exn field in state structure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.04.2023 21:48, Andrew Cooper wrote:
> On 04/04/2023 3:53 pm, Jan Beulich wrote:
>> This can now also be used to reduce the number of parameters
>> x86emul_fpu() needs to take.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> As said in the previous patch, I think this patch wants reordering
> forwards and picking up the addition into state.
> 
> "Because we're going to need it in another hook, and it simplifies an
> existing one" is a perfectly fine justification in isolation.

As said there, I'll do that.

> With that done, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>,

Thanks.

> although...> 
>> ---
>> We could of course set the struct field once early in x86_emulate(), but
>> for now I think we're better off leaving it as NULL where not actually
>> needed.
> 
> Do we gain anything useful from not doing it once?  it's certainly more
> to remember, and more code overall, to assign when needed.

Right, that's why I did add the remark. In harness and fuzzer we'll be
able to catch stray uses of the field if we keep it at NULL. Actually
if we were to always set the pointer, perhaps it wouldn't make sense
to have a pointer in struct x86_emulate_state in the first place, but
then we'd better put the struct itself there.

>> --- a/xen/arch/x86/x86_emulate/fpu.c
>> +++ b/xen/arch/x86/x86_emulate/fpu.c
>> @@ -90,9 +90,8 @@ int x86emul_fpu(struct x86_emulate_state
>>                  unsigned int *insn_bytes,
>>                  enum x86_emulate_fpu_type *fpu_type,
>>  #define fpu_type (*fpu_type) /* for get_fpu() */
>> -                struct stub_exn *stub_exn,
>> -#define stub_exn (*stub_exn) /* for invoke_stub() */
>>                  mmval_t *mmvalp)
>> +#define stub_exn (*s->stub_exn) /* for invoke_stub() */
> 
> ... honestly, I'd really like to see these macros purged.
> 
> I know the general style was done like this to avoid code churn, but
> hiding indirection is a very rude thing to do, and none of these are
> usefully shortening the expressions they replace.

Right, getting rid of those is certainly on my radar. But it'll be
further code-churn-ish changes in an area where history tells me
things often take long to get in (and hence there may be measurable
re-basing effort in the meantime).

> Also, putting stub_exn in the K&R type space is still weird to read.

What's K&R-ish there?

Jan