Nicolas Eder <nicolas.e...@lauterbach.com> writes:
> From: neder <nicolas.e...@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
