| Message ID | 20220721122937.729959-5-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor x86_load_linux and pass RNG seed via setup_data entry | expand |
On Thu, Jul 21, 2022 at 2:29 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > +static void x86_machine_get_linuxboot_seed(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + X86MachineState *x86ms = X86_MACHINE(obj); > + OnOffAuto linuxboot_seed = x86ms->linuxboot_seed; > + > + visit_type_OnOffAuto(v, name, &linuxboot_seed, errp); > +} > + > +static void x86_machine_set_linuxboot_seed(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + X86MachineState *x86ms = X86_MACHINE(obj); > + > + visit_type_OnOffAuto(v, name, &x86ms->linuxboot_seed, errp); > +} > + Gross, no! There is no reason at all to make this into a user tunable. Please don't do that. The whole point is that this is a simple transparent mechanism. There's also no need to usurp my patchset. I sent a v7 incorporating your feedback. So this isn't really appreciated either. I actually asked you not to usurp this over IRC, but you did anyway. If your goal is "alienate this contributor so he doesn't like working on QEMU," then you're succeeding. Here's v7: https://lore.kernel.org/qemu-devel/20220721125636.446842-1-Jason@zx2c4.com/ This will handle your ridiculous theoretical migratory concerns with minimal invasiveness and without having to introduce userfacing tunables. Let's keep discussion on that v7 thread, please. Thanks, Jason
On Thu, Jul 21, 2022 at 02:29:37PM +0200, Paolo Bonzini wrote: > From: "Jason A. Donenfeld" <Jason@zx2c4.com> > > Tiny machines optimized for fast boot time generally don't use EFI, > which means a random seed has to be supplied some other way. For this > purpose, Linux (≥5.20) supports passing a seed in the setup_data table > with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and > specialized bootloaders. The linked commit shows the upstream kernel > implementation. > > Link: https://git.kernel.org/tip/tip/c/68b8e9713c8 > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > Message-Id: <20220719115300.104095-1-Jason@zx2c4.com> > [Mostly rewritten to preserve guest ABI, but still starting from Jason's > code. - Paolo] > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/i386/pc.c | 1 + > hw/i386/x86.c | 31 ++++++++++++++++++++ > include/hw/i386/x86.h | 2 ++ > include/standard-headers/asm-x86/bootparam.h | 1 + > 4 files changed, 35 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 774cb2bf07..d456fbb166 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -112,6 +112,7 @@ const size_t pc_compat_7_0_len = G_N_ELEMENTS(pc_compat_7_0); > > GlobalProperty pc_compat_6_2[] = { > { "virtio-mem", "unplugged-inaccessible", "off" }, > + { TYPE_X86_MACHINE, "linuxboot-seed", "off" }, > }; > const size_t pc_compat_6_2_len = G_N_ELEMENTS(pc_compat_6_2); > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index 564bf3834b..c5d01e084a 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -26,6 +26,7 @@ > #include "qemu/cutils.h" > #include "qemu/units.h" > #include "qemu/datadir.h" > +#include "qemu/guest-random.h" > #include "qapi/error.h" > #include "qapi/qmp/qerror.h" > #include "qapi/qapi-visit-common.h" > @@ -1088,6 +1089,12 @@ void x86_load_linux(X86MachineState *x86ms, > } > fclose(f); > > + if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF && > + (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) { > + void *seed = add_setup_data(&data, 32, SETUP_RNG_SEED); > + qemu_guest_getrandom_nofail(seed, 32); > + } > + > /* append dtb to kernel */ > if (dtb_filename) { > dtb_size = get_image_size(dtb_filename); > @@ -1247,6 +1254,23 @@ static void x86_machine_set_smm(Object *obj, Visitor *v, const char *name, > visit_type_OnOffAuto(v, name, &x86ms->smm, errp); > } > > +static void x86_machine_get_linuxboot_seed(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + X86MachineState *x86ms = X86_MACHINE(obj); > + OnOffAuto linuxboot_seed = x86ms->linuxboot_seed; > + > + visit_type_OnOffAuto(v, name, &linuxboot_seed, errp); > +} > + > +static void x86_machine_set_linuxboot_seed(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + X86MachineState *x86ms = X86_MACHINE(obj); > + > + visit_type_OnOffAuto(v, name, &x86ms->linuxboot_seed, errp); > +} > + > bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms) > { > if (x86ms->acpi == ON_OFF_AUTO_OFF) { > @@ -1397,6 +1421,7 @@ static void x86_machine_initfn(Object *obj) > x86ms->acpi = ON_OFF_AUTO_AUTO; > x86ms->pit = ON_OFF_AUTO_AUTO; > x86ms->pic = ON_OFF_AUTO_AUTO; > + x86ms->linuxboot_seed = ON_OFF_AUTO_AUTO; > x86ms->pci_irq_mask = ACPI_BUILD_PCI_IRQS; > x86ms->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6); > x86ms->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8); > @@ -1435,6 +1460,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) > object_class_property_set_description(oc, X86_MACHINE_PIT, > "Enable i8254 PIT"); > > + object_class_property_add(oc, X86_MACHINE_LINUXBOOT_SEED, "OnOffAuto", > + x86_machine_get_linuxboot_seed, x86_machine_set_linuxboot_seed, > + NULL, NULL); > + object_class_property_set_description(oc, X86_MACHINE_LINUXBOOT_SEED, > + "Pass random number seed to -kernel Linux image"); > + > object_class_property_add(oc, X86_MACHINE_PIC, "OnOffAuto", > x86_machine_get_pic, > x86_machine_set_pic, > diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h > index 9089bdd99c..edf0f6799e 100644 > --- a/include/hw/i386/x86.h > +++ b/include/hw/i386/x86.h > @@ -67,6 +67,7 @@ struct X86MachineState { > OnOffAuto acpi; > OnOffAuto pit; > OnOffAuto pic; > + OnOffAuto linuxboot_seed; > > char *oem_id; > char *oem_table_id; > @@ -91,6 +92,7 @@ struct X86MachineState { > #define X86_MACHINE_OEM_ID "x-oem-id" > #define X86_MACHINE_OEM_TABLE_ID "x-oem-table-id" > #define X86_MACHINE_BUS_LOCK_RATELIMIT "bus-lock-ratelimit" > +#define X86_MACHINE_LINUXBOOT_SEED "linuxboot-seed" I am guessing we should prefix this with "x-" so we don't commit to a user interface. > > #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86") > OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE) > diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h > index 072e2ed546..b2aaad10e5 100644 > --- a/include/standard-headers/asm-x86/bootparam.h > +++ b/include/standard-headers/asm-x86/bootparam.h > @@ -10,6 +10,7 @@ > #define SETUP_EFI 4 > #define SETUP_APPLE_PROPERTIES 5 > #define SETUP_JAILHOUSE 6 > +#define SETUP_RNG_SEED 9 > > #define SETUP_INDIRECT (1<<31) > > -- > 2.36.1
Hi Michael, On Thu, Jul 21, 2022 at 10:47:57AM -0400, Michael S. Tsirkin wrote: > On Thu, Jul 21, 2022 at 02:29:37PM +0200, Paolo Bonzini wrote: > > #define X86_MACHINE_OEM_TABLE_ID "x-oem-table-id" > > #define X86_MACHINE_BUS_LOCK_RATELIMIT "bus-lock-ratelimit" > > +#define X86_MACHINE_LINUXBOOT_SEED "linuxboot-seed" > > I am guessing we should prefix this with "x-" so we don't commit > to a user interface. Actually there's no good reason to have any user interface at all. See v7 which Paolo "LGTM"ed: https://lore.kernel.org/qemu-devel/20220721125636.446842-1-Jason@zx2c4.com/ So just ignore this apocryphal patch here that Paolo sent. Jason
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 774cb2bf07..d456fbb166 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -112,6 +112,7 @@ const size_t pc_compat_7_0_len = G_N_ELEMENTS(pc_compat_7_0); GlobalProperty pc_compat_6_2[] = { { "virtio-mem", "unplugged-inaccessible", "off" }, + { TYPE_X86_MACHINE, "linuxboot-seed", "off" }, }; const size_t pc_compat_6_2_len = G_N_ELEMENTS(pc_compat_6_2); diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 564bf3834b..c5d01e084a 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -26,6 +26,7 @@ #include "qemu/cutils.h" #include "qemu/units.h" #include "qemu/datadir.h" +#include "qemu/guest-random.h" #include "qapi/error.h" #include "qapi/qmp/qerror.h" #include "qapi/qapi-visit-common.h" @@ -1088,6 +1089,12 @@ void x86_load_linux(X86MachineState *x86ms, } fclose(f); + if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF && + (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) { + void *seed = add_setup_data(&data, 32, SETUP_RNG_SEED); + qemu_guest_getrandom_nofail(seed, 32); + } + /* append dtb to kernel */ if (dtb_filename) { dtb_size = get_image_size(dtb_filename); @@ -1247,6 +1254,23 @@ static void x86_machine_set_smm(Object *obj, Visitor *v, const char *name, visit_type_OnOffAuto(v, name, &x86ms->smm, errp); } +static void x86_machine_get_linuxboot_seed(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + X86MachineState *x86ms = X86_MACHINE(obj); + OnOffAuto linuxboot_seed = x86ms->linuxboot_seed; + + visit_type_OnOffAuto(v, name, &linuxboot_seed, errp); +} + +static void x86_machine_set_linuxboot_seed(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + X86MachineState *x86ms = X86_MACHINE(obj); + + visit_type_OnOffAuto(v, name, &x86ms->linuxboot_seed, errp); +} + bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms) { if (x86ms->acpi == ON_OFF_AUTO_OFF) { @@ -1397,6 +1421,7 @@ static void x86_machine_initfn(Object *obj) x86ms->acpi = ON_OFF_AUTO_AUTO; x86ms->pit = ON_OFF_AUTO_AUTO; x86ms->pic = ON_OFF_AUTO_AUTO; + x86ms->linuxboot_seed = ON_OFF_AUTO_AUTO; x86ms->pci_irq_mask = ACPI_BUILD_PCI_IRQS; x86ms->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6); x86ms->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8); @@ -1435,6 +1460,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) object_class_property_set_description(oc, X86_MACHINE_PIT, "Enable i8254 PIT"); + object_class_property_add(oc, X86_MACHINE_LINUXBOOT_SEED, "OnOffAuto", + x86_machine_get_linuxboot_seed, x86_machine_set_linuxboot_seed, + NULL, NULL); + object_class_property_set_description(oc, X86_MACHINE_LINUXBOOT_SEED, + "Pass random number seed to -kernel Linux image"); + object_class_property_add(oc, X86_MACHINE_PIC, "OnOffAuto", x86_machine_get_pic, x86_machine_set_pic, diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h index 9089bdd99c..edf0f6799e 100644 --- a/include/hw/i386/x86.h +++ b/include/hw/i386/x86.h @@ -67,6 +67,7 @@ struct X86MachineState { OnOffAuto acpi; OnOffAuto pit; OnOffAuto pic; + OnOffAuto linuxboot_seed; char *oem_id; char *oem_table_id; @@ -91,6 +92,7 @@ struct X86MachineState { #define X86_MACHINE_OEM_ID "x-oem-id" #define X86_MACHINE_OEM_TABLE_ID "x-oem-table-id" #define X86_MACHINE_BUS_LOCK_RATELIMIT "bus-lock-ratelimit" +#define X86_MACHINE_LINUXBOOT_SEED "linuxboot-seed" #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86") OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE) diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h index 072e2ed546..b2aaad10e5 100644 --- a/include/standard-headers/asm-x86/bootparam.h +++ b/include/standard-headers/asm-x86/bootparam.h @@ -10,6 +10,7 @@ #define SETUP_EFI 4 #define SETUP_APPLE_PROPERTIES 5 #define SETUP_JAILHOUSE 6 +#define SETUP_RNG_SEED 9 #define SETUP_INDIRECT (1<<31)