Re: [PATCH 4/9] x86emul: support CMPccXADD

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jbeulich@xxxxxxxx>
>> ---
>> # 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