Date:	Sat, 4 Apr 2009 12:17:09 +0200
From:	Renzo Davoli <renzo@...unibo.it>
To:	Américo Wang <xiyou.wangcong@...il.com>
Cc:	linux-kernel@...r.kernel.org, Jeff Dike <jdike@...toit.com>,
	user-mode-linux-devel@...ts.sourceforge.net,
	mtk.manpages@...il.com, Roland McGrath <roland@...hat.com>
Subject: Re: [PATCH 0/2] ptrace_vm: ptrace for syscall emulation virtual
	machines

Dear Cong,

On Mon, Mar 30, 2009 at 12:32:28AM +0800, Américo Wang wrote:
> On Wed, Mar 25, 2009 at 12:47:53AM +0100, Renzo Davoli wrote:
> >1- the code is now extremely simple 
> Why adding a new request for ptrace is harder? I don't think so. :)
> >2- if we define a different tag for syscall (e.g. PTRACE_VM), we need also
> >different tags for PTRACE_VM_SINGLESTEP, PTRACE_VM_SINGLEBLOCK and maybe
> >others in the future.
> >Using the addr field we don't need this multiplication of tags
> >(and we could soon delete PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP).
> Yes? We could also remove PTRACE_SYSEMU* if we had PTRACE_VM to replace
> it. I would like to hear more from you on this point.

I am sorry for the delay of this reply.
I am having a quite busy time and I like to analyze problems before
giving answers.

Let me point out that the primary issue is the following one:
PTRACE_SYSEMU is a limited feature. It is twofold limited:
- it is supported only for x86
- it provides support for "total" virtual machines like User-Mode Linux
and it is useless for "partial" virtual machine like fakeroot-ng, umview
or others.
I have published a proposal for a new support that is able to overpass
these limits. PTRACE_SYSEMU/SYSEMU_SINGLESTEP could be deprecated.
There will be some cleaning up of the kernel code when the deprecated
tags are eliminated.

Whether it is nicer to use the existing tags or defining new tags is a
secondary issue. I support the hypothesis of reusing the existing tags and use
values in the addr field but if the community says that it is nicer/better to
have separate tags it is quite easy to update my patches (and umview).

Let us discuss this latter point.

PTRACE has a number of "resume" tags:
PTRACE_SYSCALL, PTRACE_SINGLESTEP, PTRACE_SINGLEBLOCK and currently
PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP.
all these call are managed in the code by the ptrace_resume function.

My patch #1 (kernel/ptrace.c function ptrace_request) forwards the addr 
parameter to ptrace_resume which saves the VM bits in some bits inside 
task_struct's ptrace field.

If we want to use different tags like:
PTRACE_VM PTRACE_VM_SINGLESTEP PTRACE_VM_SINGLEBLOCK:
the better implementation I can envision, adds another group of switch cases
as follows (kernel/ptrace.c function ptrace_request):
 ....
 #ifdef PTRACE_SINGLESTEP
   case PTRACE_SINGLESTEP:
 #endif
 #ifdef PTRACE_SINGLEBLOCK
   case PTRACE_SINGLEBLOCK:
 #endif
 #ifdef PTRACE_SYSEMU
   case PTRACE_SYSEMU:
   case PTRACE_SYSEMU_SINGLESTEP:
 #endif
   case PTRACE_SYSCALL:
   case PTRACE_CONT:
     return ptrace_resume(child, request, 0, data);
+/* statements added for PTRACE_VM management */
+#ifdef PTRACE_VM
+  case PTRACE_VM:
+#ifdef PTRACE_VM_SINGLESTEP
+  case PTRACE_VM_SINGLESTEP:
+#endif
+#ifdef PTRACE_VM_SINGLEBLOCK
+  case PTRACE_VM_SINGLEBLOCK:
+#endif
+    return ptrace_resume(child, PTRACE_VM_TAGS_MAPPING(request), addr, data);
+#endif
....
  
where PTRACE_VM_TAGS_MAPPING is a macro that maps each VM tag to the
corresponding one (PTRACE_VM->PTRACE_SYSCALL, 
PTRACE_VM_SINGLESTEP->PTRACE_SINGLESTEP, and so on).

Other implementations inside ptrace_resume would end up in a nightmare of
definitions like is_vm, is_vm_singlestep ... similar to is_singlestep,
is_sysemu_singlestep and a lot of cases and subcases in ptrace_resume: more
lines of code and more cases during the execution.

This is my position:
There are three points in favour of reusing the existing tags and 
one against.

In favour #1: Do we really want to add some tags to the jungle of PTRACE tags?
at the end of this message I have attached a list of the tags defined in
2.6.29.  I know that the set of integers although limited is quite large, but
the idea to have a slalom between already defined constant is not nice.  (Note
that some globally defined tags like PTRACE_ATTACH have been redefined in some
architectures. PTRACE_SYSEMU value itself has been redefined in frv, blackfin,
sh). Of course, each architecture uses its own constants but this is not an 
example of good design.

We could use a high order bit to map VM calls to make things simpler.
e.g.
#define PTRACE_VM_BIT 0x80000000
#define PTRACE_VM (PTRACE_VM_BIT | PTRACE_SYSCALL)
#define PTRACE_VM_SINGLESTEP (PTRACE_VM_BIT | PTRACE_SINGLESTEP)
...
in this way:
#define PTRACE_VM_TAGS_MAPPING(REQ) ((REQ) & ~PTRACE_VM_BIT)

In favour#2: Using specific PTRACE_VM* tags the definitions above should be
added in the specific ptrace.h files for each architecture supporting PTRACE_VM
(those supporting tracehook), skipping the others, thus adding a number of
lines.  When a new "resume" tag is added, all these ptrace.h files will need
to be patched.  
If we use the addr field, it is simply ignored for architectures where
the new feature is not supported, there is no need of architecure dependent
definitions and new tags are managed automagically.
Summing up "in favour" #1 and #2 the saving in terms of lines of code is
higher with the current patch than by using specific tags.

In favour#3: The more cases in a switch the more code is generated.
Using specific PTRACE_VM* tags there are some more instruction to be executed
twice for each system call.  The loss of performance is very low, but not null.
When dealing with virtual machines speed is a must, and when possible, the
single useless machine instruction should be avoided.

Against #1: Using the patch I proposed which encodes PTRACE_VM tags in the
addr field, there is a remote possibility to break some existing code.
gdb, fakeroot_ng, UML uses addr = 0. strace for some unknown reasons
uses addr = (char *) 1. 
The addr tags in my patches do not use the less significant bit.
The tools I tested on a kernel with my patch installed worked like a charm.

I'd summarize this discussion as follows. I have a strong position about
the need of a more powerful/portable support for virtual machines based
in system call emulation (and partial emulation).
I have a weak preference about the interface, it is my opinion that the one 
I proposed is better for some reasons:
- less lines of code/more lines of code will be saved when SYSEMU is eliminated
- a bit faster
- no need for arch dependent definitions.

I have in my todo file to update the ptrace(2) man page for this new feature.
I'll do it as soon as we decide for the best interface.

Thank you to have read my posting up to here ;-)

	renzo

-----
Summary of ptrace tags. 
Entries without leading path are globally defined in include/linux/ptrace.h.

#define PTRACE_TRACEME		   0
#define PTRACE_PEEKTEXT		   1
#define PTRACE_PEEKDATA		   2
#define PTRACE_PEEKUSR		   3
#define PTRACE_POKETEXT		   4
#define PTRACE_POKEDATA		   5
#define PTRACE_POKEUSR		   6
#define PTRACE_CONT		   7
#define PTRACE_KILL		   8
#define PTRACE_SINGLESTEP	   9
arch/sparc/include/asm/ptrace.h:#define PTRACE_SPARC_DETACH       11
include/asm-frv/ptrace.h:#define PTRACE_GETREGS		12
include/asm-m32r/ptrace.h:#define PTRACE_GETREGS		12
include/asm-mn10300/ptrace.h:#define PTRACE_GETREGS            12
arch/arm/include/asm/ptrace.h:#define PTRACE_GETREGS		12
arch/avr32/include/asm/ptrace.h:#define PTRACE_GETREGS		12
arch/blackfin/include/asm/ptrace.h:#define PTRACE_GETREGS            12
arch/cris/include/asm/ptrace.h:#define PTRACE_GETREGS            12
arch/h8300/include/asm/ptrace.h:#define PTRACE_GETREGS            12
arch/ia64/include/asm/ptrace.h:#define PTRACE_SINGLEBLOCK	12	/* resume execution until next branch */
arch/m68k/include/asm/ptrace.h:#define PTRACE_GETREGS            12
arch/mips/include/asm/ptrace.h:#define PTRACE_GETREGS		12
arch/parisc/include/asm/ptrace.h:#define PTRACE_SINGLEBLOCK	12	/* resume execution until next branch */
arch/powerpc/include/asm/ptrace.h:#define PTRACE_GETREGS            12
arch/sparc/include/asm/ptrace.h:#define PTRACE_GETREGS            12
arch/sh/include/asm/ptrace.h:#define PTRACE_GETREGS		12	/* General registers */
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_GETREGS            12
arch/xtensa/include/asm/ptrace.h:#define PTRACE_GETREGS		12
include/asm-frv/ptrace.h:#define PTRACE_SETREGS		13
include/asm-m32r/ptrace.h:#define PTRACE_SETREGS		13
include/asm-mn10300/ptrace.h:#define PTRACE_SETREGS            13
arch/arm/include/asm/ptrace.h:#define PTRACE_SETREGS		13
arch/avr32/include/asm/ptrace.h:#define PTRACE_SETREGS		13
arch/blackfin/include/asm/ptrace.h:#define PTRACE_SETREGS            13	/* ptrace signal  */
arch/cris/include/asm/ptrace.h:#define PTRACE_SETREGS            13
arch/h8300/include/asm/ptrace.h:#define PTRACE_SETREGS            13
arch/ia64/include/asm/ptrace.h:#define PTRACE_OLD_GETSIGINFO	13	/* (replaced by PTRACE_GETSIGINFO in <linux/ptrace.h>)  */
arch/powerpc/include/asm/ptrace.h:#define PTRACE_SETREGS            13
arch/m68k/include/asm/ptrace.h:#define PTRACE_SETREGS            13
arch/mips/include/asm/ptrace.h:#define PTRACE_SETREGS		13
arch/sparc/include/asm/ptrace.h:#define PTRACE_SETREGS            13
arch/sh/include/asm/ptrace.h:#define PTRACE_SETREGS		13
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_SETREGS            13
arch/xtensa/include/asm/ptrace.h:#define PTRACE_SETREGS		13
include/asm-frv/ptrace.h:#define PTRACE_GETFPREGS	14
include/asm-mn10300/ptrace.h:#define PTRACE_GETFPREGS          14
arch/arm/include/asm/ptrace.h:#define PTRACE_GETFPREGS	14
arch/ia64/include/asm/ptrace.h:#define PTRACE_OLD_SETSIGINFO	14	/* (replaced by PTRACE_SETSIGINFO in <linux/ptrace.h>)  */
arch/m68k/include/asm/ptrace.h:#define PTRACE_GETFPREGS          14
arch/mips/include/asm/ptrace.h:#define PTRACE_GETFPREGS		14
arch/powerpc/include/asm/ptrace.h:#define PTRACE_GETFPREGS          14
arch/sparc/include/asm/ptrace.h:#define PTRACE_GETFPREGS          14
arch/sh/include/asm/ptrace.h:#define PTRACE_GETFPREGS	14	/* FPU registers */
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_GETFPREGS          14
include/asm-frv/ptrace.h:#define PTRACE_SETFPREGS	15
include/asm-mn10300/ptrace.h:#define PTRACE_SETFPREGS          15
arch/arm/include/asm/ptrace.h:#define PTRACE_SETFPREGS	15
arch/m68k/include/asm/ptrace.h:#define PTRACE_SETFPREGS          15
arch/mips/include/asm/ptrace.h:#define PTRACE_SETFPREGS		15
arch/powerpc/include/asm/ptrace.h:#define PTRACE_SETFPREGS          15
arch/sparc/include/asm/ptrace.h:#define PTRACE_SETFPREGS          15
arch/sh/include/asm/ptrace.h:#define PTRACE_SETFPREGS	15
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_SETFPREGS          15
#define PTRACE_ATTACH		  16
arch/sparc/include/asm/ptrace.h:#define PTRACE_READDATA           16
#define PTRACE_DETACH		  17
arch/sparc/include/asm/ptrace.h:#define PTRACE_WRITEDATA          17
arch/arm/include/asm/ptrace.h:#define PTRACE_GETWMMXREGS	18
arch/ia64/include/asm/ptrace.h:#define PTRACE_GETREGS		18	/* get all registers (pt_all_user_regs) in one shot */
arch/mips/include/asm/ptrace.h:/* #define PTRACE_GETFPXREGS		18 */
arch/powerpc/include/asm/ptrace.h:#define PTRACE_GETVRREGS	18
arch/sparc/include/asm/ptrace.h:#define PTRACE_READTEXT           18
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_GETFPXREGS         18
arch/xtensa/include/asm/ptrace.h:#define PTRACE_GETXTREGS	18
arch/arm/include/asm/ptrace.h:#define PTRACE_SETWMMXREGS	19
arch/ia64/include/asm/ptrace.h:#define PTRACE_SETREGS		19	/* set all registers (pt_all_user_regs) in one shot */
arch/mips/include/asm/ptrace.h:/* #define PTRACE_SETFPXREGS		19 */
arch/powerpc/include/asm/ptrace.h:#define PTRACE_SETVRREGS	19
arch/sparc/include/asm/ptrace.h:#define PTRACE_WRITETEXT          19
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_SETFPXREGS         19
arch/xtensa/include/asm/ptrace.h:#define PTRACE_SETXTREGS	19
arch/powerpc/include/asm/ptrace.h:#define PTRACE_GETEVRREGS	20
arch/sparc/include/asm/ptrace.h:#define PTRACE_GETFPAREGS         20
include/asm-m32r/ptrace.h:#define PTRACE_OLDSETOPTIONS	21
arch/arm/include/asm/ptrace.h:#define PTRACE_OLDSETOPTIONS	21
arch/ia64/include/asm/ptrace.h:#define PTRACE_OLDSETOPTIONS	21
arch/mips/include/asm/ptrace.h:#define PTRACE_OLDSETOPTIONS	21
arch/powerpc/include/asm/ptrace.h:#define PTRACE_SETEVRREGS	21
arch/s390/include/asm/ptrace.h:#define PTRACE_OLDSETOPTIONS         21
arch/s390/include/asm/ptrace.h:#define PTRACE_PROT                       21
arch/sparc/include/asm/ptrace.h:#define PTRACE_SETFPAREGS         21
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_OLDSETOPTIONS      21
arch/arm/include/asm/ptrace.h:#define PTRACE_GET_THREAD_AREA	22
arch/powerpc/include/asm/ptrace.h:#define PTRACE_GETREGS64	  22
arch/sparc/include/asm/ptrace.h:#define PTRACE_GETREGS64	  22
arch/arm/include/asm/ptrace.h:#define PTRACE_SET_SYSCALL	23
arch/powerpc/include/asm/ptrace.h:#define PTRACE_SETREGS64	  23
arch/sparc/include/asm/ptrace.h:#define PTRACE_SETREGS64	  23
#define PTRACE_SYSCALL		  24
arch/arm/include/asm/ptrace.h:#define PTRACE_GETCRUNCHREGS	25
arch/mips/include/asm/ptrace.h:#define PTRACE_GET_THREAD_AREA	25
arch/powerpc/include/asm/ptrace.h:#define PTRACE_GET_DEBUGREG	25
arch/sparc/include/asm/ptrace.h:#define PTRACE_GETFPREGS64	  25
arch/arm/include/asm/ptrace.h:#define PTRACE_SETCRUNCHREGS	26
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_GET_THREAD_AREA    25
arch/mips/include/asm/ptrace.h:#define PTRACE_SET_THREAD_AREA	26
arch/powerpc/include/asm/ptrace.h:#define PTRACE_SET_DEBUGREG	26
arch/sparc/include/asm/ptrace.h:#define PTRACE_SETFPREGS64	  26
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_SET_THREAD_AREA    26
arch/powerpc/include/asm/ptrace.h:#define PTRACE_GETVSRREGS	27
arch/powerpc/include/asm/ptrace.h:#define PTRACE_SETVSRREGS	28
arch/x86/include/asm/ptrace-abi.h:# define PTRACE_ARCH_PRCTL	  30
arch/um/include/shared/ptrace_user.h:#define PTRACE_SYSEMU 31
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_SYSEMU		  31
include/asm-frv/ptrace.h:#define PTRACE_GETFDPIC		31	/* get the ELF fdpic loadmap address */
arch/blackfin/include/asm/ptrace.h:#define PTRACE_GETFDPIC           31
arch/sh/include/asm/ptrace.h:#define PTRACE_GETFDPIC		31	/* get the ELF fdpic loadmap address */
arch/um/include/shared/ptrace_user.h:#define PTRACE_SYSEMU_SINGLESTEP 32
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_SYSEMU_SINGLESTEP  32
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_SINGLEBLOCK	33	/* resume execution until next branch */
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_BTS_CONFIG	40
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_BTS_STATUS	41
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_BTS_SIZE		42
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_BTS_GET		43
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_BTS_CLEAR	44
arch/x86/include/asm/ptrace-abi.h:#define PTRACE_BTS_DRAIN	45
arch/sh/include/asm/ptrace.h:#define	PTRACE_GETDSPREGS	55	/* DSP registers */
arch/sh/include/asm/ptrace.h:#define	PTRACE_SETDSPREGS	56
arch/mips/include/asm/ptrace.h:#define PTRACE_PEEKTEXT_3264	0xc0
arch/mips/include/asm/ptrace.h:#define PTRACE_PEEKDATA_3264	0xc1
arch/mips/include/asm/ptrace.h:#define PTRACE_POKETEXT_3264	0xc2
arch/mips/include/asm/ptrace.h:#define PTRACE_POKEDATA_3264	0xc3
arch/mips/include/asm/ptrace.h:#define PTRACE_GET_THREAD_AREA_3264	0xc4
arch/mips/include/asm/ptrace.h:#define PTRACE_GET_WATCH_REGS	0xd0
arch/mips/include/asm/ptrace.h:#define PTRACE_SET_WATCH_REGS	0xd1
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_POKEUSR_3264  0x90
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_PEEKUSR_3264  0x91
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_POKEDATA_3264 0x92
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_POKETEXT_3264 0x93
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_PEEKDATA_3264 0x94
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_PEEKTEXT_3264 0x95
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_SETFPREGS	0x96	/* Set FPRs 0 - 31 */
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_GETFPREGS	0x97	/* Get FPRs 0 - 31 */
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_SETREGS	0x98	/* Set GPRs 0 - 31 */
arch/powerpc/include/asm/ptrace.h:#define PPC_PTRACE_GETREGS	0x99	/* Get GPRs 0 - 31 */
#define PTRACE_SETOPTIONS	0x4200
#define PTRACE_GETEVENTMSG	0x4201
#define PTRACE_GETSIGINFO	0x4202
#define PTRACE_SETSIGINFO	0x4203
arch/s390/include/asm/ptrace.h:#define PTRACE_PEEKUSR_AREA           0x5000
arch/s390/include/asm/ptrace.h:#define PTRACE_POKEUSR_AREA           0x5001
arch/s390/include/asm/ptrace.h:#define PTRACE_PEEKTEXT_AREA	      0x5002
arch/s390/include/asm/ptrace.h:#define PTRACE_PEEKDATA_AREA	      0x5003
arch/s390/include/asm/ptrace.h:#define PTRACE_POKETEXT_AREA	      0x5004
arch/s390/include/asm/ptrace.h:#define PTRACE_POKEDATA_AREA 	      0x5005
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists