On 06.04.2023 21:28, Andrew Cooper wrote: > On 04/04/2023 3:52 pm, Jan Beulich wrote: >> Unconditionally wire this through the ->rmw() hook. Since x86_emul_rmw() >> now wants to construct and invoke a stub, make stub_exn available to it >> via a new field in the emulator state structure. > > IMO, patch 5 should be re-ordered with this, because it removes one > incidental change that's not really related to CMPccXADD.
Yeah, I can probably do that. The order here really is simply the order things were written; I did notice the potential for the subsequent patch only when already done with the one here. >> Signed-off-by: Jan Beulich <jbeul...@suse.com> >> --- >> # SDE: -grr or -srf > > The ISE makes a point of noting that CMPccXADD is implicitly locked, > like XCHG. (Unlike XCHG, there isn't a valid reg/reg encoding.) > > Right now, the xchg emulation overrides lock_prefix, but I have a > feeling that's stale now with the rmw() hook in place. But it is > dubious that we let xchg fall back to a non-atomic exchange if the rmw() > hook is missing. > Either way, I think it would be nice to clean that up so we don't have > differences in the handling of instructions which the ISE at least > claims are similar. We can certainly revisit this (independently). > Tangentially, what about the RAO instructions? With the infrastructure we have right now, I don't see how we could get their exception behavior (non-WB -> #GP) correct. Hence while I have these on my todo list, I don't have immediate plans to deal with them. >> --- a/tools/tests/x86_emulator/x86-emulate.h >> +++ b/tools/tests/x86_emulator/x86-emulate.h >> @@ -934,6 +935,8 @@ decode_0f38(struct x86_emulate_state *s, >> ctxt->opcode |= MASK_INSR(s->vex.pfx, X86EMUL_OPC_PFX_MASK); >> break; >> >> + case X86EMUL_OPC_VEX_66(0, 0xe0) >> + ... X86EMUL_OPC_VEX_66(0, 0xef): /* cmp<cc>xadd */ > > I know the style is a little mixed in the emulator, but > > + case X86EMUL_OPC_VEX_66(0, 0xe0) ... > + X86EMUL_OPC_VEX_66(0, 0xef): /* cmp<cc>xadd */ > > is more consistent with Xen style (because it's somewhat of a binary > operator), and more readable IMO. I don't mind; done. >> --- a/xen/include/public/arch-x86/cpufeatureset.h >> +++ b/xen/include/public/arch-x86/cpufeatureset.h >> @@ -278,6 +278,7 @@ XEN_CPUFEATURE(SSBD, 9*32+31) / >> /* Intel-defined CPU features, CPUID level 0x00000007:1.eax, word 10 */ >> XEN_CPUFEATURE(AVX_VNNI, 10*32+ 4) /*A AVX-VNNI Instructions */ >> XEN_CPUFEATURE(AVX512_BF16, 10*32+ 5) /*A AVX512 BFloat16 Instructions */ >> +XEN_CPUFEATURE(CMPCCXADD, 10*32+ 7) /*A CMPccXADD Instructions */ > > Given the non-triviality of this instruction, I'd prefer to keep this > "a" until we've tried it on real hardware. Hmm, yes, probably better. Jan
