Re: [PATCH] pc: add property for Linux setup_data seed

[Michael S. Tsirkin]

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);