x86-64 tools fix question

x86-64 tools fix question

[Jerone Young]

In the tools there is a declaration:

#if defined(__i386__)
#define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%esp)" : : :
"memory" )
#define wmb() __asm__ __volatile__ ( "" : : : "memory" )
#else
#error "Define barriers"
#endif

located in:
	xcs/xcs.h
	tools/python/xen/lowlevel/xu/xu.c
	
	I'm assuming this has a convenient side-effect that it prevents read
reordering. Otherwise I can't figure out why this is being done at all.
Now I'm guessing that that using rsp instead of esp since we are in
64bit mode will give the same effect needed.

#elif defined(__x86_64__)
#define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%rsp)" : : :
"memory" )
#define wmb() __asm__ __volatile__ ( "" : : : "memory" )


	I would like to discuss is this correct, dead wrong, or even needed at
all?

-- 
Jerone Young
Open Virtualization
IBM Linux Technology Center
jyoung5@us.ibm.com
512-838-1157 (T/L: 678-1157)



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: x86-64 tools fix question

[Keir Fraser]

On 1 Mar 2005, at 19:56, Jerone Young wrote:

> 	I'm assuming this has a convenient side-effect that it prevents read
> reordering. Otherwise I can't figure out why this is being done at all.
> Now I'm guessing that that using rsp instead of esp since we are in
> 64bit mode will give the same effect needed.
>
> #elif defined(__x86_64__)
> #define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%rsp)" : : :
> "memory" )
> #define wmb() __asm__ __volatile__ ( "" : : : "memory" )
>
>
> 	I would like to discuss is this correct, dead wrong, or even needed at
> all?

x86/64 has proper barrier instructions -- see 
include/asm-x86_64/system.h in Linux. It is from there that we should 
pull our definitions. Barrier macros are defined in a few places in the 
tools -- we ought to pull them all into one single header incorporated 
by all tools that need it.

  -- Keir



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: x86-64 tools fix question

[Anthony Liguori]

Keir Fraser wrote:

>
> On 1 Mar 2005, at 19:56, Jerone Young wrote:
>
>>     I would like to discuss is this correct, dead wrong, or even 
>> needed at
>> all?
>
>
> x86/64 has proper barrier instructions -- see 
> include/asm-x86_64/system.h in Linux. It is from there that we should 
> pull our definitions. Barrier macros are defined in a few places in 
> the tools -- we ought to pull them all into one single header 
> incorporated by all tools that need it.

I was just about to send this out myself :-)  I think the consensus was 
to put them all in xc.h.  I posted a patch recently that made everything 
include asm/system.h.  All it should take is replacing asm/system.h with 
xc.h in the patch and then making the necessary changes to xc.h.

For reference, the proper x86-64 barriers are:

#define mb()     asm volatile("mfence":::"memory")
#define rmb()    asm volatile("lfence":::"memory")
#define wmb()    asm volatile("sfence":::"memory")

Regards,
Anthony Liguori

-- 
Regards,

Anthony Liguori



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

[PATCH] cpu barriers moved and x86-64 barriers add

[Jerone Young]

[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]

We actually should have wmb() be defined as "asm volatile("":::"memory")
since we are not using out of order io.

The patch attached removes all the instances and places the definitions
in libxc/xc.h as per the discussion last week between Anthony & Keir.


Signed-off-by: Jerone Young <jyoung5@us.ibm.com>


On Tue, 2005-03-01 at 15:23 -0600, Anthony Liguori wrote:
> Keir Fraser wrote:
> 
> >
> > On 1 Mar 2005, at 19:56, Jerone Young wrote:
> >
> >>     I would like to discuss is this correct, dead wrong, or even 
> >> needed at
> >> all?
> >
> >
> > x86/64 has proper barrier instructions -- see 
> > include/asm-x86_64/system.h in Linux. It is from there that we should 
> > pull our definitions. Barrier macros are defined in a few places in 
> > the tools -- we ought to pull them all into one single header 
> > incorporated by all tools that need it.
> 
> I was just about to send this out myself :-)  I think the consensus was 
> to put them all in xc.h.  I posted a patch recently that made everything 
> include asm/system.h.  All it should take is replacing asm/system.h with 
> xc.h in the patch and then making the necessary changes to xc.h.
> 
> For reference, the proper x86-64 barriers are:
> 
> #define mb()     asm volatile("mfence":::"memory")
> #define rmb()    asm volatile("lfence":::"memory")
> #define wmb()    asm volatile("sfence":::"memory")
> 
> Regards,
> Anthony Liguori
> 
-- 
Jerone Young
Open Virtualization
IBM Linux Technology Center
jyoung5@us.ibm.com
512-838-1157 (T/L: 678-1157)

[-- Attachment #2: cpu_barrier_patch.diff --]
[-- Type: text/x-patch, Size: 3223 bytes --]

diff -Nur xen-unstable-0301.orig/tools/blktap/blktaplib.h xen-unstable-0301-test/tools/blktap/blktaplib.h
--- xen-unstable-0301.orig/tools/blktap/blktaplib.h	2005-03-01 06:54:49.000000000 -0600
+++ xen-unstable-0301-test/tools/blktap/blktaplib.h	2005-03-01 16:24:25.780124280 -0600
@@ -10,22 +10,6 @@
 
 #include <stdint.h>
 
-typedef uint8_t            u8;
-typedef uint16_t           u16;
-typedef uint32_t           u32;
-typedef uint64_t           u64;
-typedef int8_t             s8;
-typedef int16_t            s16;
-typedef int32_t            s32;
-typedef int64_t            s64;
-                                                                                
-#if defined(__i386__)
-#define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%esp)" : : : "memory" )
-#define wmb() __asm__ __volatile__ ( "" : : : "memory" )
-#else
-#error "Define barriers"
-#endif
-    
 #include <sys/user.h>
 #include <xen/xen.h>
 #include <xen/io/blkif.h>
diff -Nur xen-unstable-0301.orig/tools/libxc/xc.h xen-unstable-0301-test/tools/libxc/xc.h
--- xen-unstable-0301.orig/tools/libxc/xc.h	2005-03-01 06:54:49.000000000 -0600
+++ xen-unstable-0301-test/tools/libxc/xc.h	2005-03-01 16:05:53.248254832 -0600
@@ -25,6 +25,21 @@
 #include <xen/event_channel.h>
 #include <xen/sched_ctl.h>
 
+
+/*\
+ *  DEFINITIONS FOR CPU BARRIERS
+\*/ 
+#if defined(__i386__)
+#define rmb() __asm__ __volatile__ ("lock; addl $0,0(%%esp)" : : : "memory")
+#define wmb() __asm__ __volatile__ ("" : : : "memory")
+#elif defined(__x86_64__)
+#define mb()     asm volatile("mfence":::"memory")
+#define rmb()    asm volatile("lfence":::"memory")
+#define wmb()    asm volatile( "" :::"memory")
+#else
+#error "Define barriers"
+#endif
+
 /*\
  *  INITIALIZATION FUNCTIONS
 \*/ 
diff -Nur xen-unstable-0301.orig/tools/python/xen/lowlevel/xu/xu.c xen-unstable-0301-test/tools/python/xen/lowlevel/xu/xu.c
--- xen-unstable-0301.orig/tools/python/xen/lowlevel/xu/xu.c	2005-03-01 06:54:49.000000000 -0600
+++ xen-unstable-0301-test/tools/python/xen/lowlevel/xu/xu.c	2005-03-01 16:01:13.083846296 -0600
@@ -49,14 +49,6 @@
 /* Size of a machine page frame. */
 #define PAGE_SIZE 4096
 
-#if defined(__i386__)
-#define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%esp)" : : : "memory" )
-#define wmb() __asm__ __volatile__ ( "" : : : "memory" )
-#else
-#error "Define barriers"
-#endif
-
-
 /* Set the close-on-exec flag on a file descriptor.  Doesn't currently bother
  * to check for errors. */
 /*
diff -Nur xen-unstable-0301.orig/tools/xcs/xcs.h xen-unstable-0301-test/tools/xcs/xcs.h
--- xen-unstable-0301.orig/tools/xcs/xcs.h	2005-03-01 06:54:49.000000000 -0600
+++ xen-unstable-0301-test/tools/xcs/xcs.h	2005-03-01 16:02:22.707261928 -0600
@@ -39,13 +39,6 @@
 /* Size of a machine page frame. */
 #define PAGE_SIZE 4096
 
-#if defined(__i386__)
-#define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%esp)" : : : "memory" )
-#define wmb() __asm__ __volatile__ ( "" : : : "memory" )
-#else
-#error "Define barriers"
-#endif
-
 #ifndef timersub /* XOPEN and __BSD don't cooperate well... */
 #define timersub(a, b, result)                                                \
   do {                                                                        \

Re: [PATCH] cpu barriers moved and x86-64 barriers add

[Jerone Young]

Ok pay this email no mind. This was the first one I sent yesterday.
Something must have happened with the IBM smtp server and caused my
email to be held and outragously long time then sent. Sorry about this
guys.


On Tue, 01 Mar 2005 16:24:37 -0600, Jerone Young <jyoung5@us.ibm.com> wrote:
> We actually should have wmb() be defined as "asm volatile("":::"memory")
> since we are not using out of order io.
> 
> The patch attached removes all the instances and places the definitions
> in libxc/xc.h as per the discussion last week between Anthony & Keir.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> On Tue, 2005-03-01 at 15:23 -0600, Anthony Liguori wrote:
> > Keir Fraser wrote:
> >
> > >
> > > On 1 Mar 2005, at 19:56, Jerone Young wrote:
> > >
> > >>     I would like to discuss is this correct, dead wrong, or even
> > >> needed at
> > >> all?
> > >
> > >
> > > x86/64 has proper barrier instructions -- see
> > > include/asm-x86_64/system.h in Linux. It is from there that we should
> > > pull our definitions. Barrier macros are defined in a few places in
> > > the tools -- we ought to pull them all into one single header
> > > incorporated by all tools that need it.
> >
> > I was just about to send this out myself :-)  I think the consensus was
> > to put them all in xc.h.  I posted a patch recently that made everything
> > include asm/system.h.  All it should take is replacing asm/system.h with
> > xc.h in the patch and then making the necessary changes to xc.h.
> >
> > For reference, the proper x86-64 barriers are:
> >
> > #define mb()     asm volatile("mfence":::"memory")
> > #define rmb()    asm volatile("lfence":::"memory")
> > #define wmb()    asm volatile("sfence":::"memory")
> >
> > Regards,
> > Anthony Liguori
> >
> --
> Jerone Young
> Open Virtualization
> IBM Linux Technology Center
> jyoung5@us.ibm.com
> 512-838-1157 (T/L: 678-1157)
> 
> 
>


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: x86-64 tools fix question

[David Hopwood]

Jerone Young wrote:
> In the tools there is a declaration:
> 
> #if defined(__i386__)
> #define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%esp)" : : :
> "memory" )
> #define wmb() __asm__ __volatile__ ( "" : : : "memory" )
> #else
> #error "Define barriers"
> #endif
> 
> located in:
> 	xcs/xcs.h
> 	tools/python/xen/lowlevel/xu/xu.c
> 	
> 	I'm assuming this has a convenient side-effect that it prevents read
> reordering. Otherwise I can't figure out why this is being done at all.
> Now I'm guessing that that using rsp instead of esp since we are in
> 64bit mode will give the same effect needed.
> 
> #elif defined(__x86_64__)
> #define rmb() __asm__ __volatile__ ( "lock; addl $0,0(%%rsp)" : : :
> "memory" )
> #define wmb() __asm__ __volatile__ ( "" : : : "memory" )
> 
> 	I would like to discuss is this correct, dead wrong, or even needed at
> all?

See volume 1, section 3.9.2 of the AMD64 architecture manual
(http://www.amd.com/us-en/Processors/DevelopWithAMD/0,,30_2252_875_7044,00.html),
quoted below for convenience.

This should be guaranteed to work:

#define rmb() __asm__ __volatile__ ( "mfence" : : : "memory" )
#define wmb() __asm__ __volatile__ ( "mfence" : : : "memory" )

but may be overkill. Are these macros used in any performance-critical
situations? If not then the overkill wouldn't matter.

# 3.9.2 Forcing Memory Order
#
# Special instructions are provided for application software to force
# memory ordering in situations where such ordering is important. These
# instructions are:
#  * Load Fence -- The LFENCE instruction forces ordering of
#    memory loads (reads). All memory loads preceding the
#    LFENCE (in program order) are completed prior to
#    completing memory loads following the LFENCE. Memory
#    loads cannot be reordered around an LFENCE instruction,
#    but other non-serializing instructions (such as memory
#    writes) can be reordered around the LFENCE.
#  * Store Fence -- The SFENCE instruction forces ordering of
#    memory stores (writes). All memory stores preceding the
#    SFENCE (in program order) are completed prior to
#    completing memory stores following the SFENCE. Memory
#    stores cannot be reordered around an SFENCE instruction,
#    but other non-serializing instructions (such as memory
#    loads) can be reordered around the SFENCE.
#  * Memory Fence -- The MFENCE instruction forces ordering of
#    all memory accesses (reads and writes). All memory accesses
#    preceding the MFENCE (in program order) are completed
#    prior to completing any memory access following the
#    MFENCE. Memory accesses cannot be reordered around an
#    MFENCE instruction, but other non-serializing instructions
#    that do not access memory can be reordered around the
#    MFENCE.

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: x86-64 tools fix question

[Anthony Liguori]

David Hopwood wrote:

> Jerone Young wrote:
> See volume 1, section 3.9.2 of the AMD64 architecture manual
> (http://www.amd.com/us-en/Processors/DevelopWithAMD/0,,30_2252_875_7044,00.html), 
>
> quoted below for convenience.
>
> This should be guaranteed to work:
>
> #define rmb() __asm__ __volatile__ ( "mfence" : : : "memory" )
> #define wmb() __asm__ __volatile__ ( "mfence" : : : "memory" )

sfence and lfence would seem to be more appropriate.  mfence would seem 
to be appropriate for an mb() macro (which is usually also defined--but 
in this case not necessary).

> but may be overkill. Are these macros used in any performance-critical
> situations? If not then the overkill wouldn't matter.

Not at all.  They're currently used to prevent race conditions within 
the domain control channel ring queue.  This queue is relatively 
low-traffic.

The confusion stemmed from the x86-32 code that doesn't actually 
implement a wmb() since no relevant architecture does out-of-order stores.

x86-64 is another story.  The linux includes still have the same 
comments indicating that wmb() is not necessary (an obvious 
cut-and-paste job) even though wmb() is defined to sfence.  To make 
matters worse, the wmb() is wrapped in the same CONFIG_UNORDERED_IO 
guarded (that's enabled by default in x86-64 but disabled by default in 
x86-32).

It's understandably confusing.  I'm hoping we can do a much better job 
in Xen in supporting both architectures to avoid these sort of confusions.

Regards,
Anthony Liguori


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click