Re: [v3 03/15] Add cmpxchg16b support for x86-64

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.07.15 at 09:06, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org 
>> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Andrew Cooper
>> Sent: Thursday, June 25, 2015 2:35 AM
>> To: Wu, Feng; xen-devel@lists.xen.org 
>> Cc: george.dunlap@eu.citrix.com; Zhang, Yang Z; Tian, Kevin; keir@xen.org;
>> jbeulich@suse.com 
>> Subject: Re: [Xen-devel] [v3 03/15] Add cmpxchg16b support for x86-64
>> 
>> On 24/06/15 06:18, Feng Wu wrote:
>> > This patch adds cmpxchg16b support for x86-64, so software
>> > can perform 128-bit atomic write/read.
>> >
>> > Signed-off-by: Feng Wu <feng.wu@intel.com>
>> > ---
>> > v3:
>> > Newly added.
>> >
>> >  xen/include/asm-x86/x86_64/system.h | 28
>> ++++++++++++++++++++++++++++
>> >  xen/include/xen/types.h             |  5 +++++
>> >  2 files changed, 33 insertions(+)
>> >
>> > diff --git a/xen/include/asm-x86/x86_64/system.h
>> b/xen/include/asm-x86/x86_64/system.h
>> > index 662813a..a910d00 100644
>> > --- a/xen/include/asm-x86/x86_64/system.h
>> > +++ b/xen/include/asm-x86/x86_64/system.h
>> > @@ -6,6 +6,34 @@
>> >                                     (unsigned
>> long)(n),sizeof(*(ptr))))
>> >
>> >  /*
>> > + * Atomic 16 bytes compare and exchange.  Compare OLD with MEM, if
>> > + * identical, store NEW in MEM.  Return the initial value in MEM.
>> > + * Success is indicated by comparing RETURN with OLD.
>> > + *
>> > + * This function can only be called when cpu_has_cx16 is ture.
>> > + */
>> > +
>> > +static always_inline uint128_t __cmpxchg16b(
>> > +    volatile void *ptr, uint128_t old, uint128_t new)
>> 
>> It is not nice for register scheduling taking uint128_t's by value.
>> Instead, I would pass them by pointer and let the inlining sort the
>> eventual references out.
>> 
>> > +{
>> > +    uint128_t prev;
>> > +
>> > +    ASSERT(cpu_has_cx16);
>> 
>> Given that if this assertion were to fail, cmpxchg16b would fail with
>> #UD, I would hand-code a asm_fixup section which in turn panics.  This
>> avoids a situation where non-debug builds could die with an unqualified
>> #UD exception.
> 
> Is there an existing way to panic the hypervisor in assembler code, I
> don't find it, it would be appreciated if you can point it out.

I'm not convinced such a #UD would be a significant problem: Looking
at the disassembly will show the cause right away. The out of line
ud2-s in some of VMX'es inline assembly wrappers are far worse.

As to panic()ing from assembly code:

	movq	$<string-label>, %rdi
	call	panic

>> Also, you must enforce 16-byte alignment of the memory reference, as
>> described in the manual.
> 
> What should I do if the caller passes an non 16-byte alignment data
> (struct iremap_entry in this case) ? Do this mean I need to define
> it like this?
> 
> struct iremap_entry {
> 
> ......
> 
> } __attribute__ ((aligned (16)));

How would that help? The table entries hardware uses are supposed
to be 16-byte aligned anyway, aren't they? I think Andrew's "enforce"
really means ASSERT() or BUG_ON(), again to avoid an unqualified
exception. However - see above.

Plus, all that said, without having seen the actual use sites of
cmpxchg16b yet, I'm not at all convinced we really need this patch.

Jan