| Message ID | a9c212a8-8c63-91e0-eb07-8c927b62c1ca@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86emul: misc additions | expand |
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@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.cooper3@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
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@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. As said there, I'll do that. > With that done, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, 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
--- 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() */ { uint8_t b; int rc; --- a/xen/arch/x86/x86_emulate/private.h +++ b/xen/arch/x86/x86_emulate/private.h @@ -764,7 +764,6 @@ int x86emul_fpu(struct x86_emulate_state const struct x86_emulate_ops *ops, unsigned int *insn_bytes, enum x86_emulate_fpu_type *fpu_type, - struct stub_exn *stub_exn, mmval_t *mmvalp); int x86emul_0f01(struct x86_emulate_state *s, struct cpu_user_regs *regs, --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2058,8 +2058,9 @@ x86_emulate( #ifndef X86EMUL_NO_FPU case 0x9b: /* wait/fwait */ case 0xd8 ... 0xdf: /* FPU */ + state->stub_exn = &stub_exn; rc = x86emul_fpu(state, &_regs, &dst, &src, ctxt, ops, - &insn_bytes, &fpu_type, &stub_exn, mmvalp); + &insn_bytes, &fpu_type, mmvalp); goto dispatch_from_helper; #endif
This can now also be used to reduce the number of parameters x86emul_fpu() needs to take. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- 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.