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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

With that done, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>,
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.

>
> --- 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.

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

~Andrew