On Thu, Aug 04, 2022 at 04:31:06PM +0200, Jason A. Donenfeld wrote: > Hi Daniel, > > On Thu, Aug 04, 2022 at 02:31:06PM +0100, Daniel P. Berrangé wrote: > > On Thu, Aug 04, 2022 at 03:20:59PM +0200, Jason A. Donenfeld wrote: > > > On Thu, Aug 04, 2022 at 03:13:20PM +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. > > > > > > Strong NACK from me here. > > > > > > If this kind of thing is off by default, it's as good as useless. Indeed > > > it shouldn't even be a knob at all. Don't do this. > > > > You're misunderstanding the patch. This remains on by default for > > the 7.1 machine type. > > Ahhh, I think you're right. Sorry for mis understanding. The "even if it > is disabled by default" of the commit message isn't quite true then, > right?
I think it refers to the fact that we do not have a fix yet, and if we don't get one by release we might flip the default, but the feature will still be usable. > If I understand correctly, this is a yes/no/auto, which defaults to > auto on newer machine types. And auto triggers if the kernel has a newer > boot header. Is that correct? > > if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF && > (protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) { > > I think it's working as described (after applying the below fixup to > this broken patch). > > > The patch is merely exposing a knob so that users can override the > > built-in default if they need to. Imagine if we had shipped this > > existing code before today's bugs were discovered. The knob > > proposed her would allow users to turn off the broken pieces. > > This is a good thing. > > I'm still not really keen on adding a knob for this. I understand ARM > has a knob for it for different reasons (better named "dtb-randomness"). > If this knob thing is to live on here, maybe it should have > "-randomness" in the name also. > > > > Rather, let's fix the bug. The code as-is -- going back to the 2016 DTB > > > addition -- is problematic and needs to be fixed. So let's fix that. > > > Trying to cover up the problem with a default-off knob just ensures this > > > stuff will never be made to work right. > > > > It isn't covering up the problem, just providing a workaround > > option, should another bug be discovered after release. We > > still need to fix current discussed problems of course. > > Thanks for the explanation. I don't like adding a knob. But if it's on > by default for the default machine type, then that's a compromise I > could accept. > > Jason > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 00c21f6e4d..074571bc03 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -446,6 +446,7 @@ 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; > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 5bcf100b35..f3aa4694a2 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -383,6 +383,7 @@ 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->enforce_amd_1tb_hole = false; > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index 3fbab258a9..206ce6c547 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -785,6 +785,7 @@ 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; > @@ -1075,7 +1076,7 @@ void x86_load_linux(X86MachineState *x86ms, > } > > if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF && > - (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) > { > + (protocol >= 0x209 || x86ms->linuxboot_seed == 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);