(Also, I'd like to mention that once the commits get this big, code review via email gets tedious and not very easy to read... +1 for tools.)
On Fri, Aug 12, 2016 at 3:54 AM Christopher Koch <[email protected]> wrote: > 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 > -- 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.
