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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, July 08, 2015 4:13 PM
> To: Wu, Feng
> Cc: Andrew Cooper; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin; Zhang, Yang Z;
> xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: RE: [Xen-devel] [v3 03/15] Add cmpxchg16b support for x86-64
> 
> >>> On 08.07.15 at 09:06, <feng.wu@xxxxxxxxx> wrote:
> 
> >
> >> -----Original Message-----
> >> From: xen-devel-bounces@xxxxxxxxxxxxx
> >> [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Andrew Cooper
> >> Sent: Thursday, June 25, 2015 2:35 AM
> >> To: Wu, Feng; xen-devel@xxxxxxxxxxxxx
> >> Cc: george.dunlap@xxxxxxxxxxxxx; Zhang, Yang Z; Tian, Kevin; keir@xxxxxxx;
> >> jbeulich@xxxxxxxx
> >> 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@xxxxxxxxx>
> >> > ---
> >> > 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.
> 

So, do you agree with the fixup section or not?

> 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?

Oh, yes, the base address of the remapping table is 4K aligned.

> 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.

After introducing posted format in IRTE, some fields exist in both the
High 64 bit and the low 64 bit,such as pda_h and pda_l, how to make
sure it is atomic when updating the pda field?

Thanks,
Feng

> 
> Jan


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