[PATCH v2] pc: add property for Linux setup_data random number seed

Paolo Bonzini posted 1 patch 1 year, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220804212441.458478-1-pbonzini@redhat.com
Maintainers: Sergio Lopez <slp@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
hw/i386/microvm.c     |  2 +-
hw/i386/pc.c          |  5 +++--
hw/i386/pc_piix.c     |  2 +-
hw/i386/pc_q35.c      |  2 +-
hw/i386/x86.c         | 33 +++++++++++++++++++++++++++++----
include/hw/i386/pc.h  |  3 ---
include/hw/i386/x86.h |  5 +++--
7 files changed, 38 insertions(+), 14 deletions(-)
[PATCH v2] pc: add property for Linux setup_data random number seed
Posted by Paolo Bonzini 1 year, 8 months ago
Using a property makes it possible to use the normal compat property
mechanism instead of ad hoc code; it avoids parameter proliferation
in x86_load_linux; and allows shipping the code even if it is
disabled by default.

Cc: Michael S. Tsirkin <mst@redhat.com>
Co-developed-by: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/microvm.c     |  2 +-
 hw/i386/pc.c          |  5 +++--
 hw/i386/pc_piix.c     |  2 +-
 hw/i386/pc_q35.c      |  2 +-
 hw/i386/x86.c         | 33 +++++++++++++++++++++++++++++----
 include/hw/i386/pc.h  |  3 ---
 include/hw/i386/x86.h |  5 +++--
 7 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 7fe8cce03e..dc929727dc 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -332,7 +332,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 7280c02ce3..9b192373c0 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-randomness", "off" },
 };
 const size_t pc_compat_6_2_len = G_N_ELEMENTS(pc_compat_6_2);
 
@@ -796,7 +797,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") ||
@@ -1118,7 +1119,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 a5c65c1c35..1526b7e3fd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -447,10 +447,10 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
 static void pc_i440fx_7_0_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
     pc_i440fx_7_1_machine_options(m);
     m->alias = NULL;
     m->is_default = false;
-    pcmc->legacy_no_rng_seed = true;
     pcmc->enforce_amd_1tb_hole = false;
     compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
     compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 3a35193ff7..c5b38edc65 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -384,9 +384,9 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
 static void pc_q35_7_0_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
     pc_q35_7_1_machine_options(m);
     m->alias = NULL;
-    pcmc->legacy_no_rng_seed = true;
     pcmc->enforce_amd_1tb_hole = false;
     compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
     compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 050eedc0c8..8c6450ee07 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -60,6 +60,8 @@
 #include CONFIG_DEVICES
 #include "kvm/kvm_i386.h"
 
+#define RNG_SEED_LENGTH 32
+
 /* Physical Address of PVH entry point read from kernel ELF NOTE */
 static size_t pvh_start_addr;
 
@@ -767,8 +769,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;
@@ -786,7 +787,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;
@@ -1076,7 +1076,8 @@ void x86_load_linux(X86MachineState *x86ms,
         load_image_size(dtb_filename, setup_data->data, dtb_size);
     }
 
-    if (!legacy_no_rng_seed) {
+    if (x86ms->linuxboot_randomness != ON_OFF_AUTO_OFF &&
+        (protocol >= 0x209 || x86ms->linuxboot_randomness == ON_OFF_AUTO_ON)) {
         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);
@@ -1237,6 +1238,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_randomness(Object *obj, Visitor *v, const char *name,
+                                           void *opaque, Error **errp)
+{
+    X86MachineState *x86ms = X86_MACHINE(obj);
+    OnOffAuto linuxboot_randomness = x86ms->linuxboot_randomness;
+
+    visit_type_OnOffAuto(v, name, &linuxboot_randomness, errp);
+}
+
+static void x86_machine_set_linuxboot_randomness(Object *obj, Visitor *v, const char *name,
+                                           void *opaque, Error **errp)
+{
+    X86MachineState *x86ms = X86_MACHINE(obj);
+
+    visit_type_OnOffAuto(v, name, &x86ms->linuxboot_randomness, errp);
+}
+
 bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms)
 {
     if (x86ms->acpi == ON_OFF_AUTO_OFF) {
@@ -1387,6 +1405,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_randomness = 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);
@@ -1426,6 +1445,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_RANDOMNESS, "OnOffAuto",
+        x86_machine_get_linuxboot_randomness, x86_machine_set_linuxboot_randomness,
+        NULL, NULL);
+    object_class_property_set_description(oc, X86_MACHINE_LINUXBOOT_RANDOMNESS,
+        "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/pc.h b/include/hw/i386/pc.h
index 8435733bd6..9cc3f5d338 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -128,9 +128,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..0487c065c8 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -70,6 +70,7 @@ struct X86MachineState {
     OnOffAuto acpi;
     OnOffAuto pit;
     OnOffAuto pic;
+    OnOffAuto linuxboot_randomness;
 
     char *oem_id;
     char *oem_table_id;
@@ -94,6 +95,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_RANDOMNESS "linuxboot-randomness"
 
 #define TYPE_X86_MACHINE   MACHINE_TYPE_NAME("x86")
 OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
@@ -126,8 +128,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);
-- 
2.37.1
Re: [PATCH v2] pc: add property for Linux setup_data random number seed
Posted by Michael S. Tsirkin 1 year, 8 months ago
On Thu, Aug 04, 2022 at 11:24:41PM +0200, Paolo Bonzini wrote:
> Using a property makes it possible to use the normal compat property
> mechanism instead of ad hoc code; it avoids parameter proliferation
> in x86_load_linux; and allows shipping the code even if it is
> disabled by default.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Co-developed-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/i386/microvm.c     |  2 +-
>  hw/i386/pc.c          |  5 +++--
>  hw/i386/pc_piix.c     |  2 +-
>  hw/i386/pc_q35.c      |  2 +-
>  hw/i386/x86.c         | 33 +++++++++++++++++++++++++++++----
>  include/hw/i386/pc.h  |  3 ---
>  include/hw/i386/x86.h |  5 +++--
>  7 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 7fe8cce03e..dc929727dc 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -332,7 +332,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 7280c02ce3..9b192373c0 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-randomness", "off" },
>  };
>  const size_t pc_compat_6_2_len = G_N_ELEMENTS(pc_compat_6_2);
>  
> @@ -796,7 +797,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") ||
> @@ -1118,7 +1119,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 a5c65c1c35..1526b7e3fd 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -447,10 +447,10 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
>  static void pc_i440fx_7_0_machine_options(MachineClass *m)
>  {
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +
>      pc_i440fx_7_1_machine_options(m);
>      m->alias = NULL;
>      m->is_default = false;
> -    pcmc->legacy_no_rng_seed = true;
>      pcmc->enforce_amd_1tb_hole = false;
>      compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
>      compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 3a35193ff7..c5b38edc65 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -384,9 +384,9 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
>  static void pc_q35_7_0_machine_options(MachineClass *m)
>  {
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +
>      pc_q35_7_1_machine_options(m);
>      m->alias = NULL;
> -    pcmc->legacy_no_rng_seed = true;
>      pcmc->enforce_amd_1tb_hole = false;
>      compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
>      compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 050eedc0c8..8c6450ee07 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -60,6 +60,8 @@
>  #include CONFIG_DEVICES
>  #include "kvm/kvm_i386.h"
>  
> +#define RNG_SEED_LENGTH 32
> +
>  /* Physical Address of PVH entry point read from kernel ELF NOTE */
>  static size_t pvh_start_addr;
>  
> @@ -767,8 +769,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;
> @@ -786,7 +787,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;
> @@ -1076,7 +1076,8 @@ void x86_load_linux(X86MachineState *x86ms,
>          load_image_size(dtb_filename, setup_data->data, dtb_size);
>      }
>  
> -    if (!legacy_no_rng_seed) {
> +    if (x86ms->linuxboot_randomness != ON_OFF_AUTO_OFF &&
> +        (protocol >= 0x209 || x86ms->linuxboot_randomness == ON_OFF_AUTO_ON)) {

Hmm so if user requested "on" but protocol is too old then we just
ignore it silently? I'd rather we failed initialization.
So:

if (x86ms->linuxboot_randomness == ON_OFF_AUTO_ON &&
    protocol < 0x209) {
	fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
	exit(1);
}





>          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);
> @@ -1237,6 +1238,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_randomness(Object *obj, Visitor *v, const char *name,
> +                                           void *opaque, Error **errp)
> +{
> +    X86MachineState *x86ms = X86_MACHINE(obj);
> +    OnOffAuto linuxboot_randomness = x86ms->linuxboot_randomness;
> +
> +    visit_type_OnOffAuto(v, name, &linuxboot_randomness, errp);
> +}
> +
> +static void x86_machine_set_linuxboot_randomness(Object *obj, Visitor *v, const char *name,
> +                                           void *opaque, Error **errp)
> +{
> +    X86MachineState *x86ms = X86_MACHINE(obj);
> +
> +    visit_type_OnOffAuto(v, name, &x86ms->linuxboot_randomness, errp);
> +}
> +
>  bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms)
>  {
>      if (x86ms->acpi == ON_OFF_AUTO_OFF) {

Not a comment on this code, but I've been thinking about this code we
have spread all over, it's a lot of boilerplate code, and e.g. whether
the default "auto" means "on if possible" or "off if possible" is
actually hidden inside code logic.

I wish we had a macro for this kind of thing.


> @@ -1387,6 +1405,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_randomness = 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);
> @@ -1426,6 +1445,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_RANDOMNESS, "OnOffAuto",
> +        x86_machine_get_linuxboot_randomness, x86_machine_set_linuxboot_randomness,
> +        NULL, NULL);
> +    object_class_property_set_description(oc, X86_MACHINE_LINUXBOOT_RANDOMNESS,
> +        "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,

BTW we don't check that this isn't used without -kernel, and
if it is it's silently ignored. Which isn't terrible but something
to think about.


> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 8435733bd6..9cc3f5d338 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -128,9 +128,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..0487c065c8 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -70,6 +70,7 @@ struct X86MachineState {
>      OnOffAuto acpi;
>      OnOffAuto pit;
>      OnOffAuto pic;
> +    OnOffAuto linuxboot_randomness;
>  
>      char *oem_id;
>      char *oem_table_id;
> @@ -94,6 +95,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_RANDOMNESS "linuxboot-randomness"
>  
>  #define TYPE_X86_MACHINE   MACHINE_TYPE_NAME("x86")
>  OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
> @@ -126,8 +128,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);
> -- 
> 2.37.1
Re: [PATCH v2] pc: add property for Linux setup_data random number seed
Posted by Paolo Bonzini 1 year, 8 months ago
On 8/5/22 09:01, Michael S. Tsirkin wrote:
>> -    if (!legacy_no_rng_seed) {
>> +    if (x86ms->linuxboot_randomness != ON_OFF_AUTO_OFF &&
>> +        (protocol >= 0x209 || x86ms->linuxboot_randomness == ON_OFF_AUTO_ON)) {
> Hmm so if user requested "on" but protocol is too old then we just
> ignore it silently? I'd rather we failed initialization.
> So:
> 
> if (x86ms->linuxboot_randomness == ON_OFF_AUTO_ON &&
>      protocol < 0x209) {
> 	fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
> 	exit(1);
> }

It doesn't ignore the "on" setting; it passes the seed anyway even if 
the protocol is too old.  Basically, a kernel that is too old to support 
setup data is treated the same as a kernel that supports setup data but 
doesn't know about the seed datum.

It seemed the more sensible implementation because anyway you cannot 
know if the kernel will use the datum.

Paolo
Re: [PATCH v2] pc: add property for Linux setup_data random number seed
Posted by Michael S. Tsirkin 1 year, 8 months ago
On Fri, Aug 05, 2022 at 10:16:37AM +0200, Paolo Bonzini wrote:
> On 8/5/22 09:01, Michael S. Tsirkin wrote:
> > > -    if (!legacy_no_rng_seed) {
> > > +    if (x86ms->linuxboot_randomness != ON_OFF_AUTO_OFF &&
> > > +        (protocol >= 0x209 || x86ms->linuxboot_randomness == ON_OFF_AUTO_ON)) {
> > Hmm so if user requested "on" but protocol is too old then we just
> > ignore it silently? I'd rather we failed initialization.
> > So:
> > 
> > if (x86ms->linuxboot_randomness == ON_OFF_AUTO_ON &&
> >      protocol < 0x209) {
> > 	fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
> > 	exit(1);
> > }
> 
> It doesn't ignore the "on" setting; it passes the seed anyway even if the
> protocol is too old.  Basically, a kernel that is too old to support setup
> data is treated the same as a kernel that supports setup data but doesn't
> know about the seed datum.

Oh it's an || not an &&. You are right. I needed more coffee.

> It seemed the more sensible implementation because anyway you cannot know if
> the kernel will use the datum.
> 
> Paolo

OK then.

-- 
MST