Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220625153841.143928-1-Jason@zx2c4.com> <512bdf97-5468-e2d2-75bd-24107aaf8a34@vivier.eu>
 <CAHmME9qKkL8r7QSwq+2DA54mpMHzqpnq=pi5f0DhKOcB-qezoQ@mail.gmail.com> <fa4f2fba-ad77-e5a9-a78a-680ed5137157@vivier.eu>
In-Reply-To: <fa4f2fba-ad77-e5a9-a78a-680ed5137157@vivier.eu>
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
Date:   Sat, 25 Jun 2022 18:26:38 +0200
Message-ID: <CAHmME9oGOduYbw_-=0QeKzW6Lj07S6tevssvUpyWLgutuxE+UA@mail.gmail.com>
Subject: Re: [PATCH] m68k: virt: pass RNG seed via bootinfo block
To:     Laurent Vivier <laurent@vivier.eu>
Cc:     Geert Uytterhoeven <geert@linux-m68k.org>,
        linux-m68k <linux-m68k@lists.linux-m68k.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Jun 25, 2022 at 6:24 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 25/06/2022 =C3=A0 18:19, Jason A. Donenfeld a =C3=A9crit :
> > On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <laurent@vivier.eu> wrot=
e:
> >>
> >> Le 25/06/2022 =C3=A0 17:38, Jason A. Donenfeld a =C3=A9crit :
> >>> 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 it=
s
> >>> own bootinfo protocol. So this commit adds support for receiving a RN=
G
> >>> seed from it, which will be used at the earliest possible time in boo=
t,
> >>> just like device tree.
> >>>
> >>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >>> ---
> >>>    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/i=
nclude/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_reco=
rd *record)
> >>>                data +=3D 4;
> >>>                virt_bi_data.virtio.irq =3D 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 buf=
fer?
> >>
> >> 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 encod=
ed length cannot be greater
> than the record length?

I guess but that's different from all other length fields, and means
we can't expand past 65k if somebody wants to use this for something
more interesting later. Again I wonder what stinginess here gets us.
This is just a boot parameter... No need to go crazy optimizing it.

Jason