Re: [PATCH] hw/riscv: virt: pass random seed to fdt

[Jason A. Donenfeld]

Hi Alistair,

On Mon, Jul 11, 2022 at 01:36:28PM +1000, Alistair Francis wrote:
> On Mon, Jul 11, 2022 at 10:28 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > On 7/11/22, Alistair Francis <alistair23@gmail.com> wrote:
> > > On Fri, Jul 8, 2022 at 7:56 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >>
> > >> Hi Alistair,
> > >>
> > >> On 7/8/22, Alistair Francis <alistair23@gmail.com> wrote:
> > >>
> > >> >> > but I think that's just the way things go unfortunately.
> > >> >
> > >> > Hmm... That's a pain. So there is a bug in older kernels where they
> > >> > won't boot if we specify this?
> > >> >
> > >> > Can you point to the fixes?
> > >>
> > >> Actually, in trying to reproduce this, I don't actually think this is
> > >> affected by those old random.c bugs.
> > >>
> > >>
> > >> >> As for your 5.8 issue, I've been trying to reproduce that to
> > >> >> understand
> > >> >> more about it, but I'm unable to. I've been trying with
> > >> >> nommu_virt_defconfig using my patch ontop of qemu master. Maybe it's
> > >> >> possible in testing this out you were testing the wrong branch?
> > >> >> Anyway,
> > >> >> it'd be nice to get this queued up...
> > >> >
> > >> > Hmm... you can't reproduce it?
> > >>
> > >> No, I can't, and I'm now no longer convinced that there *is* a bug.
> > >> Can you try to repro again and send me detailed reproduction steps?
> > >
> > > I just checked again and I can confirm it is this patch that causes
> > > the regression.
> > >
> > > This is the command line:
> > >
> > > qemu-system-riscv64 \
> > > -machine virt -m 64M \
> > > -cpu rv64,mmu=false \
> > > -serial mon:stdio -serial null -nographic \
> > > -append "root=/dev/vda rw highres=off  console=ttyS0 mem=1G ip=dhcp
> > > earlycon=sbi" \
> > > -device virtio-net-device,netdev=net0,mac=52:54:00:12:34:02 -netdev
> > > user,id=net0 \
> > > -object rng-random,filename=/dev/urandom,id=rng0 -device
> > > virtio-rng-device,rng=rng0 \
> > > -smp 1 -d guest_errors \
> > > -kernel ./images/qemuriscv64/nommu-Image \
> > > -drive
> > > id=disk0,file=./images/qemuriscv64/nommu-rootfs.ext2,if=none,format=raw \
> > > -device virtio-blk-device,drive=disk0 -bios none
> > >
> > > You can access the images from:
> > > https://nextcloud.alistair23.me/index.php/s/a2zrtbT7DjdTx9t
> > >
> >
> > Thanks. Can you send the kernel . config too?
> 
> Unfortunately I don't have it, it should just be the 5.8 no MMU defconfig 
> though

I've reproduced the problem and determined the root cause. This is a
generic issue with the mmio get_cycles() implementation before 5.9 on
no-MMU configs, which was fixed during the 5.9 cycle. I don't believe
that this is the only thing affected on that .0 kernel, where fixes were
ostensibly backported. Given the relative age of risc-v, the fact that
5.8.0 was broken anyway, and that likely nobody is using this kernel in
that configuration without applying updates, I'm pretty sure my patch is
safe to apply. I'd recommend updating the broken kernel in your CI.

Meanwhile, the rng-seed field is part of the DT spec. Holding back the
(virtual) hardware just because some random dot-zero non-LTS release had
a quickly fixed bug seems ridiculous, and the way in which progress gets
held up, hacks accumulate, and generally nothing good gets done. It will
only hamper security, functionality, and boot speed, while helping no
real practical case that can't be fixed in a better way.

So I believe you should apply the rng-seed commit so that the RISC-V
machine honors that DT field.

Regards,
Jason