Re: [PATCH v2 01/29] mcdstub initial commit, mcdstub file structure added

["Alex Bennée" <alex.bennee@linaro.org>]

Nicolas Eder <nicolas.eder@lauterbach.com> writes:

> From: neder <nicolas.eder@lauterbach.com>
>
> ---
>  include/exec/mcdstub.h   | 31 +++++++++++++
>  mcdstub/mcd_softmmu.c    | 85 +++++++++++++++++++++++++++++++++++
>  mcdstub/mcd_syscalls.c   |  0
>  mcdstub/mcd_tcp_server.c | 95 ++++++++++++++++++++++++++++++++++++++++
>  mcdstub/mcdstub.c        |  0
>  softmmu/vl.c             |  4 ++

I'm afraid you got caught up in some clean-up and this file is now under
the more correctly names:

  system/vl.c

>  6 files changed, 215 insertions(+)
>  create mode 100644 include/exec/mcdstub.h
>  create mode 100644 mcdstub/mcd_softmmu.c
>  create mode 100644 mcdstub/mcd_syscalls.c
>  create mode 100644 mcdstub/mcd_tcp_server.c
>  create mode 100644 mcdstub/mcdstub.c
>
> diff --git a/include/exec/mcdstub.h b/include/exec/mcdstub.h
> new file mode 100644
> index 0000000000..8afbc09367
> --- /dev/null
> +++ b/include/exec/mcdstub.h

include/exec is a bit of a dumping ground. Maybe
include/mcdstub/mcdstub.h to keep it cleaner? For gdbstub we further
divide into user, system and api.h but that might be overkill.

> @@ -0,0 +1,31 @@
> +#ifndef MCDSTUB_H
> +#define MCDSTUB_H
> +
> +#define DEFAULT_MCDSTUB_PORT "1234"
> +
> +/* MCD breakpoint/watchpoint types */
> +#define MCD_BREAKPOINT_SW        0
> +#define MCD_BREAKPOINT_HW        1
> +#define MCD_WATCHPOINT_WRITE     2
> +#define MCD_WATCHPOINT_READ      3
> +#define MCD_WATCHPOINT_ACCESS    4
> +
> +
> +/* Get or set a register.  Returns the size of the register.  */
> +typedef int (*gdb_get_reg_cb)(CPUArchState *env, GByteArray *buf, int reg);
> +typedef int (*gdb_set_reg_cb)(CPUArchState *env, uint8_t *buf, int reg);
> +void gdb_register_coprocessor(CPUState *cpu,
> +                              gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
> +                              int num_regs, const char *xml, int
> g_pos);

Why repeat these definitions instead just including gdbstub.h? We can
move the typedefs if you don't want to pollute the include with other
ephemera.

> +
> +/**
> + * mcdserver_start: start the mcd server
> + * @port_or_device: connection spec for mcd
> + *
> + * This is a TCP port
> + */
> +int mcdserver_start(const char *port_or_device);
> +
> +void gdb_set_stop_cpu(CPUState *cpu);
> +
> +#endif
> diff --git a/mcdstub/mcd_softmmu.c b/mcdstub/mcd_softmmu.c
> new file mode 100644
> index 0000000000..17e1d3ca1b
> --- /dev/null
> +++ b/mcdstub/mcd_softmmu.c

rename ;-)

> @@ -0,0 +1,85 @@
> +/*
> + * this handeles all system emulation functions for the mcdstub
> + */
> +
> +#include "exec/mcdstub.h"
> +
> +int mcdserver_start(const char *device)
> +{
> +    trace_gdbstub_op_start(device);
> +
> +    char gdbstub_device_name[128];
> +    Chardev *chr = NULL;
> +    Chardev *mon_chr;
> +
> +    if (!first_cpu) {
> +        error_report("gdbstub: meaningless to attach gdb to a "
> +                     "machine without any CPU.");
> +        return -1;
> +    }
> +
> +    if (!gdb_supports_guest_debug()) {
> +        error_report("gdbstub: current accelerator doesn't "
> +                     "support guest debugging");
> +        return -1;
> +    }
> +
> +    if (!device) {
> +        return -1;
> +    }
> +    if (strcmp(device, "none") != 0) {
> +        if (strstart(device, "tcp:", NULL)) {
> +            /* enforce required TCP attributes */
> +            snprintf(gdbstub_device_name, sizeof(gdbstub_device_name),
> +                     "%s,wait=off,nodelay=on,server=on", device);
> +            device = gdbstub_device_name;
> +        }
> +#ifndef _WIN32
> +        else if (strcmp(device, "stdio") == 0) {
> +            struct sigaction act;
> +
> +            memset(&act, 0, sizeof(act));
> +            act.sa_handler = gdb_sigterm_handler;
> +            sigaction(SIGINT, &act, NULL);
> +        }
> +#endif
> +        /*
> +         * FIXME: it's a bit weird to allow using a mux chardev here
> +         * and implicitly setup a monitor. We may want to break this.
> +         */
> +        chr = qemu_chr_new_noreplay("gdb", device, true, NULL);
> +        if (!chr) {
> +            return -1;
> +        }
> +    }
> +
> +    if (!gdbserver_state.init) {
> +        gdb_init_gdbserver_state();
> +
> +        qemu_add_vm_change_state_handler(gdb_vm_state_change, NULL);
> +
> +        /* Initialize a monitor terminal for gdb */
> +        mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB,
> +                                   NULL, NULL, &error_abort);
> +        monitor_init_hmp(mon_chr, false, &error_abort);
> +    } else {
> +        qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
> +        mon_chr = gdbserver_system_state.mon_chr;
> +        reset_gdbserver_state();
> +    }
> +
> +    create_processes(&gdbserver_state);
> +
> +    if (chr) {
> +        qemu_chr_fe_init(&gdbserver_system_state.chr, chr, &error_abort);
> +        qemu_chr_fe_set_handlers(&gdbserver_system_state.chr,
> +                                 gdb_chr_can_receive,
> +                                 gdb_chr_receive, gdb_chr_event,
> +                                 NULL, &gdbserver_state, NULL, true);
> +    }
> +    gdbserver_state.state = chr ? RS_IDLE : RS_INACTIVE;
> +    gdbserver_system_state.mon_chr = mon_chr;
> +    gdb_syscall_reset();

So this is showing a lot of c&p of the gdbstub but if the intention is
to re-use chunks of gdbstub we should do it properly rather than
duplicating code.

> +
> +    return 0;
> +}
> \ No newline at end of file
> diff --git a/mcdstub/mcd_syscalls.c b/mcdstub/mcd_syscalls.c
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/mcdstub/mcd_tcp_server.c b/mcdstub/mcd_tcp_server.c
> new file mode 100644
> index 0000000000..9a1baea2e4
> --- /dev/null
> +++ b/mcdstub/mcd_tcp_server.c
> @@ -0,0 +1,95 @@
> +#include <stdio.h>
> +#include <netdb.h>
> +#include <netinet/in.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +#include <unistd.h> // read(), write(), close()

#include "qemu/osdep.h" will bring in most of the standard stuff:

  Order include directives as follows:

  .. code-block:: c

      #include "qemu/osdep.h"  /* Always first... */
      #include <...>           /* then system headers... */
      #include "..."           /* and finally QEMU headers. */

  The "qemu/osdep.h" header contains preprocessor macros that affect the behavior
  of core system headers like <stdint.h>.  It must be the first include so that
  core system headers included by external libraries get the preprocessor macros
  that QEMU depends on.

See https://qemu.readthedocs.io/en/v8.1.0/devel/style.html

Running ./scripts/checkpatch.pl will pick a lot of this up.

> +#define MAX 80
> +#define DEFAULT_MCDSTUB_PORT "1234"
> +#define SA struct sockaddr
> +
> +// Function designed for chat between client and server.

c.f. style

> +void func(int connfd)
> +{
> +	char buff[MAX];
> +	int n;
> +	// infinite loop for chat
> +	for (;;) {
> +		bzero(buff, MAX);
> +
> +		// read the message from client and copy it in buffer
> +		read(connfd, buff, sizeof(buff));
> +		// print buffer which contains the client contents
> +		printf("From client: %s\t To client : ", buff);
> +		bzero(buff, MAX);
> +		n = 0;
> +		// copy server message in the buffer
> +		while ((buff[n++] = getchar()) != '\n')
> +			;
> +
> +		// and send that buffer to client
> +		write(connfd, buff, sizeof(buff));
> +
> +		// if msg contains "Exit" then server exit and chat ended.
> +		if (strncmp("exit", buff, 4) == 0) {
> +			printf("Server Exit...\n");
> +			break;
> +		}
> +	}
> +}
> +
> +// Driver function
> +int main()

why main? Is this a helper?

> +{
> +	int sockfd, connfd, len;
> +	struct sockaddr_in servaddr, cli;
> +
> +	// socket create and verification
> +	sockfd = socket(AF_INET, SOCK_STREAM, 0);
> +	if (sockfd == -1) {
> +		printf("socket creation failed...\n");
> +		exit(0);
> +	}
> +	else
> +		printf("Socket successfully created..\n");
> +	bzero(&servaddr, sizeof(servaddr));
> +
> +	// assign IP, PORT
> +	servaddr.sin_family = AF_INET;
> +	servaddr.sin_addr.s_addr = htonl(INADDR_ANY);
> +	servaddr.sin_port = htons(DEFAULT_MCDSTUB_PORT);
> +
> +	// Binding newly created socket to given IP and verification
> +	if ((bind(sockfd, (SA*)&servaddr, sizeof(servaddr))) != 0) {
> +		printf("socket bind failed...\n");
> +		exit(0);
> +	}
> +	else
> +		printf("Socket successfully binded..\n");
> +
> +	// Now server is ready to listen and verification
> +	if ((listen(sockfd, 5)) != 0) {
> +		printf("Listen failed...\n");
> +		exit(0);
> +	}
> +	else
> +		printf("Server listening..\n");
> +	len = sizeof(cli);
> +
> +	// Accept the data packet from client and verification
> +	connfd = accept(sockfd, (SA*)&cli, &len);
> +	if (connfd < 0) {
> +		printf("server accept failed...\n");
> +		exit(0);
> +	}
> +	else
> +		printf("server accept the client...\n");
> +
> +	// Function for chatting between client and server
> +	func(connfd);
> +
> +	// After chatting close the socket
> +	close(sockfd);
> +}

I think you need to make a design choice here about if MCD wants to
support *-user and system emulation or just system emulation. The
gdbstub does hand roll its own bind/accept code for *-user because it
doesn't include most of the rest of QEMU. However for system emulation
we use the chardev system which already provides and abstracts a lot of
this stuff. Then the code becomes simpler:

    if (chr) {
        qemu_chr_fe_init(&gdbserver_system_state.chr, chr, &error_abort);
        qemu_chr_fe_set_handlers(&gdbserver_system_state.chr,
                                 gdb_chr_can_receive,
                                 gdb_chr_receive, gdb_chr_event,
                                 NULL, &gdbserver_state, NULL, true);
    }

and you check need to plug in the handlers.

> diff --git a/mcdstub/mcdstub.c b/mcdstub/mcdstub.c
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 98e071e63b..3278f204ea 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1258,6 +1258,7 @@ struct device_config {
>          DEV_PARALLEL,  /* -parallel      */
>          DEV_DEBUGCON,  /* -debugcon */
>          DEV_GDB,       /* -gdb, -s */
> +        DEV_MCD,       /* -mcd */
>          DEV_SCLP,      /* s390 sclp */
>      } type;
>      const char *cmdline;
> @@ -3011,6 +3012,9 @@ void qemu_init(int argc, char **argv)
>              case QEMU_OPTION_gdb:
>                  add_device_config(DEV_GDB, optarg);
>                  break;
> +            case QEMU_OPTION_mcd:
> +                add_device_config(DEV_MCD, optarg);
> +                break;

This breaks the compile because presumably qemu-options.hx hasn't an
entry for this yet.

>              case QEMU_OPTION_L:
>                  if (is_help_option(optarg)) {
>                      list_data_dirs = true;


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro