[PATCH 0/4] Refactor x86_load_linux and pass RNG seed via setup_data entry

[PATCH 0/4] Refactor x86_load_linux and pass RNG seed via setup_data entry

[Paolo Bonzini]

As mentioned in the reviews of Jason's patches, the fw_cfg data, or at
least its structure including the size, is part of the guest ABI and
must match across two sides of migration.

It would be possible to handle this with some duplicated code between
the rng seed and DTB handling, but the conditionals to handle the linked
list would be ugly.  Unfortunately the code of x86_load_linux has no
data structures available, it's all of a jumble of local variables.
Hence the first two and largest patches in this series, which remove all
non-Linux code from the function and move the local variables to a struct
as necessary.  The function was long overdue for some cleanup anyway.

With this in place, adding the seed setup_data entry is just a
couple lines of code, plus the scaffolding for a new machine property
"linuxboot-seed".  The property supports on/off/auto values, where "auto"
disables/enables depending on the kernel support for setup data (which was
added in 2.6.26); "on" currently fails when starting with an old kernel,
and probably it should also fail when starting a PVH or multiboot kernel.

Paolo

Jason A. Donenfeld (1):
  hw/i386: pass RNG seed via setup_data entry

Paolo Bonzini (3):
  hw/i386: extract PVH load to a separate function
  hw/i386: define a struct for Linux boot protocol data
  hw/i386: extract handling of setup data linked list

 hw/i386/pc.c                                 |   1 +
 hw/i386/x86.c                                | 303 +++++++++++--------
 include/hw/i386/x86.h                        |   2 +
 include/standard-headers/asm-x86/bootparam.h |   1 +
 4 files changed, 185 insertions(+), 122 deletions(-)

-- 
2.36.1

[PATCH 1/4] hw/i386: extract PVH load to a separate function

[Paolo Bonzini]

Only keep the code x86_load_linux the code that is relevant to the Linux
boot protocol.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/x86.c | 131 ++++++++++++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 62 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 6003b4b2df..449edb076e 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -763,6 +763,62 @@ static bool load_elfboot(const char *kernel_filename,
     return true;
 }
 
+static bool load_pvh(X86MachineState *x86ms, FWCfgState *fw_cfg, const char *kernel_filename,
+                     const char *initrd_filename, uint32_t initrd_max, const char *kernel_cmdline,
+                     size_t kernel_size, uint8_t *header)
+{
+    if (!load_elfboot(kernel_filename, kernel_size,
+                      header, pvh_start_addr, fw_cfg)) {
+        return false;
+    }
+
+    fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
+        strlen(kernel_cmdline) + 1);
+    fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
+
+    fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, sizeof(header));
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA,
+                     header, sizeof(header));
+
+    /* load initrd */
+    if (initrd_filename) {
+        GMappedFile *mapped_file;
+        gsize initrd_size;
+        gchar *initrd_data;
+        GError *gerr = NULL;
+
+        mapped_file = g_mapped_file_new(initrd_filename, false, &gerr);
+        if (!mapped_file) {
+            fprintf(stderr, "qemu: error reading initrd %s: %s\n",
+                    initrd_filename, gerr->message);
+            exit(1);
+        }
+        x86ms->initrd_mapped_file = mapped_file;
+
+        initrd_data = g_mapped_file_get_contents(mapped_file);
+        initrd_size = g_mapped_file_get_length(mapped_file);
+        if (initrd_size >= initrd_max) {
+            fprintf(stderr, "qemu: initrd is too large, cannot support."
+                    "(max: %"PRIu32", need %"PRId64")\n",
+                    initrd_max, (uint64_t)initrd_size);
+            exit(1);
+        }
+
+        hwaddr initrd_addr = (initrd_max - initrd_size) & ~4095;
+
+        fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr);
+        fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
+        fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, initrd_data,
+                         initrd_size);
+    }
+
+    option_rom[nb_option_roms].bootindex = 0;
+    option_rom[nb_option_roms].name = "pvh.bin";
+    nb_option_roms++;
+
+    return true;
+}
+
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
@@ -774,7 +830,7 @@ void x86_load_linux(X86MachineState *x86ms,
     int dtb_size, setup_data_offset;
     uint32_t initrd_max;
     uint8_t header[8192], *setup, *kernel;
-    hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
+    hwaddr real_addr, prot_addr, cmdline_addr;
     FILE *f;
     char *vmode;
     MachineState *machine = MACHINE(x86ms);
@@ -785,6 +841,8 @@ void x86_load_linux(X86MachineState *x86ms,
     const char *kernel_cmdline = machine->kernel_cmdline;
     SevKernelLoaderContext sev_load_ctx = {};
 
+    initrd_max = x86ms->below_4g_mem_size - acpi_data_size - 1;
+
     /* Align to 16 bytes as a paranoia measure */
     cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
 
@@ -806,9 +864,7 @@ void x86_load_linux(X86MachineState *x86ms,
     }
 
     /* kernel protocol version */
-    if (ldl_p(header + 0x202) == 0x53726448) {
-        protocol = lduw_p(header + 0x206);
-    } else {
+    if (ldl_p(header + 0x202) != 0x53726448) {
         /*
          * This could be a multiboot kernel. If it is, let's stop treating it
          * like a Linux kernel.
@@ -826,58 +882,14 @@ void x86_load_linux(X86MachineState *x86ms,
          * If load_elfboot() is successful, populate the fw_cfg info.
          */
         if (pvh_enabled &&
-            load_elfboot(kernel_filename, kernel_size,
-                         header, pvh_start_addr, fw_cfg)) {
+            load_pvh(x86ms, fw_cfg, kernel_filename, initrd_filename,
+                     initrd_max, kernel_cmdline, kernel_size, header)) {
             fclose(f);
-
-            fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
-                strlen(kernel_cmdline) + 1);
-            fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
-
-            fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, sizeof(header));
-            fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA,
-                             header, sizeof(header));
-
-            /* load initrd */
-            if (initrd_filename) {
-                GMappedFile *mapped_file;
-                gsize initrd_size;
-                gchar *initrd_data;
-                GError *gerr = NULL;
-
-                mapped_file = g_mapped_file_new(initrd_filename, false, &gerr);
-                if (!mapped_file) {
-                    fprintf(stderr, "qemu: error reading initrd %s: %s\n",
-                            initrd_filename, gerr->message);
-                    exit(1);
-                }
-                x86ms->initrd_mapped_file = mapped_file;
-
-                initrd_data = g_mapped_file_get_contents(mapped_file);
-                initrd_size = g_mapped_file_get_length(mapped_file);
-                initrd_max = x86ms->below_4g_mem_size - acpi_data_size - 1;
-                if (initrd_size >= initrd_max) {
-                    fprintf(stderr, "qemu: initrd is too large, cannot support."
-                            "(max: %"PRIu32", need %"PRId64")\n",
-                            initrd_max, (uint64_t)initrd_size);
-                    exit(1);
-                }
-
-                initrd_addr = (initrd_max - initrd_size) & ~4095;
-
-                fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr);
-                fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
-                fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, initrd_data,
-                                 initrd_size);
-            }
-
-            option_rom[nb_option_roms].bootindex = 0;
-            option_rom[nb_option_roms].name = "pvh.bin";
-            nb_option_roms++;
-
             return;
-        }
+            }
         protocol = 0;
+    } else  {
+        protocol = lduw_p(header + 0x206);
     }
 
     if (protocol < 0x200 || !(header[0x211] & 0x01)) {
@@ -914,17 +926,12 @@ void x86_load_linux(X86MachineState *x86ms,
          * support the 64-bit boot protocol (specifically the ext_ramdisk_image
          * field).
          *
-         * Therefore here just limit initrd_max to UINT32_MAX simply as well.
+         * Therefore here just limit initrd_max to the available memory below 4G.
          */
-        initrd_max = UINT32_MAX;
     } else if (protocol >= 0x203) {
-        initrd_max = ldl_p(header + 0x22c);
+        initrd_max = MIN(initrd_max, ldl_p(header + 0x22c));
     } else {
-        initrd_max = 0x37ffffff;
-    }
-
-    if (initrd_max >= x86ms->below_4g_mem_size - acpi_data_size) {
-        initrd_max = x86ms->below_4g_mem_size - acpi_data_size - 1;
+        initrd_max = MIN(initrd_max, 0x37ffffff);
     }
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
@@ -1008,7 +1015,7 @@ void x86_load_linux(X86MachineState *x86ms,
             exit(1);
         }
 
-        initrd_addr = (initrd_max - initrd_size) & ~4095;
+        hwaddr initrd_addr = (initrd_max - initrd_size) & ~4095;
 
         fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr);
         fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
-- 
2.36.1

[PATCH 2/4] hw/i386: define a struct for Linux boot protocol data

[Paolo Bonzini]

In preparation for moving parts of x86_load_linux to separate function,
define a struct with the data that needs to be passed back and forth.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/x86.c | 116 +++++++++++++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 54 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 449edb076e..253a6ff536 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -819,22 +819,30 @@ static bool load_pvh(X86MachineState *x86ms, FWCfgState *fw_cfg, const char *ker
     return true;
 }
 
+typedef struct LinuxBootData {
+    uint16_t protocol;
+    uint8_t header[8192];
+    hwaddr prot_addr;
+    size_t kernel_size;
+    uint8_t *kernel;
+    size_t setup_data_offset;
+} LinuxBootData;
+
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
                     bool pvh_enabled)
 {
     bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
-    uint16_t protocol;
-    int setup_size, kernel_size, cmdline_size;
-    int dtb_size, setup_data_offset;
+    int setup_size, cmdline_size;
+    int dtb_size;
     uint32_t initrd_max;
-    uint8_t header[8192], *setup, *kernel;
-    hwaddr real_addr, prot_addr, cmdline_addr;
+    uint8_t *setup;
+    hwaddr real_addr, cmdline_addr;
     FILE *f;
     char *vmode;
     MachineState *machine = MACHINE(x86ms);
-    struct setup_data *setup_data;
+    LinuxBootData data = { 0 };
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
     const char *dtb_filename = machine->dtb;
@@ -854,17 +862,17 @@ void x86_load_linux(X86MachineState *x86ms,
         exit(1);
     }
 
-    kernel_size = get_file_size(f);
-    if (!kernel_size ||
-        fread(header, 1, MIN(ARRAY_SIZE(header), kernel_size), f) !=
-        MIN(ARRAY_SIZE(header), kernel_size)) {
+    data.kernel_size = get_file_size(f);
+    if (!data.kernel_size ||
+        fread(data.header, 1, MIN(ARRAY_SIZE(data.header), data.kernel_size), f) !=
+        MIN(ARRAY_SIZE(data.header), data.kernel_size)) {
         fprintf(stderr, "qemu: could not load kernel '%s': %s\n",
                 kernel_filename, strerror(errno));
         exit(1);
     }
 
     /* kernel protocol version */
-    if (ldl_p(header + 0x202) != 0x53726448) {
+    if (ldl_p(data.header + 0x202) != 0x53726448) {
         /*
          * This could be a multiboot kernel. If it is, let's stop treating it
          * like a Linux kernel.
@@ -873,7 +881,7 @@ void x86_load_linux(X86MachineState *x86ms,
          * header before to load it.
          */
         if (load_multiboot(x86ms, fw_cfg, f, kernel_filename, initrd_filename,
-                           kernel_cmdline, kernel_size, header)) {
+                           kernel_cmdline, data.kernel_size, data.header)) {
             return;
         }
         /*
@@ -883,35 +891,35 @@ void x86_load_linux(X86MachineState *x86ms,
          */
         if (pvh_enabled &&
             load_pvh(x86ms, fw_cfg, kernel_filename, initrd_filename,
-                     initrd_max, kernel_cmdline, kernel_size, header)) {
+                     initrd_max, kernel_cmdline, data.kernel_size, data.header)) {
             fclose(f);
             return;
             }
-        protocol = 0;
+        data.protocol = 0;
     } else  {
-        protocol = lduw_p(header + 0x206);
+        data.protocol = lduw_p(data.header + 0x206);
     }
 
-    if (protocol < 0x200 || !(header[0x211] & 0x01)) {
+    if (data.protocol < 0x200 || !(data.header[0x211] & 0x01)) {
         /* Low kernel */
         real_addr    = 0x90000;
         cmdline_addr = 0x9a000 - cmdline_size;
-        prot_addr    = 0x10000;
-    } else if (protocol < 0x202) {
+        data.prot_addr = 0x10000;
+    } else if (data.protocol < 0x202) {
         /* High but ancient kernel */
         real_addr    = 0x90000;
         cmdline_addr = 0x9a000 - cmdline_size;
-        prot_addr    = 0x100000;
+        data.prot_addr = 0x100000;
     } else {
         /* High and recent kernel */
         real_addr    = 0x10000;
         cmdline_addr = 0x20000;
-        prot_addr    = 0x100000;
+        data.prot_addr = 0x100000;
     }
 
     /* highest address for loading the initrd */
-    if (protocol >= 0x20c &&
-        lduw_p(header + 0x236) & XLF_CAN_BE_LOADED_ABOVE_4G) {
+    if (data.protocol >= 0x20c &&
+        lduw_p(data.header + 0x236) & XLF_CAN_BE_LOADED_ABOVE_4G) {
         /*
          * Linux has supported initrd up to 4 GB for a very long time (2007,
          * long before XLF_CAN_BE_LOADED_ABOVE_4G which was added in 2013),
@@ -928,8 +936,8 @@ void x86_load_linux(X86MachineState *x86ms,
          *
          * Therefore here just limit initrd_max to the available memory below 4G.
          */
-    } else if (protocol >= 0x203) {
-        initrd_max = MIN(initrd_max, ldl_p(header + 0x22c));
+    } else if (data.protocol >= 0x203) {
+        initrd_max = MIN(initrd_max, ldl_p(data.header + 0x22c));
     } else {
         initrd_max = MIN(initrd_max, 0x37ffffff);
     }
@@ -940,11 +948,11 @@ void x86_load_linux(X86MachineState *x86ms,
     sev_load_ctx.cmdline_data = (char *)kernel_cmdline;
     sev_load_ctx.cmdline_size = strlen(kernel_cmdline) + 1;
 
-    if (protocol >= 0x202) {
-        stl_p(header + 0x228, cmdline_addr);
+    if (data.protocol >= 0x202) {
+        stl_p(data.header + 0x228, cmdline_addr);
     } else {
-        stw_p(header + 0x20, 0xA33F);
-        stw_p(header + 0x22, cmdline_addr - real_addr);
+        stw_p(data.header + 0x20, 0xA33F);
+        stw_p(data.header + 0x22, cmdline_addr - real_addr);
     }
 
     /* handle vga= parameter */
@@ -968,7 +976,7 @@ void x86_load_linux(X86MachineState *x86ms,
                 exit(1);
             }
         }
-        stw_p(header + 0x1fa, video_mode);
+        stw_p(data.header + 0x1fa, video_mode);
     }
 
     /* loader type */
@@ -977,13 +985,13 @@ void x86_load_linux(X86MachineState *x86ms,
      * If this code is substantially changed, you may want to consider
      * incrementing the revision.
      */
-    if (protocol >= 0x200) {
-        header[0x210] = 0xB0;
+    if (data.protocol >= 0x200) {
+        data.header[0x210] = 0xB0;
     }
     /* heap */
-    if (protocol >= 0x201) {
-        header[0x211] |= 0x80; /* CAN_USE_HEAP */
-        stw_p(header + 0x224, cmdline_addr - real_addr - 0x200);
+    if (data.protocol >= 0x201) {
+        data.header[0x211] |= 0x80; /* CAN_USE_HEAP */
+        stw_p(data.header + 0x224, cmdline_addr - real_addr - 0x200);
     }
 
     /* load initrd */
@@ -993,7 +1001,7 @@ void x86_load_linux(X86MachineState *x86ms,
         gchar *initrd_data;
         GError *gerr = NULL;
 
-        if (protocol < 0x200) {
+        if (data.protocol < 0x200) {
             fprintf(stderr, "qemu: linux kernel too old to load a ram disk\n");
             exit(1);
         }
@@ -1023,30 +1031,30 @@ void x86_load_linux(X86MachineState *x86ms,
         sev_load_ctx.initrd_data = initrd_data;
         sev_load_ctx.initrd_size = initrd_size;
 
-        stl_p(header + 0x218, initrd_addr);
-        stl_p(header + 0x21c, initrd_size);
+        stl_p(data.header + 0x218, initrd_addr);
+        stl_p(data.header + 0x21c, initrd_size);
     }
 
     /* load kernel and setup */
-    setup_size = header[0x1f1];
+    setup_size = data.header[0x1f1];
     if (setup_size == 0) {
         setup_size = 4;
     }
     setup_size = (setup_size + 1) * 512;
-    if (setup_size > kernel_size) {
+    if (setup_size > data.kernel_size) {
         fprintf(stderr, "qemu: invalid kernel header\n");
         exit(1);
     }
-    kernel_size -= setup_size;
+    data.kernel_size -= setup_size;
 
     setup  = g_malloc(setup_size);
-    kernel = g_malloc(kernel_size);
+    data.kernel = g_malloc(data.kernel_size);
     fseek(f, 0, SEEK_SET);
     if (fread(setup, 1, setup_size, f) != setup_size) {
         fprintf(stderr, "fread() failed\n");
         exit(1);
     }
-    if (fread(kernel, 1, kernel_size, f) != kernel_size) {
+    if (fread(data.kernel, 1, data.kernel_size, f) != data.kernel_size) {
         fprintf(stderr, "fread() failed\n");
         exit(1);
     }
@@ -1054,7 +1062,7 @@ void x86_load_linux(X86MachineState *x86ms,
 
     /* append dtb to kernel */
     if (dtb_filename) {
-        if (protocol < 0x209) {
+        if (data.protocol < 0x209) {
             fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
             exit(1);
         }
@@ -1066,13 +1074,13 @@ void x86_load_linux(X86MachineState *x86ms,
             exit(1);
         }
 
-        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
-        kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
-        kernel = g_realloc(kernel, kernel_size);
+        data.setup_data_offset = QEMU_ALIGN_UP(data.kernel_size, 16);
+        data.kernel_size = data.setup_data_offset + sizeof(struct setup_data) + dtb_size;
+        data.kernel = g_realloc(data.kernel, data.kernel_size);
 
-        stq_p(header + 0x250, prot_addr + setup_data_offset);
+        stq_p(data.header + 0x250, data.prot_addr + data.setup_data_offset);
 
-        setup_data = (struct setup_data *)(kernel + setup_data_offset);
+        struct setup_data *setup_data = (struct setup_data *)(data.kernel + data.setup_data_offset);
         setup_data->next = 0;
         setup_data->type = cpu_to_le32(SETUP_DTB);
         setup_data->len = cpu_to_le32(dtb_size);
@@ -1088,14 +1096,14 @@ void x86_load_linux(X86MachineState *x86ms,
      * file the user passed in.
      */
     if (!sev_enabled()) {
-        memcpy(setup, header, MIN(sizeof(header), setup_size));
+        memcpy(setup, data.header, MIN(sizeof(data.header), setup_size));
     }
 
-    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
-    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
-    sev_load_ctx.kernel_data = (char *)kernel;
-    sev_load_ctx.kernel_size = kernel_size;
+    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, data.prot_addr);
+    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, data.kernel_size);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, data.kernel, data.kernel_size);
+    sev_load_ctx.kernel_data = (char *)data.kernel;
+    sev_load_ctx.kernel_size = data.kernel_size;
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
-- 
2.36.1

[PATCH 3/4] hw/i386: extract handling of setup data linked list

[Paolo Bonzini]

In preparation for the introduction of a second setup data block,
manage the linked list and the reallocation of data.kernel in
a separate function.  Unlike the code that's being moved, the
function can be called more than once.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/x86.c | 47 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 253a6ff536..564bf3834b 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -828,6 +828,34 @@ typedef struct LinuxBootData {
     size_t setup_data_offset;
 } LinuxBootData;
 
+static void *add_setup_data(LinuxBootData *data, uint32_t size, uint32_t type)
+{
+    struct setup_data *setup_data;
+    size_t prev_setup_data_offset = data->setup_data_offset;
+
+    if (data->protocol < 0x209) {
+        fprintf(stderr, "qemu: Linux kernel too old to add setup data\n");
+        exit(1);
+    }
+
+    data->setup_data_offset = QEMU_ALIGN_UP(data->kernel_size, 16);
+    data->kernel_size = data->setup_data_offset + sizeof(struct setup_data) + size;
+    data->kernel = g_realloc(data->kernel, data->kernel_size);
+
+    if (prev_setup_data_offset) {
+        setup_data = (struct setup_data *)(data->kernel + prev_setup_data_offset);
+        setup_data->next = cpu_to_le64(data->prot_addr + data->setup_data_offset);
+    } else {
+        stq_p(data->header + 0x250, data->prot_addr + data->setup_data_offset);
+    }
+
+    setup_data = (struct setup_data *)(data->kernel + data->setup_data_offset);
+    setup_data->next = 0;
+    setup_data->type = cpu_to_le32(type);
+    setup_data->len = cpu_to_le32(size);
+    return setup_data->data;
+}
+
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
@@ -1062,11 +1090,6 @@ void x86_load_linux(X86MachineState *x86ms,
 
     /* append dtb to kernel */
     if (dtb_filename) {
-        if (data.protocol < 0x209) {
-            fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
-            exit(1);
-        }
-
         dtb_size = get_image_size(dtb_filename);
         if (dtb_size <= 0) {
             fprintf(stderr, "qemu: error reading dtb %s: %s\n",
@@ -1074,18 +1097,8 @@ void x86_load_linux(X86MachineState *x86ms,
             exit(1);
         }
 
-        data.setup_data_offset = QEMU_ALIGN_UP(data.kernel_size, 16);
-        data.kernel_size = data.setup_data_offset + sizeof(struct setup_data) + dtb_size;
-        data.kernel = g_realloc(data.kernel, data.kernel_size);
-
-        stq_p(data.header + 0x250, data.prot_addr + data.setup_data_offset);
-
-        struct setup_data *setup_data = (struct setup_data *)(data.kernel + data.setup_data_offset);
-        setup_data->next = 0;
-        setup_data->type = cpu_to_le32(SETUP_DTB);
-        setup_data->len = cpu_to_le32(dtb_size);
-
-        load_image_size(dtb_filename, setup_data->data, dtb_size);
+        void *dtb = add_setup_data(&data, dtb_size, SETUP_DTB);
+        load_image_size(dtb_filename, dtb, dtb_size);
     }
 
     /*
-- 
2.36.1

[PATCH 4/4] hw/i386: pass RNG seed via setup_data entry

[Paolo Bonzini]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>

Tiny machines optimized for fast boot time generally don't use EFI,
which means a random seed has to be supplied some other way. For this
purpose, Linux (≥5.20) supports passing a seed in the setup_data table
with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and
specialized bootloaders. The linked commit shows the upstream kernel
implementation.

Link: https://git.kernel.org/tip/tip/c/68b8e9713c8
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Message-Id: <20220719115300.104095-1-Jason@zx2c4.com>
[Mostly rewritten to preserve guest ABI, but still starting from Jason's
 code. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/pc.c                                 |  1 +
 hw/i386/x86.c                                | 31 ++++++++++++++++++++
 include/hw/i386/x86.h                        |  2 ++
 include/standard-headers/asm-x86/bootparam.h |  1 +
 4 files changed, 35 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 774cb2bf07..d456fbb166 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -112,6 +112,7 @@ const size_t pc_compat_7_0_len = G_N_ELEMENTS(pc_compat_7_0);
 
 GlobalProperty pc_compat_6_2[] = {
     { "virtio-mem", "unplugged-inaccessible", "off" },
+    { TYPE_X86_MACHINE, "linuxboot-seed", "off" },
 };
 const size_t pc_compat_6_2_len = G_N_ELEMENTS(pc_compat_6_2);
 
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 564bf3834b..c5d01e084a 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -26,6 +26,7 @@
 #include "qemu/cutils.h"
 #include "qemu/units.h"
 #include "qemu/datadir.h"
+#include "qemu/guest-random.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qapi-visit-common.h"
@@ -1088,6 +1089,12 @@ void x86_load_linux(X86MachineState *x86ms,
     }
     fclose(f);
 
+    if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF &&
+        (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
+        void *seed = add_setup_data(&data, 32, SETUP_RNG_SEED);
+        qemu_guest_getrandom_nofail(seed, 32);
+    }
+
     /* append dtb to kernel */
     if (dtb_filename) {
         dtb_size = get_image_size(dtb_filename);
@@ -1247,6 +1254,23 @@ static void x86_machine_set_smm(Object *obj, Visitor *v, const char *name,
     visit_type_OnOffAuto(v, name, &x86ms->smm, errp);
 }
 
+static void x86_machine_get_linuxboot_seed(Object *obj, Visitor *v, const char *name,
+                                           void *opaque, Error **errp)
+{
+    X86MachineState *x86ms = X86_MACHINE(obj);
+    OnOffAuto linuxboot_seed = x86ms->linuxboot_seed;
+
+    visit_type_OnOffAuto(v, name, &linuxboot_seed, errp);
+}
+
+static void x86_machine_set_linuxboot_seed(Object *obj, Visitor *v, const char *name,
+                                           void *opaque, Error **errp)
+{
+    X86MachineState *x86ms = X86_MACHINE(obj);
+
+    visit_type_OnOffAuto(v, name, &x86ms->linuxboot_seed, errp);
+}
+
 bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms)
 {
     if (x86ms->acpi == ON_OFF_AUTO_OFF) {
@@ -1397,6 +1421,7 @@ static void x86_machine_initfn(Object *obj)
     x86ms->acpi = ON_OFF_AUTO_AUTO;
     x86ms->pit = ON_OFF_AUTO_AUTO;
     x86ms->pic = ON_OFF_AUTO_AUTO;
+    x86ms->linuxboot_seed = ON_OFF_AUTO_AUTO;
     x86ms->pci_irq_mask = ACPI_BUILD_PCI_IRQS;
     x86ms->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
     x86ms->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
@@ -1435,6 +1460,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, X86_MACHINE_PIT,
         "Enable i8254 PIT");
 
+    object_class_property_add(oc, X86_MACHINE_LINUXBOOT_SEED, "OnOffAuto",
+        x86_machine_get_linuxboot_seed, x86_machine_set_linuxboot_seed,
+        NULL, NULL);
+    object_class_property_set_description(oc, X86_MACHINE_LINUXBOOT_SEED,
+        "Pass random number seed to -kernel Linux image");
+
     object_class_property_add(oc, X86_MACHINE_PIC, "OnOffAuto",
                               x86_machine_get_pic,
                               x86_machine_set_pic,
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 9089bdd99c..edf0f6799e 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -67,6 +67,7 @@ struct X86MachineState {
     OnOffAuto acpi;
     OnOffAuto pit;
     OnOffAuto pic;
+    OnOffAuto linuxboot_seed;
 
     char *oem_id;
     char *oem_table_id;
@@ -91,6 +92,7 @@ struct X86MachineState {
 #define X86_MACHINE_OEM_ID           "x-oem-id"
 #define X86_MACHINE_OEM_TABLE_ID     "x-oem-table-id"
 #define X86_MACHINE_BUS_LOCK_RATELIMIT  "bus-lock-ratelimit"
+#define X86_MACHINE_LINUXBOOT_SEED      "linuxboot-seed"
 
 #define TYPE_X86_MACHINE   MACHINE_TYPE_NAME("x86")
 OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h
index 072e2ed546..b2aaad10e5 100644
--- a/include/standard-headers/asm-x86/bootparam.h
+++ b/include/standard-headers/asm-x86/bootparam.h
@@ -10,6 +10,7 @@
 #define SETUP_EFI			4
 #define SETUP_APPLE_PROPERTIES		5
 #define SETUP_JAILHOUSE			6
+#define SETUP_RNG_SEED			9
 
 #define SETUP_INDIRECT			(1<<31)
 
-- 
2.36.1

Re: [PATCH 4/4] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

On Thu, Jul 21, 2022 at 2:29 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> +static void x86_machine_get_linuxboot_seed(Object *obj, Visitor *v, const char *name,
> +                                           void *opaque, Error **errp)
> +{
> +    X86MachineState *x86ms = X86_MACHINE(obj);
> +    OnOffAuto linuxboot_seed = x86ms->linuxboot_seed;
> +
> +    visit_type_OnOffAuto(v, name, &linuxboot_seed, errp);
> +}
> +
> +static void x86_machine_set_linuxboot_seed(Object *obj, Visitor *v, const char *name,
> +                                           void *opaque, Error **errp)
> +{
> +    X86MachineState *x86ms = X86_MACHINE(obj);
> +
> +    visit_type_OnOffAuto(v, name, &x86ms->linuxboot_seed, errp);
> +}
> +

Gross, no! There is no reason at all to make this into a user tunable.
Please don't do that. The whole point is that this is a simple
transparent mechanism.

There's also no need to usurp my patchset. I sent a v7 incorporating
your feedback. So this isn't really appreciated either. I actually
asked you not to usurp this over IRC, but you did anyway. If your goal
is "alienate this contributor so he doesn't like working on QEMU,"
then you're succeeding.

Here's v7:
https://lore.kernel.org/qemu-devel/20220721125636.446842-1-Jason@zx2c4.com/

This will handle your ridiculous theoretical migratory concerns with
minimal invasiveness and without having to introduce userfacing
tunables.

Let's keep discussion on that v7 thread, please.

Thanks,
Jason

Re: [PATCH 4/4] hw/i386: pass RNG seed via setup_data entry

[Michael S. Tsirkin]

On Thu, Jul 21, 2022 at 02:29:37PM +0200, Paolo Bonzini wrote:
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> 
> Tiny machines optimized for fast boot time generally don't use EFI,
> which means a random seed has to be supplied some other way. For this
> purpose, Linux (≥5.20) supports passing a seed in the setup_data table
> with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and
> specialized bootloaders. The linked commit shows the upstream kernel
> implementation.
> 
> Link: https://git.kernel.org/tip/tip/c/68b8e9713c8
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Message-Id: <20220719115300.104095-1-Jason@zx2c4.com>
> [Mostly rewritten to preserve guest ABI, but still starting from Jason's
>  code. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/i386/pc.c                                 |  1 +
>  hw/i386/x86.c                                | 31 ++++++++++++++++++++
>  include/hw/i386/x86.h                        |  2 ++
>  include/standard-headers/asm-x86/bootparam.h |  1 +
>  4 files changed, 35 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 774cb2bf07..d456fbb166 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -112,6 +112,7 @@ const size_t pc_compat_7_0_len = G_N_ELEMENTS(pc_compat_7_0);
>  
>  GlobalProperty pc_compat_6_2[] = {
>      { "virtio-mem", "unplugged-inaccessible", "off" },
> +    { TYPE_X86_MACHINE, "linuxboot-seed", "off" },
>  };
>  const size_t pc_compat_6_2_len = G_N_ELEMENTS(pc_compat_6_2);
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 564bf3834b..c5d01e084a 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -26,6 +26,7 @@
>  #include "qemu/cutils.h"
>  #include "qemu/units.h"
>  #include "qemu/datadir.h"
> +#include "qemu/guest-random.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qapi-visit-common.h"
> @@ -1088,6 +1089,12 @@ void x86_load_linux(X86MachineState *x86ms,
>      }
>      fclose(f);
>  
> +    if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF &&
> +        (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
> +        void *seed = add_setup_data(&data, 32, SETUP_RNG_SEED);
> +        qemu_guest_getrandom_nofail(seed, 32);
> +    }
> +
>      /* append dtb to kernel */
>      if (dtb_filename) {
>          dtb_size = get_image_size(dtb_filename);
> @@ -1247,6 +1254,23 @@ static void x86_machine_set_smm(Object *obj, Visitor *v, const char *name,
>      visit_type_OnOffAuto(v, name, &x86ms->smm, errp);
>  }
>  
> +static void x86_machine_get_linuxboot_seed(Object *obj, Visitor *v, const char *name,
> +                                           void *opaque, Error **errp)
> +{
> +    X86MachineState *x86ms = X86_MACHINE(obj);
> +    OnOffAuto linuxboot_seed = x86ms->linuxboot_seed;
> +
> +    visit_type_OnOffAuto(v, name, &linuxboot_seed, errp);
> +}
> +
> +static void x86_machine_set_linuxboot_seed(Object *obj, Visitor *v, const char *name,
> +                                           void *opaque, Error **errp)
> +{
> +    X86MachineState *x86ms = X86_MACHINE(obj);
> +
> +    visit_type_OnOffAuto(v, name, &x86ms->linuxboot_seed, errp);
> +}
> +
>  bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms)
>  {
>      if (x86ms->acpi == ON_OFF_AUTO_OFF) {
> @@ -1397,6 +1421,7 @@ static void x86_machine_initfn(Object *obj)
>      x86ms->acpi = ON_OFF_AUTO_AUTO;
>      x86ms->pit = ON_OFF_AUTO_AUTO;
>      x86ms->pic = ON_OFF_AUTO_AUTO;
> +    x86ms->linuxboot_seed = ON_OFF_AUTO_AUTO;
>      x86ms->pci_irq_mask = ACPI_BUILD_PCI_IRQS;
>      x86ms->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
>      x86ms->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
> @@ -1435,6 +1460,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_set_description(oc, X86_MACHINE_PIT,
>          "Enable i8254 PIT");
>  
> +    object_class_property_add(oc, X86_MACHINE_LINUXBOOT_SEED, "OnOffAuto",
> +        x86_machine_get_linuxboot_seed, x86_machine_set_linuxboot_seed,
> +        NULL, NULL);
> +    object_class_property_set_description(oc, X86_MACHINE_LINUXBOOT_SEED,
> +        "Pass random number seed to -kernel Linux image");
> +
>      object_class_property_add(oc, X86_MACHINE_PIC, "OnOffAuto",
>                                x86_machine_get_pic,
>                                x86_machine_set_pic,
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index 9089bdd99c..edf0f6799e 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -67,6 +67,7 @@ struct X86MachineState {
>      OnOffAuto acpi;
>      OnOffAuto pit;
>      OnOffAuto pic;
> +    OnOffAuto linuxboot_seed;
>  
>      char *oem_id;
>      char *oem_table_id;
> @@ -91,6 +92,7 @@ struct X86MachineState {
>  #define X86_MACHINE_OEM_ID           "x-oem-id"
>  #define X86_MACHINE_OEM_TABLE_ID     "x-oem-table-id"
>  #define X86_MACHINE_BUS_LOCK_RATELIMIT  "bus-lock-ratelimit"
> +#define X86_MACHINE_LINUXBOOT_SEED      "linuxboot-seed"

I am guessing we should prefix this with "x-" so we don't commit
to a user interface.

>  
>  #define TYPE_X86_MACHINE   MACHINE_TYPE_NAME("x86")
>  OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
> diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h
> index 072e2ed546..b2aaad10e5 100644
> --- a/include/standard-headers/asm-x86/bootparam.h
> +++ b/include/standard-headers/asm-x86/bootparam.h
> @@ -10,6 +10,7 @@
>  #define SETUP_EFI			4
>  #define SETUP_APPLE_PROPERTIES		5
>  #define SETUP_JAILHOUSE			6
> +#define SETUP_RNG_SEED			9
>  
>  #define SETUP_INDIRECT			(1<<31)
>  
> -- 
> 2.36.1

Re: [PATCH 0/4] Refactor x86_load_linux and pass RNG seed via setup_data entry

[Michael S. Tsirkin]

On Thu, Jul 21, 2022 at 02:29:33PM +0200, Paolo Bonzini wrote:
> As mentioned in the reviews of Jason's patches, the fw_cfg data, or at
> least its structure including the size, is part of the guest ABI and
> must match across two sides of migration.
> 
> It would be possible to handle this with some duplicated code between
> the rng seed and DTB handling, but the conditionals to handle the linked
> list would be ugly.  Unfortunately the code of x86_load_linux has no
> data structures available, it's all of a jumble of local variables.
> Hence the first two and largest patches in this series, which remove all
> non-Linux code from the function and move the local variables to a struct
> as necessary.  The function was long overdue for some cleanup anyway.
> 
> With this in place, adding the seed setup_data entry is just a
> couple lines of code, plus the scaffolding for a new machine property
> "linuxboot-seed".  The property supports on/off/auto values, where "auto"
> disables/enables depending on the kernel support for setup data (which was
> added in 2.6.26); "on" currently fails when starting with an old kernel,
> and probably it should also fail when starting a PVH or multiboot kernel.
> 
> Paolo

I like the refactoring

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

To avoid creating extra work for Jason and confusing
attribution, maybe apply Jason's patch then your refactoring
on top?

> Jason A. Donenfeld (1):
>   hw/i386: pass RNG seed via setup_data entry
> 
> Paolo Bonzini (3):
>   hw/i386: extract PVH load to a separate function
>   hw/i386: define a struct for Linux boot protocol data
>   hw/i386: extract handling of setup data linked list
> 
>  hw/i386/pc.c                                 |   1 +
>  hw/i386/x86.c                                | 303 +++++++++++--------
>  include/hw/i386/x86.h                        |   2 +
>  include/standard-headers/asm-x86/bootparam.h |   1 +
>  4 files changed, 185 insertions(+), 122 deletions(-)
> 
> -- 
> 2.36.1

Re: [PATCH 0/4] Refactor x86_load_linux and pass RNG seed via setup_data entry

[Jason A. Donenfeld]

Hi Michael,

On Thu, Jul 21, 2022 at 10:52:32AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jul 21, 2022 at 02:29:33PM +0200, Paolo Bonzini wrote:
> > As mentioned in the reviews of Jason's patches, the fw_cfg data, or at
> > least its structure including the size, is part of the guest ABI and
> > must match across two sides of migration.
> > 
> > It would be possible to handle this with some duplicated code between
> > the rng seed and DTB handling, but the conditionals to handle the linked
> > list would be ugly.  Unfortunately the code of x86_load_linux has no
> > data structures available, it's all of a jumble of local variables.
> > Hence the first two and largest patches in this series, which remove all
> > non-Linux code from the function and move the local variables to a struct
> > as necessary.  The function was long overdue for some cleanup anyway.
> > 
> > With this in place, adding the seed setup_data entry is just a
> > couple lines of code, plus the scaffolding for a new machine property
> > "linuxboot-seed".  The property supports on/off/auto values, where "auto"
> > disables/enables depending on the kernel support for setup data (which was
> > added in 2.6.26); "on" currently fails when starting with an old kernel,
> > and probably it should also fail when starting a PVH or multiboot kernel.
> > 
> > Paolo
> 
> I like the refactoring
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> To avoid creating extra work for Jason and confusing
> attribution, maybe apply Jason's patch then your refactoring
> on top?

Yes, I think it'd make sense to apply:
https://lore.kernel.org/qemu-devel/20220721125636.446842-1-Jason@zx2c4.com/
as-is, without any changes, since that handles your migration concerns.

And then after, if you want to refactor things in general, apply that on
top.

As I mentioned before, we really don't need nor want a user-facing
option for this. What I do in that v7 there is sufficient and fine.

Michael - do you want to take that v7 into your tree?

Jason

Re: [PATCH 4/4] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

Hi Michael,

On Thu, Jul 21, 2022 at 10:47:57AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jul 21, 2022 at 02:29:37PM +0200, Paolo Bonzini wrote:
> >  #define X86_MACHINE_OEM_TABLE_ID     "x-oem-table-id"
> >  #define X86_MACHINE_BUS_LOCK_RATELIMIT  "bus-lock-ratelimit"
> > +#define X86_MACHINE_LINUXBOOT_SEED      "linuxboot-seed"
> 
> I am guessing we should prefix this with "x-" so we don't commit
> to a user interface.

Actually there's no good reason to have any user interface at all. See
v7 which Paolo "LGTM"ed:
https://lore.kernel.org/qemu-devel/20220721125636.446842-1-Jason@zx2c4.com/

So just ignore this apocryphal patch here that Paolo sent.

Jason

Re: [PATCH 0/4] Refactor x86_load_linux and pass RNG seed via setup_data entry

[Michael S. Tsirkin]

On Thu, Jul 21, 2022 at 05:11:31PM +0200, Jason A. Donenfeld wrote:
> Hi Michael,
> 
> On Thu, Jul 21, 2022 at 10:52:32AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jul 21, 2022 at 02:29:33PM +0200, Paolo Bonzini wrote:
> > > As mentioned in the reviews of Jason's patches, the fw_cfg data, or at
> > > least its structure including the size, is part of the guest ABI and
> > > must match across two sides of migration.
> > > 
> > > It would be possible to handle this with some duplicated code between
> > > the rng seed and DTB handling, but the conditionals to handle the linked
> > > list would be ugly.  Unfortunately the code of x86_load_linux has no
> > > data structures available, it's all of a jumble of local variables.
> > > Hence the first two and largest patches in this series, which remove all
> > > non-Linux code from the function and move the local variables to a struct
> > > as necessary.  The function was long overdue for some cleanup anyway.
> > > 
> > > With this in place, adding the seed setup_data entry is just a
> > > couple lines of code, plus the scaffolding for a new machine property
> > > "linuxboot-seed".  The property supports on/off/auto values, where "auto"
> > > disables/enables depending on the kernel support for setup data (which was
> > > added in 2.6.26); "on" currently fails when starting with an old kernel,
> > > and probably it should also fail when starting a PVH or multiboot kernel.
> > > 
> > > Paolo
> > 
> > I like the refactoring
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > To avoid creating extra work for Jason and confusing
> > attribution, maybe apply Jason's patch then your refactoring
> > on top?
> 
> Yes, I think it'd make sense to apply:
> https://lore.kernel.org/qemu-devel/20220721125636.446842-1-Jason@zx2c4.com/
> as-is, without any changes, since that handles your migration concerns.
>
> And then after, if you want to refactor things in general, apply that on
> top.
> 
> As I mentioned before, we really don't need nor want a user-facing
> option for this.

Yes I think we don't want to support such an option.
We have a general rule of prefixing properties with "x-" for this
these are considered unstable and we are often adding them for
internal purposes.

> What I do in that v7 there is sufficient and fine.
> 
> Michael - do you want to take that v7 into your tree?
> 
> Jason

Can be my tree or Paolo's but I'll wait for him to respond, I like consensus.

-- 
MST