Hi Geert,
On Sun, Jun 26, 2022 at 11:39:46AM +0200, Geert Uytterhoeven wrote:
Hi Jason,
Thanks for your patch!
On Sat, Jun 25, 2022 at 6:26 PM Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
On Sat, Jun 25, 2022 at 6:24 PM Laurent Vivier <laurent@xxxxxxxxx> wrote:
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
FTR, "rng-seed" does not seem to be documented anywhere, not under
Documentation/devicetree/bindings/, and not in the Devicetree
Specification?
Good point. It's quite old, this field, so odd it was missed. I'll send
in a separate patch for that.
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>
--- 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
Please add a comment documenting the record format.
Ack.
Laurent: Would be nice if you could add this for the other BI_*, too.
#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?
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.
You cannot extend this past (64 KiB - sizeof(struct bi_record))
anyway, as the total record size is limited to 64 KiB, regardless of
the additional buffer size you try to encode inside your own 32-bit
size field.
So either just store the data inside the record, rely on bi_record.size,
and live with random data that must be a number of even bytes (does
it really hurt to drop the last byte, or add a dummy byte?), or store
a pointer/size, like is done for e.g. BI_RAMDISK.
I modeled this on BOOTINFOSTR, which benefits from null termination.
I'll just reduce the length field to 2 bytes. I really don't want to
play padding games here, and anyway the length field needs to be
separate for reasons that will become apparent in v2 (zeroing for
kexec).
Jason
