Hi, Below is my very first attempt to write inline assembler, for atomic updates of PAE pagetable entries using cmpxchg8b. It builds, but is completely untested otherwise as I don''t have a PAE-enabled guest kernel yet ... Can someone with more experience than me please have a look at it? Thanks, Gerd --- xen.orig/arch/x86/mm.c 2005-04-19 11:56:20.000000000 +0200 +++ xen/arch/x86/mm.c 2005-04-19 15:29:01.000000000 +0200 @@ -834,12 +846,55 @@ static void free_l4_table(struct pfn_inf #endif /* __x86_64__ */ +static inline int cmpxchg8b_user(u64 *ptr, u64 oval, u64 nval) +{ + u32 o_hi = oval >> 32; + u32 o_lo = oval & 0xffffffff; + u32 n_hi = nval >> 32; + u32 n_lo = nval & 0xffffffff; + u32 rc = 0; + + __asm__ __volatile__ ( + "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n" + "2:\n" + ".section .fixup,\"ax\"\n" + "3: movl $1, %[rc]\n" + " jmp 2b\n" + ".previous\n" + ".section __ex_table,\"a\"\n" + " .align 4\n" + " .long 1b,3b\n" + ".previous" + : "=d" (o_hi), "=a" (o_lo), "=c" (n_hi), "=b" (n_lo), + [rc] "=r" (rc), [ptr] "m" (*__xg((volatile void *)ptr)) + : "0" (o_hi), "1" (o_lo) + : "memory"); + if ((o_hi != (oval >> 32)) || + (o_lo != (oval & 0xffffffff))) + rc = 1; + return rc; +} static inline int update_l1e(l1_pgentry_t *pl1e, l1_pgentry_t ol1e, l1_pgentry_t nl1e) { - /* FIXME: breaks with PAE */ +#if defined(__i386__) && defined(CONFIG_X86_PAE) + + int rc; + + rc = cmpxchg8b_user(pl1e, l1e_get_value(ol1e), + l1e_get_value(nl1e)); + if (unlikely(rc)) + { + MEM_LOG("Failed to update %llx -> %llx [pae]\n", + l1e_get_value(ol1e), l1e_get_value(nl1e)); + return 0; + } + return 1; + +#else + unsigned long o = l1e_get_value(ol1e); unsigned long n = l1e_get_value(nl1e); @@ -850,8 +905,9 @@ static inline int update_l1e(l1_pgentry_ l1e_get_value(ol1e), l1e_get_value(nl1e), o); return 0; } - return 1; + +#endif } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hollis Blanchard
2005-Apr-19 15:46 UTC
Re: [Xen-devel] [request for review] cmpxchg8b asm stuff
On Tue, 2005-04-19 at 15:50 +0200, Gerd Knorr wrote:> Below is my very first attempt to write inline assembler, for atomic > updates of PAE pagetable entries using cmpxchg8b. It builds, but is > completely untested otherwise as I don''t have a PAE-enabled guest kernel > yet ... > > Can someone with more experience than me please have a look at it? > > Thanks, > > Gerd > > --- xen.orig/arch/x86/mm.c 2005-04-19 11:56:20.000000000 +0200 > +++ xen/arch/x86/mm.c 2005-04-19 15:29:01.000000000 +0200 > @@ -834,12 +846,55 @@ static void free_l4_table(struct pfn_inf > > #endif /* __x86_64__ */ > > +static inline int cmpxchg8b_user(u64 *ptr, u64 oval, u64 nval) > +{...> +}Shouldn''t this go into include/asm-x86/system.h? You can add another case to the __i386__ cmpxchg_user switch. -- Hollis Blanchard IBM Linux Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> > +++ xen/arch/x86/mm.c 2005-04-19 15:29:01.000000000 +0200 > > > > +static inline int cmpxchg8b_user(u64 *ptr, u64 oval, u64 nval)> Shouldn''t this go into include/asm-x86/system.h?Well, maybe later. I want to have it working correctly first, then optimize and then check how to integrate that nicely.> You can add another case to the __i386__ cmpxchg_user switch.That would be the most obvious place, yes. Not fully sure yet that this is really a good idea though as it is sort-of special case for 64bit data on a 32bit machine, you have to split the 64bit values into two 32bit regs and so on (unlike the 64bit version on x86_64 which simply uses the 64bit registers). Maybe it''s better to keep that separate as it might be easier to optimize it then, not evaluated yet. Gerd -- #define printk(args...) fprintf(stderr, ## args) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2005-Apr-21 10:54 UTC
Re: [Xen-devel] [request for review] cmpxchg8b asm stuff
> Hi, > > Below is my very first attempt to write inline assembler, for atomic > updates of PAE pagetable entries using cmpxchg8b. It builds, but is > completely untested otherwise as I don''t have a PAE-enabled guest kernel > yet ... > > Can someone with more experience than me please have a look at it?Thanks - I''ve checked in an 8-byte extension to cmpxchg_user() for 32-bit builds. Differences from yours include: 1. The "A" register specifier is used to match a 64-bit C variable with EDX:EAX. This is neater than manually splitting into "a" and "d" constraints, then manually recombining at the end. Unfortunately there is no such shortcut for ECX:EBX. 2. n_hi, n_lo and ptr are not output constraints, so they belong in the second constraint list, with no "=" in the specifier string. 3. I don''t like the non-positional argument specifiers (e.g., [rc], [ptr]) very much. Maybe worthwhile for longer code fragments but for very short ones I''d rather have less clutter in the constraint lists. 4. No need to return non-zero if the cmpxchg fails. The return code indicates only whether the cmpxchg access faulted or not: it is up to the caller to compare the value seen with what they expected. This gets rid of a few lines in your cmpxchg8b_user. I think that covers it. :-) -- Keir> > Thanks, > > Gerd > > --- xen.orig/arch/x86/mm.c 2005-04-19 11:56:20.000000000 +0200 > +++ xen/arch/x86/mm.c 2005-04-19 15:29:01.000000000 +0200 > @@ -834,12 +846,55 @@ static void free_l4_table(struct pfn_inf > > #endif /* __x86_64__ */ > > +static inline int cmpxchg8b_user(u64 *ptr, u64 oval, u64 nval) > +{ > + u32 o_hi = oval >> 32; > + u32 o_lo = oval & 0xffffffff; > + u32 n_hi = nval >> 32; > + u32 n_lo = nval & 0xffffffff; > + u32 rc = 0; > + > + __asm__ __volatile__ ( > + "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n" > + "2:\n" > + ".section .fixup,\"ax\"\n" > + "3: movl $1, %[rc]\n" > + " jmp 2b\n" > + ".previous\n" > + ".section __ex_table,\"a\"\n" > + " .align 4\n" > + " .long 1b,3b\n" > + ".previous" > + : "=d" (o_hi), "=a" (o_lo), "=c" (n_hi), "=b" (n_lo), > + [rc] "=r" (rc), [ptr] "m" (*__xg((volatile void *)ptr)) > + : "0" (o_hi), "1" (o_lo) > + : "memory"); > + if ((o_hi != (oval >> 32)) || > + (o_lo != (oval & 0xffffffff))) > + rc = 1; > + return rc; > +} > > static inline int update_l1e(l1_pgentry_t *pl1e, > l1_pgentry_t ol1e, > l1_pgentry_t nl1e) > { > - /* FIXME: breaks with PAE */ > +#if defined(__i386__) && defined(CONFIG_X86_PAE) > + > + int rc; > + > + rc = cmpxchg8b_user(pl1e, l1e_get_value(ol1e), > + l1e_get_value(nl1e)); > + if (unlikely(rc)) > + { > + MEM_LOG("Failed to update %llx -> %llx [pae]\n", > + l1e_get_value(ol1e), l1e_get_value(nl1e)); > + return 0; > + } > + return 1; > + > +#else > + > unsigned long o = l1e_get_value(ol1e); > unsigned long n = l1e_get_value(nl1e); > > @@ -850,8 +905,9 @@ static inline int update_l1e(l1_pgentry_ > l1e_get_value(ol1e), l1e_get_value(nl1e), o); > return 0; > } > - > return 1; > + > +#endif > } > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel