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 <jbeul...@suse.com>
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.coop...@citrix.com>, 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
