| Message ID | 20221003103627.947985-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mips/malta: pass RNG seed to to kernel via env var | expand |
On Mon, Oct 03, 2022 at 12:36:27PM +0200, Jason A. Donenfeld wrote: > As of the kernel commit linked below, Linux ingests an RNG seed > passed from the hypervisor. So, pass this for the Malta platform, and > reinitialize it on reboot too, so that it's always fresh. > > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> > Cc: Aurelien Jarno <aurelien@aurel32.net> > Link: https://git.kernel.org/mips/c/056a68cea01 FYI, the kernel side of things has now been merged to Linus' tree and will be in 6.1-rc1: https://git.kernel.org/torvalds/c/056a68cea01 (Still waiting on this QEMU patch obviously). Jason
Hi Jason, Per https://www.qemu.org/docs/master/devel/submitting-a-patch.html#when-resending-patches-add-a-version-tag: Send each new revision as a new top-level thread, rather than burying it in-reply-to an earlier revision, as many reviewers are not looking inside deep threads for new patches. On 3/10/22 12:36, Jason A. Donenfeld wrote: > As of the kernel commit linked below, Linux ingests an RNG seed > passed from the hypervisor. So, pass this for the Malta platform, and > reinitialize it on reboot too, so that it's always fresh. > > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> > Cc: Aurelien Jarno <aurelien@aurel32.net> > Link: https://git.kernel.org/mips/c/056a68cea01 You seem to justify this commit by the kernel commit, which justifies itself mentioning hypervisor use... So the egg comes first before the chicken. > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > Changes v1->v2: > - Update commit message. > - No code changes. > > hw/mips/malta.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/hw/mips/malta.c b/hw/mips/malta.c > index 0e932988e0..9d793b3c17 100644 > --- a/hw/mips/malta.c > +++ b/hw/mips/malta.c > @@ -26,6 +26,7 @@ > #include "qemu/units.h" > #include "qemu/bitops.h" > #include "qemu/datadir.h" > +#include "qemu/guest-random.h" > #include "hw/clock.h" > #include "hw/southbridge/piix.h" > #include "hw/isa/superio.h" > @@ -1017,6 +1018,17 @@ static void G_GNUC_PRINTF(3, 4) prom_set(uint32_t *prom_buf, int index, > va_end(ap); > } > > +static void reinitialize_rng_seed(void *opaque) > +{ > + char *rng_seed_hex = opaque; > + uint8_t rng_seed[32]; > + > + qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed)); > + for (size_t i = 0; i < sizeof(rng_seed); ++i) { > + sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]); > + } > +} > + > /* Kernel */ > static uint64_t load_kernel(void) > { > @@ -1028,6 +1040,8 @@ static uint64_t load_kernel(void) > long prom_size; > int prom_index = 0; > uint64_t (*xlate_to_kseg0) (void *opaque, uint64_t addr); > + uint8_t rng_seed[32]; > + char rng_seed_hex[sizeof(rng_seed) * 2 + 1]; > > #if TARGET_BIG_ENDIAN > big_endian = 1; > @@ -1115,9 +1129,20 @@ static uint64_t load_kernel(void) > > prom_set(prom_buf, prom_index++, "modetty0"); > prom_set(prom_buf, prom_index++, "38400n8r"); > + > + qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed)); > + for (size_t i = 0; i < sizeof(rng_seed); ++i) { > + sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]); > + } > + prom_set(prom_buf, prom_index++, "rngseed"); > + prom_set(prom_buf, prom_index++, "%s", rng_seed_hex); You use the firmware interface to pass rng data to an hypervisor... Look to me you are forcing one API to ease another one. From the FW PoV it is a lie, because the FW will only change this value if an operator is involved. Here PROM stands for "programmable read-only memory", rarely modified. Having the 'rngseed' updated on each reset is surprising. Do you have an example of firmware doing that? (So I can understand whether this is the best way to mimic this behavior here). Aren't they better APIs to have hypervisors pass data to a kernel? Regards, Phil. > prom_set(prom_buf, prom_index++, NULL); > > rom_add_blob_fixed("prom", prom_buf, prom_size, ENVP_PADDR); > + qemu_register_reset(reinitialize_rng_seed, > + memmem(rom_ptr(ENVP_PADDR, prom_size), prom_size, > + rng_seed_hex, sizeof(rng_seed_hex))); > > g_free(prom_buf); > return kernel_entry;
Hi Philippe, On Tue, Oct 4, 2022 at 12:36 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Send each new revision as a new top-level thread, rather than burying it > in-reply-to an earlier revision, as many reviewers are not looking > inside deep threads for new patches. Will do. > You seem to justify this commit by the kernel commit, which justifies > itself mentioning hypervisor use... So the egg comes first before the > chicken. Oh, that's not really the intention. My goal is to provide sane interfaces for preboot environments -- whether those are in a hypervisor like QEMU or in firmware like CFE -- to pass a random seed along to the kernel. To that end, I've been making sure there's both a kernel side and a QEMU side, and submitting both to see what folks think. The fact that you have some questions (below) is a good thing; I'm glad to have your input on it. > > + > > + qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed)); > > + for (size_t i = 0; i < sizeof(rng_seed); ++i) { > > + sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]); > > + } > > + prom_set(prom_buf, prom_index++, "rngseed"); > > + prom_set(prom_buf, prom_index++, "%s", rng_seed_hex); > > You use the firmware interface to pass rng data to an hypervisor... > > Look to me you are forcing one API to ease another one. From the > FW PoV it is a lie, because the FW will only change this value if > an operator is involved. Here PROM stands for "programmable read-only > memory", rarely modified. Having the 'rngseed' updated on each > reset is surprising. > > Do you have an example of firmware doing that? (So I can understand > whether this is the best way to mimic this behavior here). > > Aren't they better APIs to have hypervisors pass data to a kernel? So a firmware interface *is* the intended situation here. To answer your last question first: the "standard" firmware interface for passing these seeds is via device tree's "rng-seed" field. There's also a EFI protocol for this. And on x86 it can be passed through the setup_data field. And on m68k the bootinfo bootloader/firmware struct has a BI_RNG_SEED type. There's plenty of ARM and x86 hardware that uses device tree and EFI for this, where the firmware is involved in generating the seeds, and in the device tree case, in mangling the device tree to have the right values. So, to answer your first question, yes I think this is indeed a firmware-style interface. Right now this is obviously intended for QEMU (and other hypervisors) to implement. Later I'm hoping that firmware environments like CFE might gain support for setting this. (You could do so interactively now with "setenv".) So it seems like the environment block here really is the right way to pass this. If you have a MIPS/malta platform alternative, I'd be happy to consider it with you, but in my look at things so far, the fw env block seems like by far the best way of doing this, especially so considering it's part of both real firmware environments and QEMU, and is relatively straightforward to implement. Jason
On Tue, Oct 04, 2022 at 12:36:03AM +0200, Philippe Mathieu-Daudé wrote: > Hi Jason, > > Per https://www.qemu.org/docs/master/devel/submitting-a-patch.html#when-resending-patches-add-a-version-tag: > > Send each new revision as a new top-level thread, rather than burying it > in-reply-to an earlier revision, as many reviewers are not looking inside > deep threads for new patches. > > On 3/10/22 12:36, Jason A. Donenfeld wrote: > > As of the kernel commit linked below, Linux ingests an RNG seed > > passed from the hypervisor. So, pass this for the Malta platform, and > > reinitialize it on reboot too, so that it's always fresh. > > > > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> > > Cc: Aurelien Jarno <aurelien@aurel32.net> > > Link: https://git.kernel.org/mips/c/056a68cea01 > > You seem to justify this commit by the kernel commit, which justifies > itself mentioning hypervisor use... So the egg comes first before the > chicken. The kernel justification is that the guest OS needs a good RNG seed. The kernel patch is just saying that the firmware / hypervisor side is where this seed generally expected to come from. This is fine, and not notably different from what Jason's already got wired up for the various other targets in QEMU. With regards, Daniel
And just to give you some idea that this truly is possible from firmware
and I'm not just making it up, consider this patch to U-Boot:
u-boot:
diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
index cab8da4860..27f3ee68c0 100644
--- a/arch/mips/lib/bootm.c
+++ b/arch/mips/lib/bootm.c
@@ -211,6 +211,8 @@ static void linux_env_legacy(bootm_headers_t *images)
sprintf(env_buf, "%un8r", gd->baudrate);
linux_env_set("modetty0", env_buf);
}
+
+ linux_env_set("rngseed", "4142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f60");
}
static int boot_reloc_fdt(bootm_headers_t *images)
Now, obviously that seed should be generated from some real method (a
seed file in flash, a hardware RNG U-Boot knows about, etc), but for the
purposes of showing that this is how things are passed to Linux, the
above suffices. To show that this ingested by Linux, let's then add:
linux:
diff --git a/drivers/char/random.c b/drivers/char/random.c
index a007e3dad80f..05d5b8bcb7e9 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -890,6 +890,7 @@ EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
*/
void __init add_bootloader_randomness(const void *buf, size_t len)
{
+ print_hex_dump(KERN_ERR, "SARU seed: ", DUMP_PREFIX_OFFSET, 16, 1, buf, len, 1);
mix_pool_bytes(buf, len);
if (trust_bootloader)
credit_init_bits(len * 8);
And now let's boot it:
$ qemu-system-mips -nographic -bios ./u-boot.bin -m 1G -netdev user,tftp=arch/mips/boot,bootfile=/uImage,id=net -device pcnet,netdev=net
U-Boot 2022.10-dirty (Oct 04 2022 - 12:31:05 +0200)
Board: MIPS Malta CoreLV
DRAM: 256 MiB
Core: 3 devices, 3 uclasses, devicetree: separate
PCI: Failed autoconfig bar 10
PCI: Failed autoconfig bar 14
PCI: Failed autoconfig bar 18
PCI: Failed autoconfig bar 1c
PCI: Failed autoconfig bar 20
PCI: Failed autoconfig bar 24
Flash: 4 MiB
Loading Environment from Flash... *** Warning - bad CRC, using default environment
In: serial@3f8
Out: serial@3f8
Err: serial@3f8
Net: eth0: pcnet#0
IDE: Bus 0: not available
malta # bootp
BOOTP broadcast 1
DHCP client bound to address 10.0.2.15 (1 ms)
Using pcnet#0 device
TFTP from server 10.0.2.2; our IP address is 10.0.2.15
Filename '/uImage'.
Load address: 0x81000000
Loading: #################################################################
#################################################################
#################################################################
#################################################################
####################################################
169.6 MiB/s
done
Bytes transferred = 4446702 (43d9ee hex)
malta # bootm
## Booting kernel from Legacy Image at 81000000 ...
Image Name: Linux-6.0.0-rc6+
Created: 2022-10-04 10:23:27 UTC
Image Type: MIPS Linux Kernel Image (gzip compressed)
Data Size: 4446638 Bytes = 4.2 MiB
Load Address: 80100000
Entry Point: 8054939c
Verifying Checksum ... OK
Uncompressing Kernel Image
[ 0.000000] Linux version 6.0.0-rc6+ (zx2c4@thinkpad) (mips-linux-musl-gcc (GCC) 11.2.1 20211120, GNU ld (GNU Binutils) 2.37) #5 SMP PREEMPT Fri Jun 5 15:58:00 CEST 2015
[ 0.000000] earlycon: uart8250 at I/O port 0x3f8 (options '38400n8')
[ 0.000000] printk: bootconsole [uart8250] enabled
[ 0.000000] Config serial console: console=ttyS0,38400n8r
[ 0.000000] MIPS CPS SMP unable to proceed without a CM
[ 0.000000] CPU0 revision is: 00019300 (MIPS 24Kc)
[ 0.000000] FPU revision is: 00739300
[ 0.000000] OF: fdt: No chosen node found, continuing without
[ 0.000000] OF: fdt: Ignoring memory range 0x100000000 - 0x17ffff000
[ 0.000000] MIPS: machine is mti,malta
[ 0.000000] Software DMA cache coherency enabled
[ 0.000000] Initrd not found or empty - disabling initrd
[ 0.000000] Primary instruction cache 2kB, VIPT, 2-way, linesize 16 bytes.
[ 0.000000] Primary data cache 2kB, 2-way, VIPT, no aliases, linesize 16 bytes
[ 0.000000] Zone ranges:
[ 0.000000] DMA [mem 0x0000000000000000-0x0000000000ffffff]
[ 0.000000] Normal [mem 0x0000000001000000-0x000000001fffffff]
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000000000000-0x000000000fffffff]
[ 0.000000] node 0: [mem 0x0000000090000000-0x00000000ffffefff]
[ 0.000000] Initmem setup node 0 [mem 0x0000000000000000-0x00000000ffffefff]
[ 0.000000] SARU seed: 00000000: 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f 50 ABCDEFGHIJKLMNOP
[ 0.000000] SARU seed: 00000010: 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f 60 QRSTUVWXYZ[\]^_`
[ 0.000000] random: crng init done
...
So, as you can see, it works perfectly. Thus, setting this in QEMU
follows *exactly* *the* *same* *pattern* as every other architecture
that allows for this kind of mechanism. There's nothing weird or unusual
or out of place happening here.
Hope this helps clarify.
Regards,
Jason
On Tue, 4 Oct 2022 at 11:40, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > And just to give you some idea that this truly is possible from firmware > and I'm not just making it up, consider this patch to U-Boot: > > u-boot: > diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c > index cab8da4860..27f3ee68c0 100644 > --- a/arch/mips/lib/bootm.c > +++ b/arch/mips/lib/bootm.c > @@ -211,6 +211,8 @@ static void linux_env_legacy(bootm_headers_t *images) > sprintf(env_buf, "%un8r", gd->baudrate); > linux_env_set("modetty0", env_buf); > } > + > + linux_env_set("rngseed", "4142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f60"); > } > > > So, as you can see, it works perfectly. Thus, setting this in QEMU > follows *exactly* *the* *same* *pattern* as every other architecture > that allows for this kind of mechanism. There's nothing weird or unusual > or out of place happening here. I think the unusual thing here is that this patch isn't "this facility is implemented by u-boot [commit whatever, docs whatever], and here is the patch adding it to QEMU's handling of the same interface". That is, for boards like Malta the general expectation is that we're emulating a piece of real hardware and the firmware/bootloader that it would be running, so "this is a patch that implements an interface that the real bootloader doesn't have" is a bit odd. thanks -- PMM
On Tue, Oct 4, 2022 at 12:53 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 4 Oct 2022 at 11:40, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > And just to give you some idea that this truly is possible from firmware > > and I'm not just making it up, consider this patch to U-Boot: > > > > u-boot: > > diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c > > index cab8da4860..27f3ee68c0 100644 > > --- a/arch/mips/lib/bootm.c > > +++ b/arch/mips/lib/bootm.c > > @@ -211,6 +211,8 @@ static void linux_env_legacy(bootm_headers_t *images) > > sprintf(env_buf, "%un8r", gd->baudrate); > > linux_env_set("modetty0", env_buf); > > } > > + > > + linux_env_set("rngseed", "4142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f60"); > > } > > > > > > > So, as you can see, it works perfectly. Thus, setting this in QEMU > > follows *exactly* *the* *same* *pattern* as every other architecture > > that allows for this kind of mechanism. There's nothing weird or unusual > > or out of place happening here. > > I think the unusual thing here is that this patch isn't > "this facility is implemented by u-boot [commit whatever, > docs whatever], and here is the patch adding it to QEMU's > handling of the same interface". That is, for boards like > Malta the general expectation is that we're emulating > a piece of real hardware and the firmware/bootloader > that it would be running, so "this is a patch that > implements an interface that the real bootloader doesn't > have" is a bit odd. Except it's not different from other platforms that get bootloader seeds as such. A bootloader can easily pass this; QEMU most certainly should pass this. (I sincerely hope you're not arguing in favor of holding back progress in this area for yet another decade.) Jason
On Tue, Oct 4, 2022 at 12:56 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > So, as you can see, it works perfectly. Thus, setting this in QEMU > > > follows *exactly* *the* *same* *pattern* as every other architecture > > > that allows for this kind of mechanism. There's nothing weird or unusual > > > or out of place happening here. > > > > I think the unusual thing here is that this patch isn't > > "this facility is implemented by u-boot [commit whatever, > > docs whatever], and here is the patch adding it to QEMU's > > handling of the same interface". That is, for boards like > > Malta the general expectation is that we're emulating > > a piece of real hardware and the firmware/bootloader > > that it would be running, so "this is a patch that > > implements an interface that the real bootloader doesn't > > have" is a bit odd. > > Except it's not different from other platforms that get bootloader > seeds as such. A bootloader can easily pass this; QEMU most certainly > should pass this. (I sincerely hope you're not arguing in favor of > holding back progress in this area for yet another decade.) Also, in case you've missed the actual context of this patch, it happens for `-kernel ...`. So we're now strictly in the realm of QEMU things. This is how things work on all platforms. If you use `-kernel`, then QEMU will put things in the right place to directly boot the kernel, or pass some prepared blob to an option ROM, or whatever else. This isn't related to running `-bios u-boot.bin`, except insofar as U-Boot can implement the same thing, as I've shown. Jason
On Tue, 4 Oct 2022 at 11:56, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Tue, Oct 4, 2022 at 12:53 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Tue, 4 Oct 2022 at 11:40, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > I think the unusual thing here is that this patch isn't > > "this facility is implemented by u-boot [commit whatever, > > docs whatever], and here is the patch adding it to QEMU's > > handling of the same interface". That is, for boards like > > Malta the general expectation is that we're emulating > > a piece of real hardware and the firmware/bootloader > > that it would be running, so "this is a patch that > > implements an interface that the real bootloader doesn't > > have" is a bit odd. > > Except it's not different from other platforms that get bootloader > seeds as such. A bootloader can easily pass this; QEMU most certainly > should pass this. (I sincerely hope you're not arguing in favor of > holding back progress in this area for yet another decade.) What I'm asking, I guess, is why you're messing with this board model at all if you haven't added this functionality to u-boot. This is just an emulation of an ancient bit of MIPS hardware, which nobody really cares about very much I hope. I'm not saying this is a bad patch -- I'm just saying that QEMU should not be in the business of defining bootloader-to-kernel interfaces if it can avoid it, so usually the expectation is that we are just implementing interfaces that are already defined, documented and implemented by a real bootloader and kernel. [from your other mail] > Also, in case you've missed the actual context of this patch, it > happens for `-kernel ...`. So we're now strictly in the realm of QEMU > things. -kernel generally means "emulate the platform's boot loader": it is exactly because we do not want to be in a realm of QEMU-defined interfaces that we try to follow what the platform boot loader does rather than defining new interfaces. That's not always possible or the right thing, but it's usually the cleaner way to go. thanks -- PMM
On Tue, Oct 4, 2022 at 1:03 PM Peter Maydell <peter.maydell@linaro.org> wrote: > What I'm asking, I guess, is why you're messing with this board > model at all if you haven't added this functionality to u-boot. > This is just an emulation of an ancient bit of MIPS hardware, which > nobody really cares about very much I hope. I think most people emulating MIPS would disagree. This is basically a reference platform for most intents and purposes. As I mentioned, this involves `-kernel` -- the thing that's used when you explicitly opt-in to not using a bootloader, so when you sign up for QEMU arranging the kernel image and its environment. Neglecting to pass an RNG seed would be a grave mistake. > I'm not saying this is a bad patch -- I'm just saying that > QEMU should not be in the business of defining bootloader-to-kernel > interfaces if it can avoid it, so usually the expectation is > that we are just implementing interfaces that are already > defined, documented and implemented by a real bootloader and kernel. Except that's not really the way things have ever worked here. The kernel now has the "rngseed" env var functionality, which is useful for a variety of scenarios -- kexec, firmware, and *most importantly* for QEMU. Don't block progress here. > -kernel generally means "emulate the platform's boot loader" And here, a platform bootloader could pass this, just as is the case with m68k's BI_RNG_SEED or x86's setup_data RNG SEED or device tree's rng-seed or EFI's LINUX_EFI_RANDOM_SEED_TABLE_GUID or MIPS' "rngseed" fw environment variable. These are important facilities to have. Bootloaders and hypervisors alike must implement them. *Do not block progress here.* Jason
On Tue, 4 Oct 2022, Jason A. Donenfeld wrote: > On Tue, Oct 4, 2022 at 1:03 PM Peter Maydell <peter.maydell@linaro.org> wrote: >> What I'm asking, I guess, is why you're messing with this board >> model at all if you haven't added this functionality to u-boot. >> This is just an emulation of an ancient bit of MIPS hardware, which >> nobody really cares about very much I hope. > > I think most people emulating MIPS would disagree. This is basically a > reference platform for most intents and purposes. As I mentioned, this > involves `-kernel` -- the thing that's used when you explicitly opt-in > to not using a bootloader, so when you sign up for QEMU arranging the > kernel image and its environment. Neglecting to pass an RNG seed would > be a grave mistake. > >> I'm not saying this is a bad patch -- I'm just saying that >> QEMU should not be in the business of defining bootloader-to-kernel >> interfaces if it can avoid it, so usually the expectation is >> that we are just implementing interfaces that are already >> defined, documented and implemented by a real bootloader and kernel. > > Except that's not really the way things have ever worked here. The > kernel now has the "rngseed" env var functionality, which is useful > for a variety of scenarios -- kexec, firmware, and *most importantly* > for QEMU. Don't block progress here. > >> -kernel generally means "emulate the platform's boot loader" > > And here, a platform bootloader could pass this, just as is the case > with m68k's BI_RNG_SEED or x86's setup_data RNG SEED or device tree's > rng-seed or EFI's LINUX_EFI_RANDOM_SEED_TABLE_GUID or MIPS' "rngseed" > fw environment variable. These are important facilities to have. > Bootloaders and hypervisors alike must implement them. *Do not block > progress here.* Cool dowm. Peter does not want to block progress here. What he said was that the malta is (or should be) emulating a real piece of hardware so adding some stuff to it which is not on that real hardware may not be preferred. If you want to experiment with generic mips hardware maybe you need a virt board instead that is free from such restrictions to emulate a real hardware. Some archs already have such board and there seems to be loongson3-virt but no generic mips virt machine yet. Defining and implementing such board may be more than you want to do for this but maybe that would be a better way to go. Regards, BALATON Zoltan
On Tue, Oct 4, 2022 at 1:39 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > > On Tue, 4 Oct 2022, Jason A. Donenfeld wrote: > > On Tue, Oct 4, 2022 at 1:03 PM Peter Maydell <peter.maydell@linaro.org> wrote: > >> What I'm asking, I guess, is why you're messing with this board > >> model at all if you haven't added this functionality to u-boot. > >> This is just an emulation of an ancient bit of MIPS hardware, which > >> nobody really cares about very much I hope. > > > > I think most people emulating MIPS would disagree. This is basically a > > reference platform for most intents and purposes. As I mentioned, this > > involves `-kernel` -- the thing that's used when you explicitly opt-in > > to not using a bootloader, so when you sign up for QEMU arranging the > > kernel image and its environment. Neglecting to pass an RNG seed would > > be a grave mistake. > > > >> I'm not saying this is a bad patch -- I'm just saying that > >> QEMU should not be in the business of defining bootloader-to-kernel > >> interfaces if it can avoid it, so usually the expectation is > >> that we are just implementing interfaces that are already > >> defined, documented and implemented by a real bootloader and kernel. > > > > Except that's not really the way things have ever worked here. The > > kernel now has the "rngseed" env var functionality, which is useful > > for a variety of scenarios -- kexec, firmware, and *most importantly* > > for QEMU. Don't block progress here. > > > >> -kernel generally means "emulate the platform's boot loader" > > > > And here, a platform bootloader could pass this, just as is the case > > with m68k's BI_RNG_SEED or x86's setup_data RNG SEED or device tree's > > rng-seed or EFI's LINUX_EFI_RANDOM_SEED_TABLE_GUID or MIPS' "rngseed" > > fw environment variable. These are important facilities to have. > > Bootloaders and hypervisors alike must implement them. *Do not block > > progress here.* > > Cool dowm. Peter does not want to block progress here. What he said was > that the malta is (or should be) emulating a real piece of hardware so > adding some stuff to it which is not on that real hardware may not be > preferred. If you want to experiment with generic mips hardware maybe you > need a virt board instead that is free from such restrictions to emulate a > real hardware. Some archs already have such board and there seems to be > loongson3-virt but no generic mips virt machine yet. Defining and > implementing such board may be more than you want to do for this but maybe > that would be a better way to go. This is the bikeshed suggestion that puts along the path of nothing ever getting done. This is an interface that's available for real firmware; there's no reason not to implement it here. It's the same situation as the MIPS boston board setting the rng-seed device tree property. There's nothing new or unusual about this, and it fits with how things work elsewhere on the architecture and QEMU at large. Besides, "malta" is the de facto platform used for emulating MIPS. Again, this is obvious progress blocking in action. Look how it's done elsewhere; look at how it's done in this patch; there's no difference. This patch is boring and unoffensive. We don't need to waste time bikeshedding it. Jason
diff --git a/hw/mips/malta.c b/hw/mips/malta.c index 0e932988e0..9d793b3c17 100644 --- a/hw/mips/malta.c +++ b/hw/mips/malta.c @@ -26,6 +26,7 @@ #include "qemu/units.h" #include "qemu/bitops.h" #include "qemu/datadir.h" +#include "qemu/guest-random.h" #include "hw/clock.h" #include "hw/southbridge/piix.h" #include "hw/isa/superio.h" @@ -1017,6 +1018,17 @@ static void G_GNUC_PRINTF(3, 4) prom_set(uint32_t *prom_buf, int index, va_end(ap); } +static void reinitialize_rng_seed(void *opaque) +{ + char *rng_seed_hex = opaque; + uint8_t rng_seed[32]; + + qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed)); + for (size_t i = 0; i < sizeof(rng_seed); ++i) { + sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]); + } +} + /* Kernel */ static uint64_t load_kernel(void) { @@ -1028,6 +1040,8 @@ static uint64_t load_kernel(void) long prom_size; int prom_index = 0; uint64_t (*xlate_to_kseg0) (void *opaque, uint64_t addr); + uint8_t rng_seed[32]; + char rng_seed_hex[sizeof(rng_seed) * 2 + 1]; #if TARGET_BIG_ENDIAN big_endian = 1; @@ -1115,9 +1129,20 @@ static uint64_t load_kernel(void) prom_set(prom_buf, prom_index++, "modetty0"); prom_set(prom_buf, prom_index++, "38400n8r"); + + qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed)); + for (size_t i = 0; i < sizeof(rng_seed); ++i) { + sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]); + } + prom_set(prom_buf, prom_index++, "rngseed"); + prom_set(prom_buf, prom_index++, "%s", rng_seed_hex); + prom_set(prom_buf, prom_index++, NULL); rom_add_blob_fixed("prom", prom_buf, prom_size, ENVP_PADDR); + qemu_register_reset(reinitialize_rng_seed, + memmem(rom_ptr(ENVP_PADDR, prom_size), prom_size, + rng_seed_hex, sizeof(rng_seed_hex))); g_free(prom_buf); return kernel_entry;
As of the kernel commit linked below, Linux ingests an RNG seed passed from the hypervisor. So, pass this for the Malta platform, and reinitialize it on reboot too, so that it's always fresh. Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> Cc: Aurelien Jarno <aurelien@aurel32.net> Link: https://git.kernel.org/mips/c/056a68cea01 Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- Changes v1->v2: - Update commit message. - No code changes. hw/mips/malta.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)