[PATCH RFC 0/7] revert RNG seed mess

[PATCH RFC 0/7] revert RNG seed mess

[Michael S. Tsirkin]

All attempts to fix up passing RNG seed via setup_data entry failed.
Let's just rip out all of it.  We'll start over.


Warning: all I did was git revert the relevant patches and resolve the
(trivial) conflicts. Not even compiled - it's almost midnight here.

Jason this is the kind of approach I'd like to see, not yet another
pointer math rich patch I need to spend time reviewing. Just get us back
to where we started. We can redo "x86: use typedef for SetupData struct"
later if we want, it's benign.

Could you do something like this pls?
Or test and ack if this patchset happens to work by luck.

Michael S. Tsirkin (7):
  Revert "x86: don't let decompressed kernel image clobber setup_data"
  Revert "x86: do not re-randomize RNG seed on snapshot load"
  Revert "x86: re-initialize RNG seed when selecting kernel"
  Revert "x86: reinitialize RNG seed on system reboot"
  Revert "x86: use typedef for SetupData struct"
  Revert "x86: return modified setup_data only if read as memory, not as
    file"
  Revert "hw/i386: pass RNG seed via setup_data entry"

 include/hw/i386/microvm.h |   5 +-
 include/hw/i386/pc.h      |   3 -
 include/hw/i386/x86.h     |   3 +-
 include/hw/nvram/fw_cfg.h |  31 ----------
 hw/i386/microvm.c         |  17 ++----
 hw/i386/pc.c              |   4 +-
 hw/i386/pc_piix.c         |   2 -
 hw/i386/pc_q35.c          |   2 -
 hw/i386/x86.c             | 122 ++++++++++----------------------------
 hw/nvram/fw_cfg.c         |  21 ++-----
 10 files changed, 49 insertions(+), 161 deletions(-)

-- 
MST

[PATCH RFC 1/7] Revert "x86: don't let decompressed kernel image clobber setup_data"

[Michael S. Tsirkin]

This reverts commit eac7a7791bb6d719233deed750034042318ffd56.

Fixes: eac7a7791b ("x86: don't let decompressed kernel image clobber setup_data")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/i386/microvm.h |  5 ++--
 include/hw/nvram/fw_cfg.h |  9 -------
 hw/i386/microvm.c         | 15 ++++-------
 hw/i386/x86.c             | 52 ++++++++++++++++++---------------------
 hw/nvram/fw_cfg.c         |  9 -------
 5 files changed, 31 insertions(+), 59 deletions(-)

diff --git a/include/hw/i386/microvm.h b/include/hw/i386/microvm.h
index e8af61f194..fad97a891d 100644
--- a/include/hw/i386/microvm.h
+++ b/include/hw/i386/microvm.h
@@ -50,9 +50,8 @@
  */
 
 /* Platform virtio definitions */
-#define VIRTIO_MMIO_BASE                0xfeb00000
-#define VIRTIO_CMDLINE_MAXLEN           64
-#define VIRTIO_CMDLINE_TOTAL_MAX_LEN    ((VIRTIO_CMDLINE_MAXLEN + 1) * 16)
+#define VIRTIO_MMIO_BASE      0xfeb00000
+#define VIRTIO_CMDLINE_MAXLEN 64
 
 #define GED_MMIO_BASE         0xfea00000
 #define GED_MMIO_BASE_MEMHP   (GED_MMIO_BASE + 0x100)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 990dcdbb2e..2e503904dc 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -139,15 +139,6 @@ void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
                                void *data, size_t len,
                                bool read_only);
 
-/**
- * fw_cfg_read_bytes_ptr:
- * @s: fw_cfg device being modified
- * @key: selector key value for new fw_cfg item
- *
- * Reads an existing fw_cfg data pointer.
- */
-void *fw_cfg_read_bytes_ptr(FWCfgState *s, uint16_t key);
-
 /**
  * fw_cfg_add_string:
  * @s: fw_cfg device being modified
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 29f30dd6d3..170a331e3f 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -378,8 +378,7 @@ static void microvm_fix_kernel_cmdline(MachineState *machine)
     MicrovmMachineState *mms = MICROVM_MACHINE(machine);
     BusState *bus;
     BusChild *kid;
-    char *cmdline, *existing_cmdline;
-    size_t len;
+    char *cmdline;
 
     /*
      * Find MMIO transports with attached devices, and add them to the kernel
@@ -388,8 +387,7 @@ static void microvm_fix_kernel_cmdline(MachineState *machine)
      * Yes, this is a hack, but one that heavily improves the UX without
      * introducing any significant issues.
      */
-    existing_cmdline = fw_cfg_read_bytes_ptr(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA);
-    cmdline = g_strdup(existing_cmdline);
+    cmdline = g_strdup(machine->kernel_cmdline);
     bus = sysbus_get_default();
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
         DeviceState *dev = kid->child;
@@ -413,12 +411,9 @@ static void microvm_fix_kernel_cmdline(MachineState *machine)
         }
     }
 
-    len = strlen(cmdline);
-    if (len > VIRTIO_CMDLINE_TOTAL_MAX_LEN + strlen(existing_cmdline)) {
-        fprintf(stderr, "qemu: virtio mmio cmdline too large, skipping\n");
-    } else {
-        memcpy(existing_cmdline, cmdline, len + 1);
-    }
+    fw_cfg_modify_i32(x86ms->fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(cmdline) + 1);
+    fw_cfg_modify_string(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA, cmdline);
+
     g_free(cmdline);
 }
 
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index eaff4227bd..78cc131926 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -50,7 +50,6 @@
 #include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "target/i386/sev.h"
-#include "hw/i386/microvm.h"
 
 #include "hw/acpi/cpu_hotplug.h"
 #include "hw/irq.h"
@@ -814,18 +813,12 @@ void x86_load_linux(X86MachineState *x86ms,
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
     const char *dtb_filename = machine->dtb;
-    char *kernel_cmdline;
+    const char *kernel_cmdline = machine->kernel_cmdline;
     SevKernelLoaderContext sev_load_ctx = {};
     enum { RNG_SEED_LENGTH = 32 };
 
-    /*
-     * Add the NUL terminator, some padding for the microvm cmdline fiddling
-     * hack, and then align to 16 bytes as a paranoia measure
-     */
-    cmdline_size = (strlen(machine->kernel_cmdline) + 1 +
-                    VIRTIO_CMDLINE_TOTAL_MAX_LEN + 16) & ~15;
-    /* Make a copy, since we might append arbitrary bytes to it later. */
-    kernel_cmdline = g_strndup(machine->kernel_cmdline, cmdline_size);
+    /* Align to 16 bytes as a paranoia measure */
+    cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
 
     /* load the kernel header */
     f = fopen(kernel_filename, "rb");
@@ -966,6 +959,12 @@ void x86_load_linux(X86MachineState *x86ms,
         initrd_max = x86ms->below_4g_mem_size - acpi_data_size - 1;
     }
 
+    fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
+    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);
+    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);
     } else {
@@ -1092,24 +1091,27 @@ void x86_load_linux(X86MachineState *x86ms,
             exit(1);
         }
 
-        setup_data_offset = cmdline_size;
-        cmdline_size += sizeof(SetupData) + dtb_size;
-        kernel_cmdline = g_realloc(kernel_cmdline, cmdline_size);
-        setup_data = (void *)kernel_cmdline + setup_data_offset;
+        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
+        kernel_size = setup_data_offset + sizeof(SetupData) + dtb_size;
+        kernel = g_realloc(kernel, kernel_size);
+
+
+        setup_data = (SetupData *)(kernel + setup_data_offset);
         setup_data->next = cpu_to_le64(first_setup_data);
-        first_setup_data = cmdline_addr + setup_data_offset;
+        first_setup_data = prot_addr + setup_data_offset;
         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);
     }
 
-    if (!legacy_no_rng_seed && protocol >= 0x209) {
-        setup_data_offset = cmdline_size;
-        cmdline_size += sizeof(SetupData) + RNG_SEED_LENGTH;
-        kernel_cmdline = g_realloc(kernel_cmdline, cmdline_size);
-        setup_data = (void *)kernel_cmdline + setup_data_offset;
+    if (!legacy_no_rng_seed) {
+        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
+        kernel_size = setup_data_offset + sizeof(SetupData) + RNG_SEED_LENGTH;
+        kernel = g_realloc(kernel, kernel_size);
+        setup_data = (SetupData *)(kernel + setup_data_offset);
         setup_data->next = cpu_to_le64(first_setup_data);
-        first_setup_data = cmdline_addr + setup_data_offset;
+        first_setup_data = prot_addr + setup_data_offset;
         setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
         setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
@@ -1120,12 +1122,6 @@ void x86_load_linux(X86MachineState *x86ms,
         fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
     }
 
-    fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
-    fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, cmdline_size);
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline, cmdline_size);
-    sev_load_ctx.cmdline_data = (char *)kernel_cmdline;
-    sev_load_ctx.cmdline_size = cmdline_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);
     sev_load_ctx.kernel_data = (char *)kernel;
@@ -1138,7 +1134,7 @@ void x86_load_linux(X86MachineState *x86ms,
      * kernel on the other side of the fw_cfg interface matches the hash of the
      * file the user passed in.
      */
-    if (!sev_enabled() && first_setup_data) {
+    if (!sev_enabled()) {
         SetupDataFixup *fixup = g_malloc(sizeof(*fixup));
 
         memcpy(setup, header, MIN(sizeof(header), setup_size));
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 432754eda4..a00881bc64 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -741,15 +741,6 @@ void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
     fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
 }
 
-void *fw_cfg_read_bytes_ptr(FWCfgState *s, uint16_t key)
-{
-    int arch = !!(key & FW_CFG_ARCH_LOCAL);
-
-    key &= FW_CFG_ENTRY_MASK;
-    assert(key < fw_cfg_max_entry(s));
-    return s->entries[arch][key].data;
-}
-
 void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
 {
     size_t sz = strlen(value) + 1;
-- 
MST

[PATCH RFC 2/7] Revert "x86: do not re-randomize RNG seed on snapshot load"

[Michael S. Tsirkin]

This reverts commit 14b29fea742034186403914b4d013d0e83f19e78.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Fixes: 14b29fea74 ("x86: do not re-randomize RNG seed on snapshot load")
---
 hw/i386/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 78cc131926..7984f65352 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1115,7 +1115,7 @@ void x86_load_linux(X86MachineState *x86ms,
         setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
         setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
-        qemu_register_reset_nosnapshotload(reset_rng_seed, setup_data);
+        qemu_register_reset(reset_rng_seed, setup_data);
         fw_cfg_add_bytes_callback(fw_cfg, FW_CFG_KERNEL_DATA, reset_rng_seed, NULL,
                                   setup_data, kernel, kernel_size, true);
     } else {
-- 
MST

[PATCH RFC 3/7] Revert "x86: re-initialize RNG seed when selecting kernel"

[Michael S. Tsirkin]

This reverts commit cc63374a5a7c240b7d3be734ef589dabbefc7527.

Fixes: cc63374a5a ("x86: re-initialize RNG seed when selecting kernel")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/x86.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 7984f65352..e1a5f244a9 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1116,14 +1116,11 @@ void x86_load_linux(X86MachineState *x86ms,
         setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
         qemu_register_reset(reset_rng_seed, setup_data);
-        fw_cfg_add_bytes_callback(fw_cfg, FW_CFG_KERNEL_DATA, reset_rng_seed, NULL,
-                                  setup_data, kernel, kernel_size, true);
-    } else {
-        fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_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;
 
-- 
MST

[PATCH RFC 4/7] Revert "x86: reinitialize RNG seed on system reboot"

[Michael S. Tsirkin]

This reverts commit 763a2828bf313ed55878b09759dc435355035f2e.

Fixes: 763a2828bf ("x86: reinitialize RNG seed on system reboot")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/x86.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index e1a5f244a9..32f37ab7c2 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -787,12 +787,6 @@ static void reset_setup_data(void *opaque)
     stq_p(fixup->pos, fixup->orig_val);
 }
 
-static void reset_rng_seed(void *opaque)
-{
-    SetupData *setup_data = opaque;
-    qemu_guest_getrandom_nofail(setup_data->data, le32_to_cpu(setup_data->len));
-}
-
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
@@ -1115,7 +1109,6 @@ void x86_load_linux(X86MachineState *x86ms,
         setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
         setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
-        qemu_register_reset(reset_rng_seed, setup_data);
     }
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
-- 
MST

[PATCH RFC 5/7] Revert "x86: use typedef for SetupData struct"

[Michael S. Tsirkin]

This reverts commit eebb38a5633a77f5fa79d6486d5b2fcf8fbe3c07.

Fixes: eebb38a563 ("x86: use typedef for SetupData struct")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/x86.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 32f37ab7c2..76b12108b4 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -657,12 +657,12 @@ DeviceState *ioapic_init_secondary(GSIState *gsi_state)
     return dev;
 }
 
-typedef struct SetupData {
+struct setup_data {
     uint64_t next;
     uint32_t type;
     uint32_t len;
     uint8_t data[];
-} __attribute__((packed)) SetupData;
+} __attribute__((packed));
 
 
 /*
@@ -803,7 +803,7 @@ void x86_load_linux(X86MachineState *x86ms,
     FILE *f;
     char *vmode;
     MachineState *machine = MACHINE(x86ms);
-    SetupData *setup_data;
+    struct setup_data *setup_data;
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
     const char *dtb_filename = machine->dtb;
@@ -1086,11 +1086,11 @@ void x86_load_linux(X86MachineState *x86ms,
         }
 
         setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
-        kernel_size = setup_data_offset + sizeof(SetupData) + dtb_size;
+        kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
         kernel = g_realloc(kernel, kernel_size);
 
 
-        setup_data = (SetupData *)(kernel + setup_data_offset);
+        setup_data = (struct setup_data *)(kernel + setup_data_offset);
         setup_data->next = cpu_to_le64(first_setup_data);
         first_setup_data = prot_addr + setup_data_offset;
         setup_data->type = cpu_to_le32(SETUP_DTB);
@@ -1101,9 +1101,9 @@ void x86_load_linux(X86MachineState *x86ms,
 
     if (!legacy_no_rng_seed) {
         setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
-        kernel_size = setup_data_offset + sizeof(SetupData) + RNG_SEED_LENGTH;
+        kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
         kernel = g_realloc(kernel, kernel_size);
-        setup_data = (SetupData *)(kernel + setup_data_offset);
+        setup_data = (struct setup_data *)(kernel + setup_data_offset);
         setup_data->next = cpu_to_le64(first_setup_data);
         first_setup_data = prot_addr + setup_data_offset;
         setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
-- 
MST

[PATCH RFC 6/7] Revert "x86: return modified setup_data only if read as memory, not as file"

[Michael S. Tsirkin]

This reverts commit e935b735085dfa61d8e6d276b6f9e7687796a3c7.

Fixes: e935b73508 ("x86: return modified setup_data only if read as memory, not as file")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/nvram/fw_cfg.h | 22 -------------------
 hw/i386/x86.c             | 46 +++++++++------------------------------
 hw/nvram/fw_cfg.c         | 12 +++++-----
 3 files changed, 16 insertions(+), 64 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 2e503904dc..c1f81a5f13 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -117,28 +117,6 @@ struct FWCfgMemState {
  */
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
 
-/**
- * fw_cfg_add_bytes_callback:
- * @s: fw_cfg device being modified
- * @key: selector key value for new fw_cfg item
- * @select_cb: callback function when selecting
- * @write_cb: callback function after a write
- * @callback_opaque: argument to be passed into callback function
- * @data: pointer to start of item data
- * @len: size of item data
- * @read_only: is file read only
- *
- * Add a new fw_cfg item, available by selecting the given key, as a raw
- * "blob" of the given size. The data referenced by the starting pointer
- * is only linked, NOT copied, into the data structure of the fw_cfg device.
- */
-void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
-                               FWCfgCallback select_cb,
-                               FWCfgWriteCallback write_cb,
-                               void *callback_opaque,
-                               void *data, size_t len,
-                               bool read_only);
-
 /**
  * fw_cfg_add_string:
  * @s: fw_cfg device being modified
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 76b12108b4..4831193c86 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -37,7 +37,6 @@
 #include "sysemu/whpx.h"
 #include "sysemu/numa.h"
 #include "sysemu/replay.h"
-#include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpu-timers.h"
 #include "sysemu/xen.h"
@@ -769,24 +768,6 @@ static bool load_elfboot(const char *kernel_filename,
     return true;
 }
 
-typedef struct SetupDataFixup {
-    void *pos;
-    hwaddr orig_val, new_val;
-    uint32_t addr;
-} SetupDataFixup;
-
-static void fixup_setup_data(void *opaque)
-{
-    SetupDataFixup *fixup = opaque;
-    stq_p(fixup->pos, fixup->new_val);
-}
-
-static void reset_setup_data(void *opaque)
-{
-    SetupDataFixup *fixup = opaque;
-    stq_p(fixup->pos, fixup->orig_val);
-}
-
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
@@ -1111,11 +1092,8 @@ void x86_load_linux(X86MachineState *x86ms,
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
     }
 
-    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;
+    /* Offset 0x250 is a pointer to the first setup_data link. */
+    stq_p(header + 0x250, first_setup_data);
 
     /*
      * If we're starting an encrypted VM, it will be OVMF based, which uses the
@@ -1125,20 +1103,16 @@ void x86_load_linux(X86MachineState *x86ms,
      * file the user passed in.
      */
     if (!sev_enabled()) {
-        SetupDataFixup *fixup = g_malloc(sizeof(*fixup));
-
         memcpy(setup, header, MIN(sizeof(header), setup_size));
-        /* Offset 0x250 is a pointer to the first setup_data link. */
-        fixup->pos = setup + 0x250;
-        fixup->orig_val = ldq_p(fixup->pos);
-        fixup->new_val = first_setup_data;
-        fixup->addr = cpu_to_le32(real_addr);
-        fw_cfg_add_bytes_callback(fw_cfg, FW_CFG_SETUP_ADDR, fixup_setup_data, NULL,
-                                  fixup, &fixup->addr, sizeof(fixup->addr), true);
-        qemu_register_reset(reset_setup_data, fixup);
-    } else {
-        fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
     }
+
+    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_SETUP_ADDR, real_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
     sev_load_ctx.setup_data = (char *)setup;
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index a00881bc64..29a5bef1d5 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -693,12 +693,12 @@ static const VMStateDescription vmstate_fw_cfg = {
     }
 };
 
-void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
-                               FWCfgCallback select_cb,
-                               FWCfgWriteCallback write_cb,
-                               void *callback_opaque,
-                               void *data, size_t len,
-                               bool read_only)
+static void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
+                                      FWCfgCallback select_cb,
+                                      FWCfgWriteCallback write_cb,
+                                      void *callback_opaque,
+                                      void *data, size_t len,
+                                      bool read_only)
 {
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
-- 
MST

[PATCH RFC 7/7] Revert "hw/i386: pass RNG seed via setup_data entry"

[Michael S. Tsirkin]

This reverts commit 67f7e426e53833a5db75b0d813e8d537b8a75bd2.

Fixes: 67f7e426e5 ("hw/i386: pass RNG seed via setup_data entry")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/i386/pc.h  |  3 ---
 include/hw/i386/x86.h |  3 +--
 hw/i386/microvm.c     |  2 +-
 hw/i386/pc.c          |  4 ++--
 hw/i386/pc_piix.c     |  2 --
 hw/i386/pc_q35.c      |  2 --
 hw/i386/x86.c         | 26 ++++----------------------
 7 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 66e3d059ef..44b08554fa 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -127,9 +127,6 @@ struct PCMachineClass {
 
     /* create kvmclock device even when KVM PV features are not exposed */
     bool kvmclock_create_always;
-
-    /* skip passing an rng seed for legacy machines */
-    bool legacy_no_rng_seed;
 };
 
 #define TYPE_PC_MACHINE "generic-pc-machine"
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 62fa5774f8..df82c5fd42 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -126,8 +126,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
-                    bool pvh_enabled,
-                    bool legacy_no_rng_seed);
+                    bool pvh_enabled);
 
 bool x86_machine_is_smm_enabled(const X86MachineState *x86ms);
 bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms);
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 170a331e3f..b231ceda9a 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -330,7 +330,7 @@ static void microvm_memory_init(MicrovmMachineState *mms)
     rom_set_fw(fw_cfg);
 
     if (machine->kernel_filename != NULL) {
-        x86_load_linux(x86ms, fw_cfg, 0, true, false);
+        x86_load_linux(x86ms, fw_cfg, 0, true);
     }
 
     if (mms->option_roms) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6e592bd969..2c5f675ba4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -804,7 +804,7 @@ void xen_load_linux(PCMachineState *pcms)
     rom_set_fw(fw_cfg);
 
     x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
-                   pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
+                   pcmc->pvh_enabled);
     for (i = 0; i < nb_option_roms; i++) {
         assert(!strcmp(option_rom[i].name, "linuxboot.bin") ||
                !strcmp(option_rom[i].name, "linuxboot_dma.bin") ||
@@ -1124,7 +1124,7 @@ void pc_memory_init(PCMachineState *pcms,
 
     if (linux_boot) {
         x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
-                       pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
+                       pcmc->pvh_enabled);
     }
 
     for (i = 0; i < nb_option_roms; i++) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index df64dd8dcc..839bd65df5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -476,9 +476,7 @@ DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,
 
 static void pc_i440fx_7_1_machine_options(MachineClass *m)
 {
-    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_7_2_machine_options(m);
-    pcmc->legacy_no_rng_seed = true;
     compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
     compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 66cd718b70..e6e3966262 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -395,9 +395,7 @@ DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,
 
 static void pc_q35_7_1_machine_options(MachineClass *m)
 {
-    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_7_2_machine_options(m);
-    pcmc->legacy_no_rng_seed = true;
     compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
     compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
 }
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 4831193c86..80be3032cc 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -26,7 +26,6 @@
 #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"
@@ -771,8 +770,7 @@ static bool load_elfboot(const char *kernel_filename,
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
-                    bool pvh_enabled,
-                    bool legacy_no_rng_seed)
+                    bool pvh_enabled)
 {
     bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
     uint16_t protocol;
@@ -780,7 +778,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, first_setup_data = 0;
+    hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
     FILE *f;
     char *vmode;
     MachineState *machine = MACHINE(x86ms);
@@ -790,7 +788,6 @@ void x86_load_linux(X86MachineState *x86ms,
     const char *dtb_filename = machine->dtb;
     const char *kernel_cmdline = machine->kernel_cmdline;
     SevKernelLoaderContext sev_load_ctx = {};
-    enum { RNG_SEED_LENGTH = 32 };
 
     /* Align to 16 bytes as a paranoia measure */
     cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -1070,31 +1067,16 @@ void x86_load_linux(X86MachineState *x86ms,
         kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
         kernel = g_realloc(kernel, kernel_size);
 
+        stq_p(header + 0x250, prot_addr + setup_data_offset);
 
         setup_data = (struct setup_data *)(kernel + setup_data_offset);
-        setup_data->next = cpu_to_le64(first_setup_data);
-        first_setup_data = prot_addr + 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);
     }
 
-    if (!legacy_no_rng_seed) {
-        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
-        kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
-        kernel = g_realloc(kernel, kernel_size);
-        setup_data = (struct setup_data *)(kernel + setup_data_offset);
-        setup_data->next = cpu_to_le64(first_setup_data);
-        first_setup_data = prot_addr + setup_data_offset;
-        setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
-        setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
-        qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
-    }
-
-    /* Offset 0x250 is a pointer to the first setup_data link. */
-    stq_p(header + 0x250, first_setup_data);
-
     /*
      * If we're starting an encrypted VM, it will be OVMF based, which uses the
      * efi stub for booting and doesn't require any values to be placed in the
-- 
MST

Re: [PATCH RFC 0/7] revert RNG seed mess

[Dov Murik]

Hi Michael,

On 08/02/2023 23:12, Michael S. Tsirkin wrote:
> All attempts to fix up passing RNG seed via setup_data entry failed.
> Let's just rip out all of it.  We'll start over.
> 
> 
> Warning: all I did was git revert the relevant patches and resolve the
> (trivial) conflicts. Not even compiled - it's almost midnight here.
> 
> Jason this is the kind of approach I'd like to see, not yet another
> pointer math rich patch I need to spend time reviewing. Just get us back
> to where we started. We can redo "x86: use typedef for SetupData struct"
> later if we want, it's benign.
> 
> Could you do something like this pls?
> Or test and ack if this patchset happens to work by luck.
> 
> Michael S. Tsirkin (7):
>   Revert "x86: don't let decompressed kernel image clobber setup_data"
>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>   Revert "x86: re-initialize RNG seed when selecting kernel"
>   Revert "x86: reinitialize RNG seed on system reboot"
>   Revert "x86: use typedef for SetupData struct"
>   Revert "x86: return modified setup_data only if read as memory, not as
>     file"
>   Revert "hw/i386: pass RNG seed via setup_data entry"
> 
>  include/hw/i386/microvm.h |   5 +-
>  include/hw/i386/pc.h      |   3 -
>  include/hw/i386/x86.h     |   3 +-
>  include/hw/nvram/fw_cfg.h |  31 ----------
>  hw/i386/microvm.c         |  17 ++----
>  hw/i386/pc.c              |   4 +-
>  hw/i386/pc_piix.c         |   2 -
>  hw/i386/pc_q35.c          |   2 -
>  hw/i386/x86.c             | 122 ++++++++++----------------------------
>  hw/nvram/fw_cfg.c         |  21 ++-----
>  10 files changed, 49 insertions(+), 161 deletions(-)
> 


I tested this series with SEV measured boot using AmdSev OVMF.  The boot
succeeds, and the hashes computed for kernel/initrd/cmdline are
identical to the ones computed by qemu 7.1.0, which are the hashes that
the guest owner would expect.

As for non-EFI (non-SEV) guests, I did a very simple test of starting a
non-EFI guest and it looks OK, but more testing is needed.

So:

Tested-by: Dov Murik <dovmurik@linux.ibm.com>


Thank you!

-Dov

Re: [PATCH RFC 0/7] revert RNG seed mess

[Dov Murik]

On 09/02/2023 8:03, Dov Murik wrote:
> Hi Michael,
> 
> On 08/02/2023 23:12, Michael S. Tsirkin wrote:
>> All attempts to fix up passing RNG seed via setup_data entry failed.
>> Let's just rip out all of it.  We'll start over.
>>
>>
>> Warning: all I did was git revert the relevant patches and resolve the
>> (trivial) conflicts. Not even compiled - it's almost midnight here.
>>
>> Jason this is the kind of approach I'd like to see, not yet another
>> pointer math rich patch I need to spend time reviewing. Just get us back
>> to where we started. We can redo "x86: use typedef for SetupData struct"
>> later if we want, it's benign.
>>
>> Could you do something like this pls?
>> Or test and ack if this patchset happens to work by luck.
>>
>> Michael S. Tsirkin (7):
>>   Revert "x86: don't let decompressed kernel image clobber setup_data"
>>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>>   Revert "x86: re-initialize RNG seed when selecting kernel"
>>   Revert "x86: reinitialize RNG seed on system reboot"
>>   Revert "x86: use typedef for SetupData struct"
>>   Revert "x86: return modified setup_data only if read as memory, not as
>>     file"
>>   Revert "hw/i386: pass RNG seed via setup_data entry"
>>
>>  include/hw/i386/microvm.h |   5 +-
>>  include/hw/i386/pc.h      |   3 -
>>  include/hw/i386/x86.h     |   3 +-
>>  include/hw/nvram/fw_cfg.h |  31 ----------
>>  hw/i386/microvm.c         |  17 ++----
>>  hw/i386/pc.c              |   4 +-
>>  hw/i386/pc_piix.c         |   2 -
>>  hw/i386/pc_q35.c          |   2 -
>>  hw/i386/x86.c             | 122 ++++++++++----------------------------
>>  hw/nvram/fw_cfg.c         |  21 ++-----
>>  10 files changed, 49 insertions(+), 161 deletions(-)
>>
> 
> 
> I tested this series with SEV measured boot using AmdSev OVMF.  The boot
> succeeds, and the hashes computed for kernel/initrd/cmdline are
> identical to the ones computed by qemu 7.1.0, which are the hashes that
> the guest owner would expect.
> 

I also tested that backporting this series to 7.2.0 (skipping
PATCH 1/7 because it was added after the 7.2.0 release) works OK for
booting SEV guest with AmdSev measured boot (and retains the same
kernel/initrd/cmdline hashes as qemu 7.1.0).

-Dov


> As for non-EFI (non-SEV) guests, I did a very simple test of starting a
> non-EFI guest and it looks OK, but more testing is needed.
> 
> So:
> 
> Tested-by: Dov Murik <dovmurik@linux.ibm.com>
> 
> 
> Thank you!
> 
> -Dov

Re: [PATCH RFC 5/7] Revert "x86: use typedef for SetupData struct"

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 04:12:51PM -0500, Michael S. Tsirkin wrote:
> This reverts commit eebb38a5633a77f5fa79d6486d5b2fcf8fbe3c07.
> 
> Fixes: eebb38a563 ("x86: use typedef for SetupData struct")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


This one was actually good, I reverted so other reverts are clean.
Jason I would appreciate it if you can rebase this on top
of the revert.


> ---
>  hw/i386/x86.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 32f37ab7c2..76b12108b4 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -657,12 +657,12 @@ DeviceState *ioapic_init_secondary(GSIState *gsi_state)
>      return dev;
>  }
>  
> -typedef struct SetupData {
> +struct setup_data {
>      uint64_t next;
>      uint32_t type;
>      uint32_t len;
>      uint8_t data[];
> -} __attribute__((packed)) SetupData;
> +} __attribute__((packed));
>  
>  
>  /*
> @@ -803,7 +803,7 @@ void x86_load_linux(X86MachineState *x86ms,
>      FILE *f;
>      char *vmode;
>      MachineState *machine = MACHINE(x86ms);
> -    SetupData *setup_data;
> +    struct setup_data *setup_data;
>      const char *kernel_filename = machine->kernel_filename;
>      const char *initrd_filename = machine->initrd_filename;
>      const char *dtb_filename = machine->dtb;
> @@ -1086,11 +1086,11 @@ void x86_load_linux(X86MachineState *x86ms,
>          }
>  
>          setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
> -        kernel_size = setup_data_offset + sizeof(SetupData) + dtb_size;
> +        kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
>          kernel = g_realloc(kernel, kernel_size);
>  
>  
> -        setup_data = (SetupData *)(kernel + setup_data_offset);
> +        setup_data = (struct setup_data *)(kernel + setup_data_offset);
>          setup_data->next = cpu_to_le64(first_setup_data);
>          first_setup_data = prot_addr + setup_data_offset;
>          setup_data->type = cpu_to_le32(SETUP_DTB);
> @@ -1101,9 +1101,9 @@ void x86_load_linux(X86MachineState *x86ms,
>  
>      if (!legacy_no_rng_seed) {
>          setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
> -        kernel_size = setup_data_offset + sizeof(SetupData) + RNG_SEED_LENGTH;
> +        kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
>          kernel = g_realloc(kernel, kernel_size);
> -        setup_data = (SetupData *)(kernel + setup_data_offset);
> +        setup_data = (struct setup_data *)(kernel + setup_data_offset);
>          setup_data->next = cpu_to_le64(first_setup_data);
>          first_setup_data = prot_addr + setup_data_offset;
>          setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
> -- 
> MST
> 

Re: [PATCH RFC 0/7] revert RNG seed mess

[Nathan Chancellor]

On Wed, Feb 08, 2023 at 04:12:23PM -0500, Michael S. Tsirkin wrote:
> All attempts to fix up passing RNG seed via setup_data entry failed.
> Let's just rip out all of it.  We'll start over.
> 
> 
> Warning: all I did was git revert the relevant patches and resolve the
> (trivial) conflicts. Not even compiled - it's almost midnight here.
> 
> Jason this is the kind of approach I'd like to see, not yet another
> pointer math rich patch I need to spend time reviewing. Just get us back
> to where we started. We can redo "x86: use typedef for SetupData struct"
> later if we want, it's benign.
> 
> Could you do something like this pls?
> Or test and ack if this patchset happens to work by luck.
> 
> Michael S. Tsirkin (7):
>   Revert "x86: don't let decompressed kernel image clobber setup_data"
>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>   Revert "x86: re-initialize RNG seed when selecting kernel"
>   Revert "x86: reinitialize RNG seed on system reboot"
>   Revert "x86: use typedef for SetupData struct"
>   Revert "x86: return modified setup_data only if read as memory, not as
>     file"
>   Revert "hw/i386: pass RNG seed via setup_data entry"
> 
>  include/hw/i386/microvm.h |   5 +-
>  include/hw/i386/pc.h      |   3 -
>  include/hw/i386/x86.h     |   3 +-
>  include/hw/nvram/fw_cfg.h |  31 ----------
>  hw/i386/microvm.c         |  17 ++----
>  hw/i386/pc.c              |   4 +-
>  hw/i386/pc_piix.c         |   2 -
>  hw/i386/pc_q35.c          |   2 -
>  hw/i386/x86.c             | 122 ++++++++++----------------------------
>  hw/nvram/fw_cfg.c         |  21 ++-----
>  10 files changed, 49 insertions(+), 161 deletions(-)
> 
> -- 
> MST
> 

For the record, all three of the cases that I tested (i386 no EFI,
x86_64 with and without EFI) worked fine with this series. In case it is
useful:

Tested-by: Nathan Chancellor <nathan@kernel.org>

Cheers,
Nathan

Re: [PATCH RFC 6/7] Revert "x86: return modified setup_data only if read as memory, not as file"

[Jason A. Donenfeld]

On Wed, Feb 08, 2023 at 04:12:51PM -0500, Michael S. Tsirkin wrote:
> This reverts commit e935b735085dfa61d8e6d276b6f9e7687796a3c7.
> 
> Fixes: e935b73508 ("x86: return modified setup_data only if read as memory, not as file")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/nvram/fw_cfg.h | 22 -------------------
>  hw/i386/x86.c             | 46 +++++++++------------------------------
>  hw/nvram/fw_cfg.c         | 12 +++++-----
>  3 files changed, 16 insertions(+), 64 deletions(-)
> 
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 2e503904dc..c1f81a5f13 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -117,28 +117,6 @@ struct FWCfgMemState {
>   */
>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
>  
> -/**
> - * fw_cfg_add_bytes_callback:
> - * @s: fw_cfg device being modified
> - * @key: selector key value for new fw_cfg item
> - * @select_cb: callback function when selecting
> - * @write_cb: callback function after a write
> - * @callback_opaque: argument to be passed into callback function
> - * @data: pointer to start of item data
> - * @len: size of item data
> - * @read_only: is file read only
> - *
> - * Add a new fw_cfg item, available by selecting the given key, as a raw
> - * "blob" of the given size. The data referenced by the starting pointer
> - * is only linked, NOT copied, into the data structure of the fw_cfg device.
> - */
> -void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> -                               FWCfgCallback select_cb,
> -                               FWCfgWriteCallback write_cb,
> -                               void *callback_opaque,
> -                               void *data, size_t len,
> -                               bool read_only);
> -
>  /**
>   * fw_cfg_add_string:
>   * @s: fw_cfg device being modified
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index a00881bc64..29a5bef1d5 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -693,12 +693,12 @@ static const VMStateDescription vmstate_fw_cfg = {
>      }
>  };
>  
> -void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> -                               FWCfgCallback select_cb,
> -                               FWCfgWriteCallback write_cb,
> -                               void *callback_opaque,
> -                               void *data, size_t len,
> -                               bool read_only)
> +static void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> +                                      FWCfgCallback select_cb,
> +                                      FWCfgWriteCallback write_cb,
> +                                      void *callback_opaque,
> +                                      void *data, size_t len,
> +                                      bool read_only)
>  {
>      int arch = !!(key & FW_CFG_ARCH_LOCAL);

Could you leave these snippets in? This function is useful and will be
needed in the reprise.

Jason

Re: [PATCH RFC 6/7] Revert "x86: return modified setup_data only if read as memory, not as file"

[Daniel P. Berrangé]

On Thu, Feb 09, 2023 at 04:52:32PM +0100, Jason A. Donenfeld wrote:
> On Wed, Feb 08, 2023 at 04:12:51PM -0500, Michael S. Tsirkin wrote:
> > This reverts commit e935b735085dfa61d8e6d276b6f9e7687796a3c7.
> > 
> > Fixes: e935b73508 ("x86: return modified setup_data only if read as memory, not as file")
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/nvram/fw_cfg.h | 22 -------------------
> >  hw/i386/x86.c             | 46 +++++++++------------------------------
> >  hw/nvram/fw_cfg.c         | 12 +++++-----
> >  3 files changed, 16 insertions(+), 64 deletions(-)
> > 
> > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> > index 2e503904dc..c1f81a5f13 100644
> > --- a/include/hw/nvram/fw_cfg.h
> > +++ b/include/hw/nvram/fw_cfg.h
> > @@ -117,28 +117,6 @@ struct FWCfgMemState {
> >   */
> >  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
> >  
> > -/**
> > - * fw_cfg_add_bytes_callback:
> > - * @s: fw_cfg device being modified
> > - * @key: selector key value for new fw_cfg item
> > - * @select_cb: callback function when selecting
> > - * @write_cb: callback function after a write
> > - * @callback_opaque: argument to be passed into callback function
> > - * @data: pointer to start of item data
> > - * @len: size of item data
> > - * @read_only: is file read only
> > - *
> > - * Add a new fw_cfg item, available by selecting the given key, as a raw
> > - * "blob" of the given size. The data referenced by the starting pointer
> > - * is only linked, NOT copied, into the data structure of the fw_cfg device.
> > - */
> > -void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> > -                               FWCfgCallback select_cb,
> > -                               FWCfgWriteCallback write_cb,
> > -                               void *callback_opaque,
> > -                               void *data, size_t len,
> > -                               bool read_only);
> > -
> >  /**
> >   * fw_cfg_add_string:
> >   * @s: fw_cfg device being modified
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index a00881bc64..29a5bef1d5 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -693,12 +693,12 @@ static const VMStateDescription vmstate_fw_cfg = {
> >      }
> >  };
> >  
> > -void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> > -                               FWCfgCallback select_cb,
> > -                               FWCfgWriteCallback write_cb,
> > -                               void *callback_opaque,
> > -                               void *data, size_t len,
> > -                               bool read_only)
> > +static void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> > +                                      FWCfgCallback select_cb,
> > +                                      FWCfgWriteCallback write_cb,
> > +                                      void *callback_opaque,
> > +                                      void *data, size_t len,
> > +                                      bool read_only)
> >  {
> >      int arch = !!(key & FW_CFG_ARCH_LOCAL);
> 
> Could you leave these snippets in? This function is useful and will be
> needed in the reprise.

IMHO it is better to do a full clean revert of the patches.

Switching this one function from static to public is trivial
enough that it is not burden to do in a new impl of the RNG
seed work.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 0/7] revert RNG seed mess

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 04:12:23PM -0500, Michael S. Tsirkin wrote:
> All attempts to fix up passing RNG seed via setup_data entry failed.
> Let's just rip out all of it.  We'll start over.
> 
> 
> Warning: all I did was git revert the relevant patches and resolve the
> (trivial) conflicts. Not even compiled - it's almost midnight here.
> 
> Jason this is the kind of approach I'd like to see, not yet another
> pointer math rich patch I need to spend time reviewing. Just get us back
> to where we started. We can redo "x86: use typedef for SetupData struct"
> later if we want, it's benign.

This approach looks suitable for applying to the 7.2 tree too,
which will be good for fixing the regressions in stable.

> 
> Could you do something like this pls?
> Or test and ack if this patchset happens to work by luck.
> 
> Michael S. Tsirkin (7):
>   Revert "x86: don't let decompressed kernel image clobber setup_data"
>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>   Revert "x86: re-initialize RNG seed when selecting kernel"
>   Revert "x86: reinitialize RNG seed on system reboot"
>   Revert "x86: use typedef for SetupData struct"
>   Revert "x86: return modified setup_data only if read as memory, not as
>     file"
>   Revert "hw/i386: pass RNG seed via setup_data entry"
> 
>  include/hw/i386/microvm.h |   5 +-
>  include/hw/i386/pc.h      |   3 -
>  include/hw/i386/x86.h     |   3 +-
>  include/hw/nvram/fw_cfg.h |  31 ----------
>  hw/i386/microvm.c         |  17 ++----
>  hw/i386/pc.c              |   4 +-
>  hw/i386/pc_piix.c         |   2 -
>  hw/i386/pc_q35.c          |   2 -
>  hw/i386/x86.c             | 122 ++++++++++----------------------------
>  hw/nvram/fw_cfg.c         |  21 ++-----
>  10 files changed, 49 insertions(+), 161 deletions(-)
> 
> -- 
> MST
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 1/7] Revert "x86: don't let decompressed kernel image clobber setup_data"

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 04:12:27PM -0500, Michael S. Tsirkin wrote:
> This reverts commit eac7a7791bb6d719233deed750034042318ffd56.
> 
> Fixes: eac7a7791b ("x86: don't let decompressed kernel image clobber setup_data")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/i386/microvm.h |  5 ++--
>  include/hw/nvram/fw_cfg.h |  9 -------
>  hw/i386/microvm.c         | 15 ++++-------
>  hw/i386/x86.c             | 52 ++++++++++++++++++---------------------
>  hw/nvram/fw_cfg.c         |  9 -------
>  5 files changed, 31 insertions(+), 59 deletions(-)


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 2/7] Revert "x86: do not re-randomize RNG seed on snapshot load"

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 04:12:31PM -0500, Michael S. Tsirkin wrote:
> This reverts commit 14b29fea742034186403914b4d013d0e83f19e78.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Fixes: 14b29fea74 ("x86: do not re-randomize RNG seed on snapshot load")
> ---
>  hw/i386/x86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 3/7] Revert "x86: re-initialize RNG seed when selecting kernel"

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 04:12:37PM -0500, Michael S. Tsirkin wrote:
> This reverts commit cc63374a5a7c240b7d3be734ef589dabbefc7527.
> 
> Fixes: cc63374a5a ("x86: re-initialize RNG seed when selecting kernel")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/x86.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 4/7] Revert "x86: reinitialize RNG seed on system reboot"

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 04:12:42PM -0500, Michael S. Tsirkin wrote:
> This reverts commit 763a2828bf313ed55878b09759dc435355035f2e.
> 
> Fixes: 763a2828bf ("x86: reinitialize RNG seed on system reboot")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/x86.c | 7 -------
>  1 file changed, 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 5/7] Revert "x86: use typedef for SetupData struct"

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 04:12:46PM -0500, Michael S. Tsirkin wrote:
> This reverts commit eebb38a5633a77f5fa79d6486d5b2fcf8fbe3c07.
> 
> Fixes: eebb38a563 ("x86: use typedef for SetupData struct")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/x86.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 6/7] Revert "x86: return modified setup_data only if read as memory, not as file"

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 04:12:51PM -0500, Michael S. Tsirkin wrote:
> This reverts commit e935b735085dfa61d8e6d276b6f9e7687796a3c7.
> 
> Fixes: e935b73508 ("x86: return modified setup_data only if read as memory, not as file")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/nvram/fw_cfg.h | 22 -------------------
>  hw/i386/x86.c             | 46 +++++++++------------------------------
>  hw/nvram/fw_cfg.c         | 12 +++++-----
>  3 files changed, 16 insertions(+), 64 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 7/7] Revert "hw/i386: pass RNG seed via setup_data entry"

[Daniel P. Berrangé]

On Wed, Feb 08, 2023 at 04:12:56PM -0500, Michael S. Tsirkin wrote:
> This reverts commit 67f7e426e53833a5db75b0d813e8d537b8a75bd2.
> 
> Fixes: 67f7e426e5 ("hw/i386: pass RNG seed via setup_data entry")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/i386/pc.h  |  3 ---
>  include/hw/i386/x86.h |  3 +--
>  hw/i386/microvm.c     |  2 +-
>  hw/i386/pc.c          |  4 ++--
>  hw/i386/pc_piix.c     |  2 --
>  hw/i386/pc_q35.c      |  2 --
>  hw/i386/x86.c         | 26 ++++----------------------
>  7 files changed, 8 insertions(+), 34 deletions(-)

All the patches prior to this were clean reverts. This one though
does not look clean, rather it looks like reverting a combination
of 3 commits

commit ffe2d2382e5f1aae1abc4081af407905ef380311
Author: Jason A. Donenfeld <Jason@zx2c4.com>
Date:   Wed Sep 21 11:31:34 2022 +0200

    x86: re-enable rng seeding via SetupData

commit 3824e25db1a84fadc50b88dfbe27047aa2f7f85d
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Wed Aug 17 10:39:40 2022 +0200

    x86: disable rng seeding via setup_data
    
commit 67f7e426e53833a5db75b0d813e8d537b8a75bd2
Author: Jason A. Donenfeld <Jason@zx2c4.com>
Date:   Thu Jul 21 14:56:36 2022 +0200

    hw/i386: pass RNG seed via setup_data entry
    


though, even taking the individual commits isn't a perfectly
clean revert due to other unrelated changes in machine
type versioning.

I'm not too fussed if this last one isn't a clean revert,
but lets just call out in the commit message that we're
just manually resolving conflicts and skipping those
two intermediate commits for simplicity.

Assuming an improve commit message:

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>



> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 66e3d059ef..44b08554fa 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -127,9 +127,6 @@ struct PCMachineClass {
>  
>      /* create kvmclock device even when KVM PV features are not exposed */
>      bool kvmclock_create_always;
> -
> -    /* skip passing an rng seed for legacy machines */
> -    bool legacy_no_rng_seed;
>  };
>  
>  #define TYPE_PC_MACHINE "generic-pc-machine"
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index 62fa5774f8..df82c5fd42 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -126,8 +126,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
>  void x86_load_linux(X86MachineState *x86ms,
>                      FWCfgState *fw_cfg,
>                      int acpi_data_size,
> -                    bool pvh_enabled,
> -                    bool legacy_no_rng_seed);
> +                    bool pvh_enabled);
>  
>  bool x86_machine_is_smm_enabled(const X86MachineState *x86ms);
>  bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms);
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 170a331e3f..b231ceda9a 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -330,7 +330,7 @@ static void microvm_memory_init(MicrovmMachineState *mms)
>      rom_set_fw(fw_cfg);
>  
>      if (machine->kernel_filename != NULL) {
> -        x86_load_linux(x86ms, fw_cfg, 0, true, false);
> +        x86_load_linux(x86ms, fw_cfg, 0, true);
>      }
>  
>      if (mms->option_roms) {
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6e592bd969..2c5f675ba4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -804,7 +804,7 @@ void xen_load_linux(PCMachineState *pcms)
>      rom_set_fw(fw_cfg);
>  
>      x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
> -                   pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
> +                   pcmc->pvh_enabled);
>      for (i = 0; i < nb_option_roms; i++) {
>          assert(!strcmp(option_rom[i].name, "linuxboot.bin") ||
>                 !strcmp(option_rom[i].name, "linuxboot_dma.bin") ||
> @@ -1124,7 +1124,7 @@ void pc_memory_init(PCMachineState *pcms,
>  
>      if (linux_boot) {
>          x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
> -                       pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
> +                       pcmc->pvh_enabled);
>      }
>  
>      for (i = 0; i < nb_option_roms; i++) {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index df64dd8dcc..839bd65df5 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -476,9 +476,7 @@ DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,
>  
>  static void pc_i440fx_7_1_machine_options(MachineClass *m)
>  {
> -    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_7_2_machine_options(m);
> -    pcmc->legacy_no_rng_seed = true;
>      compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
>      compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 66cd718b70..e6e3966262 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -395,9 +395,7 @@ DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,
>  
>  static void pc_q35_7_1_machine_options(MachineClass *m)
>  {
> -    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_7_2_machine_options(m);
> -    pcmc->legacy_no_rng_seed = true;
>      compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
>      compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
>  }
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 4831193c86..80be3032cc 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -26,7 +26,6 @@
>  #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"
> @@ -771,8 +770,7 @@ static bool load_elfboot(const char *kernel_filename,
>  void x86_load_linux(X86MachineState *x86ms,
>                      FWCfgState *fw_cfg,
>                      int acpi_data_size,
> -                    bool pvh_enabled,
> -                    bool legacy_no_rng_seed)
> +                    bool pvh_enabled)
>  {
>      bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
>      uint16_t protocol;
> @@ -780,7 +778,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, first_setup_data = 0;
> +    hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
>      FILE *f;
>      char *vmode;
>      MachineState *machine = MACHINE(x86ms);
> @@ -790,7 +788,6 @@ void x86_load_linux(X86MachineState *x86ms,
>      const char *dtb_filename = machine->dtb;
>      const char *kernel_cmdline = machine->kernel_cmdline;
>      SevKernelLoaderContext sev_load_ctx = {};
> -    enum { RNG_SEED_LENGTH = 32 };
>  
>      /* Align to 16 bytes as a paranoia measure */
>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -1070,31 +1067,16 @@ void x86_load_linux(X86MachineState *x86ms,
>          kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
>          kernel = g_realloc(kernel, kernel_size);
>  
> +        stq_p(header + 0x250, prot_addr + setup_data_offset);
>  
>          setup_data = (struct setup_data *)(kernel + setup_data_offset);
> -        setup_data->next = cpu_to_le64(first_setup_data);
> -        first_setup_data = prot_addr + 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);
>      }
>  
> -    if (!legacy_no_rng_seed) {
> -        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
> -        kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
> -        kernel = g_realloc(kernel, kernel_size);
> -        setup_data = (struct setup_data *)(kernel + setup_data_offset);
> -        setup_data->next = cpu_to_le64(first_setup_data);
> -        first_setup_data = prot_addr + setup_data_offset;
> -        setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
> -        setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
> -        qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
> -    }
> -
> -    /* Offset 0x250 is a pointer to the first setup_data link. */
> -    stq_p(header + 0x250, first_setup_data);
> -
>      /*
>       * If we're starting an encrypted VM, it will be OVMF based, which uses the
>       * efi stub for booting and doesn't require any values to be placed in the
> -- 
> MST
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 7/7] Revert "hw/i386: pass RNG seed via setup_data entry"

[Michael S. Tsirkin]

On Tue, Feb 14, 2023 at 04:45:57PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 04:12:56PM -0500, Michael S. Tsirkin wrote:
> > This reverts commit 67f7e426e53833a5db75b0d813e8d537b8a75bd2.
> > 
> > Fixes: 67f7e426e5 ("hw/i386: pass RNG seed via setup_data entry")
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/i386/pc.h  |  3 ---
> >  include/hw/i386/x86.h |  3 +--
> >  hw/i386/microvm.c     |  2 +-
> >  hw/i386/pc.c          |  4 ++--
> >  hw/i386/pc_piix.c     |  2 --
> >  hw/i386/pc_q35.c      |  2 --
> >  hw/i386/x86.c         | 26 ++++----------------------
> >  7 files changed, 8 insertions(+), 34 deletions(-)
> 
> All the patches prior to this were clean reverts. This one though
> does not look clean, rather it looks like reverting a combination
> of 3 commits
> 
> commit ffe2d2382e5f1aae1abc4081af407905ef380311
> Author: Jason A. Donenfeld <Jason@zx2c4.com>
> Date:   Wed Sep 21 11:31:34 2022 +0200
> 
>     x86: re-enable rng seeding via SetupData
> 
> commit 3824e25db1a84fadc50b88dfbe27047aa2f7f85d
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Wed Aug 17 10:39:40 2022 +0200
> 
>     x86: disable rng seeding via setup_data
>     
> commit 67f7e426e53833a5db75b0d813e8d537b8a75bd2
> Author: Jason A. Donenfeld <Jason@zx2c4.com>
> Date:   Thu Jul 21 14:56:36 2022 +0200
> 
>     hw/i386: pass RNG seed via setup_data entry
>     
> 
> 
> though, even taking the individual commits isn't a perfectly
> clean revert due to other unrelated changes in machine
> type versioning.
> 
> I'm not too fussed if this last one isn't a clean revert,
> but lets just call out in the commit message that we're
> just manually resolving conflicts and skipping those
> two intermediate commits for simplicity.
> 
> Assuming an improve commit message:
> 
>   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Yes - what I did as part of revert is drop all mentions of
legacy_no_rng_seed manually.
I will clarify that, thanks!

> 
> 
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 66e3d059ef..44b08554fa 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -127,9 +127,6 @@ struct PCMachineClass {
> >  
> >      /* create kvmclock device even when KVM PV features are not exposed */
> >      bool kvmclock_create_always;
> > -
> > -    /* skip passing an rng seed for legacy machines */
> > -    bool legacy_no_rng_seed;
> >  };
> >  
> >  #define TYPE_PC_MACHINE "generic-pc-machine"
> > diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> > index 62fa5774f8..df82c5fd42 100644
> > --- a/include/hw/i386/x86.h
> > +++ b/include/hw/i386/x86.h
> > @@ -126,8 +126,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
> >  void x86_load_linux(X86MachineState *x86ms,
> >                      FWCfgState *fw_cfg,
> >                      int acpi_data_size,
> > -                    bool pvh_enabled,
> > -                    bool legacy_no_rng_seed);
> > +                    bool pvh_enabled);
> >  
> >  bool x86_machine_is_smm_enabled(const X86MachineState *x86ms);
> >  bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms);
> > diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> > index 170a331e3f..b231ceda9a 100644
> > --- a/hw/i386/microvm.c
> > +++ b/hw/i386/microvm.c
> > @@ -330,7 +330,7 @@ static void microvm_memory_init(MicrovmMachineState *mms)
> >      rom_set_fw(fw_cfg);
> >  
> >      if (machine->kernel_filename != NULL) {
> > -        x86_load_linux(x86ms, fw_cfg, 0, true, false);
> > +        x86_load_linux(x86ms, fw_cfg, 0, true);
> >      }
> >  
> >      if (mms->option_roms) {
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 6e592bd969..2c5f675ba4 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -804,7 +804,7 @@ void xen_load_linux(PCMachineState *pcms)
> >      rom_set_fw(fw_cfg);
> >  
> >      x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
> > -                   pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
> > +                   pcmc->pvh_enabled);
> >      for (i = 0; i < nb_option_roms; i++) {
> >          assert(!strcmp(option_rom[i].name, "linuxboot.bin") ||
> >                 !strcmp(option_rom[i].name, "linuxboot_dma.bin") ||
> > @@ -1124,7 +1124,7 @@ void pc_memory_init(PCMachineState *pcms,
> >  
> >      if (linux_boot) {
> >          x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
> > -                       pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
> > +                       pcmc->pvh_enabled);
> >      }
> >  
> >      for (i = 0; i < nb_option_roms; i++) {
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index df64dd8dcc..839bd65df5 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -476,9 +476,7 @@ DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,
> >  
> >  static void pc_i440fx_7_1_machine_options(MachineClass *m)
> >  {
> > -    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >      pc_i440fx_7_2_machine_options(m);
> > -    pcmc->legacy_no_rng_seed = true;
> >      compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
> >      compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
> >  }
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 66cd718b70..e6e3966262 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -395,9 +395,7 @@ DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,
> >  
> >  static void pc_q35_7_1_machine_options(MachineClass *m)
> >  {
> > -    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >      pc_q35_7_2_machine_options(m);
> > -    pcmc->legacy_no_rng_seed = true;
> >      compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
> >      compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
> >  }
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > index 4831193c86..80be3032cc 100644
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -26,7 +26,6 @@
> >  #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"
> > @@ -771,8 +770,7 @@ static bool load_elfboot(const char *kernel_filename,
> >  void x86_load_linux(X86MachineState *x86ms,
> >                      FWCfgState *fw_cfg,
> >                      int acpi_data_size,
> > -                    bool pvh_enabled,
> > -                    bool legacy_no_rng_seed)
> > +                    bool pvh_enabled)
> >  {
> >      bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
> >      uint16_t protocol;
> > @@ -780,7 +778,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, first_setup_data = 0;
> > +    hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
> >      FILE *f;
> >      char *vmode;
> >      MachineState *machine = MACHINE(x86ms);
> > @@ -790,7 +788,6 @@ void x86_load_linux(X86MachineState *x86ms,
> >      const char *dtb_filename = machine->dtb;
> >      const char *kernel_cmdline = machine->kernel_cmdline;
> >      SevKernelLoaderContext sev_load_ctx = {};
> > -    enum { RNG_SEED_LENGTH = 32 };
> >  
> >      /* Align to 16 bytes as a paranoia measure */
> >      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> > @@ -1070,31 +1067,16 @@ void x86_load_linux(X86MachineState *x86ms,
> >          kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
> >          kernel = g_realloc(kernel, kernel_size);
> >  
> > +        stq_p(header + 0x250, prot_addr + setup_data_offset);
> >  
> >          setup_data = (struct setup_data *)(kernel + setup_data_offset);
> > -        setup_data->next = cpu_to_le64(first_setup_data);
> > -        first_setup_data = prot_addr + 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);
> >      }
> >  
> > -    if (!legacy_no_rng_seed) {
> > -        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
> > -        kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
> > -        kernel = g_realloc(kernel, kernel_size);
> > -        setup_data = (struct setup_data *)(kernel + setup_data_offset);
> > -        setup_data->next = cpu_to_le64(first_setup_data);
> > -        first_setup_data = prot_addr + setup_data_offset;
> > -        setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
> > -        setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
> > -        qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
> > -    }
> > -
> > -    /* Offset 0x250 is a pointer to the first setup_data link. */
> > -    stq_p(header + 0x250, first_setup_data);
> > -
> >      /*
> >       * If we're starting an encrypted VM, it will be OVMF based, which uses the
> >       * efi stub for booting and doesn't require any values to be placed in the
> > -- 
> > MST
> > 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 0/7] revert RNG seed mess

[Daniel P. Berrangé]

On Fri, Feb 10, 2023 at 11:32:58AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 04:12:23PM -0500, Michael S. Tsirkin wrote:
> > All attempts to fix up passing RNG seed via setup_data entry failed.
> > Let's just rip out all of it.  We'll start over.
> > 
> > 
> > Warning: all I did was git revert the relevant patches and resolve the
> > (trivial) conflicts. Not even compiled - it's almost midnight here.
> > 
> > Jason this is the kind of approach I'd like to see, not yet another
> > pointer math rich patch I need to spend time reviewing. Just get us back
> > to where we started. We can redo "x86: use typedef for SetupData struct"
> > later if we want, it's benign.
> 
> This approach looks suitable for applying to the 7.2 tree too,
> which will be good for fixing the regressions in stable.

Since no further alternative has been proposed, can you consider sending
a pull request for this series. This has been broken for too long and
many users & vendors are looking for an official fix to be applied to
master before they backport to 7.2

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 0/7] revert RNG seed mess

[Michael S. Tsirkin]

On Mon, Feb 20, 2023 at 10:48:57AM +0000, Daniel P. Berrangé wrote:
> On Fri, Feb 10, 2023 at 11:32:58AM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 08, 2023 at 04:12:23PM -0500, Michael S. Tsirkin wrote:
> > > All attempts to fix up passing RNG seed via setup_data entry failed.
> > > Let's just rip out all of it.  We'll start over.
> > > 
> > > 
> > > Warning: all I did was git revert the relevant patches and resolve the
> > > (trivial) conflicts. Not even compiled - it's almost midnight here.
> > > 
> > > Jason this is the kind of approach I'd like to see, not yet another
> > > pointer math rich patch I need to spend time reviewing. Just get us back
> > > to where we started. We can redo "x86: use typedef for SetupData struct"
> > > later if we want, it's benign.
> > 
> > This approach looks suitable for applying to the 7.2 tree too,
> > which will be good for fixing the regressions in stable.
> 
> Since no further alternative has been proposed, can you consider sending
> a pull request for this series. This has been broken for too long and
> many users & vendors are looking for an official fix to be applied to
> master before they backport to 7.2
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Will do. Thanks!