A lot of comments, and I changed a *lot*, I'll send up the commits when I address the next few comments.
See https://github.com/brho/akaros/compare/master...hugelgupf:gdbserver-support-new?expand=1 for what it currently looks like. Chris On Tue, Aug 9, 2016 at 1:59 PM Barret Rhoden <[email protected]> wrote: > Hi - > > Regarding patch 2, it looks good overall. Comments inline below. > > > From 44e1f7b6303886cfdd705a64a9d457cb924c83ee Mon Sep 17 00:00:00 2001 > > From: Christopher Koch <[email protected]> > > Date: Thu, 9 Jun 2016 11:12:19 -0700 > > Subject: Defines protocol between gdbserver and a 2LS. > > > > The protocol is called D9. > > > > This will be part of parlib and in its current form can be used with any > > 2LS. Also sets up some helpful structures to deal with indexing / listing > > threads in parlib. > > > > The protocol is intended to be asynchronous and looks a lot like 9P > (hence, > > the name). Some of the issues around async are not solved yet; e.g. the > > client side (gdbserver) currently expects all operations to be serialized > > without interleaving responses. E.g. it could not currently handle this > > sequence of operations: > > Does the server side currently have similar limitations? > No, though it doesn't matter much for the server -- under the current assumptions made, the server will only be sent one request at a time. It is however possible to send multiple requests to the 2LS and get multiple responses if one were to write a different client. > > > TWAIT > > TREADMEM > > RWAIT > > RREADMEM > > > > Change-Id: Ib7c8fd800b1550f6b72ce0feae57c7f5dbdf11f6 > > Signed-off-by: Christopher Koch <[email protected]> > > --- > > kern/arch/x86/trap.c | 1 + > > user/parlib/debug.c | 470 > ++++++++++++++++++++++++++++++++- > > user/parlib/include/parlib/debug.h | 67 +++++ > > user/parlib/include/parlib/x86/debug.h | 32 +++ > > user/parlib/uthread.c | 8 + > > user/parlib/x86/debug.c | 18 ++ > > 6 files changed, 594 insertions(+), 2 deletions(-) > > create mode 100644 user/parlib/include/parlib/debug.h > > create mode 100644 user/parlib/include/parlib/x86/debug.h > > create mode 100644 user/parlib/x86/debug.c > > > > diff --git a/kern/arch/x86/trap.c b/kern/arch/x86/trap.c > > index 83ad3532edbf..53615af930d2 100644 > > --- a/kern/arch/x86/trap.c > > +++ b/kern/arch/x86/trap.c > > @@ -549,6 +549,7 @@ static void trap_dispatch(struct hw_trapframe *hw_tf) > > > > // Handle processor exceptions. > > switch(hw_tf->tf_trapno) { > > + case T_DEBUG: > > case T_BRKPT: > > enable_irq(); > > monitor(hw_tf); > > diff --git a/user/parlib/debug.c b/user/parlib/debug.c > > index 46a3e4ed72c1..9f967f09f109 100644 > > --- a/user/parlib/debug.c > > +++ b/user/parlib/debug.c > > @@ -1,9 +1,17 @@ > > +#include <fcntl.h> > > +#include <parlib/assert.h> > > #include <parlib/common.h> > > +#include <parlib/debug.h> > > +#include <parlib/ros_debug.h> > > +#include <parlib/event.h> > > #include <parlib/parlib.h> > > -#include <stdio.h> > > -#include <unistd.h> > > #include <parlib/spinlock.h> > > #include <ros/common.h> > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <sys/types.h> > > +#include <sys/stat.h> > > +#include <unistd.h> > > > > int akaros_printf(const char *format, ...) > > { > > @@ -75,3 +83,461 @@ void __print_func_exit(const char *func, const char > *file) > > printf("---- %s()\n", func); > > spinlock_unlock(&lock); > > } > > + > > +static void handle_debug_msg(struct syscall *sysc); > > +static void debug_read(int fd, void *buf, size_t len); > > +static int handle_one_msg(struct event_mbox *ev_mbox); > > +static void debug_read_handler(struct event_queue *ev_q); > > +static int debug_send_packet(int fd, void *data, size_t size); > > +static int debug_send_error(uint32_t errnum); > > +static int debug_send_response(struct d9_header *packet); > > +static void *d9_read_packet(struct d9_header *hdr); > > +static int d9_treadmem(struct d9_header *hdr, void *data); > > +static int d9_tstoremem(struct d9_header *hdr, void *data); > > +static int d9_tfetchreg(struct d9_header *hdr, void *data); > > The definitions for these functions (below) are usually missing the > 'static' part. Same for some helpers. > Done. > > > + > > +/* ev_q for read syscall on debug pipe. */ > > +static struct event_queue *debug_read_ev_q; > > +static struct syscall debug_read_sysc; > > +static int debug_fd; > > + > > +/* async_read_buf is the buffer used to read the header of a packet. > > + * > > + * We can only have one of these reads at a time anyway. */ > > +static char async_read_buf[sizeof(struct d9_header)]; > > + > > +/* d9_ops are user-supplied routines that fill in the response packet > with the > > + * appropriate information and/or do the requested operation. These may > block. > > + */ > > +static struct d9_ops *d9_ops; > > + > > +/* A message handler is an internal routine that allocates the > appropriate > > + * response packet and coordinates sending the appropriate response to > > + * gdbserver. */ > > +typedef int (*message_handler)(struct d9_header *hdr, void *data); > > +#define D9_HANDLER(x) (x - D9_TREADMEM) > > +static message_handler msg_handlers[D9_HANDLER(D9_NUM_MESSAGES)]; > > + > > +/* queue_handle_one_message extracts one message from the mbox and > calls the > > + * appropriate handler for that message. */ > > +int queue_handle_one_msg(struct event_mbox *ev_mbox) > > This func can be static. > Done. > > > +{ > > + struct event_msg msg; > > + struct syscall *sysc; > > + > > + if (!extract_one_mbox_msg(ev_mbox, &msg)) > > + return 0; > > + > > + assert(msg.ev_type == EV_SYSCALL); > > + sysc = msg.ev_arg3; > > + assert(sysc); > > + handle_debug_msg(sysc); > > + return 1; > > +} > > + > > +/* queue_read_handler is the event queue handler for the async read > evq. */ > > +void queue_read_handler(struct event_queue *ev_q) > > This func can be static. > Done. > > > +{ > > + assert(ev_q); > > + assert(ev_q->ev_mbox); > > + while (queue_handle_one_msg(ev_q->ev_mbox)) > > + ; > > +} > > + > > +void d9_server_init(struct schedule_ops *ops, struct d9_ops *dops) > > +{ > > + int fd; > > + int p[2]; > > + char buf[60]; > > + int ret; > > + > > + /* Set up parlib queue for asynchronous read notifications. */ > > + debug_read_ev_q = get_eventq(EV_MBOX_UCQ); > > + debug_read_ev_q->ev_flags = EVENT_IPI | EVENT_INDIR | > EVENT_SPAM_INDIR | > > + EVENT_WAKEUP; > > + debug_read_ev_q->ev_handler = queue_read_handler; > > + > > + /* Set up d9 handlers. */ > > + msg_handlers[D9_HANDLER(D9_TREADMEM)] = d9_treadmem; > > + msg_handlers[D9_HANDLER(D9_TSTOREMEM)] = d9_tstoremem; > > + msg_handlers[D9_HANDLER(D9_TFETCHREG)] = d9_tfetchreg; > > + > > + /* Set d9 ops. */ > > + d9_ops = dops; > > Are we ever going to change d9_ops at runtime? I noticed down in > uthread_2ls_init() that you dynamically set the values for dops, then > here we assign it to d9_ops. My guess is this never changes, (i.e. no > specific 2LS will override them), so we can just set up d9_ops statically. > Actually, yes, specific 2LS' should have the ability to override these operations. For example, for the VM 2LS, I'll have a separate read_memory and write_memory that convert the address from guest virtual to host virtual. > > Also, if these aren't dynamic, then you won't need to do things like > this: > > + if (d9_ops == NULL || d9_ops->store_memory == NULL) > + return debug_send_error(EBADF /* TODO: better error code > */); > > Since there will always be a store_memory operation. > > > + > > + /* Open a pipe and post it in #srv. > > + * TODO(chrisko): add special file #proc/PID/debug that works like > #srv. > > + */ > > When are these srv files removed? After running for a bit, do you have > a large number of srv files (one for every process that ever existed)? > We probably can deal with this with the #proc file in the future. > They aren't currently :( I hadn't thought of that. Should I do the #proc file first? > > > + ret = pipe(p); > > + if (ret < 0) > > + return; > > + > > + snprintf(buf, sizeof(buf), "#srv/debug-%d", getpid()); > > + fd = open(buf, O_WRONLY|O_CREAT, 0666); > > + if (fd < 0) > > + return; > > + > > + snprintf(buf, sizeof(buf), "%d", p[1]); > > + if (write(fd, buf, strlen(buf)) != strlen(buf)) > > + return; > > For all of these error cases, I'd just assert success / panic on > failure. If you just return, we won't notice the bug. Since this is > startup code, it should always succeed. > Done. > > > + > > + close(p[1]); > > + > > + debug_fd = p[0]; > > + debug_read(debug_fd, async_read_buf, sizeof(struct d9_header)); > > + > > + asm("int $3"); > > You can call breakpoint(), which is arch-independent. Though what is > the point of this breakpoint? > Leftover from old code, I took it out. > > > +} > > + > > +void debug_read(int fd, void *buf, size_t len) > > This func can be static. > Done. > > > +{ > > + memset(&debug_read_sysc, 0, sizeof(struct syscall)); > > + syscall_async(&debug_read_sysc, SYS_read, fd, buf, len); > > + > > + if (!register_evq(&debug_read_sysc, debug_read_ev_q)) > > + handle_debug_msg(&debug_read_sysc); > > This is correct, and no need to change it now, but for those curious, > you can sign up for an event on a syscall before you submit it, and then > *only* handle the completion from the event handler. > > > +} > > + > > +void handle_debug_msg(struct syscall *sysc) > > This func can be static. > Done. > > > +{ > > + struct d9_header *hdr; > > + > > + switch (sysc->retval) { > > + case 0: > > + panic("read syscall: got 0 bytes."); > > + break; > > + > > + case -1: > > + panic("read failed"); > > + break; > > + > > + default: > > + if (sysc->retval != sizeof(struct d9_header)) > > + panic("2LS debug: should have received D9 > header."); > > + > > + /* The buffer used will be reused before we are done > handling this call. > > + */ > > This might have issues. If you delayed debug_read until after you ran > the handlers, then you could avoid these mallocs, memcpys, and other > stuff. You'd just have the one async_read_buf, which always has your > current command. > > Other than code complexity, there might be problems with having multiple > commands operating at the same time. Since you debug_read() before > running a handler, there might be a scenario where another command > arrives and we run two handlers in parallel (say another vcore handled > the message). If you do the debug_read() after handling the message, > that will never happen - you enforce the synchronicity. > > The current gdb setup might not trigger this, but other > "2LS-interrogations" in the future might not have the same assumptions. > Agreed. Done. > > > + hdr = malloc(sizeof(struct d9_header)); > > + if (hdr == NULL) > > + panic("2LS debug: failed to malloc."); > > + > > + memcpy(hdr, (void *) sysc->arg1, sizeof(struct d9_header)); > > + > > + void *data = d9_read_packet(hdr); > > + > > + /* It is now safe to set off another async read syscall, > reusing the > > + * buffer. */ > > + debug_read(debug_fd, async_read_buf, sizeof(struct > d9_header)); > > + > > + /* Call the appropriate handler for this packet. */ > > + if (msg_handlers[D9_HANDLER(hdr->msg_type)] != NULL) > > + msg_handlers[D9_HANDLER(hdr->msg_type)](hdr, data); > > + else > > + panic("2LS debug: no message handler found."); > > + > > + if (data) > > + free(data); > > + free(hdr); > > + break; > > + } > > +} > > + > > +/* alloc_packet allocates memory for a packet of given type. */ > > +struct d9_header *alloc_packet(size_t pck_len, enum d9_msg_t msg_type) > > Given the type, it seems like you usually know the pck_len. Other than > D9_RREADMEM, the other callers are all passing in a static value. Is > there a reasonable limit to the size of the readlen? For instance, can > we tell GDB that PacketSize=1024 or something? > RREADMEM and TSTOREMEM (which isn't part of this yet I think). There currently isn't, because I don't know what the max gdb will ask for is. (I'll think about this some more.) > > Not a huge deal, but it would avoid any confusion with getting the wrong > pck_len and would remove this error check: > > > +{ > > + struct d9_header *rhdr; > > + > > + if (pck_len >= UINT_MAX - sizeof(struct d9_header)) > > + panic("2LS debug: packet too long."); > > Incidentally, pck_len is a size_t, not a uint. > This check is here because pck_len is a size_t, but d9_header's size field is a uint32. Stupidly, I left out the 32 here. > > Basically, we're saying you can't ask for more than about 4GB of RAM, > which is ridiculously large. > > > + > > + rhdr = calloc(sizeof(struct d9_header) + pck_len, 1); > > + if (rhdr == NULL) > > + return NULL; > > + > > + rhdr->msg_type = msg_type; > > + rhdr->size = sizeof(struct d9_header) + pck_len; > > + return rhdr; > > +} > > + > > +/* d9_read_memory is the d9_ops func called to fulfill a TREADMEM > request. */ > > +int d9_read_memory(const struct d9_treadmem *req, struct d9_rreadmem > *resp) > > +{ > > + memcpy(resp->data, (void *) req->address, req->length); > > + return 0; > > +} > > + > > +/* d9_store_memory is the d9_ops func called to fulfill a TSTOREMEM > request. */ > > +int d9_store_memory(const struct d9_tstoremem *req, uint32_t length) > > +{ > > + memcpy((void *) req->address, req->data, length); > > + return 0; > > +} > > I would have expected length to come from the d9_tstoremem struct, > instead of being implied by the packet size (down below). Though you > might want a sanity check regardless. > This was in the next patch, moving it up. > > > + > > +/* d9_tstoremem allocates the response packet, calls the user-supplied > ops > > + * function for storing memory, and sends the response packet. */ > > +int d9_tstoremem(struct d9_header *hdr, void *data) > > +{ > > + int length, ret; > > + struct d9_header *resp; > > + struct d9_tstoremem *tstoremem = (struct d9_tstoremem *) data; > > + > > + if (d9_ops == NULL || d9_ops->store_memory == NULL) > > + return debug_send_error(EBADF /* TODO: better error code > */); > > + > > + length = hdr->size - sizeof(struct d9_header) - 1 - > > + sizeof(struct d9_tstoremem); > > Are you sure you want to subtract 1 here? I imagine that 1 came from > data[1] in the struct definition. I'd just remove it from the struct > (which I mention below). > > If anything, I think you'd add the 1 Since when you subtract > sizeof(struct d9_tstoremem), that removed the extra 1 byte that actually > belonged to length. Anyway, I'd just remove the '1' completely. > same, next patch, moving it. > > > + > > + /* Call user-supplied routine for filling in response packet. */ > > + ret = d9_ops->store_memory(tstoremem, length); > > + > > + if (ret < 0) { > > + /* Error with errno -ret. */ > > + return debug_send_error(-ret); > > + } > > + > > + /* Successful response. */ > > + resp = alloc_packet(0, D9_RSTOREMEM); > > + if (resp == NULL) > > + return debug_send_error(ENOMEM); > > + > > + ret = debug_send_response(resp); > > + free(resp); > > + return ret; > > +} > > + > > +/* d9_treadmem allocates the response packet, calls the user-supplied > ops > > + * function for reading from memory, and sends the response packet. */ > > +int d9_treadmem(struct d9_header *hdr, void *data) > > +{ > > + int ret; > > + struct d9_header *rpack; > > + struct d9_rreadmem *rreadmem; > > + struct d9_treadmem *treadmem = (struct d9_treadmem *) data; > > + > > + if (d9_ops == NULL || d9_ops->read_memory == NULL) > > + return debug_send_error(EBADF /* TODO */); > > + > > + /* Allocate response packet. */ > > + rpack = alloc_packet(treadmem->length, D9_RREADMEM); > > + if (rpack == NULL) > > + return debug_send_error(ENOMEM); > > + > > + rreadmem = (struct d9_rreadmem *) (rpack + 1); > > As an example, if the d9_rreadmem contains the header, you won't need to > do this pointer arithmetic - you just alloc the struct directly. Not a > big deal though. Yeah, done. This ended up being way nicer. There's some changes still to be made. > > > + > > + /* Call user-supplied routine for filling in response packet. */ > > + ret = d9_ops->read_memory(treadmem, rreadmem); > > + > > + if (ret < 0) { > > + /* Error with errno -ret. */ > > + free(rpack); > > + return debug_send_error(-ret); > > + } > > + > > + /* Successful response. */ > > + ret = debug_send_response(rpack); > > + free(rpack); > > + return ret; > > +} > > + > > +/* d9_tfetchreg allocates the response packet, finds the appropriate > thread > > + * structure, calls the user-supplied ops function for reading its > registers, > > + * and sends the response packet. */ > > +int d9_tfetchreg(struct d9_header *hdr, void *data) > > +{ > > + int ret; > > + struct uthread *t; > > + struct d9_header *rpack; > > + struct d9_rfetchreg *resp; > > + struct d9_tfetchreg *req = (struct d9_tfetchreg *) data; > > + > > + if (d9_ops == NULL || d9_ops->fetch_registers == NULL) > > + return debug_send_error(EBADF /* TODO */); > > + > > + /* Find the appropriate thread. */ > > + t = uthread_get_thread_by_id(req->threadid); > > + if (t == NULL) > > + return debug_send_error(EBADF /* TODO */); > > + > > + /* Allocate response packet. */ > > + rpack = alloc_packet(sizeof(struct d9_rfetchreg), D9_RFETCHREG); > > + if (rpack == NULL) > > + return debug_send_error(ENOMEM); > > + > > + resp = (struct d9_rfetchreg *) (rpack + 1); > > + > > + /* Call user-supplied routine for filling in response packet. */ > > + ret = d9_ops->fetch_registers(t, &(resp->regs)); > > + > > + if (ret < 0) { > > + /* Error with errno -ret. */ > > + free(rpack); > > + return debug_send_error(-ret); > > + } > > + > > + /* Successful response. */ > > + ret = debug_send_response(rpack); > > + free(rpack); > > + return ret; > > +} > > + > > +/* debug_send_error sends an error response to gdbserver. */ > > +int debug_send_error(uint32_t errnum) > > +{ > > + int ret; > > + struct d9_rerror *rerror; > > + struct d9_header *rpack; > > + > > + /* Allocate error response packet. */ > > + rpack = alloc_packet(sizeof(struct d9_rerror), D9_RERROR); > > + if (rpack == NULL) > > + return -1; /* What now? */ > > + > > + /* Fill in error number. */ > > + rerror = (struct d9_rerror *) (rpack + 1); > > + rerror->errnum = errnum; > > + > > + ret = debug_send_response(rpack); > > + free(rpack); > > + return ret; > > +} > > + > > +/* debug_send_response sends a response to gdbserver. */ > > +int debug_send_response(struct d9_header *packet) > > +{ > > + return debug_send_packet(debug_fd, (void *) packet, packet->size); > > +} > > + > > +/* d9_read_packet reads the rest of the packet given the packet header. > > + * > > + * The header contains the size of the packet. */ > > +void *d9_read_packet(struct d9_header *hdr) > > +{ > > + size_t msg_size = hdr->size - sizeof(struct d9_header); > > + void *data = NULL; > > + int ret; > > + > > + if (msg_size > 0) { > > + /* Read the remaining data of this packet. */ > > + data = calloc(msg_size, 1); > > + if (data == NULL) > > + return NULL; > > + > > + if (read(debug_fd, data, msg_size) != msg_size) { > > + free(data); > > + return NULL; > > + } > > We should handle partial reads here. It's probably unlikely with gdb > and the current pipe implementation, but they might only write a byte at > a time or something. Especially if we ever have bigger payloads coming > in for a read (maybe a storemem?). > done. > > > + } > > + > > + return data; > > +} > > + > > +int debug_send_packet(int fd, void *data, size_t size) > > +{ > > + ssize_t wlen, len_written = 0; > > + > > + while (len_written < size) { > > + wlen = write(fd, (uint8_t *) data + len_written, size - > len_written); > > + if (wlen < 0) { > > + if (errno == EINTR) > > + continue; > > + else > > + return -1; > > + } > > + len_written += wlen; > > + } > > + return 0; > > +} > > + > > +int check_error_packet(struct d9_header *hdr, enum d9_msg_t msg_type) > > +{ > > + struct d9_rerror *rerror = NULL; > > + > > + /* Msg is of expected type -- no error. */ > > + if (hdr->msg_type == msg_type) > > + return 0; > > + > > + if (hdr->msg_type == D9_RERROR) { > > + /* Got error message. */ > > + rerror = (struct d9_rerror *) (hdr + 1); > > + errno = rerror->errnum; > > + } else { > > + /* Neither got the expected message nor an error message. > */ > > + errno = EIO; > > + } > > + return 1; > > +} > > + > > +int debug_read_memory(int fd, uintptr_t address, uint32_t length, > uint8_t *buf) > > Not sure, but do we want to somehow mark the client side operations by a > naming convention? > Next patch had this, moved it up (d9c_ versus d9s_). > > > +{ > > + size_t return_packet_size; > > + struct d9_header *hdr, *rhdr; > > + struct d9_treadmem *req; > > + struct d9_rreadmem *resp; > > + > > + hdr = alloc_packet(sizeof(struct d9_treadmem), D9_TREADMEM); > > + if (hdr == NULL) > > + return -1; > > + > > + req = (struct d9_treadmem *) (hdr + 1); > > + req->address = address; > > + req->length = length; > > + > > + int ret = debug_send_packet(fd, hdr, hdr->size); > > Don't forget to free hdr (looks you aren't). > Oops. > > > + > > + /* Block on return packet. */ > > + > > + /* TODO(chrisko): need to do this asynchronously */ > > What's the deal here? Does the client need to be async? > Welp, yes. I know I said earlier that it wouldn't need to be, and strictly speaking it doesn't, but handling everything becomes a lot nicer when it's asynchronous. In akaros_wait, we'd have to launch a thread anyway to read asynchronously (one thread to call waitpid, one thread to read from the pipe, and the main thread waits for a response from either). > > > + return_packet_size = sizeof(struct d9_header) + length; > > + char pkt_buf[return_packet_size]; > > Depending on the size of return_packet_size, this might be a little > dangerous. A lot of our 2LSs have a limit to the stacksize. For > pthreads, which gdbserver uses, it is currently 4MB. How big are these > return packets? > So far, the largest that I've seen by observation is 64 bytes for a read memory or store memory request, and the register packets are larger (208, I think). I think it'd be useful to impose an upper limit at some point, but with how I just rewrote things, there is none. The messages that have dynamic size are on the heap, most of the others are on the stack and <= 208 bytes. > > > + > > + if (read(fd, pkt_buf, return_packet_size) <= 0) { > > + perror("d9 read memory"); > > + return -1; > > + } > > As mentioned above, you want to handle partial reads. You should be > able to use the same helper function that the server side ops use. > done. > > > + > > + rhdr = (struct d9_header *) pkt_buf; > > + if (check_error_packet(rhdr, D9_RREADMEM)) { > > + perror("d9 read memory"); > > + return -1; > > + } > > + > > + resp = (struct d9_rreadmem *) (rhdr + 1); > > + memcpy(buf, resp->data, length); > > + return 0; > > +} > > + > > +int debug_fetch_registers(int fd, uint64_t tid, struct d9_regs *regs) > > +{ > > + size_t return_packet_size; > > + struct d9_header *hdr, *rhdr; > > + struct d9_tfetchreg *req; > > + > > + hdr = alloc_packet(sizeof(struct d9_tfetchreg), D9_TFETCHREG); > > + if (hdr == NULL) > > + return -1; > > + > > + req = (struct d9_tfetchreg *) (hdr + 1); > > + req->threadid = tid; > > + > > + int ret = debug_send_packet(fd, hdr, hdr->size); > > Don't forget to free hdr (looks you aren't). > Oops. > > + > > + /* Block on return packet. */ > > + > > + /* TODO(chrisko): need to do this asynchronously */ > > + return_packet_size = sizeof(struct d9_header) + sizeof(struct > d9_rfetchreg); > > + char pkt_buf[return_packet_size]; > > + > > + if (read(fd, pkt_buf, return_packet_size) != return_packet_size) { > > + perror("d9 fetch registers"); > > + return -1; > > + } > > + > > + rhdr = (struct d9_header *) pkt_buf; > > + if (check_error_packet(rhdr, D9_RFETCHREG)) { > > + perror("d9 fetch registers"); > > + return -1; > > + } > > + > > + memcpy(regs, rhdr + 1, sizeof(struct d9_regs)); > > + return 0; > > +} > > diff --git a/user/parlib/include/parlib/debug.h > b/user/parlib/include/parlib/debug.h > > new file mode 100644 > > index 000000000000..68b462dca58f > > --- /dev/null > > +++ b/user/parlib/include/parlib/debug.h > > @@ -0,0 +1,67 @@ > > +#pragma once > > + > > +#include <parlib/arch/debug.h> > > +#include <parlib/uthread.h> > > + > > +enum d9_msg_t { > > + D9_TREADMEM = 10, > > + D9_RREADMEM, > > + D9_TSTOREMEM, > > + D9_RSTOREMEM, > > + D9_TFETCHREG, > > + D9_RFETCHREG, > > + D9_TERROR, /* Do not use. */ > > + D9_RERROR, > > + D9_NUM_MESSAGES, /* Do not use. */ > > +}; > > + > > +struct d9_header { > > + uint32_t size; > > + uint32_t msg_type; > > +}; > > + > > +struct d9_rerror { > > + uint32_t errnum; > > +}; > > Is the actual format for all of these messages something like: > > struct d9_foo { > struct d9_header hdr; > something_specific_to_foo; > } > > If so, it's usually better to embed the header into the outer struct. > See how Linux did it (with perf) for details: > tools/dev-util/perf/perf_format.h. I think we want to make sure they > are __attribute__((packed)), to avoid confusion in the future. > > done. > On a related note, in the future, we probably want to make these message > structures endian-independent. Right now, everything has to be > host-endian. For an endian-independent example, check out what Davide > did with Akaros's perf: kern/arch/x86/ros/perfmon.h > > + > > +struct d9_treadmem { > > + uintptr_t address; > > + uint32_t length; > > +}; > > + > > +struct d9_rreadmem { > > + uint8_t data[1]; /* Must be the last member. */ > > +}; > > + > > +struct d9_tstoremem { > > + uintptr_t address; > > + uint8_t data[1]; /* Must be the last member. */ > > For these two, I think you don't need the '1'. It makes the size > calculations confusing too. > yeah, fixed that in the next commit, moved it up. > > > +}; > > + > > +struct d9_rstoremem { > > +}; > > + > > +struct d9_tfetchreg { > > + uint64_t threadid; > > +}; > > + > > +struct d9_rfetchreg { > > + struct d9_regs regs; /* Architecture-dependent. */ > > +}; > > + > > +/* Server-side ops. */ > > +struct d9_ops { > > + int (*read_memory)(const struct d9_treadmem *req, struct > d9_rreadmem *resp); > > + int (*store_memory)(const struct d9_tstoremem *req, uint32_t > length); > > + int (*fetch_registers)(struct uthread *t, struct d9_regs *resp); > > +}; > > + > > +/* Server-side functions. */ > > +void d9_server_init(struct schedule_ops *ops, struct d9_ops *debug_ops); > > +int d9_read_memory(const struct d9_treadmem *req, struct d9_rreadmem > *resp); > > +int d9_store_memory(const struct d9_tstoremem *req, uint32_t length); > > If you statically set up d9_ops, then these don't need to be exposed > outside debug.c, I think. Though I think you'd still need the > arch-dependent d9_fetch_registers in a debug header. > see above for reason why a specific 2LS would want to not use these. > > > + > > +/* Client-side ops. */ > > +int debug_read_memory(int fd, uintptr_t address, uint32_t length, > uint8_t *buf); > > +int debug_store_memory(int fd, uintptr_t address, void *data, uint32_t > length); > > +int debug_fetch_registers(int fd, uint64_t tid, struct d9_regs *regs); > > diff --git a/user/parlib/include/parlib/x86/debug.h > b/user/parlib/include/parlib/x86/debug.h > > new file mode 100644 > > index 000000000000..2fe452744abf > > --- /dev/null > > +++ b/user/parlib/include/parlib/x86/debug.h > > @@ -0,0 +1,32 @@ > > +#pragma once > > + > > +#include <parlib/uthread.h> > > + > > +struct d9_regs { > > + uint64_t reg_rax; > > + uint64_t reg_rbx; > > + uint64_t reg_rcx; > > + uint64_t reg_rdx; > > + uint64_t reg_rsp; > > + uint64_t reg_rbp; > > + uint64_t reg_rsi; > > + uint64_t reg_rdi; > > + uint64_t reg_rip; > > + uint64_t reg_r8; > > + uint64_t reg_r9; > > + uint64_t reg_r10; > > + uint64_t reg_r11; > > + uint64_t reg_r12; > > + uint64_t reg_r13; > > + uint64_t reg_r14; > > + uint64_t reg_r15; > > + uint64_t reg_eflags; > > + uint64_t reg_cs; > > + uint64_t reg_ss; > > + uint64_t reg_ds; > > + uint64_t reg_es; > > + uint64_t reg_fs; > > + uint64_t reg_gs; > > +}; > > + > > +int d9_fetch_registers(struct uthread *t, struct d9_regs *resp); > > diff --git a/user/parlib/uthread.c b/user/parlib/uthread.c > > index 582e4cb50ec1..4b53d0f2b141 100644 > > --- a/user/parlib/uthread.c > > +++ b/user/parlib/uthread.c > > @@ -5,6 +5,7 @@ > > #include <parlib/arch/atomic.h> > > #include <parlib/arch/trap.h> > > #include <parlib/assert.h> > > +#include <parlib/debug.h> > > #include <parlib/event.h> > > #include <parlib/spinlock.h> > > #include <parlib/parlib.h> > > @@ -16,6 +17,7 @@ > > * pthreads, should override sched_ops in its init code. */ > > extern struct schedule_ops thread0_2ls_ops; > > struct schedule_ops *sched_ops = &thread0_2ls_ops; > > +struct d9_ops d9ops; > > > > __thread struct uthread *current_uthread = 0; > > /* ev_q for all preempt messages (handled here to keep 2LSs from > worrying > > @@ -178,6 +180,12 @@ void uthread_2ls_init(struct uthread *uthread, > struct schedule_ops *ops) > > cmb(); > > __vcore_context = FALSE; > > enable_notifs(0); /* will trigger a self_notif if we missed > a notif */ > > + > > + d9ops.read_memory = &d9_read_memory; > > + d9ops.store_memory = &d9_store_memory; > > + d9ops.fetch_registers = &d9_fetch_registers; > > + /* Make ourselves available for debugging */ > > + d9_server_init(ops, &d9ops); > > It might be better to do this at the end of uthread_lib_init(), instead > of in uthread_2ls_init. (More syscall and event infrastructure is set > up). And you probably don't need the d9ops stuff in uthread.c at all. > I don't remember quite what the reason was, but I wanted this to be done before we changed contexts. (I should have written down what the bug was...) Anyway, this was moved slightly in the next commit anyway, so I moved it here, too. > > > } > > > > /* Helper: tells the kernel our SCP is capable of going into vcore > context on > > diff --git a/user/parlib/x86/debug.c b/user/parlib/x86/debug.c > > new file mode 100644 > > index 000000000000..0cc976b7f4f1 > > --- /dev/null > > +++ b/user/parlib/x86/debug.c > > @@ -0,0 +1,18 @@ > > +#include <parlib/arch/debug.h> > > + > > +int d9_fetch_registers(struct uthread *t, struct d9_regs *regs) > > +{ > > + switch (t->u_ctx.type) { > > + case ROS_HW_CTX: > > + regs->reg_rax = t->u_ctx.tf.hw_tf.tf_rax; > > + regs->reg_rbx = t->u_ctx.tf.hw_tf.tf_rbx; > > + regs->reg_rip = t->u_ctx.tf.hw_tf.tf_rip; > > + break; > > + case ROS_SW_CTX: > > + regs->reg_rbx = t->u_ctx.tf.sw_tf.tf_rbx; > > + regs->reg_rip = t->u_ctx.tf.sw_tf.tf_rip; > > + break; > > Don't forget about all the other registers (e.g. r15). We also ought to > have support for reading the VM ctx registers too. They aren't going to > hit breakpoints (for now!), but when we stop-the-world, they'll get > caught up too. > Next commit, not moved yet. > > Barret > > > + } > > + > > + return 0; > > +} > > -- > > 2.8.0.rc3.226.g39d4020 > > > > -- > You received this message because you are subscribed to the Google Groups > "Akaros" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- Christopher Koch | Software Engineer | [email protected] | 650-214-3546 -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
