From: Joel Soete <joel.soete@freebel.net>
To: Randolph Chung <randolph@tausq.org>
Cc: parisc-linux@parisc-linux.org
Subject: Re: [parisc-linux] Need help to improve uaccess.h patch
Date: Sat, 05 Oct 2002 20:22:21 +0000 [thread overview]
Message-ID: <3D9F49FD.2040703@freebel.net> (raw)
In-Reply-To: 20021004160833.GB5602@tausq.org
Randolph Chung wrote:
> Note: i haven't tested the patch, just some first-pass comments:
>
>
>> #else
>> #define LDD_KERNEL(ptr) __get_kernel_asm("ldd",ptr)
>> #define LDD_USER(ptr) __get_user_asm("ldd",ptr)
>>@@ -68,27 +73,28 @@
>> ({ \
>> register long __gu_err __asm__ ("r8") = 0; \
>> register long __gu_val __asm__ ("r9") = 0; \
>>+ \
>>+ unsigned int __tmp = 0; \
>> \
>> if (segment_eq(get_fs(),KERNEL_DS)) { \
>> switch (sizeof(*(ptr))) { \
>>- case 1: __get_kernel_asm("ldb",ptr); break; \
>>- case 2: __get_kernel_asm("ldh",ptr); break; \
>>- case 4: __get_kernel_asm("ldw",ptr); break; \
>>- case 8: LDD_KERNEL(ptr); break; \
>>+ case 1: __get_kernel_asm("ldb",x, ptr); break; \
>>+ case 2: __get_kernel_asm("ldh",x, ptr); break; \
>>+ case 4: __get_kernel_asm("ldw",x, ptr); break; \
>>+ case 8: LDD_KERNEL(x, ptr); break; \
>> default: BUG(); break; \
>> } \
>> } \
>> else { \
>> switch (sizeof(*(ptr))) { \
>>- case 1: __get_user_asm("ldb",ptr); break; \
>>- case 2: __get_user_asm("ldh",ptr); break; \
>>- case 4: __get_user_asm("ldw",ptr); break; \
>>- case 8: LDD_USER(ptr); break; \
>>+ case 1: __get_user_asm("ldb",x, ptr); break; \
>>+ case 2: __get_user_asm("ldh",x, ptr); break; \
>>+ case 4: __get_user_asm("ldw",x, ptr); break; \
>>+ case 8: LDD_USER(x, ptr); break; \
>> default: BUG(); break; \
>> } \
>> } \
>> \
>>- (x) = (__typeof__(*(ptr))) __gu_val; \
>> __gu_err; \
>> })
>
>
> why did you push the cast into the asm functions?
>
Not actualy in the asm but well in the std #define __get_user_asm just
after asm, because with gcc-3.0.4 I do not find any way to access (in
__asm extension) the two integer part of a long long (which C allow). So
I try (as better as I can) to use ptr and this cast does not seems
anymore usefull in 64bit parts.
[Well, in mips 32 bits uaccess.h, I see that some prefix as D, M, L can
be used to do this access. But if I test those one (example %D1) with
hppa, gcc asks me send a bug report. I think that I would have to do it
to improve gcc-3.3?]
>
>>-#define __get_user_asm(ldx,ptr) \
>>+#define __get_kernel_asm_64(x, ptr) \
>>+ __asm__ ("\n\tldw\t4(%1),%3\n" \
>>+ "1:\tstw\t%3,4(%2)\n" \
>>+ "\tldw\t0(%1),%3\n" \
>>+ "2:\tstw\t%3,0(%2)\n" \
>>+ "3:\n" \
>>+ "\t.section __ex_table,\"a\"\n" \
>>+ "\t.word\t1b\n" \
>>+ "\t.word\t(3b-1b)+1\n" \
>>+ "\t.word\t2b\n" \
>>+ "\t.word\t(3b-2b)+1\n" \
>
>
> what's with the +1?
That is my main doubt :(
In this uaccess.h, I read (but i am not quit sure to have understand all
fine aspect) that we have to 'jump' after the erronious code (for me
3b-[12]b + 1 ? am i wrong? ). And understand +3 in get_user_asm because
we would have to jump after the cast "(x) = (__typeof__(*(ptr)))
__gu_val;". Is it wrong?
>
>
>> #define __put_user(x,ptr) \
>> ({ \
>> register long __pu_err __asm__ ("r8") = 0; \
>>+ unsigned long long X = (unsigned long long) x; \
>
>
> why do you need this cast?
I need in fact to assigned x to X because I encounter put_user(0,PTR)
(which became later .. (&0)... which is wrong). And the cast is just to
avoid anoying warning if x is not a long long.
>
>
>>+ unsigned long long * __tmp = 0; \
>
>
> if __tmp is only used in the STD macro, you should eiter pass it to the
> macro or define it inside that macro.
>
>
>> if (segment_eq(get_fs(),KERNEL_DS)) { \
>> switch (sizeof(*(ptr))) { \
>> case 1: __put_kernel_asm("stb",x,ptr); break; \
>> case 2: __put_kernel_asm("sth",x,ptr); break; \
>> case 4: __put_kernel_asm("stw",x,ptr); break; \
>>- case 8: STD_KERNEL(x,ptr); break; \
>>+ case 8: STD_KERNEL(X,ptr); break; \
>> default: BUG(); break; \
>> } \
>> } \
>>@@ -152,7 +193,7 @@
>> case 1: __put_user_asm("stb",x,ptr); break; \
>> case 2: __put_user_asm("sth",x,ptr); break; \
>> case 4: __put_user_asm("stw",x,ptr); break; \
>>- case 8: STD_USER(x,ptr); break; \
>>+ case 8: STD_USER(X,ptr); break; \
>> default: BUG(); break; \
>> } \
>> } \
>>@@ -200,6 +241,22 @@
>> : "=r"(__pu_err) \
>> : "r"(ptr), "r"(x), "0"(__pu_err))
>>
>>+#define __put_kernel_asm_64(X, ptr) \
>>+ __asm__ __volatile__ ( \
>>+ "\n\tldw\t0(%2),%3\n" \
>>+ "1:\tstw\t%3,0(%1)\n" \
>>+ "\tldw\t4(%2),%3\n" \
>>+ "2:\tstw\t%3,4(%1)\n" \
>>+ "3:\n" \
>>+ "\t.section __ex_table,\"a\"\n" \
>>+ "\t.word\t1b\n" \
>>+ "\t.word\t(3b-1b)+1\n" \
>>+ "\t.word\t2b\n" \
>>+ "\t.word\t(3b-2b)+1\n" \
>
>
> as above, why +1?
As early mentionned I do not well understand the original +1 (or +3) in
the original put_user (get_user) and use what I think to have understand
(which seems to be wrong :( )
>
>
>>+ "\t.previous" \
>>+ : "=r"(__pu_err) \
>>+ : "r"(ptr), "r"(&(X)), "r"(__tmp), "0"(__pu_err))
>>+
>> #define __put_user_asm(stx,x,ptr) \
>> __asm__ __volatile__ ( \
>> "\n1:\t" stx "\t%2,0(%%sr3,%1)\n" \
>>@@ -210,6 +267,23 @@
>> "\t.previous" \
>> : "=r"(__pu_err) \
>> : "r"(ptr), "r"(x), "0"(__pu_err))
>>+
>>+#define __put_user_asm_64(X, ptr) \
>>+ __asm__ __volatile__ ( \
>>+ "\n\tldw\t0(%2),%3\n" \
>>+ "1:\tstw\t%3,0(%%sr3,%1)\n" \
>>+ "\tldw\t4(%2),%3\n" \
>>+ "2:\tstw\t%3,4(%%sr3,%1)\n" \
>>+ "3:\n" \
>>+ "\t.section __ex_table,\"a\"\n" \
>>+ "\t.word\t1b\n" \
>>+ "\t.word\t(3b-1b)+1\n" \
>>+ "\t.word\t2b\n" \
>>+ "\t.word\t(3b-2b)+1\n" \
>
>
> ditto, this probably should be +3 to indicate a userspace address.
Ok (I do not have the oppotunity to test this patch in wrong conditions
ie segv :(, I would so much), I have to analyse in more details
exception table handler to try to better understand this.
>
> your code also creates an artifact that may or may not be ok -- if you
> do a 64-bit put_user or get_user on a 32-bit aligned address, this will
> work with a 32-bit kernel but will trap on a 64-bit kernel. i guess
> that's ok, but just something to keep in mind.
IMHO the absolute solution would be, to have in 32bits a pseudo ldd asm
extension which do the actual 64bits processor ldd job. Don't you think so?
[as %D1, I also test ldd with a gcc 32bits but it also ask me to send a
bug report. What do you thing? would I have to report this one (I doubt
: it is not an actual pa1.1 code but more a dream of mine)?]
Thanks a lot for your kind attention and relevant remarks,
Joel
next prev parent reply other threads:[~2002-10-05 19:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2002-10-04 15:24 [parisc-linux] Need help to improve uaccess.h patch Joel.soete
2002-10-04 16:08 ` Randolph Chung
2002-10-05 20:22 ` Joel Soete [this message]
2002-10-05 22:10 ` John David Anglin
2002-10-06 0:07 ` Randolph Chung
-- strict thread matches above, loose matches on Subject: below --
2002-10-07 16:12 jsoe0708
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3D9F49FD.2040703@freebel.net \
--to=joel.soete@freebel.net \
--cc=parisc-linux@parisc-linux.org \
--cc=randolph@tausq.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.