Re: [Xen-devel] [PATCH 2/6] x86emul: reduce CMPXCHG{8, 16}B footprint and casting

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.12.16 at 15:26, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 13/12/16 14:02, Jan Beulich wrote:
>> --- a/tools/tests/x86_emulator/x86_emulate.h
>> +++ b/tools/tests/x86_emulator/x86_emulate.h
>> @@ -31,6 +31,11 @@
>>  #define likely(x)   __builtin_expect(!!(x), true)
>>  #define unlikely(x) __builtin_expect(!!(x), false)
>>  
>> +#define container_of(ptr, type, member) ({             \
>> +    typeof(((type *)0)->member) *mptr__ = (ptr);       \
>> +    (type *)((char *)mptr__ - offsetof(type, member)); \
>> +})
>> +
> 
> Please could we use __builtin_containerof()?  I believe It is available
> on all of the compilers we support.

Where have you found reference to this? I can't find it in the doc
(nor anything similar, consulting the index), and grep-ing the gcc
sources doesn't reveal anything either.

>> @@ -5295,35 +5298,52 @@ x86_emulate(
>>          else
>>              op_bytes = 8;
>>  
>> +        old = container_of(&mmvalp->ymm[0], typeof(*old), u64[0]);
>> +        aux = container_of(&mmvalp->ymm[2], typeof(*old), u64[0]);
> 
> This should be typeof(*aux), although it makes no difference at the moment.

Oh, right - copy-and-paste mistake. Fixed.

>> -        if ( memcmp(old, exp, op_bytes) )
>> +        if ( memcmp(old, aux, op_bytes) )
>>          {
>>              /* Expected != actual: store actual to rDX:rAX and clear ZF. */
>> -            _regs.eax = (op_bytes == 8) ? ((uint32_t *)old)[0] : old[0];
>> -            _regs.edx = (op_bytes == 8) ? ((uint32_t *)old)[1] : old[1];
>> +            _regs.eax = !(rex_prefix & REX_W) ? old->u32[0] : old->u64[0];
>> +            _regs.edx = !(rex_prefix & REX_W) ? old->u32[1] : old->u64[1];
>>              _regs.eflags &= ~EFLG_ZF;
> 
> This clearing of ZF should be unconditional.  I think this is a latent
> bug, but if the cmpxchg() fails, we would exit having failed the xchg,
> but leaving ZF stale.

First of all - this request of yours is unrelated to this patch: The
conditions under which ZF gets altered don't get changed here
at all. And then this comment or yours is similar to one you gave
elsewhere not all that long ago; my answer is the same here -
we don't commit _regs.eflags to ctxt->regs if anything fails. And
namely when ops->cmpxchg() returns RETRY, then we indeed
want to retry with the flag unaltered.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel