Re: [PATCH] m68k: virt: pass RNG seed via bootinfo block

[Laurent Vivier]

Le 25/06/2022 à 18:19, Jason A. Donenfeld a écrit :
> On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <laurent@xxxxxxxxx> wrote:
> > Le 25/06/2022 à 17:38, Jason A. Donenfeld a écrit :
> > > Other virt VMs can pass RNG seeds via the "rng-seed" device tree
> > > property or via UEFI, but m68k doesn't have either. Instead it has its
> > > own bootinfo protocol. So this commit adds support for receiving a RNG
> > > seed from it, which will be used at the earliest possible time in boot,
> > > just like device tree.
> > >
> > > Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
> > > ---
> > >    arch/m68k/include/uapi/asm/bootinfo-virt.h | 1 +
> > >    arch/m68k/virt/config.c                    | 4 ++++
> > >    2 files changed, 5 insertions(+)
> > >
> > > diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> > > index e4db7e2213ab..7c3044acdf4a 100644
> > > --- a/arch/m68k/include/uapi/asm/bootinfo-virt.h
> > > +++ b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> > > @@ -12,6 +12,7 @@
> > >    #define BI_VIRT_GF_TTY_BASE 0x8003
> > >    #define BI_VIRT_VIRTIO_BASE 0x8004
> > >    #define BI_VIRT_CTRL_BASE   0x8005
> > > +#define BI_VIRT_RNG_SEED     0x8006
> > >
> > >    #define VIRT_BOOTI_VERSION  MK_BI_VERSION(2, 0)
> > >
> > > diff --git a/arch/m68k/virt/config.c b/arch/m68k/virt/config.c
> > > index 632ba200ad42..ad71af8273ec 100644
> > > --- a/arch/m68k/virt/config.c
> > > +++ b/arch/m68k/virt/config.c
> > > @@ -2,6 +2,7 @@
> > >
> > >    #include <linux/reboot.h>
> > >    #include <linux/serial_core.h>
> > > +#include <linux/random.h>
> > >    #include <clocksource/timer-goldfish.h>
> > >
> > >    #include <asm/bootinfo.h>
> > > @@ -92,6 +93,9 @@ int __init virt_parse_bootinfo(const struct bi_record *record)
> > >                data += 4;
> > >                virt_bi_data.virtio.irq = be32_to_cpup(data);
> > >                break;
> > > +     case BI_VIRT_RNG_SEED:
> > > +             add_bootloader_randomness(data + 4, be32_to_cpup(data));
> >
> > In fact, why don't you use the record->size to get the size of the buffer?
> >
> > It seems useless to encode twice the length of the buffer, the second time on a 32bit while the
> > length cannot exceed a 16bit value.
>
> Doesn't that make the length ambiguous because of required alignment?

I agree, it's why I understand reviewing the QEMU part of your patch.

> Would rather keep this general. As is, it's also much more like the
> others and more uniform to keep it that way. You were able to review
> it and see that it was right after glancing for a second. That seems
> superior to any imaginary gains we'd get by overloading the record
> size.

And what about using a 16bit field rather than a 32bit field as the encoded length cannot be greater 
than the record length?

Thanks,
Laurent