Series comparison

-[Qemu-devel] [PATCH 0/3]
+[PATCH v10 0/3]
-From: Sameeh Jubran <sjubran@redhat.com>
+Hi,
-This series fixes qemu-ga's behaviour upon facing a missing serial/serial
+This new version removed the translate_fn() from patch 1 because it
-driver by listening to the serial device's events.
+wasn't removing the sign-extension for pentry as we thought it would.
 A more detailed explanation is given in the commit msg of patch 1.
-For more info on why this series is needed checkout the commit message
+We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and
-of the third patch and the following bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=990629.
+using it when we're running a 32-bit CPU. This worked with 32 bit
 'virt' machine booting with the -kernel option.
-Sameeh Jubran (3):
+If this approach doesn't work for the Xvisor use case, IMO  we should
-  qga: Channel: Add functions for checking serial status
+just filter kernel_load_addr bits directly as we were doing a handful of
-  qga: main: make qga config and socket activation global
+versions ago.
   qga: Prevent qemu-ga exit if serial doesn't exist
- Makefile            |   4 +
+Patches are based on current riscv-to-apply.next.
- qga/channel-posix.c |  54 ++++++++++
- qga/channel-win32.c |  60 +++++++++++
+Changes from v9:
- qga/channel.h       |   9 ++
+- patch 1:
- qga/main.c          | 284 ++++++++++++++++++++++++++++++++++++++++++++++------
+  - removed the translate_fn() callback
- qga/service-win32.h |   4 +
+  - return 'kernel_low' when running a 32-bit CPU
-files changed, 385 insertions(+), 30 deletions(-)
+- v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html
 Daniel Henrique Barboza (3):
   hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()
   hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
   hw/riscv/boot.c: make riscv_load_initrd() static
  hw/riscv/boot.c            | 96 +++++++++++++++++++++++---------------
  hw/riscv/microchip_pfsoc.c | 12 +----
  hw/riscv/opentitan.c       |  4 +-
  hw/riscv/sifive_e.c        |  4 +-
  hw/riscv/sifive_u.c        | 12 +----
  hw/riscv/spike.c           | 14 ++----
  hw/riscv/virt.c            | 12 +----
  include/hw/riscv/boot.h    |  3 +-
 files changed, 76 insertions(+), 81 deletions(-)
 --
-.9.4
+.39.1

-[Qemu-devel] [PATCH 2/3] qga: main: make qga config and socket activation global
+[PATCH v10 1/3] hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()
-From: Sameeh Jubran <sjubran@redhat.com>
+load_elf_ram_sym() will sign-extend 32 bit addresses. If a 32 bit QEMU
 guest happens to be running in a hypervisor that are using 64 bits to
 encode its address, kernel_entry can be padded with '1's and create
 problems [1].
-Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
+Using a translate_fn() callback in load_elf_ram_sym() to filter the
 padding from the address doesn't work. A more detailed explanation can
 be found in [2]. The short version is that glue(load_elf, SZ), from
 include/hw/elf_ops.h, will calculate 'pentry' (mapped into the
 'kernel_load_base' var in riscv_load_Kernel()) before using
 translate_fn(), and will not recalculate it after executing it. This
 means that the callback does not prevent the padding from
 kernel_load_base to appear.
 Let's instead use a kernel_low var to capture the 'lowaddr' value from
 load_elf_ram_sim(), and return it when we're dealing with 32 bit CPUs.
 [1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02281.html
 [2] https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00099.html
 Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
 ---
- qga/main.c | 44 +++++++++++++++++++++++---------------------
+ hw/riscv/boot.c            | 15 +++++++++++----
-file changed, 23 insertions(+), 21 deletions(-)
+ hw/riscv/microchip_pfsoc.c |  3 ++-
  hw/riscv/opentitan.c       |  3 ++-
  hw/riscv/sifive_e.c        |  3 ++-
  hw/riscv/sifive_u.c        |  3 ++-
  hw/riscv/spike.c           |  3 ++-
  hw/riscv/virt.c            |  3 ++-
  include/hw/riscv/boot.h    |  1 +
 files changed, 24 insertions(+), 10 deletions(-)
-diff --git a/qga/main.c b/qga/main.c
+diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
 index XXXXXXX..XXXXXXX 100644
---- a/qga/main.c
+--- a/hw/riscv/boot.c
-+++ b/qga/main.c
++++ b/hw/riscv/boot.c
-@@ -XXX,XX +XXX,XX @@ struct GAState {
+@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
-     GAPersistentState pstate;
+ }
- };
+ target_ulong riscv_load_kernel(MachineState *machine,
-+typedef struct GAConfig {
++                               RISCVHartArrayState *harts,
-+    char *channel_path;
+                                target_ulong kernel_start_addr,
-+    char *method;
+                                symbol_fn_t sym_cb)
-+    char *log_filepath;
+ {
-+    char *pid_filepath;
+     const char *kernel_filename = machine->kernel_filename;
-+#ifdef CONFIG_FSFREEZE
+-    uint64_t kernel_load_base, kernel_entry;
-+    char *fsfreeze_hook;
++    uint64_t kernel_load_base, kernel_entry, kernel_low;
-+#endif
-+    char *state_dir;
+     g_assert(kernel_filename != NULL);
-+#ifdef _WIN32
-+    const char *service;
+@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,
-+#endif
+      * the (expected) load address load address. This allows kernels to have
-+    gchar *bliststr; /* blacklist may point to this string */
+      * separate SBI and ELF entry points (used by FreeBSD, for example).
-+    GList *blacklist;
+      */
-+    int daemonize;
+-    if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
-+    GLogLevelFlags log_level;
+-                         NULL, &kernel_load_base, NULL, NULL, 0,
-+    int dumpconf;
++    if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL, NULL,
-+} GAConfig;
++                         &kernel_load_base, &kernel_low, NULL, 0,
                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
 -        return kernel_load_base;
 +        kernel_entry = kernel_load_base;
 +
- struct GAState *ga_state;
++        if (riscv_is_32bit(harts)) {
-+struct GAConfig *ga_config;
++            kernel_entry = kernel_low;
-+int ga_socket_activation;
++        }
- QmpCommandList ga_commands;
++
++        return kernel_entry;
- /* commands that are safe to issue while filesystems are frozen */
+     }
-@@ -XXX,XX +XXX,XX @@ static GList *split_list(const gchar *str, const gchar *delim)
-     return list;
+     if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL,
 diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/riscv/microchip_pfsoc.c
 +++ b/hw/riscv/microchip_pfsoc.c
@@ -XXX,XX +XXX,XX @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
                                                           firmware_end_addr);
 -        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
 +        kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
 +                                         kernel_start_addr, NULL);
          if (machine->initrd_filename) {
              riscv_load_initrd(machine, kernel_entry);
 diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/riscv/opentitan.c
 +++ b/hw/riscv/opentitan.c
@@ -XXX,XX +XXX,XX @@ static void opentitan_board_init(MachineState *machine)
      }
      if (machine->kernel_filename) {
 -        riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL);
 +        riscv_load_kernel(machine, &s->soc.cpus,
 +                          memmap[IBEX_DEV_RAM].base, NULL);
      }
  }
--typedef struct GAConfig {
+diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
--    char *channel_path;
+index XXXXXXX..XXXXXXX 100644
--    char *method;
+--- a/hw/riscv/sifive_e.c
--    char *log_filepath;
++++ b/hw/riscv/sifive_e.c
--    char *pid_filepath;
+@@ -XXX,XX +XXX,XX @@ static void sifive_e_machine_init(MachineState *machine)
--#ifdef CONFIG_FSFREEZE
+                           memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
--    char *fsfreeze_hook;
--#endif
+     if (machine->kernel_filename) {
--    char *state_dir;
+-        riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL);
--#ifdef _WIN32
++        riscv_load_kernel(machine, &s->soc.cpus,
--    const char *service;
++                          memmap[SIFIVE_E_DEV_DTIM].base, NULL);
 -#endif
 -    gchar *bliststr; /* blacklist may point to this string */
 -    GList *blacklist;
 -    int daemonize;
 -    GLogLevelFlags log_level;
 -    int dumpconf;
 -} GAConfig;
 -
  static void config_load(GAConfig *config)
  {
      GError *gerr = NULL;
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
  {
      int ret = EXIT_SUCCESS;
      GAState *s = g_new0(GAState, 1);
 -    GAConfig *config = g_new0(GAConfig, 1);
 +    GAConfig *config = ga_config = g_new0(GAConfig, 1);
      int socket_activation;
      config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
          config->method = g_strdup("virtio-serial");
      }
+ }
--    socket_activation = check_socket_activation();
-+    ga_socket_activation = socket_activation = check_socket_activation();
+diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
-     if (socket_activation > 1) {
+index XXXXXXX..XXXXXXX 100644
-         g_critical("qemu-ga only supports listening on one socket");
+--- a/hw/riscv/sifive_u.c
-         ret = EXIT_FAILURE;
++++ b/hw/riscv/sifive_u.c
@@ -XXX,XX +XXX,XX @@ static void sifive_u_machine_init(MachineState *machine)
          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
                                                           firmware_end_addr);
 -        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
 +        kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
 +                                         kernel_start_addr, NULL);
          if (machine->initrd_filename) {
              riscv_load_initrd(machine, kernel_entry);
 diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/riscv/spike.c
 +++ b/hw/riscv/spike.c
@@ -XXX,XX +XXX,XX @@ static void spike_board_init(MachineState *machine)
          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                           firmware_end_addr);
 -        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
 +        kernel_entry = riscv_load_kernel(machine, &s->soc[0],
 +                                         kernel_start_addr,
                                           htif_symbol_callback);
          if (machine->initrd_filename) {
 diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/riscv/virt.c
 +++ b/hw/riscv/virt.c
@@ -XXX,XX +XXX,XX @@ static void virt_machine_done(Notifier *notifier, void *data)
          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                           firmware_end_addr);
 -        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
 +        kernel_entry = riscv_load_kernel(machine, &s->soc[0],
 +                                         kernel_start_addr, NULL);
          if (machine->initrd_filename) {
              riscv_load_initrd(machine, kernel_entry);
 diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
 index XXXXXXX..XXXXXXX 100644
 --- a/include/hw/riscv/boot.h
 +++ b/include/hw/riscv/boot.h
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
                                   hwaddr firmware_load_addr,
                                   symbol_fn_t sym_cb);
  target_ulong riscv_load_kernel(MachineState *machine,
 +                               RISCVHartArrayState *harts,
                                 target_ulong firmware_end_addr,
                                 symbol_fn_t sym_cb);
  void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
 --
-.9.4
+.39.1

-[Qemu-devel] [PATCH 1/3] qga: Channel: Add functions for checking serial status
+[PATCH v10 2/3] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
-From: Sameeh Jubran <sjubran@redhat.com>
+The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
+the same steps when '-kernel' is used:
-This commit adds functions to check if the serial is
-connected/disconnected or else if it has been attached or detached.
+- execute load_kernel()
+- load init_rd()
-Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
+- write kernel_cmdline
 Let's fold everything inside riscv_load_kernel() to avoid code
 repetition. To not change the behavior of boards that aren't calling
 riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
 allow these boards to opt out from initrd loading.
 Cc: Palmer Dabbelt <palmer@dabbelt.com>
 Reviewed-by: Bin Meng <bmeng@tinylab.org>
 Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
 Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
 ---
- qga/channel-posix.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++
+ hw/riscv/boot.c            | 21 ++++++++++++++++++---
- qga/channel-win32.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
+ hw/riscv/microchip_pfsoc.c | 11 +----------
- qga/channel.h       |  9 ++++++++
+ hw/riscv/opentitan.c       |  3 ++-
-files changed, 123 insertions(+)
+ hw/riscv/sifive_e.c        |  3 ++-
+ hw/riscv/sifive_u.c        | 11 +----------
-diff --git a/qga/channel-posix.c b/qga/channel-posix.c
+ hw/riscv/spike.c           | 11 +----------
-index XXXXXXX..XXXXXXX 100644
+ hw/riscv/virt.c            | 11 +----------
---- a/qga/channel-posix.c
+ include/hw/riscv/boot.h    |  1 +
-+++ b/qga/channel-posix.c
+files changed, 27 insertions(+), 45 deletions(-)
-@@ -XXX,XX +XXX,XX @@ void ga_channel_free(GAChannel *c)
-     }
+diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
-     g_free(c);
+index XXXXXXX..XXXXXXX 100644
- }
+--- a/hw/riscv/boot.c
 +++ b/hw/riscv/boot.c
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
  target_ulong riscv_load_kernel(MachineState *machine,
                                 RISCVHartArrayState *harts,
                                 target_ulong kernel_start_addr,
 +                               bool load_initrd,
                                 symbol_fn_t sym_cb)
  {
      const char *kernel_filename = machine->kernel_filename;
      uint64_t kernel_load_base, kernel_entry, kernel_low;
 +    void *fdt = machine->fdt;
      g_assert(kernel_filename != NULL);
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,
              kernel_entry = kernel_low;
          }
 -        return kernel_entry;
 +        goto out;
      }
      if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL,
                         NULL, NULL, NULL) > 0) {
 -        return kernel_entry;
 +        goto out;
      }
      if (load_image_targphys_as(kernel_filename, kernel_start_addr,
                                 current_machine->ram_size, NULL) > 0) {
 -        return kernel_start_addr;
 +        kernel_entry = kernel_start_addr;
 +        goto out;
      }
      error_report("could not load kernel '%s'", kernel_filename);
      exit(1);
 +
-+static bool is_serial_present(GAChannelMethod method, const gchar *path,
++out:
-+    int *error_code)
++    if (load_initrd && machine->initrd_filename) {
-+{
++        riscv_load_initrd(machine, kernel_entry);
 +    int fd = -1;
 +    bool ret = true;
 +
 +    assert(error_code);
 +    *error_code = 0;
 +
 +    switch (method) {
 +    case GA_CHANNEL_VIRTIO_SERIAL:
 +        fd = qemu_open(path, O_RDWR | O_NONBLOCK
 +#ifndef CONFIG_SOLARIS
 +            | O_ASYNC
 +#endif
 +        );
 +        break;
 +    case GA_CHANNEL_ISA_SERIAL:
 +        fd = qemu_open(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
 +        break;
 +    default:
 +        ret = false;
 +    }
 +    if (fd < 0) {
 +        *error_code = errno;
 +        ret = false;
 +    } else {
 +        close(fd);
 +    }
 +    return ret;
 +}
 +
 +bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path)
 +{
 +    int error_code = 0;
 +    return is_serial_present(method, path, &error_code) ||
 +        error_code == EBUSY;
 +}
 +
 +bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
 +    bool is_serial_attached)
 +{
 +    int error_code = 0;
 +    return !is_serial_attached &&
 +        is_serial_present(method, path, &error_code);
 +}
 +bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
 +    bool is_serial_attached)
 +{
 +    int error_code = 0;
 +    return is_serial_attached && !is_serial_present(method, path, &error_code)
 +        && error_code == ENOENT;
 +}
 diff --git a/qga/channel-win32.c b/qga/channel-win32.c
 index XXXXXXX..XXXXXXX 100644
 --- a/qga/channel-win32.c
 +++ b/qga/channel-win32.c
@@ -XXX,XX +XXX,XX @@ void ga_channel_free(GAChannel *c)
      g_free(c->rstate.buf);
      g_free(c);
  }
 +
 +static bool is_serial_present(GAChannelMethod method, const gchar *path,
 +    DWORD *err)
 +{
 +    gchar newpath[MAXPATHLEN] = { 0 };
 +    bool ret = false;
 +
 +    assert(err);
 +
 +    if (method != GA_CHANNEL_VIRTIO_SERIAL && method != GA_CHANNEL_ISA_SERIAL) {
 +        g_critical("unsupported communication method");
 +        return false;
 +    }
 +
-+    if (method == GA_CHANNEL_ISA_SERIAL) {
++    if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
-+        snprintf(newpath, sizeof(newpath), "\\\\.\\%s", path);
++        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
-+    } else {
++                                machine->kernel_cmdline);
 +        g_strlcpy(newpath, path, sizeof(newpath));
 +    }
 +
-+    HANDLE handle = CreateFile(newpath, GENERIC_READ | GENERIC_WRITE, 0, NULL,
++    return kernel_entry;
-+        OPEN_EXISTING,
+ }
-+        FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL);
-+
+ void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
-+    if (handle == INVALID_HANDLE_VALUE) {
+diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
-+        *err = GetLastError();
+index XXXXXXX..XXXXXXX 100644
-+        ret = false;
+--- a/hw/riscv/microchip_pfsoc.c
-+    } else {
++++ b/hw/riscv/microchip_pfsoc.c
-+        ret = true;
+@@ -XXX,XX +XXX,XX @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
-+    }
+                                                          firmware_end_addr);
-+
-+    CloseHandle(handle);
+         kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
-+    return ret;
+-                                         kernel_start_addr, NULL);
-+}
+-
-+
+-        if (machine->initrd_filename) {
-+bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path)
+-            riscv_load_initrd(machine, kernel_entry);
-+{
+-        }
-+    DWORD err_code;
+-
-+    return is_serial_present(method, path, &err_code) ||
+-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-+        err_code == ERROR_ACCESS_DENIED;
+-            qemu_fdt_setprop_string(machine->fdt, "/chosen",
-+}
+-                                    "bootargs", machine->kernel_cmdline);
-+
+-        }
-+bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
++                                         kernel_start_addr, true, NULL);
-+    bool is_serial_attached)
-+{
+         /* Compute the fdt load address in dram */
-+    DWORD err_code;
+         fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
-+    return !is_serial_attached && is_serial_present(method, path, &err_code);
+diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
-+}
+index XXXXXXX..XXXXXXX 100644
-+
+--- a/hw/riscv/opentitan.c
-+bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
++++ b/hw/riscv/opentitan.c
-+    bool is_serial_attached)
+@@ -XXX,XX +XXX,XX @@ static void opentitan_board_init(MachineState *machine)
-+{
-+    DWORD err_code = NO_ERROR;
+     if (machine->kernel_filename) {
-+        /* In order to make sure the serial that qemu-ga uses is the one that
+         riscv_load_kernel(machine, &s->soc.cpus,
-+         * was detached. We'll get the error ERROR_FILE_NOT_FOUND when
+-                          memmap[IBEX_DEV_RAM].base, NULL);
-+         * attempting to call CreateFile with the serial path.
++                          memmap[IBEX_DEV_RAM].base,
-+        */
++                          false, NULL);
-+       return is_serial_attached && !is_serial_present(method, path, &err_code)
+     }
-+           && err_code == ERROR_FILE_NOT_FOUND;
+ }
-+}
-diff --git a/qga/channel.h b/qga/channel.h
+diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
 index XXXXXXX..XXXXXXX 100644
---- a/qga/channel.h
+--- a/hw/riscv/sifive_e.c
-+++ b/qga/channel.h
++++ b/hw/riscv/sifive_e.c
-@@ -XXX,XX +XXX,XX @@
+@@ -XXX,XX +XXX,XX @@ static void sifive_e_machine_init(MachineState *machine)
- #ifndef QGA_CHANNEL_H
- #define QGA_CHANNEL_H
+     if (machine->kernel_filename) {
+         riscv_load_kernel(machine, &s->soc.cpus,
-+#ifndef _WIN32
+-                          memmap[SIFIVE_E_DEV_DTIM].base, NULL);
-+#define SUBSYSTEM_VIRTIO_SERIAL "virtio-ports";
++                          memmap[SIFIVE_E_DEV_DTIM].base,
-+#define SUBSYSTEM_ISA_SERIAL "isa-serial";
++                          false, NULL);
-+#endif
+     }
+ }
- typedef struct GAChannel GAChannel;
+diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
-@@ -XXX,XX +XXX,XX @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
+index XXXXXXX..XXXXXXX 100644
- void ga_channel_free(GAChannel *c);
+--- a/hw/riscv/sifive_u.c
- GIOStatus ga_channel_read(GAChannel *c, gchar *buf, gsize size, gsize *count);
++++ b/hw/riscv/sifive_u.c
- GIOStatus ga_channel_write_all(GAChannel *c, const gchar *buf, gsize size);
+@@ -XXX,XX +XXX,XX @@ static void sifive_u_machine_init(MachineState *machine)
-+bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path);
+                                                          firmware_end_addr);
-+bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
-+    bool is_serial_attached);
+         kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
-+bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
+-                                         kernel_start_addr, NULL);
-+    bool is_serial_attached);
+-
+-        if (machine->initrd_filename) {
- #endif
+-            riscv_load_initrd(machine, kernel_entry);
 -        }
 -
 -        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
 -            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
 -                                    machine->kernel_cmdline);
 -        }
 +                                         kernel_start_addr, true, NULL);
      } else {
         /*
          * If dynamic firmware is used, it doesn't know where is the next mode
 diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/riscv/spike.c
 +++ b/hw/riscv/spike.c
@@ -XXX,XX +XXX,XX @@ static void spike_board_init(MachineState *machine)
          kernel_entry = riscv_load_kernel(machine, &s->soc[0],
                                           kernel_start_addr,
 -                                         htif_symbol_callback);
 -
 -        if (machine->initrd_filename) {
 -            riscv_load_initrd(machine, kernel_entry);
 -        }
 -
 -        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
 -            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
 -                                    machine->kernel_cmdline);
 -        }
 +                                         true, htif_symbol_callback);
      } else {
         /*
          * If dynamic firmware is used, it doesn't know where is the next mode
 diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/riscv/virt.c
 +++ b/hw/riscv/virt.c
@@ -XXX,XX +XXX,XX @@ static void virt_machine_done(Notifier *notifier, void *data)
                                                           firmware_end_addr);
          kernel_entry = riscv_load_kernel(machine, &s->soc[0],
 -                                         kernel_start_addr, NULL);
 -
 -        if (machine->initrd_filename) {
 -            riscv_load_initrd(machine, kernel_entry);
 -        }
 -
 -        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
 -            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
 -                                    machine->kernel_cmdline);
 -        }
 +                                         kernel_start_addr, true, NULL);
      } else {
         /*
          * If dynamic firmware is used, it doesn't know where is the next mode
 diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
 index XXXXXXX..XXXXXXX 100644
 --- a/include/hw/riscv/boot.h
 +++ b/include/hw/riscv/boot.h
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
  target_ulong riscv_load_kernel(MachineState *machine,
                                 RISCVHartArrayState *harts,
                                 target_ulong firmware_end_addr,
 +                               bool load_initrd,
                                 symbol_fn_t sym_cb);
  void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
  uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
 --
-.9.4
+.39.1

-[Qemu-devel] [PATCH 3/3] qga: Prevent qemu-ga exit if serial doesn't exist
+[PATCH v10 3/3] hw/riscv/boot.c: make riscv_load_initrd() static
-From: Sameeh Jubran <sjubran@redhat.com>
+The only remaining caller is riscv_load_kernel_and_initrd() which
 belongs to the same file.
-Currently whenever the qemu-ga's service doesn't find the virtio-serial
+Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
-it terminates. This commit addresses this issue by listening to the serial events
+Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
-by registering for notifications for the chosen serial and it handles channel
+Reviewed-by: Bin Meng <bmeng@tinylab.org>
-initialization accordingily.
+Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
 ---
  hw/riscv/boot.c         | 80 ++++++++++++++++++++---------------------
  include/hw/riscv/boot.h |  1 -
 files changed, 40 insertions(+), 41 deletions(-)
-A list of possible scenarios of which could lead to this behavour:
+diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
 * qemu-ga's service is started while the virtio-serial driver hasn't been installed yet
 * hotplug/ unplug of the virtio-serial device
 * upgrading the virtio-serial driver
 Note: This problem is much more common on Windows as the virtio-serial
 driver should be installed by the user and isn't shipped with the Windows OS.
 Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
 ---
  Makefile            |   4 +
  qga/main.c          | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++--
  qga/service-win32.h |   4 +
 files changed, 239 insertions(+), 9 deletions(-)
 diff --git a/Makefile b/Makefile
 index XXXXXXX..XXXXXXX 100644
---- a/Makefile
+--- a/hw/riscv/boot.c
-+++ b/Makefile
++++ b/hw/riscv/boot.c
-@@ -XXX,XX +XXX,XX @@ $(call set-vpath, $(SRC_PATH))
+@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
+     exit(1);
- LIBS+=-lz $(LIBS_TOOLS)
+ }
-+ifndef CONFIG_WIN32
++static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
-+LIBS_QGA+=-ludev
++{
-+endif
++    const char *filename = machine->initrd_filename;
 +    uint64_t mem_size = machine->ram_size;
 +    void *fdt = machine->fdt;
 +    hwaddr start, end;
 +    ssize_t size;
 +
- HELPERS-$(CONFIG_LINUX) = qemu-bridge-helper$(EXESUF)
++    g_assert(filename != NULL);
  ifdef BUILD_DOCS
 diff --git a/qga/main.c b/qga/main.c
 index XXXXXXX..XXXXXXX 100644
 --- a/qga/main.c
 +++ b/qga/main.c
@@ -XXX,XX +XXX,XX @@
  #ifndef _WIN32
  #include <syslog.h>
  #include <sys/wait.h>
 +#include <libudev.h>
 +#include <sys/types.h>
  #endif
  #include "qapi/qmp/json-streamer.h"
  #include "qapi/qmp/json-parser.h"
@@ -XXX,XX +XXX,XX @@
  #include "qemu/sockets.h"
  #include "qemu/systemd.h"
  #ifdef _WIN32
 +#include <dbt.h>
  #include "qga/service-win32.h"
  #include "qga/vss-win32.h"
  #endif
@@ -XXX,XX +XXX,XX @@ struct GAState {
      GLogLevelFlags log_level;
      FILE *log_file;
      bool logging_enabled;
 +    bool serial_connected;
 +#ifndef _WIN32
 +    struct udev_monitor *udev_monitor;
 +#endif
  #ifdef _WIN32
      GAService service;
  #endif
@@ -XXX,XX +XXX,XX @@ static const char *ga_freeze_whitelist[] = {
  #ifdef _WIN32
  DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                    LPVOID ctx);
 +DWORD WINAPI handle_serial_device_events(DWORD type, LPVOID data);
  VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
  #endif
 +static bool get_channel_method(GAChannelMethod *channel_method,
 +    const gchar *method, GAState *s);
 +static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
 +    int listen_fd, bool *serial_connected);
 +
- static void
++    /*
- init_dfl_pathnames(void)
++     * We want to put the initrd far enough into RAM that when the
- {
++     * kernel is uncompressed it will not clobber the initrd. However
-@@ -XXX,XX +XXX,XX @@ static void quit_handler(int sig)
++     * on boards without much RAM we must ensure that we still leave
- }
++     * enough room for a decent sized initrd, and on boards with large
++     * amounts of RAM we must avoid the initrd being so far up in RAM
- #ifndef _WIN32
++     * that it is outside lowmem and inaccessible to the kernel.
-+static int get_method_udev_subsystem(const char **serial_subsystem)
++     * So for boards with less  than 256MB of RAM we put the initrd
-+{
++     * halfway into RAM, and for boards with 256MB of RAM or more we put
-+    if (strcmp(ga_config->method, "virtio-serial") == 0) {
++     * the initrd at 128MB.
-+        *serial_subsystem = SUBSYSTEM_VIRTIO_SERIAL;
++     */
-+    } else if (strcmp(ga_config->method, "isa-serial") == 0) {
++    start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
 +        /* try the default path for the serial port - COM1 */
 +        *serial_subsystem = SUBSYSTEM_ISA_SERIAL;
 +    } else {
 +        serial_subsystem = NULL;
 +        return -1;
 +    }
 +    return 0;
 +}
 +
-+static gboolean serial_event_callback(GIOChannel *source,
++    size = load_ramdisk(filename, start, mem_size - start);
-+    GIOCondition condition, gpointer data)
++    if (size == -1) {
-+{
++        size = load_image_targphys(filename, start, mem_size - start);
-+    struct udev_monitor *mon = ga_state->udev_monitor;
++        if (size == -1) {
-+    const char *serial_subsystem = NULL;
++            error_report("could not load ramdisk '%s'", filename);
-+    struct udev_device *dev;
++            exit(1);
 +
 +    if (get_method_udev_subsystem(&serial_subsystem) == -1) {
 +        return false;
 +    }
 +    dev = udev_monitor_receive_device(mon);
 +
 +    if (dev && serial_subsystem && strcmp(udev_device_get_subsystem(dev),
 +        serial_subsystem) ==  0) {
 +
 +        GAChannelMethod channel_method;
 +        get_channel_method(&channel_method, ga_config->method, ga_state);
 +        if (ga_channel_was_serial_attached(channel_method,
 +            ga_config->channel_path, ga_state->serial_connected)) {
 +            ga_state->serial_connected = true;
 +            if (!channel_init(ga_state,
 +                ga_config->method, ga_config->channel_path,
 +                ga_socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
 +                &ga_state->serial_connected)) {
 +                g_critical("failed to initialize guest agent channel");
 +            }
 +        }
-+
-+        if (ga_channel_was_serial_detached(channel_method,
-+            ga_config->channel_path, ga_state->serial_connected)) {
-+            ga_state->serial_connected = false;
-+            ga_channel_free(ga_state->channel);
-+        }
-+        udev_device_unref(dev);
-+    }
-+    return true;
-+}
-+
-+static int monitor_serial_events(void)
-+{
-+    int ret = 0;
-+    const char *serial_subsystem = NULL;
-+    struct udev *udev = NULL;
-+    ga_state->udev_monitor = NULL;
-+    GIOChannel *channel = NULL;
-+    GSource *watch_source = NULL;
-+    if (get_method_udev_subsystem(&serial_subsystem) == -1) {
-+        ret = -1;
-+        goto out;
 +    }
 +
-+    udev = udev_new();
++    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
-+    if (!udev) {
++    if (fdt) {
-+        g_error("Couldn't create udev\n");
++        end = start + size;
-+        ret = -1;
++        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
-+        goto out;
++        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
 +    }
 +
 +    ga_state->udev_monitor =
 +        udev_monitor_new_from_netlink(udev, "udev");
 +    if (!ga_state->udev_monitor) {
 +        ret = -1;
 +        goto out;
 +    } else {
 +        /* We don't want the udev_monitor to be freed on out, so increase
 +         * ref count
 +         */
 +        udev_monitor_ref(ga_state->udev_monitor);
 +    }
 +
 +    if (udev_monitor_filter_add_match_subsystem_devtype(ga_state->udev_monitor,
 +        serial_subsystem, NULL) < 0 ||
 +        udev_monitor_enable_receiving(ga_state->udev_monitor) < 0) {
 +        ret = -1;
 +        goto out;
 +    }
 +
 +    channel =
 +        g_io_channel_unix_new(udev_monitor_get_fd(ga_state->udev_monitor));
 +    if (!channel) {
 +        ret = -1;
 +        goto out;
 +    }
 +    watch_source = g_io_create_watch(channel, G_IO_IN);
 +    if (!watch_source) {
 +        ret = -1;
 +        goto out;
 +    }
 +    g_source_set_callback(watch_source, (GSourceFunc)serial_event_callback,
 +        ga_state->udev_monitor, NULL);
 +    g_source_attach(watch_source, g_main_loop_get_context(ga_state->main_loop));
 +
 +out:
 +    if (udev) {
 +        udev_unref(udev);
 +    }
 +    if (ga_state->udev_monitor) {
 +        udev_monitor_unref(ga_state->udev_monitor);
 +    }
 +    if (channel) {
 +        g_io_channel_unref(channel);
 +    }
 +    if (watch_source) {
 +        g_source_unref(watch_source);
 +    }
 +
 +    return ret;
 +}
 +
 +static void free_monitor_resources(void)
 +{
 +    if (ga_state->udev_monitor) {
 +        udev_monitor_unref(ga_state->udev_monitor);
 +    }
 +}
 +
- static gboolean register_signal_handlers(void)
+ target_ulong riscv_load_kernel(MachineState *machine,
- {
+                                RISCVHartArrayState *harts,
-     struct sigaction sigact;
+                                target_ulong kernel_start_addr,
-@@ -XXX,XX +XXX,XX @@ static gboolean channel_event_cb(GIOCondition condition, gpointer data)
+@@ -XXX,XX +XXX,XX @@ out:
-     return true;
+     return kernel_entry;
  }
--static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
+-void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
--                             int listen_fd)
+-{
-+static bool get_channel_method(GAChannelMethod *channel_method,
+-    const char *filename = machine->initrd_filename;
-+    const gchar *method, GAState *s)
+-    uint64_t mem_size = machine->ram_size;
- {
+-    void *fdt = machine->fdt;
--    GAChannelMethod channel_method;
+-    hwaddr start, end;
-+    assert(channel_method);
+-    ssize_t size;
 +    assert(method);
      if (strcmp(method, "virtio-serial") == 0) {
 +        assert(s);
          s->virtio = true; /* virtio requires special handling in some cases */
 -        channel_method = GA_CHANNEL_VIRTIO_SERIAL;
 +        *channel_method = GA_CHANNEL_VIRTIO_SERIAL;
      } else if (strcmp(method, "isa-serial") == 0) {
 -        channel_method = GA_CHANNEL_ISA_SERIAL;
 +        *channel_method = GA_CHANNEL_ISA_SERIAL;
      } else if (strcmp(method, "unix-listen") == 0) {
 -        channel_method = GA_CHANNEL_UNIX_LISTEN;
 +        *channel_method = GA_CHANNEL_UNIX_LISTEN;
      } else if (strcmp(method, "vsock-listen") == 0) {
 -        channel_method = GA_CHANNEL_VSOCK_LISTEN;
 +        *channel_method = GA_CHANNEL_VSOCK_LISTEN;
      } else {
          g_critical("unsupported channel method/type: %s", method);
          return false;
      }
 +    return true;
 +}
 +
 +static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
 +                             int listen_fd, bool *serial_connected)
 +{
 +    assert(serial_connected);
 +    GAChannelMethod channel_method;
 +    if (!get_channel_method(&channel_method, method, s)) {
 +        return false;
 +    }
 +    *serial_connected = ga_channel_serial_is_present(channel_method, path);
      s->channel = ga_channel_new(channel_method, path, listen_fd,
                                  channel_event_cb, s);
@@ -XXX,XX +XXX,XX @@ static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
  }
  #ifdef _WIN32
 +DWORD WINAPI handle_serial_device_events(DWORD type, LPVOID data)
 +{
 +    DWORD ret = NO_ERROR;
 +    PDEV_BROADCAST_HDR broadcast_header = (PDEV_BROADCAST_HDR)data;
 +    GAChannelMethod channel_method;
 +
 +    if (broadcast_header->dbch_devicetype == DBT_DEVTYP_DEVICEINTERFACE) {
 +        get_channel_method(&channel_method, ga_config->method, ga_state);
 +        switch (type) {
 +            /* Device inserted */
 +        case DBT_DEVICEARRIVAL:
 +            /* Start QEMU-ga's service */
 +            if (ga_channel_was_serial_attached(channel_method,
 +                ga_config->channel_path, ga_state->serial_connected)) {
 +                if (!channel_init(ga_state,
 +                    ga_config->method, ga_config->channel_path,
 +                    ga_socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
 +                    &ga_state->serial_connected)) {
 +                    g_critical("failed to initialize guest agent channel");
 +                    ret = EXIT_FAILURE;
 +                }
 +            }
 +            break;
 +            /* Device removed */
 +        case DBT_DEVICEQUERYREMOVE:
 +        case DBT_DEVICEREMOVEPENDING:
 +        case DBT_DEVICEREMOVECOMPLETE:
 +
 +            /* Stop QEMU-ga's service */
 +            if (ga_channel_was_serial_detached(channel_method,
 +                ga_config->channel_path, ga_state->serial_connected)) {
 +                ga_state->serial_connected = false;
 +                ga_channel_free(ga_state->channel);
 +            }
 +            break;
 +        default:
 +            ret = ERROR_CALL_NOT_IMPLEMENTED;
 +        }
 +    }
 +    return ret;
 +}
 +
  DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                    LPVOID ctx)
  {
@@ -XXX,XX +XXX,XX @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
              service->status.dwCurrentState = SERVICE_STOP_PENDING;
              SetServiceStatus(service->status_handle, &service->status);
              break;
 +        case SERVICE_CONTROL_DEVICEEVENT:
 +            handle_serial_device_events(type, data);
 +            break;
          default:
              ret = ERROR_CALL_NOT_IMPLEMENTED;
@@ -XXX,XX +XXX,XX @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
      service->status.dwServiceSpecificExitCode = NO_ERROR;
      service->status.dwCheckPoint = 0;
      service->status.dwWaitHint = 0;
 +    DEV_BROADCAST_DEVICEINTERFACE notification_filter;
 +    ZeroMemory(&notification_filter, sizeof(notification_filter));
 +    notification_filter.dbcc_devicetype = DBT_DEVTYP_DEVICEINTERFACE;
 +    notification_filter.dbcc_size = sizeof(DEV_BROADCAST_DEVICEINTERFACE);
 +    notification_filter.dbcc_classguid = GUID_VIOSERIAL_PORT;
 +
 +    service->device_notification_handle =
 +        RegisterDeviceNotification(service->status_handle,
 +            &notification_filter, DEVICE_NOTIFY_SERVICE_HANDLE);
 +    if (!service->device_notification_handle) {
 +        g_critical("Failed to register device notification handle!\n");
 +        return;
 +    }
      SetServiceStatus(service->status_handle, &service->status);
      g_main_loop_run(ga_state->main_loop);
 +    UnregisterDeviceNotification(service->device_notification_handle);
      service->status.dwCurrentState = SERVICE_STOPPED;
      SetServiceStatus(service->status_handle, &service->status);
  }
@@ -XXX,XX +XXX,XX @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
  #endif
      s->main_loop = g_main_loop_new(NULL, false);
 -
-     if (!channel_init(ga_state, config->method, config->channel_path,
+-    g_assert(filename != NULL);
--                      socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) {
+-
-+                      socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
+-    /*
-+                      &ga_state->serial_connected) &&
+-     * We want to put the initrd far enough into RAM that when the
-+                      ga_state->serial_connected) {
+-     * kernel is uncompressed it will not clobber the initrd. However
-         g_critical("failed to initialize guest agent channel");
+-     * on boards without much RAM we must ensure that we still leave
-         return EXIT_FAILURE;
+-     * enough room for a decent sized initrd, and on boards with large
-     }
+-     * amounts of RAM we must avoid the initrd being so far up in RAM
- #ifndef _WIN32
+-     * that it is outside lowmem and inaccessible to the kernel.
-+    monitor_serial_events();
+-     * So for boards with less  than 256MB of RAM we put the initrd
-     g_main_loop_run(ga_state->main_loop);
+-     * halfway into RAM, and for boards with 256MB of RAM or more we put
-+    free_monitor_resources();
+-     * the initrd at 128MB.
- #else
+-     */
-     if (config->daemonize) {
+-    start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
-         SERVICE_TABLE_ENTRY service_table[] = {
+-
-             { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
+-    size = load_ramdisk(filename, start, mem_size - start);
-         StartServiceCtrlDispatcher(service_table);
+-    if (size == -1) {
-     } else {
+-        size = load_image_targphys(filename, start, mem_size - start);
-+        if (!ga_state->serial_connected) {
+-        if (size == -1) {
-+            return EXIT_FAILURE;
+-            error_report("could not load ramdisk '%s'", filename);
-+        }
+-            exit(1);
-         g_main_loop_run(ga_state->main_loop);
+-        }
-     }
+-    }
- #endif
+-
-diff --git a/qga/service-win32.h b/qga/service-win32.h
+-    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
 -    if (fdt) {
 -        end = start + size;
 -        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
 -        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
 -    }
 -}
 -
  /*
   * This function makes an assumption that the DRAM interval
   * 'dram_base' + 'dram_size' is contiguous.
 diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
 index XXXXXXX..XXXXXXX 100644
---- a/qga/service-win32.h
+--- a/include/hw/riscv/boot.h
-+++ b/qga/service-win32.h
++++ b/include/hw/riscv/boot.h
-@@ -XXX,XX +XXX,XX @@
+@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,
- #define QGA_SERVICE_NAME         "qemu-ga"
+                                target_ulong firmware_end_addr,
- #define QGA_SERVICE_DESCRIPTION  "Enables integration with QEMU machine emulator and virtualizer."
+                                bool load_initrd,
+                                symbol_fn_t sym_cb);
-+static const GUID GUID_VIOSERIAL_PORT = { 0x6fde7521, 0x1b65, 0x48ae,
+-void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
-+{ 0xb6, 0x28, 0x80, 0xbe, 0x62, 0x1, 0x60, 0x26 } };
+ uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
-+
+                                 MachineState *ms);
- typedef struct GAService {
+ void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
      SERVICE_STATUS status;
      SERVICE_STATUS_HANDLE status_handle;
 +    HDEVNOTIFY device_notification_handle;
  } GAService;
  int ga_install_service(const char *path, const char *logfile,
 --
-.9.4
+.39.1

From: Sameeh Jubran <sjubran@redhat.com>

This commit adds functions to check if the serial is
connected/disconnected or else if it has been attached or detached.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 qga/channel-posix.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 qga/channel-win32.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/channel.h       |  9 ++++++++
 3 files changed, 123 insertions(+)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index XXXXXXX..XXXXXXX 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -XXX,XX +XXX,XX @@ void ga_channel_free(GAChannel *c)
     }
     g_free(c);
 }
+
+static bool is_serial_present(GAChannelMethod method, const gchar *path,
+    int *error_code)
+{
+    int fd = -1;
+    bool ret = true;
+
+    assert(error_code);
+    *error_code = 0;
+
+    switch (method) {
+    case GA_CHANNEL_VIRTIO_SERIAL:
+        fd = qemu_open(path, O_RDWR | O_NONBLOCK
+#ifndef CONFIG_SOLARIS
+            | O_ASYNC
+#endif
+        );
+        break;
+    case GA_CHANNEL_ISA_SERIAL:
+        fd = qemu_open(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
+        break;
+    default:
+        ret = false;
+    }
+    if (fd < 0) {
+        *error_code = errno;
+        ret = false;
+    } else {
+        close(fd);
+    }
+    return ret;
+}
+
+bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path)
+{
+    int error_code = 0;
+    return is_serial_present(method, path, &error_code) ||
+        error_code == EBUSY;
+}
+
+bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached)
+{
+    int error_code = 0;
+    return !is_serial_attached &&
+        is_serial_present(method, path, &error_code);
+}
+bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached)
+{
+    int error_code = 0;
+    return is_serial_attached && !is_serial_present(method, path, &error_code)
+        && error_code == ENOENT;
+}
diff --git a/qga/channel-win32.c b/qga/channel-win32.c
index XXXXXXX..XXXXXXX 100644
--- a/qga/channel-win32.c
+++ b/qga/channel-win32.c
@@ -XXX,XX +XXX,XX @@ void ga_channel_free(GAChannel *c)
     g_free(c->rstate.buf);
     g_free(c);
 }
+
+static bool is_serial_present(GAChannelMethod method, const gchar *path,
+    DWORD *err)
+{
+    gchar newpath[MAXPATHLEN] = { 0 };
+    bool ret = false;
+
+    assert(err);
+
+    if (method != GA_CHANNEL_VIRTIO_SERIAL && method != GA_CHANNEL_ISA_SERIAL) {
+        g_critical("unsupported communication method");
+        return false;
+    }
+
+    if (method == GA_CHANNEL_ISA_SERIAL) {
+        snprintf(newpath, sizeof(newpath), "\\\\.\\%s", path);
+    } else {
+        g_strlcpy(newpath, path, sizeof(newpath));
+    }
+
+    HANDLE handle = CreateFile(newpath, GENERIC_READ | GENERIC_WRITE, 0, NULL,
+        OPEN_EXISTING,
+        FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL);
+
+    if (handle == INVALID_HANDLE_VALUE) {
+        *err = GetLastError();
+        ret = false;
+    } else {
+        ret = true;
+    }
+
+    CloseHandle(handle);
+    return ret;
+}
+
+bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path)
+{
+    DWORD err_code;
+    return is_serial_present(method, path, &err_code) ||
+        err_code == ERROR_ACCESS_DENIED;
+}
+
+bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached)
+{
+    DWORD err_code;
+    return !is_serial_attached && is_serial_present(method, path, &err_code);
+}
+
+bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached)
+{
+    DWORD err_code = NO_ERROR;
+        /* In order to make sure the serial that qemu-ga uses is the one that
+         * was detached. We'll get the error ERROR_FILE_NOT_FOUND when
+         * attempting to call CreateFile with the serial path.
+        */
+       return is_serial_attached && !is_serial_present(method, path, &err_code)
+           && err_code == ERROR_FILE_NOT_FOUND;
+}
diff --git a/qga/channel.h b/qga/channel.h
index XXXXXXX..XXXXXXX 100644
--- a/qga/channel.h
+++ b/qga/channel.h
@@ -XXX,XX +XXX,XX @@
 #ifndef QGA_CHANNEL_H
 #define QGA_CHANNEL_H
 
+#ifndef _WIN32
+#define SUBSYSTEM_VIRTIO_SERIAL "virtio-ports";
+#define SUBSYSTEM_ISA_SERIAL "isa-serial";
+#endif
 
 typedef struct GAChannel GAChannel;
 
@@ -XXX,XX +XXX,XX @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
 void ga_channel_free(GAChannel *c);
 GIOStatus ga_channel_read(GAChannel *c, gchar *buf, gsize size, gsize *count);
 GIOStatus ga_channel_write_all(GAChannel *c, const gchar *buf, gsize size);
+bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path);
+bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached);
+bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached);
 
 #endif
-- 
2.9.4

From: Sameeh Jubran <sjubran@redhat.com>

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 qga/main.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index XXXXXXX..XXXXXXX 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -XXX,XX +XXX,XX @@ struct GAState {
     GAPersistentState pstate;
 };
 
+typedef struct GAConfig {
+    char *channel_path;
+    char *method;
+    char *log_filepath;
+    char *pid_filepath;
+#ifdef CONFIG_FSFREEZE
+    char *fsfreeze_hook;
+#endif
+    char *state_dir;
+#ifdef _WIN32
+    const char *service;
+#endif
+    gchar *bliststr; /* blacklist may point to this string */
+    GList *blacklist;
+    int daemonize;
+    GLogLevelFlags log_level;
+    int dumpconf;
+} GAConfig;
+
 struct GAState *ga_state;
+struct GAConfig *ga_config;
+int ga_socket_activation;
 QmpCommandList ga_commands;
 
 /* commands that are safe to issue while filesystems are frozen */
@@ -XXX,XX +XXX,XX @@ static GList *split_list(const gchar *str, const gchar *delim)
     return list;
 }
 
-typedef struct GAConfig {
-    char *channel_path;
-    char *method;
-    char *log_filepath;
-    char *pid_filepath;
-#ifdef CONFIG_FSFREEZE
-    char *fsfreeze_hook;
-#endif
-    char *state_dir;
-#ifdef _WIN32
-    const char *service;
-#endif
-    gchar *bliststr; /* blacklist may point to this string */
-    GList *blacklist;
-    int daemonize;
-    GLogLevelFlags log_level;
-    int dumpconf;
-} GAConfig;
-
 static void config_load(GAConfig *config)
 {
     GError *gerr = NULL;
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
 {
     int ret = EXIT_SUCCESS;
     GAState *s = g_new0(GAState, 1);
-    GAConfig *config = g_new0(GAConfig, 1);
+    GAConfig *config = ga_config = g_new0(GAConfig, 1);
     int socket_activation;
 
     config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
         config->method = g_strdup("virtio-serial");
     }
 
-    socket_activation = check_socket_activation();
+    ga_socket_activation = socket_activation = check_socket_activation();
     if (socket_activation > 1) {
         g_critical("qemu-ga only supports listening on one socket");
         ret = EXIT_FAILURE;
-- 
2.9.4

From: Sameeh Jubran <sjubran@redhat.com>

Currently whenever the qemu-ga's service doesn't find the virtio-serial
it terminates. This commit addresses this issue by listening to the serial events
by registering for notifications for the chosen serial and it handles channel
initialization accordingily.

A list of possible scenarios of which could lead to this behavour:

* qemu-ga's service is started while the virtio-serial driver hasn't been installed yet
* hotplug/ unplug of the virtio-serial device
* upgrading the virtio-serial driver

Note: This problem is much more common on Windows as the virtio-serial
driver should be installed by the user and isn't shipped with the Windows OS.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 Makefile            |   4 +
 qga/main.c          | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 qga/service-win32.h |   4 +
 3 files changed, 239 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index XXXXXXX..XXXXXXX 100644
--- a/Makefile
+++ b/Makefile
@@ -XXX,XX +XXX,XX @@ $(call set-vpath, $(SRC_PATH))
 
 LIBS+=-lz $(LIBS_TOOLS)
 
+ifndef CONFIG_WIN32
+LIBS_QGA+=-ludev
+endif
+
 HELPERS-$(CONFIG_LINUX) = qemu-bridge-helper$(EXESUF)
 
 ifdef BUILD_DOCS
diff --git a/qga/main.c b/qga/main.c
index XXXXXXX..XXXXXXX 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -XXX,XX +XXX,XX @@
 #ifndef _WIN32
 #include <syslog.h>
 #include <sys/wait.h>
+#include <libudev.h>
+#include <sys/types.h>
 #endif
 #include "qapi/qmp/json-streamer.h"
 #include "qapi/qmp/json-parser.h"
@@ -XXX,XX +XXX,XX @@
 #include "qemu/sockets.h"
 #include "qemu/systemd.h"
 #ifdef _WIN32
+#include <dbt.h>
 #include "qga/service-win32.h"
 #include "qga/vss-win32.h"
 #endif
@@ -XXX,XX +XXX,XX @@ struct GAState {
     GLogLevelFlags log_level;
     FILE *log_file;
     bool logging_enabled;
+    bool serial_connected;
+#ifndef _WIN32
+    struct udev_monitor *udev_monitor;
+#endif
 #ifdef _WIN32
     GAService service;
 #endif
@@ -XXX,XX +XXX,XX @@ static const char *ga_freeze_whitelist[] = {
 #ifdef _WIN32
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx);
+DWORD WINAPI handle_serial_device_events(DWORD type, LPVOID data);
 VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
 #endif
 
+static bool get_channel_method(GAChannelMethod *channel_method,
+    const gchar *method, GAState *s);
+static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
+    int listen_fd, bool *serial_connected);
+
 static void
 init_dfl_pathnames(void)
 {
@@ -XXX,XX +XXX,XX @@ static void quit_handler(int sig)
 }
 
 #ifndef _WIN32
+static int get_method_udev_subsystem(const char **serial_subsystem)
+{
+    if (strcmp(ga_config->method, "virtio-serial") == 0) {
+        *serial_subsystem = SUBSYSTEM_VIRTIO_SERIAL;
+    } else if (strcmp(ga_config->method, "isa-serial") == 0) {
+        /* try the default path for the serial port - COM1 */
+        *serial_subsystem = SUBSYSTEM_ISA_SERIAL;
+    } else {
+        serial_subsystem = NULL;
+        return -1;
+    }
+    return 0;
+}
+
+static gboolean serial_event_callback(GIOChannel *source,
+    GIOCondition condition, gpointer data)
+{
+    struct udev_monitor *mon = ga_state->udev_monitor;
+    const char *serial_subsystem = NULL;
+    struct udev_device *dev;
+
+    if (get_method_udev_subsystem(&serial_subsystem) == -1) {
+        return false;
+    }
+    dev = udev_monitor_receive_device(mon);
+
+    if (dev && serial_subsystem && strcmp(udev_device_get_subsystem(dev),
+        serial_subsystem) ==  0) {
+
+        GAChannelMethod channel_method;
+        get_channel_method(&channel_method, ga_config->method, ga_state);
+        if (ga_channel_was_serial_attached(channel_method,
+            ga_config->channel_path, ga_state->serial_connected)) {
+            ga_state->serial_connected = true;
+            if (!channel_init(ga_state,
+                ga_config->method, ga_config->channel_path,
+                ga_socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
+                &ga_state->serial_connected)) {
+                g_critical("failed to initialize guest agent channel");
+            }
+        }
+
+        if (ga_channel_was_serial_detached(channel_method,
+            ga_config->channel_path, ga_state->serial_connected)) {
+            ga_state->serial_connected = false;
+            ga_channel_free(ga_state->channel);
+        }
+        udev_device_unref(dev);
+    }
+    return true;
+}
+
+static int monitor_serial_events(void)
+{
+    int ret = 0;
+    const char *serial_subsystem = NULL;
+    struct udev *udev = NULL;
+    ga_state->udev_monitor = NULL;
+    GIOChannel *channel = NULL;
+    GSource *watch_source = NULL;
+    if (get_method_udev_subsystem(&serial_subsystem) == -1) {
+        ret = -1;
+        goto out;
+    }
+
+    udev = udev_new();
+    if (!udev) {
+        g_error("Couldn't create udev\n");
+        ret = -1;
+        goto out;
+    }
+
+    ga_state->udev_monitor =
+        udev_monitor_new_from_netlink(udev, "udev");
+    if (!ga_state->udev_monitor) {
+        ret = -1;
+        goto out;
+    } else {
+        /* We don't want the udev_monitor to be freed on out, so increase
+         * ref count
+         */
+        udev_monitor_ref(ga_state->udev_monitor);
+    }
+
+    if (udev_monitor_filter_add_match_subsystem_devtype(ga_state->udev_monitor,
+        serial_subsystem, NULL) < 0 ||
+        udev_monitor_enable_receiving(ga_state->udev_monitor) < 0) {
+        ret = -1;
+        goto out;
+    }
+
+    channel =
+        g_io_channel_unix_new(udev_monitor_get_fd(ga_state->udev_monitor));
+    if (!channel) {
+        ret = -1;
+        goto out;
+    }
+    watch_source = g_io_create_watch(channel, G_IO_IN);
+    if (!watch_source) {
+        ret = -1;
+        goto out;
+    }
+    g_source_set_callback(watch_source, (GSourceFunc)serial_event_callback,
+        ga_state->udev_monitor, NULL);
+    g_source_attach(watch_source, g_main_loop_get_context(ga_state->main_loop));
+
+out:
+    if (udev) {
+        udev_unref(udev);
+    }
+    if (ga_state->udev_monitor) {
+        udev_monitor_unref(ga_state->udev_monitor);
+    }
+    if (channel) {
+        g_io_channel_unref(channel);
+    }
+    if (watch_source) {
+        g_source_unref(watch_source);
+    }
+
+    return ret;
+}
+
+static void free_monitor_resources(void)
+{
+    if (ga_state->udev_monitor) {
+        udev_monitor_unref(ga_state->udev_monitor);
+    }
+}
+
 static gboolean register_signal_handlers(void)
 {
     struct sigaction sigact;
@@ -XXX,XX +XXX,XX @@ static gboolean channel_event_cb(GIOCondition condition, gpointer data)
     return true;
 }
 
-static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
-                             int listen_fd)
+static bool get_channel_method(GAChannelMethod *channel_method,
+    const gchar *method, GAState *s)
 {
-    GAChannelMethod channel_method;
+    assert(channel_method);
+    assert(method);
 
     if (strcmp(method, "virtio-serial") == 0) {
+        assert(s);
         s->virtio = true; /* virtio requires special handling in some cases */
-        channel_method = GA_CHANNEL_VIRTIO_SERIAL;
+        *channel_method = GA_CHANNEL_VIRTIO_SERIAL;
     } else if (strcmp(method, "isa-serial") == 0) {
-        channel_method = GA_CHANNEL_ISA_SERIAL;
+        *channel_method = GA_CHANNEL_ISA_SERIAL;
     } else if (strcmp(method, "unix-listen") == 0) {
-        channel_method = GA_CHANNEL_UNIX_LISTEN;
+        *channel_method = GA_CHANNEL_UNIX_LISTEN;
     } else if (strcmp(method, "vsock-listen") == 0) {
-        channel_method = GA_CHANNEL_VSOCK_LISTEN;
+        *channel_method = GA_CHANNEL_VSOCK_LISTEN;
     } else {
         g_critical("unsupported channel method/type: %s", method);
         return false;
     }
+    return true;
+}
+
+static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
+                             int listen_fd, bool *serial_connected)
+{
+    assert(serial_connected);
+    GAChannelMethod channel_method;
+    if (!get_channel_method(&channel_method, method, s)) {
+        return false;
+    }
+    *serial_connected = ga_channel_serial_is_present(channel_method, path);
 
     s->channel = ga_channel_new(channel_method, path, listen_fd,
                                 channel_event_cb, s);
@@ -XXX,XX +XXX,XX @@ static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
 }
 
 #ifdef _WIN32
+DWORD WINAPI handle_serial_device_events(DWORD type, LPVOID data)
+{
+    DWORD ret = NO_ERROR;
+    PDEV_BROADCAST_HDR broadcast_header = (PDEV_BROADCAST_HDR)data;
+    GAChannelMethod channel_method;
+
+    if (broadcast_header->dbch_devicetype == DBT_DEVTYP_DEVICEINTERFACE) {
+        get_channel_method(&channel_method, ga_config->method, ga_state);
+        switch (type) {
+            /* Device inserted */
+        case DBT_DEVICEARRIVAL:
+            /* Start QEMU-ga's service */
+            if (ga_channel_was_serial_attached(channel_method,
+                ga_config->channel_path, ga_state->serial_connected)) {
+                if (!channel_init(ga_state,
+                    ga_config->method, ga_config->channel_path,
+                    ga_socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
+                    &ga_state->serial_connected)) {
+                    g_critical("failed to initialize guest agent channel");
+                    ret = EXIT_FAILURE;
+                }
+            }
+            break;
+            /* Device removed */
+        case DBT_DEVICEQUERYREMOVE:
+        case DBT_DEVICEREMOVEPENDING:
+        case DBT_DEVICEREMOVECOMPLETE:
+
+            /* Stop QEMU-ga's service */
+            if (ga_channel_was_serial_detached(channel_method,
+                ga_config->channel_path, ga_state->serial_connected)) {
+                ga_state->serial_connected = false;
+                ga_channel_free(ga_state->channel);
+            }
+            break;
+        default:
+            ret = ERROR_CALL_NOT_IMPLEMENTED;
+        }
+    }
+    return ret;
+}
+
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx)
 {
@@ -XXX,XX +XXX,XX @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
             service->status.dwCurrentState = SERVICE_STOP_PENDING;
             SetServiceStatus(service->status_handle, &service->status);
             break;
+        case SERVICE_CONTROL_DEVICEEVENT:
+            handle_serial_device_events(type, data);
+            break;
 
         default:
             ret = ERROR_CALL_NOT_IMPLEMENTED;
@@ -XXX,XX +XXX,XX @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
     service->status.dwServiceSpecificExitCode = NO_ERROR;
     service->status.dwCheckPoint = 0;
     service->status.dwWaitHint = 0;
+    DEV_BROADCAST_DEVICEINTERFACE notification_filter;
+    ZeroMemory(&notification_filter, sizeof(notification_filter));
+    notification_filter.dbcc_devicetype = DBT_DEVTYP_DEVICEINTERFACE;
+    notification_filter.dbcc_size = sizeof(DEV_BROADCAST_DEVICEINTERFACE);
+    notification_filter.dbcc_classguid = GUID_VIOSERIAL_PORT;
+
+    service->device_notification_handle =
+        RegisterDeviceNotification(service->status_handle,
+            &notification_filter, DEVICE_NOTIFY_SERVICE_HANDLE);
+    if (!service->device_notification_handle) {
+        g_critical("Failed to register device notification handle!\n");
+        return;
+    }
     SetServiceStatus(service->status_handle, &service->status);
 
     g_main_loop_run(ga_state->main_loop);
 
+    UnregisterDeviceNotification(service->device_notification_handle);
     service->status.dwCurrentState = SERVICE_STOPPED;
     SetServiceStatus(service->status_handle, &service->status);
 }
@@ -XXX,XX +XXX,XX @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
 #endif
 
     s->main_loop = g_main_loop_new(NULL, false);
-
     if (!channel_init(ga_state, config->method, config->channel_path,
-                      socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) {
+                      socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
+                      &ga_state->serial_connected) &&
+                      ga_state->serial_connected) {
         g_critical("failed to initialize guest agent channel");
         return EXIT_FAILURE;
     }
 #ifndef _WIN32
+    monitor_serial_events();
     g_main_loop_run(ga_state->main_loop);
+    free_monitor_resources();
 #else
     if (config->daemonize) {
         SERVICE_TABLE_ENTRY service_table[] = {
             { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
         StartServiceCtrlDispatcher(service_table);
     } else {
+        if (!ga_state->serial_connected) {
+            return EXIT_FAILURE;
+        }
         g_main_loop_run(ga_state->main_loop);
     }
 #endif
diff --git a/qga/service-win32.h b/qga/service-win32.h
index XXXXXXX..XXXXXXX 100644
--- a/qga/service-win32.h
+++ b/qga/service-win32.h
@@ -XXX,XX +XXX,XX @@
 #define QGA_SERVICE_NAME         "qemu-ga"
 #define QGA_SERVICE_DESCRIPTION  "Enables integration with QEMU machine emulator and virtualizer."
 
+static const GUID GUID_VIOSERIAL_PORT = { 0x6fde7521, 0x1b65, 0x48ae,
+{ 0xb6, 0x28, 0x80, 0xbe, 0x62, 0x1, 0x60, 0x26 } };
+
 typedef struct GAService {
     SERVICE_STATUS status;
     SERVICE_STATUS_HANDLE status_handle;
+    HDEVNOTIFY device_notification_handle;
 } GAService;
 
 int ga_install_service(const char *path, const char *logfile,
-- 
2.9.4

Hi,

This new version removed the translate_fn() from patch 1 because it
wasn't removing the sign-extension for pentry as we thought it would.
A more detailed explanation is given in the commit msg of patch 1.

We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and
using it when we're running a 32-bit CPU. This worked with 32 bit
'virt' machine booting with the -kernel option.

If this approach doesn't work for the Xvisor use case, IMO  we should
just filter kernel_load_addr bits directly as we were doing a handful of
versions ago.

Patches are based on current riscv-to-apply.next.

Changes from v9:
- patch 1:
  - removed the translate_fn() callback
  - return 'kernel_low' when running a 32-bit CPU
- v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html

Daniel Henrique Barboza (3):
  hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()
  hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
  hw/riscv/boot.c: make riscv_load_initrd() static

-- 
2.39.1

load_elf_ram_sym() will sign-extend 32 bit addresses. If a 32 bit QEMU
guest happens to be running in a hypervisor that are using 64 bits to
encode its address, kernel_entry can be padded with '1's and create
problems [1].

Using a translate_fn() callback in load_elf_ram_sym() to filter the
padding from the address doesn't work. A more detailed explanation can
be found in [2]. The short version is that glue(load_elf, SZ), from
include/hw/elf_ops.h, will calculate 'pentry' (mapped into the
'kernel_load_base' var in riscv_load_Kernel()) before using
translate_fn(), and will not recalculate it after executing it. This
means that the callback does not prevent the padding from
kernel_load_base to appear.

Let's instead use a kernel_low var to capture the 'lowaddr' value from
load_elf_ram_sim(), and return it when we're dealing with 32 bit CPUs.

[1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02281.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00099.html

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
 }
 
 target_ulong riscv_load_kernel(MachineState *machine,
+                               RISCVHartArrayState *harts,
                                target_ulong kernel_start_addr,
                                symbol_fn_t sym_cb)
 {
     const char *kernel_filename = machine->kernel_filename;
-    uint64_t kernel_load_base, kernel_entry;
+    uint64_t kernel_load_base, kernel_entry, kernel_low;
 
     g_assert(kernel_filename != NULL);
 
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,
      * the (expected) load address load address. This allows kernels to have
      * separate SBI and ELF entry points (used by FreeBSD, for example).
      */
-    if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
-                         NULL, &kernel_load_base, NULL, NULL, 0,
+    if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL, NULL,
+                         &kernel_load_base, &kernel_low, NULL, 0,
                          EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
-        return kernel_load_base;
+        kernel_entry = kernel_load_base;
+
+        if (riscv_is_32bit(harts)) {
+            kernel_entry = kernel_low;
+        }
+
+        return kernel_entry;
     }
 
     if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL,
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -XXX,XX +XXX,XX @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
+        kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
+                                         kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
             riscv_load_initrd(machine, kernel_entry);
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -XXX,XX +XXX,XX @@ static void opentitan_board_init(MachineState *machine)
     }
 
     if (machine->kernel_filename) {
-        riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL);
+        riscv_load_kernel(machine, &s->soc.cpus,
+                          memmap[IBEX_DEV_RAM].base, NULL);
     }
 }
 
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -XXX,XX +XXX,XX @@ static void sifive_e_machine_init(MachineState *machine)
                           memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
 
     if (machine->kernel_filename) {
-        riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL);
+        riscv_load_kernel(machine, &s->soc.cpus,
+                          memmap[SIFIVE_E_DEV_DTIM].base, NULL);
     }
 }
 
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -XXX,XX +XXX,XX @@ static void sifive_u_machine_init(MachineState *machine)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
+        kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
+                                         kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
             riscv_load_initrd(machine, kernel_entry);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -XXX,XX +XXX,XX @@ static void spike_board_init(MachineState *machine)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
+        kernel_entry = riscv_load_kernel(machine, &s->soc[0],
+                                         kernel_start_addr,
                                          htif_symbol_callback);
 
         if (machine->initrd_filename) {
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -XXX,XX +XXX,XX @@ static void virt_machine_done(Notifier *notifier, void *data)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
+        kernel_entry = riscv_load_kernel(machine, &s->soc[0],
+                                         kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
             riscv_load_initrd(machine, kernel_entry);
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index XXXXXXX..XXXXXXX 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
                                  hwaddr firmware_load_addr,
                                  symbol_fn_t sym_cb);
 target_ulong riscv_load_kernel(MachineState *machine,
+                               RISCVHartArrayState *harts,
                                target_ulong firmware_end_addr,
                                symbol_fn_t sym_cb);
 void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
-- 
2.39.1

The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
the same steps when '-kernel' is used:

- execute load_kernel()
- load init_rd()
- write kernel_cmdline

Let's fold everything inside riscv_load_kernel() to avoid code
repetition. To not change the behavior of boards that aren't calling
riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
allow these boards to opt out from initrd loading.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c            | 21 ++++++++++++++++++---
 hw/riscv/microchip_pfsoc.c | 11 +----------
 hw/riscv/opentitan.c       |  3 ++-
 hw/riscv/sifive_e.c        |  3 ++-
 hw/riscv/sifive_u.c        | 11 +----------
 hw/riscv/spike.c           | 11 +----------
 hw/riscv/virt.c            | 11 +----------
 include/hw/riscv/boot.h    |  1 +
 8 files changed, 27 insertions(+), 45 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
 target_ulong riscv_load_kernel(MachineState *machine,
                                RISCVHartArrayState *harts,
                                target_ulong kernel_start_addr,
+                               bool load_initrd,
                                symbol_fn_t sym_cb)
 {
     const char *kernel_filename = machine->kernel_filename;
     uint64_t kernel_load_base, kernel_entry, kernel_low;
+    void *fdt = machine->fdt;
 
     g_assert(kernel_filename != NULL);
 
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,
             kernel_entry = kernel_low;
         }
 
-        return kernel_entry;
+        goto out;
     }
 
     if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL,
                        NULL, NULL, NULL) > 0) {
-        return kernel_entry;
+        goto out;
     }
 
     if (load_image_targphys_as(kernel_filename, kernel_start_addr,
                                current_machine->ram_size, NULL) > 0) {
-        return kernel_start_addr;
+        kernel_entry = kernel_start_addr;
+        goto out;
     }
 
     error_report("could not load kernel '%s'", kernel_filename);
     exit(1);
+
+out:
+    if (load_initrd && machine->initrd_filename) {
+        riscv_load_initrd(machine, kernel_entry);
+    }
+
+    if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
+        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
+                                machine->kernel_cmdline);
+    }
+
+    return kernel_entry;
 }
 
 void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -XXX,XX +XXX,XX @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
                                                          firmware_end_addr);
 
         kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
-                                         kernel_start_addr, NULL);
-
-        if (machine->initrd_filename) {
-            riscv_load_initrd(machine, kernel_entry);
-        }
-
-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-            qemu_fdt_setprop_string(machine->fdt, "/chosen",
-                                    "bootargs", machine->kernel_cmdline);
-        }
+                                         kernel_start_addr, true, NULL);
 
         /* Compute the fdt load address in dram */
         fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -XXX,XX +XXX,XX @@ static void opentitan_board_init(MachineState *machine)
 
     if (machine->kernel_filename) {
         riscv_load_kernel(machine, &s->soc.cpus,
-                          memmap[IBEX_DEV_RAM].base, NULL);
+                          memmap[IBEX_DEV_RAM].base,
+                          false, NULL);
     }
 }
 
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -XXX,XX +XXX,XX @@ static void sifive_e_machine_init(MachineState *machine)
 
     if (machine->kernel_filename) {
         riscv_load_kernel(machine, &s->soc.cpus,
-                          memmap[SIFIVE_E_DEV_DTIM].base, NULL);
+                          memmap[SIFIVE_E_DEV_DTIM].base,
+                          false, NULL);
     }
 }
 
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -XXX,XX +XXX,XX @@ static void sifive_u_machine_init(MachineState *machine)
                                                          firmware_end_addr);
 
         kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
-                                         kernel_start_addr, NULL);
-
-        if (machine->initrd_filename) {
-            riscv_load_initrd(machine, kernel_entry);
-        }
-
-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
-                                    machine->kernel_cmdline);
-        }
+                                         kernel_start_addr, true, NULL);
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -XXX,XX +XXX,XX @@ static void spike_board_init(MachineState *machine)
 
         kernel_entry = riscv_load_kernel(machine, &s->soc[0],
                                          kernel_start_addr,
-                                         htif_symbol_callback);
-
-        if (machine->initrd_filename) {
-            riscv_load_initrd(machine, kernel_entry);
-        }
-
-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
-                                    machine->kernel_cmdline);
-        }
+                                         true, htif_symbol_callback);
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -XXX,XX +XXX,XX @@ static void virt_machine_done(Notifier *notifier, void *data)
                                                          firmware_end_addr);
 
         kernel_entry = riscv_load_kernel(machine, &s->soc[0],
-                                         kernel_start_addr, NULL);
-
-        if (machine->initrd_filename) {
-            riscv_load_initrd(machine, kernel_entry);
-        }
-
-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
-                                    machine->kernel_cmdline);
-        }
+                                         kernel_start_addr, true, NULL);
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index XXXXXXX..XXXXXXX 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
 target_ulong riscv_load_kernel(MachineState *machine,
                                RISCVHartArrayState *harts,
                                target_ulong firmware_end_addr,
+                               bool load_initrd,
                                symbol_fn_t sym_cb);
 void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
 uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
-- 
2.39.1

The only remaining caller is riscv_load_kernel_and_initrd() which
belongs to the same file.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/boot.c         | 80 ++++++++++++++++++++---------------------
 include/hw/riscv/boot.h |  1 -
 2 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,
     exit(1);
 }
 
+static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
+{
+    const char *filename = machine->initrd_filename;
+    uint64_t mem_size = machine->ram_size;
+    void *fdt = machine->fdt;
+    hwaddr start, end;
+    ssize_t size;
+
+    g_assert(filename != NULL);
+
+    /*
+     * We want to put the initrd far enough into RAM that when the
+     * kernel is uncompressed it will not clobber the initrd. However
+     * on boards without much RAM we must ensure that we still leave
+     * enough room for a decent sized initrd, and on boards with large
+     * amounts of RAM we must avoid the initrd being so far up in RAM
+     * that it is outside lowmem and inaccessible to the kernel.
+     * So for boards with less  than 256MB of RAM we put the initrd
+     * halfway into RAM, and for boards with 256MB of RAM or more we put
+     * the initrd at 128MB.
+     */
+    start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
+
+    size = load_ramdisk(filename, start, mem_size - start);
+    if (size == -1) {
+        size = load_image_targphys(filename, start, mem_size - start);
+        if (size == -1) {
+            error_report("could not load ramdisk '%s'", filename);
+            exit(1);
+        }
+    }
+
+    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
+    if (fdt) {
+        end = start + size;
+        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
+        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
+    }
+}
+
 target_ulong riscv_load_kernel(MachineState *machine,
                                RISCVHartArrayState *harts,
                                target_ulong kernel_start_addr,
@@ -XXX,XX +XXX,XX @@ out:
     return kernel_entry;
 }
 
-void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
-{
-    const char *filename = machine->initrd_filename;
-    uint64_t mem_size = machine->ram_size;
-    void *fdt = machine->fdt;
-    hwaddr start, end;
-    ssize_t size;
-
-    g_assert(filename != NULL);
-
-    /*
-     * We want to put the initrd far enough into RAM that when the
-     * kernel is uncompressed it will not clobber the initrd. However
-     * on boards without much RAM we must ensure that we still leave
-     * enough room for a decent sized initrd, and on boards with large
-     * amounts of RAM we must avoid the initrd being so far up in RAM
-     * that it is outside lowmem and inaccessible to the kernel.
-     * So for boards with less  than 256MB of RAM we put the initrd
-     * halfway into RAM, and for boards with 256MB of RAM or more we put
-     * the initrd at 128MB.
-     */
-    start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
-
-    size = load_ramdisk(filename, start, mem_size - start);
-    if (size == -1) {
-        size = load_image_targphys(filename, start, mem_size - start);
-        if (size == -1) {
-            error_report("could not load ramdisk '%s'", filename);
-            exit(1);
-        }
-    }
-
-    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
-    if (fdt) {
-        end = start + size;
-        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
-        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
-    }
-}
-
 /*
  * This function makes an assumption that the DRAM interval
  * 'dram_base' + 'dram_size' is contiguous.
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index XXXXXXX..XXXXXXX 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong firmware_end_addr,
                                bool load_initrd,
                                symbol_fn_t sym_cb);
-void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
 uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
                                 MachineState *ms);
 void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
-- 
2.39.1