[PULL 0/9] More fixes + random seed patches for QEMU 7.1

[PULL 0/9] More fixes + random seed patches for QEMU 7.1

[Paolo Bonzini]

The following changes since commit 8ec4bc3c8c09366a9e4859de7c0a1860911e8424:

  Merge tag 'net-pull-request' of https://github.com/jasowang/qemu into staging (2022-07-20 16:27:57 +0100)

are available in the Git repository at:

  https://gitlab.com/bonzini/qemu.git tags/for-upstream2

for you to fetch changes up to c0a0f1b6524abfb45c175fc0dbf2c0eed073b491:

  hw/i386: pass RNG seed via setup_data entry (2022-07-21 15:08:21 +0200)

----------------------------------------------------------------
* Bug fixes
* Pass random seed to x86 and other FDT platforms

----------------------------------------------------------------

I included the random seed patches because the feature is already
supported in the ppc and ARM pull requests; it makes little sense to
limit to a subset of the platforms since the code is basically the same
(except for x86).

Alexander Bulekov (1):
      oss-fuzz: ensure base_copy is a generic-fuzzer

Bin Meng (1):
      docs: Add caveats for Windows as the build platform

Jason A. Donenfeld (5):
      hw/nios2: virt: pass random seed to fdt
      hw/mips: boston: pass random seed to fdt
      hw/guest-loader: pass random seed to fdt
      hw/rx: pass random seed to fdt
      hw/i386: pass RNG seed via setup_data entry

Paolo Bonzini (1):
      oss-fuzz: remove binaries from qemu-bundle tree

Peter Maydell (1):
      accel/kvm: Avoid Coverity warning in query_stats()

 accel/kvm/kvm-all.c                          |  2 +-
 docs/about/build-platforms.rst               | 10 +++++++++-
 hw/core/guest-loader.c                       |  5 +++++
 hw/i386/microvm.c                            |  2 +-
 hw/i386/pc.c                                 |  4 ++--
 hw/i386/pc_piix.c                            |  2 ++
 hw/i386/pc_q35.c                             |  2 ++
 hw/i386/x86.c                                | 26 ++++++++++++++++++++++----
 hw/mips/boston.c                             |  5 +++++
 hw/nios2/boot.c                              |  5 +++++
 hw/rx/rx-gdbsim.c                            |  4 ++++
 include/hw/i386/pc.h                         |  3 +++
 include/hw/i386/x86.h                        |  3 ++-
 include/standard-headers/asm-x86/bootparam.h |  1 +
 scripts/oss-fuzz/build.sh                    |  6 ++++--
 15 files changed, 68 insertions(+), 12 deletions(-)
-- 
2.36.1

[PULL 1/9] docs: Add caveats for Windows as the build platform

[Paolo Bonzini]

From: Bin Meng <bin.meng@windriver.com>

Commit cf60ccc3306c ("cutils: Introduce bundle mechanism") introduced
a Python script to populate a bundle directory using os.symlink() to
point to the binaries in the pc-bios directory of the source tree.
Commit 882084a04ae9 ("datadir: Use bundle mechanism") removed previous
logic in pc-bios/meson.build to create a link/copy of pc-bios binaries
in the build tree so os.symlink() is the way to go.

However os.symlink() may fail [1] on Windows if an unprivileged Windows
user started the QEMU build process, which results in QEMU executables
generated in the build tree not able to load the default BIOS/firmware
images due to symbolic links not present in the bundle directory.

This commits updates the documentation by adding such caveats for users
who want to build QEMU on the Windows platform.

[1] https://docs.python.org/3/library/os.html#os.symlink

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Message-Id: <20220719135014.764981-1-bmeng.cn@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 docs/about/build-platforms.rst | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/docs/about/build-platforms.rst b/docs/about/build-platforms.rst
index ebde20f981..6b8496c430 100644
--- a/docs/about/build-platforms.rst
+++ b/docs/about/build-platforms.rst
@@ -94,8 +94,16 @@ not tested anymore, so it is recommended to use one of the latest versions of
 Windows instead.
 
 The project supports building QEMU with current versions of the MinGW
-toolchain, either hosted on Linux (Debian/Fedora) or via MSYS2 on Windows.
+toolchain, either hosted on Linux (Debian/Fedora) or via `MSYS2`_ on Windows.
+A more recent Windows version is always preferred as it is less likely to have
+problems with building via MSYS2. The building process of QEMU involves some
+Python scripts that call os.symlink() which needs special attention for the
+build process to successfully complete. On newer versions of Windows 10,
+unprivileged accounts can create symlinks if Developer Mode is enabled.
+When Developer Mode is not available/enabled, the SeCreateSymbolicLinkPrivilege
+privilege is required, or the process must be run as an administrator.
 
 .. _Homebrew: https://brew.sh/
 .. _MacPorts: https://www.macports.org/
+.. _MSYS2: https://www.msys2.org/
 .. _Repology: https://repology.org/
-- 
2.36.1

[PULL 2/9] accel/kvm: Avoid Coverity warning in query_stats()

[Paolo Bonzini]

From: Peter Maydell <peter.maydell@linaro.org>

Coverity complains that there is a codepath in the query_stats()
function where it can leak the memory pointed to by stats_list.  This
can only happen if the caller passes something other than
STATS_TARGET_VM or STATS_TARGET_VCPU as the 'target', which no
callsite does.  Enforce this assumption using g_assert_not_reached(),
so that if we have a future bug we hit the assert rather than
silently leaking memory.

Resolves: Coverity CID 1490140
Fixes: cc01a3f4cadd91e6 ("kvm: Support for querying fd-based stats")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20220719134853.327059-1-peter.maydell@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ed8b6b896e..eb7fceb336 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3980,7 +3980,7 @@ static void query_stats(StatsResultList **result, StatsTarget target,
                         stats_list);
         break;
     default:
-        break;
+        g_assert_not_reached();
     }
 }
 
-- 
2.36.1

[PULL 3/9] oss-fuzz: remove binaries from qemu-bundle tree

[Paolo Bonzini]

oss-fuzz is finding possible fuzzing targets even under qemu-bundle/.../bin, but they
cannot be used because the required shared libraries are missing.  Since the
fuzzing targets are already placed manually in $OUT, the bindir and libexecdir
subtrees are not needed; remove them.

Cc: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/oss-fuzz/build.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh
index 2656a89aea..5ee9141e3e 100755
--- a/scripts/oss-fuzz/build.sh
+++ b/scripts/oss-fuzz/build.sh
@@ -87,8 +87,10 @@ if [ "$GITLAB_CI" != "true" ]; then
     make "-j$(nproc)" qemu-fuzz-i386 V=1
 fi
 
-# Prepare a preinstalled tree
+# Place data files in the preinstall tree
 make install DESTDIR=$DEST_DIR/qemu-bundle
+rm -rf $DEST_DIR/qemu-bundle/opt/qemu-oss-fuzz/bin
+rm -rf $DEST_DIR/qemu-bundle/opt/qemu-oss-fuzz/libexec
 
 targets=$(./qemu-fuzz-i386 | awk '$1 ~ /\*/  {print $2}')
 base_copy="$DEST_DIR/qemu-fuzz-i386-target-$(echo "$targets" | head -n 1)"
-- 
2.36.1

[PULL 4/9] oss-fuzz: ensure base_copy is a generic-fuzzer

[Paolo Bonzini]

From: Alexander Bulekov <alxndr@bu.edu>

Depending on how the target list is sorted in by qemu, the first target
(used as the base copy of the fuzzer, to which all others are linked)
might not be a generic-fuzzer. Since we are trying to only use
generic-fuzz, on oss-fuzz, fix that, to ensure the base copy is a
generic-fuzzer.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20220720180946.2264253-1-alxndr@bu.edu>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/oss-fuzz/build.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh
index 5ee9141e3e..3bda0d72c7 100755
--- a/scripts/oss-fuzz/build.sh
+++ b/scripts/oss-fuzz/build.sh
@@ -92,7 +92,7 @@ make install DESTDIR=$DEST_DIR/qemu-bundle
 rm -rf $DEST_DIR/qemu-bundle/opt/qemu-oss-fuzz/bin
 rm -rf $DEST_DIR/qemu-bundle/opt/qemu-oss-fuzz/libexec
 
-targets=$(./qemu-fuzz-i386 | awk '$1 ~ /\*/  {print $2}')
+targets=$(./qemu-fuzz-i386 | grep generic-fuzz | awk '$1 ~ /\*/  {print $2}')
 base_copy="$DEST_DIR/qemu-fuzz-i386-target-$(echo "$targets" | head -n 1)"
 
 cp "./qemu-fuzz-i386" "$base_copy"
-- 
2.36.1

[PULL 5/9] hw/nios2: virt: pass random seed to fdt

[Paolo Bonzini]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>

If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
initialize early. Set this using the usual guest random number
generation function. This FDT node is part of the DT specification.

Cc: Chris Wulff <crwulff@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Message-Id: <20220719120113.118034-1-Jason@zx2c4.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/nios2/boot.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
index 07b8d87633..21cbffff47 100644
--- a/hw/nios2/boot.c
+++ b/hw/nios2/boot.c
@@ -34,6 +34,7 @@
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
+#include "qemu/guest-random.h"
 #include "sysemu/device_tree.h"
 #include "sysemu/reset.h"
 #include "hw/boards.h"
@@ -83,6 +84,7 @@ static int nios2_load_dtb(struct nios2_boot_info bi, const uint32_t ramsize,
     int fdt_size;
     void *fdt = NULL;
     int r;
+    uint8_t rng_seed[32];
 
     if (dtb_filename) {
         fdt = load_device_tree(dtb_filename, &fdt_size);
@@ -91,6 +93,9 @@ static int nios2_load_dtb(struct nios2_boot_info bi, const uint32_t ramsize,
         return 0;
     }
 
+    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
+    qemu_fdt_setprop(fdt, "/chosen", "rng-seed", rng_seed, sizeof(rng_seed));
+
     if (kernel_cmdline) {
         r = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
                                     kernel_cmdline);
-- 
2.36.1

[PULL 6/9] hw/mips: boston: pass random seed to fdt

[Paolo Bonzini]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>

If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
initialize early. Set this using the usual guest random number
generation function. This FDT node is part of the DT specification.

I'd do the same for other MIPS platforms but boston is the only one that
seems to use FDT.

Cc: Paul Burton <paulburton@kernel.org>
Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Message-Id: <20220719120843.134392-1-Jason@zx2c4.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mips/boston.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 1debca18ec..d2ab9da1a0 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -34,6 +34,7 @@
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "qemu/guest-random.h"
 #include "qemu/log.h"
 #include "chardev/char.h"
 #include "sysemu/device_tree.h"
@@ -363,6 +364,7 @@ static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
     size_t ram_low_sz, ram_high_sz;
     size_t fdt_sz = fdt_totalsize(fdt_orig) * 2;
     g_autofree void *fdt = g_malloc0(fdt_sz);
+    uint8_t rng_seed[32];
 
     err = fdt_open_into(fdt_orig, fdt, fdt_sz);
     if (err) {
@@ -370,6 +372,9 @@ static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
         return NULL;
     }
 
+    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
+    qemu_fdt_setprop(fdt, "/chosen", "rng-seed", rng_seed, sizeof(rng_seed));
+
     cmdline = (machine->kernel_cmdline && machine->kernel_cmdline[0])
             ? machine->kernel_cmdline : " ";
     err = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
-- 
2.36.1

[PULL 7/9] hw/guest-loader: pass random seed to fdt

[Paolo Bonzini]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>

If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
initialize early. Set this using the usual guest random number
generation function. This FDT node is part of the DT specification.

Cc: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Message-Id: <20220719121559.135355-1-Jason@zx2c4.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/guest-loader.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
index 391c875a29..4f8572693c 100644
--- a/hw/core/guest-loader.c
+++ b/hw/core/guest-loader.c
@@ -31,6 +31,7 @@
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/guest-random.h"
 #include "guest-loader.h"
 #include "sysemu/device_tree.h"
 #include "hw/boards.h"
@@ -46,6 +47,7 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
     g_autofree char *node = g_strdup_printf("/chosen/module@0x%08" PRIx64,
                                             s->addr);
     uint64_t reg_attr[2] = {cpu_to_be64(s->addr), cpu_to_be64(size)};
+    uint8_t rng_seed[32];
 
     if (!fdt) {
         error_setg(errp, "Cannot modify FDT fields if the machine has none");
@@ -55,6 +57,9 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
     qemu_fdt_add_subnode(fdt, node);
     qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
 
+    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
+    qemu_fdt_setprop(fdt, node, "rng-seed", rng_seed, sizeof(rng_seed));
+
     if (s->kernel) {
         const char *compat[2] = { "multiboot,module", "multiboot,kernel" };
         if (qemu_fdt_setprop_string_array(fdt, node, "compatible",
-- 
2.36.1

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
>
> If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
> initialize early. Set this using the usual guest random number
> generation function. This FDT node is part of the DT specification.
>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Message-Id: <20220719121559.135355-1-Jason@zx2c4.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/guest-loader.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
> index 391c875a29..4f8572693c 100644
> --- a/hw/core/guest-loader.c
> +++ b/hw/core/guest-loader.c
> @@ -31,6 +31,7 @@
>  #include "hw/qdev-properties.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include "qemu/guest-random.h"
>  #include "guest-loader.h"
>  #include "sysemu/device_tree.h"
>  #include "hw/boards.h"
> @@ -46,6 +47,7 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
>      g_autofree char *node = g_strdup_printf("/chosen/module@0x%08" PRIx64,
>                                              s->addr);
>      uint64_t reg_attr[2] = {cpu_to_be64(s->addr), cpu_to_be64(size)};
> +    uint8_t rng_seed[32];
>  
>      if (!fdt) {
>          error_setg(errp, "Cannot modify FDT fields if the machine has none");
> @@ -55,6 +57,9 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
>      qemu_fdt_add_subnode(fdt, node);
>      qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
>  
> +    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
> +    qemu_fdt_setprop(fdt, node, "rng-seed", rng_seed, sizeof(rng_seed));
> +

Why are we inserting this here? The guest-loader is only building on
what the machine type has already constructed which in the case of -M
virt for riscv and ARM already has code for this.

>      if (s->kernel) {
>          const char *compat[2] = { "multiboot,module", "multiboot,kernel" };
>          if (qemu_fdt_setprop_string_array(fdt, node, "compatible",


-- 
Alex Bennée

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Jason A. Donenfeld]

Hi Alex,

On Thu, Jul 21, 2022 at 08:36:10PM +0100, Alex Bennée wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> >
> > If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
> > initialize early. Set this using the usual guest random number
> > generation function. This FDT node is part of the DT specification.
> >
> > Cc: Alex Bennée <alex.bennee@linaro.org>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > Message-Id: <20220719121559.135355-1-Jason@zx2c4.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/core/guest-loader.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
> > index 391c875a29..4f8572693c 100644
> > --- a/hw/core/guest-loader.c
> > +++ b/hw/core/guest-loader.c
> > @@ -31,6 +31,7 @@
> >  #include "hw/qdev-properties.h"
> >  #include "qapi/error.h"
> >  #include "qemu/module.h"
> > +#include "qemu/guest-random.h"
> >  #include "guest-loader.h"
> >  #include "sysemu/device_tree.h"
> >  #include "hw/boards.h"
> > @@ -46,6 +47,7 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
> >      g_autofree char *node = g_strdup_printf("/chosen/module@0x%08" PRIx64,
> >                                              s->addr);
> >      uint64_t reg_attr[2] = {cpu_to_be64(s->addr), cpu_to_be64(size)};
> > +    uint8_t rng_seed[32];
> >  
> >      if (!fdt) {
> >          error_setg(errp, "Cannot modify FDT fields if the machine has none");
> > @@ -55,6 +57,9 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
> >      qemu_fdt_add_subnode(fdt, node);
> >      qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
> >  
> > +    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
> > +    qemu_fdt_setprop(fdt, node, "rng-seed", rng_seed, sizeof(rng_seed));
> > +
> 
> Why are we inserting this here? The guest-loader is only building on
> what the machine type has already constructed which in the case of -M
> virt for riscv and ARM already has code for this.

Wish you would have replied to the list patch before Paolo queued it.

I don't know this mechanism well but I assumed it would pass a unique
seed to each chained loader. Let me know if I'm misunderstanding the
purpose; I have no qualms about dropping this patch.

Jason

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Alex Bennée]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> Hi Alex,
>
> On Thu, Jul 21, 2022 at 08:36:10PM +0100, Alex Bennée wrote:
>> 
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
<snip>
>> >      uint64_t reg_attr[2] = {cpu_to_be64(s->addr), cpu_to_be64(size)};
>> > +    uint8_t rng_seed[32];
>> >  
>> >      if (!fdt) {
>> >          error_setg(errp, "Cannot modify FDT fields if the machine has none");
>> > @@ -55,6 +57,9 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
>> >      qemu_fdt_add_subnode(fdt, node);
>> >      qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
>> >  
>> > +    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
>> > +    qemu_fdt_setprop(fdt, node, "rng-seed", rng_seed, sizeof(rng_seed));
>> > +
>> 
>> Why are we inserting this here? The guest-loader is only building on
>> what the machine type has already constructed which in the case of -M
>> virt for riscv and ARM already has code for this.
>
> Wish you would have replied to the list patch before Paolo queued it.

I'm sorry if I didn't get to it in the *checks notes* 2 days since it
was posted. I've been on holiday.

> I don't know this mechanism well but I assumed it would pass a unique
> seed to each chained loader. Let me know if I'm misunderstanding the
> purpose; I have no qualms about dropping this patch.

All the guest-loader does is add the information about where in memory a
guest and/or it's initrd have been placed in memory to the DTB. It's
entirely up to the initial booted code (usually a hypervisor in this
case) to decide what gets passed up the chain to any subsequent guests.

-- 
Alex Bennée

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Jason A. Donenfeld]

Hey Alex,

On Fri, Jul 22, 2022 at 10:45:19AM +0100, Alex Bennée wrote:
> All the guest-loader does is add the information about where in memory a
> guest and/or it's initrd have been placed in memory to the DTB. It's
> entirely up to the initial booted code (usually a hypervisor in this
> case) to decide what gets passed up the chain to any subsequent guests.

I think that's also my understanding, but let me tell you what I was
thinking with regards to rng-seed there, and you can tell me if I'm way
off.

The guest-loader puts in memory various loaders in a multistage boot.
Let's call it stage0, stage1, stage2, and finally the kernel. Normally,
rng-seed is only given to one of these stages. That stage may or may not
pass it to the next one, and it most probably does not. And why should
it? The host is in a better position to generate these seeds, rather
than adding finicky and fragile crypto ratcheting code into each stage
bootloader. So, instead, QEMU can just give each stage its own seed, for
it to do whatever with. This way, if stage1 does nothing, at least
there's a fresh unused one available for the kernel when it finally gets
there.

Does what I describe correspond at all with the use of guest-loader? If
so, maybe this patch should stay? If not, discard it as rubbish.

Jason

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Alex Bennée]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> Hey Alex,
>
> On Fri, Jul 22, 2022 at 10:45:19AM +0100, Alex Bennée wrote:
>> All the guest-loader does is add the information about where in memory a
>> guest and/or it's initrd have been placed in memory to the DTB. It's
>> entirely up to the initial booted code (usually a hypervisor in this
>> case) to decide what gets passed up the chain to any subsequent guests.
>
> I think that's also my understanding, but let me tell you what I was
> thinking with regards to rng-seed there, and you can tell me if I'm way
> off.
>
> The guest-loader puts in memory various loaders in a multistage boot.
> Let's call it stage0, stage1, stage2, and finally the kernel. Normally,
> rng-seed is only given to one of these stages. That stage may or may not
> pass it to the next one, and it most probably does not. And why should
> it? The host is in a better position to generate these seeds, rather
> than adding finicky and fragile crypto ratcheting code into each stage
> bootloader. So, instead, QEMU can just give each stage its own seed, for
> it to do whatever with. This way, if stage1 does nothing, at least
> there's a fresh unused one available for the kernel when it finally gets
> there.

That sounds suspiciously like inventing a new ABI between QEMU and
guests which we generally try to avoid. The DTB exposed to the first
stage may never be made visible to the following stages or more likely a
sanitised version is prepared by the previous stage. Generally QEMU just
tries to get the emulation right so the firmware/software can get on
with it's thing. Indeed the dynamic DTB for -M virt and friends is an
oddity compared to most of the other machine types which assume the user
has a valid DTB.

Either way given how close to release we are I'd rather drop this patch.

> Does what I describe correspond at all with the use of guest-loader? If
> so, maybe this patch should stay? If not, discard it as rubbish.

The original intent of the guest-loader was to make testing of
hypervisors easier because the alternative is getting a multi-stage boot
chain of firmware, boot-loaders and distro specific integration working
which can be quite opaque to debug (c.f. why -kernel/-initrd exist and
not everyone boots via -bios/-pflash).

>
> Jason


-- 
Alex Bennée

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2627 bytes --]

Ok I will resend the pull request. Apologies for overstepping.

Paolo

Il ven 22 lug 2022, 16:37 Alex Bennée <alex.bennee@linaro.org> ha scritto:

>
> "Jason A. Donenfeld" <Jason@zx2c4.com> writes:
>
> > Hey Alex,
> >
> > On Fri, Jul 22, 2022 at 10:45:19AM +0100, Alex Bennée wrote:
> >> All the guest-loader does is add the information about where in memory a
> >> guest and/or it's initrd have been placed in memory to the DTB. It's
> >> entirely up to the initial booted code (usually a hypervisor in this
> >> case) to decide what gets passed up the chain to any subsequent guests.
> >
> > I think that's also my understanding, but let me tell you what I was
> > thinking with regards to rng-seed there, and you can tell me if I'm way
> > off.
> >
> > The guest-loader puts in memory various loaders in a multistage boot.
> > Let's call it stage0, stage1, stage2, and finally the kernel. Normally,
> > rng-seed is only given to one of these stages. That stage may or may not
> > pass it to the next one, and it most probably does not. And why should
> > it? The host is in a better position to generate these seeds, rather
> > than adding finicky and fragile crypto ratcheting code into each stage
> > bootloader. So, instead, QEMU can just give each stage its own seed, for
> > it to do whatever with. This way, if stage1 does nothing, at least
> > there's a fresh unused one available for the kernel when it finally gets
> > there.
>
> That sounds suspiciously like inventing a new ABI between QEMU and
> guests which we generally try to avoid. The DTB exposed to the first
> stage may never be made visible to the following stages or more likely a
> sanitised version is prepared by the previous stage. Generally QEMU just
> tries to get the emulation right so the firmware/software can get on
> with it's thing. Indeed the dynamic DTB for -M virt and friends is an
> oddity compared to most of the other machine types which assume the user
> has a valid DTB.
>
> Either way given how close to release we are I'd rather drop this patch.
>
> > Does what I describe correspond at all with the use of guest-loader? If
> > so, maybe this patch should stay? If not, discard it as rubbish.
>
> The original intent of the guest-loader was to make testing of
> hypervisors easier because the alternative is getting a multi-stage boot
> chain of firmware, boot-loaders and distro specific integration working
> which can be quite opaque to debug (c.f. why -kernel/-initrd exist and
> not everyone boots via -bios/-pflash).
>
> >
> > Jason
>
>
> --
> Alex Bennée
>
>

[-- Attachment #2: Type: text/html, Size: 3253 bytes --]

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Jason A. Donenfeld]

Hi Alex,

On Fri, Jul 22, 2022 at 4:37 PM Alex Bennée <alex.bennee@linaro.org> wrote:
> That sounds suspiciously like inventing a new ABI between QEMU and
> guests which we generally try to avoid.

Well the ABI is just the "rng-seed" param which is part of the DT
spec. But I can understand why you might find this use a bit "too
creative". So no qualms about dropping it.

Jason

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Paolo Bonzini]

On 7/21/22 22:20, Jason A. Donenfeld wrote:
>> Why are we inserting this here? The guest-loader is only building on
>> what the machine type has already constructed which in the case of -M
>> virt for riscv and ARM already has code for this.
> 
> Wish you would have replied to the list patch before Paolo queued it.

Come on.

You posted a couple patches for this work 1 week before soft freeze 
(which is when maintainer trees should be ready for merge), so that some 
platforms get the support and some don't depending on how ready they 
are for the freeze itself.

Then you post the rest of the implementation on the day of the freeze. 
This patch has a pretty bad commit message too because any discussion on 
boot loader chaining belonged there.

Your own timing was completely off, and the right thing to do would have 
been to post a single series for all machines.  This way, even if the 
patches were to go via individual trees, maintainers could coordinate on 
which version to include, on how to handle migration, and so on.

Imagine doing the same thing for Linux, you'd be either ignored until 
the merge window ends, or alternatively shouted at.  Ignoring patches 
sent so close the soft freeze was my first instinct and it would have 
been the right call, except that in the meanwhile some architecture had 
their patches merged and here we are.

If anything _I_ have to apologize to Alex for picking up the patch in 
his stead, and for bending the soft freeze rules in an attempt to avoid 
having half-assed support where some architectures export the seed and 
some don't.  But you really have no standing to complain to him about 
not replying timely.

Thanks,

Paolo

Re: [PULL 7/9] hw/guest-loader: pass random seed to fdt

[Jason A. Donenfeld]

Hi Paolo,

On Fri, Jul 22, 2022 at 02:04:45PM +0200, Paolo Bonzini wrote:
> On 7/21/22 22:20, Jason A. Donenfeld wrote:
> >> Why are we inserting this here? The guest-loader is only building on
> >> what the machine type has already constructed which in the case of -M
> >> virt for riscv and ARM already has code for this.
> > 
> > Wish you would have replied to the list patch before Paolo queued it.
> 
> Come on.
> 
> You posted a couple patches for this work 1 week before soft freeze 
> (which is when maintainer trees should be ready for merge), so that some 
> platforms get the support and some don't depending on how ready they 
> are for the freeze itself.
> 
> Then you post the rest of the implementation on the day of the freeze. 
> This patch has a pretty bad commit message too because any discussion on 
> boot loader chaining belonged there.
> 
> Your own timing was completely off, and the right thing to do would have 
> been to post a single series for all machines.  This way, even if the 
> patches were to go via individual trees, maintainers could coordinate on 
> which version to include, on how to handle migration, and so on.
> 
> Imagine doing the same thing for Linux, you'd be either ignored until 
> the merge window ends, or alternatively shouted at.  Ignoring patches 
> sent so close the soft freeze was my first instinct and it would have 
> been the right call, except that in the meanwhile some architecture had 
> their patches merged and here we are.
> 
> If anything _I_ have to apologize to Alex for picking up the patch in 
> his stead, and for bending the soft freeze rules in an attempt to avoid 
> having half-assed support where some architectures export the seed and 
> some don't.  But you really have no standing to complain to him about 
> not replying timely.

Please simmer down and quit the inane drama.

I don't have any qualms about Alex not replying in the two days before
you sent this pull. What I wish is that this was discussed on the list
before the pull so that we're now not in this awkward situation of
patch review inside of a pull. I don't know the procedures on what
happens now with that. Will this get pulled and now we have to revert?
Do you have to roll a new pull? I just have no idea, as this is all a
new thing for me. So my comment is more about the awkward state of
things than about some kind of failure from Alex. Obviously Alex is fine
here.

Your comments about my timing are also completely unjustified,
ridiculous, and actually a tad bit offensive. For the "high profile"
archs that I really wanted to hit 7.1, I started sending in DTB patches
a good deal of time ago. The only big arch I really wanted to hit 7.1
that wasn't queued up was the i386 patch, which I first posted in June.
Anyway, after it became clear that the i386 work was finally going to be
picked up, I breathed easy and decided to send in patches for the
remaining archs, to be picked up whenever. It was *your* decision that
all the DTB archs get in at the same time, hence picking this up; I had
no particular feelings on it, particularly as I don't know how to test
those remaining architectures like I did with the others. Anyway,
timing-wise, in my own planning, I handled risc-v, or1k, ppc, arm, i386,
and m68k well in advance and have been itching every single day since
posting those patches for them to be queued up somewhere.

So I really find your whole email just obnoxious and unnecessary. I've
been spending time trying to get the rng-seed stuff working on QEMU.
It's been a bit of a learning curve trying to figure out the QEMU
development model, and so I've miss-CC'd a few patches here and there.
But I've definitely tried to get an important subset of those patches in
in a timely manner. As a maintainer, you're definitely having the effect
of turning me off of the project rather than trying to acquaint me with
norms or be helpful.

Please, quit the drama. Enough of this stuff.

Jason

[PULL 8/9] hw/rx: pass random seed to fdt

[Paolo Bonzini]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>

If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
initialize early. Set this using the usual guest random number
generation function. This FDT node is part of the DT specification.

Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Message-Id: <20220719122033.135902-1-Jason@zx2c4.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/rx/rx-gdbsim.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
index be147b4bd9..8ffe1b8035 100644
--- a/hw/rx/rx-gdbsim.c
+++ b/hw/rx/rx-gdbsim.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
+#include "qemu/guest-random.h"
 #include "qapi/error.h"
 #include "hw/loader.h"
 #include "hw/rx/rx62n.h"
@@ -83,6 +84,7 @@ static void rx_gdbsim_init(MachineState *machine)
     MemoryRegion *sysmem = get_system_memory();
     const char *kernel_filename = machine->kernel_filename;
     const char *dtb_filename = machine->dtb;
+    uint8_t rng_seed[32];
 
     if (machine->ram_size < mc->default_ram_size) {
         char *sz = size_to_str(mc->default_ram_size);
@@ -140,6 +142,8 @@ static void rx_gdbsim_init(MachineState *machine)
                 error_report("Couldn't set /chosen/bootargs");
                 exit(1);
             }
+            qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
+            qemu_fdt_setprop(dtb, "/chosen", "rng-seed", rng_seed, sizeof(rng_seed));
             /* DTB is located at the end of SDRAM space. */
             dtb_offset = ROUND_DOWN(machine->ram_size - dtb_size, 16);
             rom_add_blob_fixed("dtb", dtb, dtb_size,
-- 
2.36.1

[PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Paolo Bonzini]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>

Tiny machines optimized for fast boot time generally don't use EFI,
which means a random seed has to be supplied some other way. For this
purpose, Linux (≥5.20) supports passing a seed in the setup_data table
with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and
specialized bootloaders. The linked commit shows the upstream kernel
implementation.

At Paolo's request, we don't pass these to versioned machine types ≤7.0.

Link: https://git.kernel.org/tip/tip/c/68b8e9713c8
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Message-Id: <20220721125636.446842-1-Jason@zx2c4.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/microvm.c                            |  2 +-
 hw/i386/pc.c                                 |  4 +--
 hw/i386/pc_piix.c                            |  2 ++
 hw/i386/pc_q35.c                             |  2 ++
 hw/i386/x86.c                                | 26 +++++++++++++++++---
 include/hw/i386/pc.h                         |  3 +++
 include/hw/i386/x86.h                        |  3 ++-
 include/standard-headers/asm-x86/bootparam.h |  1 +
 8 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index dc929727dc..7fe8cce03e 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -332,7 +332,7 @@ static void microvm_memory_init(MicrovmMachineState *mms)
     rom_set_fw(fw_cfg);
 
     if (machine->kernel_filename != NULL) {
-        x86_load_linux(x86ms, fw_cfg, 0, true);
+        x86_load_linux(x86ms, fw_cfg, 0, true, false);
     }
 
     if (mms->option_roms) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 774cb2bf07..d2b5823ffb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -796,7 +796,7 @@ void xen_load_linux(PCMachineState *pcms)
     rom_set_fw(fw_cfg);
 
     x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
-                   pcmc->pvh_enabled);
+                   pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
     for (i = 0; i < nb_option_roms; i++) {
         assert(!strcmp(option_rom[i].name, "linuxboot.bin") ||
                !strcmp(option_rom[i].name, "linuxboot_dma.bin") ||
@@ -992,7 +992,7 @@ void pc_memory_init(PCMachineState *pcms,
 
     if (linux_boot) {
         x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
-                       pcmc->pvh_enabled);
+                       pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
     }
 
     for (i = 0; i < nb_option_roms; i++) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a234989ac3..fbf9465318 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -438,9 +438,11 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
 
 static void pc_i440fx_7_0_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_7_1_machine_options(m);
     m->alias = NULL;
     m->is_default = false;
+    pcmc->legacy_no_rng_seed = true;
     compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
     compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index f96cbd04e2..12cc76aaf8 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -375,8 +375,10 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
 
 static void pc_q35_7_0_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_7_1_machine_options(m);
     m->alias = NULL;
+    pcmc->legacy_no_rng_seed = true;
     compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
     compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
 }
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 6003b4b2df..ecea25d249 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -26,6 +26,7 @@
 #include "qemu/cutils.h"
 #include "qemu/units.h"
 #include "qemu/datadir.h"
+#include "qemu/guest-random.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qapi-visit-common.h"
@@ -766,7 +767,8 @@ static bool load_elfboot(const char *kernel_filename,
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
-                    bool pvh_enabled)
+                    bool pvh_enabled,
+                    bool legacy_no_rng_seed)
 {
     bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
     uint16_t protocol;
@@ -774,7 +776,7 @@ void x86_load_linux(X86MachineState *x86ms,
     int dtb_size, setup_data_offset;
     uint32_t initrd_max;
     uint8_t header[8192], *setup, *kernel;
-    hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
+    hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0, first_setup_data = 0;
     FILE *f;
     char *vmode;
     MachineState *machine = MACHINE(x86ms);
@@ -784,6 +786,7 @@ void x86_load_linux(X86MachineState *x86ms,
     const char *dtb_filename = machine->dtb;
     const char *kernel_cmdline = machine->kernel_cmdline;
     SevKernelLoaderContext sev_load_ctx = {};
+    enum { RNG_SEED_LENGTH = 32 };
 
     /* Align to 16 bytes as a paranoia measure */
     cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -1063,16 +1066,31 @@ void x86_load_linux(X86MachineState *x86ms,
         kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
         kernel = g_realloc(kernel, kernel_size);
 
-        stq_p(header + 0x250, prot_addr + setup_data_offset);
 
         setup_data = (struct setup_data *)(kernel + setup_data_offset);
-        setup_data->next = 0;
+        setup_data->next = cpu_to_le64(first_setup_data);
+        first_setup_data = prot_addr + setup_data_offset;
         setup_data->type = cpu_to_le32(SETUP_DTB);
         setup_data->len = cpu_to_le32(dtb_size);
 
         load_image_size(dtb_filename, setup_data->data, dtb_size);
     }
 
+    if (!legacy_no_rng_seed) {
+        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
+        kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
+        kernel = g_realloc(kernel, kernel_size);
+        setup_data = (struct setup_data *)(kernel + setup_data_offset);
+        setup_data->next = cpu_to_le64(first_setup_data);
+        first_setup_data = prot_addr + setup_data_offset;
+        setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
+        setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
+        qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
+    }
+
+    /* Offset 0x250 is a pointer to the first setup_data link. */
+    stq_p(header + 0x250, first_setup_data);
+
     /*
      * If we're starting an encrypted VM, it will be OVMF based, which uses the
      * efi stub for booting and doesn't require any values to be placed in the
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b7735dccfc..2a8ffbcfa8 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -127,6 +127,9 @@ struct PCMachineClass {
 
     /* create kvmclock device even when KVM PV features are not exposed */
     bool kvmclock_create_always;
+
+    /* skip passing an rng seed for legacy machines */
+    bool legacy_no_rng_seed;
 };
 
 #define TYPE_PC_MACHINE "generic-pc-machine"
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 9089bdd99c..6bdf1f6ab2 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -123,7 +123,8 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
-                    bool pvh_enabled);
+                    bool pvh_enabled,
+                    bool legacy_no_rng_seed);
 
 bool x86_machine_is_smm_enabled(const X86MachineState *x86ms);
 bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms);
diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h
index 072e2ed546..b2aaad10e5 100644
--- a/include/standard-headers/asm-x86/bootparam.h
+++ b/include/standard-headers/asm-x86/bootparam.h
@@ -10,6 +10,7 @@
 #define SETUP_EFI			4
 #define SETUP_APPLE_PROPERTIES		5
 #define SETUP_JAILHOUSE			6
+#define SETUP_RNG_SEED			9
 
 #define SETUP_INDIRECT			(1<<31)
 
-- 
2.36.1

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Xiaoyao Li]

On 7/22/2022 12:36 AM, Paolo Bonzini wrote:
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> 
> Tiny machines optimized for fast boot time generally don't use EFI,
> which means a random seed has to be supplied some other way. For this
> purpose, Linux (≥5.20) supports passing a seed in the setup_data table
> with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and
> specialized bootloaders. The linked commit shows the upstream kernel
> implementation.
> 
> At Paolo's request, we don't pass these to versioned machine types ≤7.0.
> 
> Link: https://git.kernel.org/tip/tip/c/68b8e9713c8
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Message-Id: <20220721125636.446842-1-Jason@zx2c4.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/i386/microvm.c                            |  2 +-
>   hw/i386/pc.c                                 |  4 +--
>   hw/i386/pc_piix.c                            |  2 ++
>   hw/i386/pc_q35.c                             |  2 ++
>   hw/i386/x86.c                                | 26 +++++++++++++++++---
>   include/hw/i386/pc.h                         |  3 +++
>   include/hw/i386/x86.h                        |  3 ++-
>   include/standard-headers/asm-x86/bootparam.h |  1 +
>   8 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index dc929727dc..7fe8cce03e 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -332,7 +332,7 @@ static void microvm_memory_init(MicrovmMachineState *mms)
>       rom_set_fw(fw_cfg);
>   
>       if (machine->kernel_filename != NULL) {
> -        x86_load_linux(x86ms, fw_cfg, 0, true);
> +        x86_load_linux(x86ms, fw_cfg, 0, true, false);
>       }
>   
>       if (mms->option_roms) {
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 774cb2bf07..d2b5823ffb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -796,7 +796,7 @@ void xen_load_linux(PCMachineState *pcms)
>       rom_set_fw(fw_cfg);
>   
>       x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
> -                   pcmc->pvh_enabled);
> +                   pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
>       for (i = 0; i < nb_option_roms; i++) {
>           assert(!strcmp(option_rom[i].name, "linuxboot.bin") ||
>                  !strcmp(option_rom[i].name, "linuxboot_dma.bin") ||
> @@ -992,7 +992,7 @@ void pc_memory_init(PCMachineState *pcms,
>   
>       if (linux_boot) {
>           x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
> -                       pcmc->pvh_enabled);
> +                       pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
>       }
>   
>       for (i = 0; i < nb_option_roms; i++) {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a234989ac3..fbf9465318 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -438,9 +438,11 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
>   
>   static void pc_i440fx_7_0_machine_options(MachineClass *m)
>   {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>       pc_i440fx_7_1_machine_options(m);
>       m->alias = NULL;
>       m->is_default = false;
> +    pcmc->legacy_no_rng_seed = true;
>       compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
>       compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
>   }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index f96cbd04e2..12cc76aaf8 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -375,8 +375,10 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
>   
>   static void pc_q35_7_0_machine_options(MachineClass *m)
>   {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>       pc_q35_7_1_machine_options(m);
>       m->alias = NULL;
> +    pcmc->legacy_no_rng_seed = true;

Is making .legacy_no_rng_seed default false and opt-in it for old 
machines correct?

AFAICT, QEMU with machine-7.1 fails to boot with OVMF on my environment.

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

Hi,

On Tue, Aug 02, 2022 at 11:28:15AM +0800, Xiaoyao Li wrote:
> >   static void pc_q35_7_0_machine_options(MachineClass *m)
> >   {
> > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >       pc_q35_7_1_machine_options(m);
> >       m->alias = NULL;
> > +    pcmc->legacy_no_rng_seed = true;
> 
> Is making .legacy_no_rng_seed default false and opt-in it for old 
> machines correct?

Not sure I follow what you're saying. ≤7.0 won't pass the RNG seed. It's
only on ≥7.1 that the RNG seed is used.

Either way, this shouldn't cause boot failures.

Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Xiaoyao Li]

On 8/2/2022 9:21 PM, Jason A. Donenfeld wrote:
> Hi,
> 
> On Tue, Aug 02, 2022 at 11:28:15AM +0800, Xiaoyao Li wrote:
>>>    static void pc_q35_7_0_machine_options(MachineClass *m)
>>>    {
>>> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>>>        pc_q35_7_1_machine_options(m);
>>>        m->alias = NULL;
>>> +    pcmc->legacy_no_rng_seed = true;
>>
>> Is making .legacy_no_rng_seed default false and opt-in it for old
>> machines correct?
> 
> Not sure I follow what you're saying. ≤7.0 won't pass the RNG seed. It's
> only on ≥7.1 that the RNG seed is used.

yes, with >= 7.1, pcmc->legacy_no_rng_seed = false by default, and RNG 
seed is used.

> Either way, this shouldn't cause boot failures.

It does fail booting OVMF with #PF. Below diff can fix the #PF for me.

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7280c02ce3d5..1f62971759bf 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1903,6 +1903,7 @@ static void pc_machine_class_init(ObjectClass *oc, 
void *data)
      pcmc->has_reserved_memory = true;
      pcmc->kvmclock_enabled = true;
      pcmc->enforce_aligned_dimm = true;
+    pcmc->legacy_no_rng_seed = true;
      pcmc->enforce_amd_1tb_hole = true;
      /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K 
reported
       * to be used at the moment, 32K should be enough for a while.  */

> Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

Hi Xiaoyao,

On Tue, Aug 02, 2022 at 10:53:07PM +0800, Xiaoyao Li wrote:
> yes, with >= 7.1, pcmc->legacy_no_rng_seed = false by default, and RNG 
> seed is used.

This is intended behavior. Being on by default is basically the whole
point of it. Otherwise it's useless.

> 
> > Either way, this shouldn't cause boot failures.
> 
> It does fail booting OVMF with #PF. Below diff can fix the #PF for me.

Huh, interesting. Sounds like maybe there's a bug I need to fix. Can you
send me some repro instructions, and I'll look into it right away.

Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

Hi Xiaoyao,

On Tue, Aug 2, 2022 at 5:06 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Xiaoyao,
>
> On Tue, Aug 02, 2022 at 10:53:07PM +0800, Xiaoyao Li wrote:
> > yes, with >= 7.1, pcmc->legacy_no_rng_seed = false by default, and RNG
> > seed is used.
>
> This is intended behavior. Being on by default is basically the whole
> point of it. Otherwise it's useless.
>
> >
> > > Either way, this shouldn't cause boot failures.
> >
> > It does fail booting OVMF with #PF. Below diff can fix the #PF for me.
>
> Huh, interesting. Sounds like maybe there's a bug I need to fix. Can you
> send me some repro instructions, and I'll look into it right away.

I just tried booting Fedora using OVMF and didn't have any problems. I
used this command line:

qemu-system-x86_64 -machine q35 -enable-kvm -cpu host,-rdrand,-rdseed
-smp cores=8 -drive file=disk.qcow2,if=virtio -net nic,model=virtio
-net user,hostfwd=tcp::19230-:22 -m 8G -vga qxl -device
virtio-serial-pci -device
virtserialport,chardev=spicechannel0,name=com.redhat.spice.
0 -chardev spicevmc,id=spicechannel0,name=vdagent -spice
unix,addr=/tmp/vm_spice_fedora.socket,disable-ticketing,playback-compression=off,agen
t-mouse=on,seamless-migration,gl=on -device
virtserialport,chardev=spicechannel1,name=org.spice-space.webdav.0
-chardev spiceport,id=spicechan
nel1,name=org.spice-space.webdav.0 -global
driver=cfi.pflash01,property=secure,value=on -drive
if=pflash,format=raw,unit=0,file=OVMF_CODE.secb
oot.fd,readonly=on -drive if=pflash,format=raw,file=OVMF_VARS.secboot.fd

Can you tell me what you're using and give me some links with various
images and such? Doing the straight forward thing doesn't reproduce it
for me.

Thanks,
Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Xiaoyao Li]

On 8/2/2022 11:13 PM, Jason A. Donenfeld wrote:
> Hi Xiaoyao,
> 
> On Tue, Aug 2, 2022 at 5:06 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> Hi Xiaoyao,
>>
>> On Tue, Aug 02, 2022 at 10:53:07PM +0800, Xiaoyao Li wrote:
>>> yes, with >= 7.1, pcmc->legacy_no_rng_seed = false by default, and RNG
>>> seed is used.
>>
>> This is intended behavior. Being on by default is basically the whole
>> point of it. Otherwise it's useless.
>>
>>>
>>>> Either way, this shouldn't cause boot failures.
>>>
>>> It does fail booting OVMF with #PF. Below diff can fix the #PF for me.
>>
>> Huh, interesting. Sounds like maybe there's a bug I need to fix. Can you
>> send me some repro instructions, and I'll look into it right away.
> 
> I just tried booting Fedora using OVMF and didn't have any problems. I
> used this command line:
> 
> qemu-system-x86_64 -machine q35 -enable-kvm -cpu host,-rdrand,-rdseed
> -smp cores=8 -drive file=disk.qcow2,if=virtio -net nic,model=virtio
> -net user,hostfwd=tcp::19230-:22 -m 8G -vga qxl -device
> virtio-serial-pci -device
> virtserialport,chardev=spicechannel0,name=com.redhat.spice.
> 0 -chardev spicevmc,id=spicechannel0,name=vdagent -spice
> unix,addr=/tmp/vm_spice_fedora.socket,disable-ticketing,playback-compression=off,agen
> t-mouse=on,seamless-migration,gl=on -device
> virtserialport,chardev=spicechannel1,name=org.spice-space.webdav.0
> -chardev spiceport,id=spicechan
> nel1,name=org.spice-space.webdav.0 -global
> driver=cfi.pflash01,property=secure,value=on -drive
> if=pflash,format=raw,unit=0,file=OVMF_CODE.secb
> oot.fd,readonly=on -drive if=pflash,format=raw,file=OVMF_VARS.secboot.fd
> 
> Can you tell me what you're using and give me some links with various
> images and such? Doing the straight forward thing doesn't reproduce it
> for me.

I guess the key to reproduce the issue is using "-kernel" option to load 
guest kernel with QEMU

> Thanks,
> Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Daniel P. Berrangé]

On Tue, Aug 02, 2022 at 05:13:26PM +0200, Jason A. Donenfeld wrote:
> Hi Xiaoyao,
> 
> On Tue, Aug 2, 2022 at 5:06 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Hi Xiaoyao,
> >
> > On Tue, Aug 02, 2022 at 10:53:07PM +0800, Xiaoyao Li wrote:
> > > yes, with >= 7.1, pcmc->legacy_no_rng_seed = false by default, and RNG
> > > seed is used.
> >
> > This is intended behavior. Being on by default is basically the whole
> > point of it. Otherwise it's useless.
> >
> > >
> > > > Either way, this shouldn't cause boot failures.
> > >
> > > It does fail booting OVMF with #PF. Below diff can fix the #PF for me.
> >
> > Huh, interesting. Sounds like maybe there's a bug I need to fix. Can you
> > send me some repro instructions, and I'll look into it right away.
> 
> I just tried booting Fedora using OVMF and didn't have any problems. I
> used this command line:

I managed to reproduce on a Fedora 36 host, using QEMU git master from
today.

 $ git clone https://gitlab.com/berrange/tiny-vm-tools
 $ cd tiny-vm-tools
 $ ./make-tiny-image.py --run date date
 tiny-initrd.img
 Copy lib /lib/ld-musl-x86_64.so.1 -> /tmp/make-tiny-imagebcuv8i_b/lib/ld-musl-x86_64.so.1
 Copy bin /usr/bin/date -> /tmp/make-tiny-imagebcuv8i_b/bin/date
 Copy lib /lib64/libc.so.6 -> /tmp/make-tiny-imagebcuv8i_b/lib64/libc.so.6
 Copy lib /lib64/ld-linux-x86-64.so.2 -> /tmp/make-tiny-imagebcuv8i_b/lib64/ld-linux-x86-64.so.2

 $ cp /usr/share/edk2/ovmf/OVMF_VARS.fd vars.fd

 $ ~/src/virt/qemu.git/build/qemu-system-x86_64 \
   -blockdev node-name=file_ovmf_code,driver=file,filename=/usr/share/edk2/ovmf/OVMF_CODE.fd,auto-read-only=on,discard=unmap \
   -blockdev node-name=drive_ovmf_code,driver=raw,read-only=on,file=file_ovmf_code \
   -blockdev node-name=file_ovmf_vars,driver=file,filename=vars.fd,auto-read-only=on,discard=unmap \
   -blockdev node-name=drive_ovmf_vars,driver=raw,read-only=off,file=file_ovmf_vars  \
   -machine pc-q35-7.1,pflash0=drive_ovmf_code,pflash1=drive_ovmf_vars \
   -kernel /boot/vmlinuz-5.18.5-200.fc36.x86_64 \
   -initrd tiny-initrd.img \
   -m 8000 \
   -display none \
   -nodefaults \
   -serial stdio \
   -append 'console=ttyS0 quiet'

It results in OVMF crashing and displaying this dump on console:

!!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 00000000 !!!!
ExceptionData - 0000000000000000
RIP  - 0000000077EA6BBE, CS  - 0000000000000038, RFLAGS - 0000000000000206
RAX  - 28006E6F69746163, RCX - 0000000000000000, RDX - 41CBF4FA982C298B
RBX  - 000000007D9C3000, RSP - 000000007FEDF8E0, RBP - 0000000000000000
RSI  - 0000000000000000, RDI - 000000007D9C3000
R8   - 000000007D9C2F18, R9  - 000000007FEDF980, R10 - 0000000000000000
R11  - 0000000000000006, R12 - 28006E6F69746163, R13 - 000000007FEDF980
R14  - 000000007734F000, R15 - 000000007FEDFD01
DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
GS   - 0000000000000030, SS  - 0000000000000030
CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FC01000
CR4  - 0000000000000668, CR8 - 0000000000000000
DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
GDTR - 000000007F9DE000 0000000000000047, LDTR - 0000000000000000
IDTR - 000000007F40F018 0000000000000FFF,   TR - 0000000000000000
FXSAVE_STATE - 000000007FEDF540
!!!! Find image based on IP(0x77EA6BBE) (No PDB)  (ImageBase=000000007734F000, EntryPoint=0000000077EA65FC) !!!!



Changing to pc-q35-7.0 makes it work and prints current 'date' output
before shutting down.

Similarly adding  'pcmc->legacy_no_rng_seed = true;' for 7.1 machine
type also makes it work.

Turning on isa-debugcon for OVMF doesn't show anything especially
unsual - just a slightly different kernel image size, due to the
RNG seed having been added.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

Hi Daniel,

On Wed, Aug 03, 2022 at 11:52:25AM +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 02, 2022 at 05:13:26PM +0200, Jason A. Donenfeld wrote:
> > Hi Xiaoyao,
> > 
> > On Tue, Aug 2, 2022 at 5:06 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > Hi Xiaoyao,
> > >
> > > On Tue, Aug 02, 2022 at 10:53:07PM +0800, Xiaoyao Li wrote:
> > > > yes, with >= 7.1, pcmc->legacy_no_rng_seed = false by default, and RNG
> > > > seed is used.
> > >
> > > This is intended behavior. Being on by default is basically the whole
> > > point of it. Otherwise it's useless.
> > >
> > > >
> > > > > Either way, this shouldn't cause boot failures.
> > > >
> > > > It does fail booting OVMF with #PF. Below diff can fix the #PF for me.
> > >
> > > Huh, interesting. Sounds like maybe there's a bug I need to fix. Can you
> > > send me some repro instructions, and I'll look into it right away.
> > 
> > I just tried booting Fedora using OVMF and didn't have any problems. I
> > used this command line:
> 
> I managed to reproduce on a Fedora 36 host, using QEMU git master from
> today.
> 
>  $ git clone https://gitlab.com/berrange/tiny-vm-tools
>  $ cd tiny-vm-tools
>  $ ./make-tiny-image.py --run date date
>  tiny-initrd.img
>  Copy lib /lib/ld-musl-x86_64.so.1 -> /tmp/make-tiny-imagebcuv8i_b/lib/ld-musl-x86_64.so.1
>  Copy bin /usr/bin/date -> /tmp/make-tiny-imagebcuv8i_b/bin/date
>  Copy lib /lib64/libc.so.6 -> /tmp/make-tiny-imagebcuv8i_b/lib64/libc.so.6
>  Copy lib /lib64/ld-linux-x86-64.so.2 -> /tmp/make-tiny-imagebcuv8i_b/lib64/ld-linux-x86-64.so.2
> 
>  $ cp /usr/share/edk2/ovmf/OVMF_VARS.fd vars.fd
> 
>  $ ~/src/virt/qemu.git/build/qemu-system-x86_64 \
>    -blockdev node-name=file_ovmf_code,driver=file,filename=/usr/share/edk2/ovmf/OVMF_CODE.fd,auto-read-only=on,discard=unmap \
>    -blockdev node-name=drive_ovmf_code,driver=raw,read-only=on,file=file_ovmf_code \
>    -blockdev node-name=file_ovmf_vars,driver=file,filename=vars.fd,auto-read-only=on,discard=unmap \
>    -blockdev node-name=drive_ovmf_vars,driver=raw,read-only=off,file=file_ovmf_vars  \
>    -machine pc-q35-7.1,pflash0=drive_ovmf_code,pflash1=drive_ovmf_vars \
>    -kernel /boot/vmlinuz-5.18.5-200.fc36.x86_64 \
>    -initrd tiny-initrd.img \
>    -m 8000 \
>    -display none \
>    -nodefaults \
>    -serial stdio \
>    -append 'console=ttyS0 quiet'

Thanks for the info. Very helpful. Looking into it now.

Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

On Wed, Aug 03, 2022 at 03:11:48PM +0200, Jason A. Donenfeld wrote:
> Thanks for the info. Very helpful. Looking into it now.

So interestingly, this is not a new issue. If you pass any type of setup
data, OVMF appears to be doing something unusual and passing 0xffffffff
for all the entries, rather than the actual data. The reason this isn't
new is: try passing `-dtb any/dtb/at/all/from/anywhere` and you get the
same page fault, on all QEMU versions. The thing that passes the DTB is
the thing that passes the RNG seed. Same mechanism, same bug.

I'm looking into it...

Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

On Wed, Aug 03, 2022 at 03:34:04PM +0200, Jason A. Donenfeld wrote:
> On Wed, Aug 03, 2022 at 03:11:48PM +0200, Jason A. Donenfeld wrote:
> > Thanks for the info. Very helpful. Looking into it now.
> 
> So interestingly, this is not a new issue. If you pass any type of setup
> data, OVMF appears to be doing something unusual and passing 0xffffffff
> for all the entries, rather than the actual data. The reason this isn't
> new is: try passing `-dtb any/dtb/at/all/from/anywhere` and you get the
> same page fault, on all QEMU versions. The thing that passes the DTB is
> the thing that passes the RNG seed. Same mechanism, same bug.
> 
> I'm looking into it...

Fixed with: https://lore.kernel.org/all/20220803170235.1312978-1-Jason@zx2c4.com/

Feel free to join into the discussion there. I CC'd you.

Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Michael S. Tsirkin]

On Wed, Aug 03, 2022 at 07:07:52PM +0200, Jason A. Donenfeld wrote:
> On Wed, Aug 03, 2022 at 03:34:04PM +0200, Jason A. Donenfeld wrote:
> > On Wed, Aug 03, 2022 at 03:11:48PM +0200, Jason A. Donenfeld wrote:
> > > Thanks for the info. Very helpful. Looking into it now.
> > 
> > So interestingly, this is not a new issue. If you pass any type of setup
> > data, OVMF appears to be doing something unusual and passing 0xffffffff
> > for all the entries, rather than the actual data. The reason this isn't
> > new is: try passing `-dtb any/dtb/at/all/from/anywhere` and you get the
> > same page fault, on all QEMU versions. The thing that passes the DTB is
> > the thing that passes the RNG seed. Same mechanism, same bug.
> > 
> > I'm looking into it...
> 
> Fixed with: https://lore.kernel.org/all/20220803170235.1312978-1-Jason@zx2c4.com/
> 
> Feel free to join into the discussion there. I CC'd you.
> 
> Jason

Hmm I don't think this patch will make it in 7.1 given the
timeframe. I suspect we should revert the patch for now.

Which is where you maybe begin to see why we generally
prefer doing it with features - one can then work around
bugs by turning the feature on and off.

-- 
MST

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

Hi Michael,

On Wed, Aug 03, 2022 at 06:03:20PM -0400, Michael S. Tsirkin wrote:
> On Wed, Aug 03, 2022 at 07:07:52PM +0200, Jason A. Donenfeld wrote:
> > On Wed, Aug 03, 2022 at 03:34:04PM +0200, Jason A. Donenfeld wrote:
> > > On Wed, Aug 03, 2022 at 03:11:48PM +0200, Jason A. Donenfeld wrote:
> > > > Thanks for the info. Very helpful. Looking into it now.
> > > 
> > > So interestingly, this is not a new issue. If you pass any type of setup
> > > data, OVMF appears to be doing something unusual and passing 0xffffffff
> > > for all the entries, rather than the actual data. The reason this isn't
> > > new is: try passing `-dtb any/dtb/at/all/from/anywhere` and you get the
> > > same page fault, on all QEMU versions. The thing that passes the DTB is
> > > the thing that passes the RNG seed. Same mechanism, same bug.
> > > 
> > > I'm looking into it...
> > 
> > Fixed with: https://lore.kernel.org/all/20220803170235.1312978-1-Jason@zx2c4.com/
> > 
> > Feel free to join into the discussion there. I CC'd you.
> > 
> > Jason
> 
> Hmm I don't think this patch will make it in 7.1 given the
> timeframe. I suspect we should revert the patch for now.
> 
> Which is where you maybe begin to see why we generally
> prefer doing it with features - one can then work around
> bugs by turning the feature on and off.

The bug actually precedes this patch. Just boot with -dtb on any qemu
version and you'll trigger it. We're still at rc0; there should be time
enough for a bug fix. Please do chime in on that thread and maybe we can
come up with something reasonable fast enough.

Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Michael S. Tsirkin]

On Thu, Aug 04, 2022 at 12:08:07AM +0200, Jason A. Donenfeld wrote:
> Hi Michael,
> 
> On Wed, Aug 03, 2022 at 06:03:20PM -0400, Michael S. Tsirkin wrote:
> > On Wed, Aug 03, 2022 at 07:07:52PM +0200, Jason A. Donenfeld wrote:
> > > On Wed, Aug 03, 2022 at 03:34:04PM +0200, Jason A. Donenfeld wrote:
> > > > On Wed, Aug 03, 2022 at 03:11:48PM +0200, Jason A. Donenfeld wrote:
> > > > > Thanks for the info. Very helpful. Looking into it now.
> > > > 
> > > > So interestingly, this is not a new issue. If you pass any type of setup
> > > > data, OVMF appears to be doing something unusual and passing 0xffffffff
> > > > for all the entries, rather than the actual data. The reason this isn't
> > > > new is: try passing `-dtb any/dtb/at/all/from/anywhere` and you get the
> > > > same page fault, on all QEMU versions. The thing that passes the DTB is
> > > > the thing that passes the RNG seed. Same mechanism, same bug.
> > > > 
> > > > I'm looking into it...
> > > 
> > > Fixed with: https://lore.kernel.org/all/20220803170235.1312978-1-Jason@zx2c4.com/
> > > 
> > > Feel free to join into the discussion there. I CC'd you.
> > > 
> > > Jason
> > 
> > Hmm I don't think this patch will make it in 7.1 given the
> > timeframe. I suspect we should revert the patch for now.
> > 
> > Which is where you maybe begin to see why we generally
> > prefer doing it with features - one can then work around
> > bugs by turning the feature on and off.
> 
> The bug actually precedes this patch. Just boot with -dtb on any qemu
> version and you'll trigger it.

Sure but it's still a regression.

> We're still at rc0; there should be time
> enough for a bug fix. Please do chime in on that thread and maybe we can
> come up with something reasonable fast enough.
> 
> Jason

Maybe.

-- 
MST

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Laszlo Ersek]

On 08/04/22 00:23, Michael S. Tsirkin wrote:
> On Thu, Aug 04, 2022 at 12:08:07AM +0200, Jason A. Donenfeld wrote:
>> Hi Michael,
>>
>> On Wed, Aug 03, 2022 at 06:03:20PM -0400, Michael S. Tsirkin wrote:
>>> On Wed, Aug 03, 2022 at 07:07:52PM +0200, Jason A. Donenfeld wrote:
>>>> On Wed, Aug 03, 2022 at 03:34:04PM +0200, Jason A. Donenfeld wrote:
>>>>> On Wed, Aug 03, 2022 at 03:11:48PM +0200, Jason A. Donenfeld wrote:
>>>>>> Thanks for the info. Very helpful. Looking into it now.
>>>>>
>>>>> So interestingly, this is not a new issue. If you pass any type of setup
>>>>> data, OVMF appears to be doing something unusual and passing 0xffffffff
>>>>> for all the entries, rather than the actual data. The reason this isn't
>>>>> new is: try passing `-dtb any/dtb/at/all/from/anywhere` and you get the
>>>>> same page fault, on all QEMU versions. The thing that passes the DTB is
>>>>> the thing that passes the RNG seed. Same mechanism, same bug.
>>>>>
>>>>> I'm looking into it...
>>>>
>>>> Fixed with: https://lore.kernel.org/all/20220803170235.1312978-1-Jason@zx2c4.com/
>>>>
>>>> Feel free to join into the discussion there. I CC'd you.
>>>>
>>>> Jason
>>>
>>> Hmm I don't think this patch will make it in 7.1 given the
>>> timeframe. I suspect we should revert the patch for now.
>>>
>>> Which is where you maybe begin to see why we generally
>>> prefer doing it with features - one can then work around
>>> bugs by turning the feature on and off.
>>
>> The bug actually precedes this patch. Just boot with -dtb on any qemu
>> version and you'll trigger it.

Yes! That's exactly what I expected, per

https://bugzilla.redhat.com/show_bug.cgi?id=2114637#c4

There I wrote that this kind of setup_data chaining was a "first" for OVMF.

While the same logic had existed in QEMU with for chaining a dtb, there
never had been a reason (that I could imagine) for using that with OVMF
guests.

So it had to be either a preexistent bug in QEMU, or one in OVMF, that
now got triggered (as Jason's patch for chaining the seed closely
followed the pattern set by the dtb logic).

Now, regarding the patch at
<https://lore.kernel.org/all/20220803170235.1312978-1-Jason@zx2c4.com/>,
including v2 at
<https://lore.kernel.org/all/20220804004411.1343158-1-Jason@zx2c4.com/>...

I don't think this kind of setup_data block chaining, with raw
guest-physical addresses filled in by QEMU in guest RAM, in advance, is
appropriate for an edk2 guest *in general*. By the time the firmware
loads the kernel (including setup block and kernel block) from fw_cfg,
the area in question (with the seed etc) may have been overwritten
several times. Edk2 is very careful about memory ownership, but it needs
the VMM and the guest OS to play along. There is a only very small set
of "well known addresses" that are (a) open-coded in both QEMU board
code and edk2 platform code and (b) not specified by industry specs;
such addresses are used to set up everything else, and we seek not to
introduce more of them.

Consider e.g. the end of
<https://www.kernel.org/doc/Documentation/x86/boot.rst>, namely the
deprecation of the "EFI Handover Protocol". The idea is to use
well-specified information channels that don't depend on the placement
of the kernel.

At least two mechanisms exist for dealing with this; the ACPI
linker-loader, and the GUID-ed struct chaining in pflash (mostly used
with SEV and I think TDX too).

More below.

> 
> Sure but it's still a regression.
> 
>> We're still at rc0; there should be time
>> enough for a bug fix. Please do chime in on that thread and maybe we can
>> come up with something reasonable fast enough.
>>
>> Jason
> 
> Maybe.

QEMU commit 67f7e426e538 ("hw/i386: pass RNG seed via setup_data entry",
2022-07-22) references <https://git.kernel.org/tip/tip/c/68b8e9713c8>,
and the commit message on that begins with:

----------
Currently, the only way x86 can get an early boot RNG seed is via EFI,
which is generally always used now for physical machines, but is very
rarely used in VMs, especially VMs that are optimized for starting
"instantaneously", such as Firecracker's MicroVM. For tiny fast booting
VMs, EFI is not something you generally need or want.
----------

So, first, I'd quite disagree with "EFI being rarely used in VMs" (the
trend has been the opposite), and I'm not saying that because I'm
married to edk2 (I switched to a different project last summer). I do
agree with EFI being rarely used in one-shot, fast-booting VMs though.

Second, I think this segmentation of use cases is actually great. If you
need this particular kind of seed-passing for non-EFI VMs only (i.e.,
where the UEFI stub in the guest kernel cannot rely on
EFI_RNG_PROTOCOL), then implement it in both QEMU and the (guest) kernel
for non-EFI only. Both the guest kernel and QEMU can tell whether the
guest firmware is UEFI (the guest kernel can tell that precisely,
whereas in QEMU, if memory serves, we equate that with a particular
pflash setup).

Again, I don't think the setup_data chaining, using GPAs stored by QEMU
for linkage, can be salvaged at all for UEFI guests. Normally some
dedicated (possibly new) information channel would be needed that lets
the firmware stay in control, but thankfully, this use case looks
explicitly irrelevant for UEFI guests. So "just restrict the whole thing
to non-UEFI guests" would be my proposal.

(

Yes, the existent DTB linking in QEMU is broken, and has been broken
forever, for edk2 (UEFI) guests. It never mattered in practice: edk2
guest firmware very explicitly deals with DTB passing (mostly for
arm/aarch64, but maybe Gerd's microvm uses DTB too; I can't tell, I'd
not been there), so I've never seen "dtb via setup_data" in practice.
There has never been a reason to use that. On the other hand, commit
67f7e426e538 enables setup_data chaining by default, and that seems to
break UEFI guests (with direct kernel boot anyway) summarily. It's more
serious than one might think at first sight; "virt-install --location"
uses direct (= fw_cfg) kernel boot.

)

I think that restricting "seed passing via setup_data" to non-UEFI
guests should be doable during Hard Feature Freeze too. The guest kernel
patch should be possible to do later.

Sorry about the wall of text.
Laszlo

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Daniel P. Berrangé]

On Thu, Jul 21, 2022 at 06:36:21PM +0200, Paolo Bonzini wrote:
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> 
> Tiny machines optimized for fast boot time generally don't use EFI,
> which means a random seed has to be supplied some other way. For this
> purpose, Linux (≥5.20) supports passing a seed in the setup_data table
> with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and
> specialized bootloaders. The linked commit shows the upstream kernel
> implementation.
> 
> At Paolo's request, we don't pass these to versioned machine types ≤7.0.


This change has also broken direct kernel measured boot with AMD SEV
confidential virtualization.

The vmlinuz that we pass in with -kernel is measured by the BIOS and
since that gets munged with a random seed, the measurement no longer
matches the expected measurements the person attesting boot will
have pre-calculated.

The kernel binary passed to the firmware must be 100% unchanged
from what the user provided in order for boot measurements to
succeed.

So at the very least this codes needs to be conditionalized to
not run when AMD SEV is active.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Jason A. Donenfeld]

Hi Daniel,

On Thu, Aug 4, 2022 at 2:01 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jul 21, 2022 at 06:36:21PM +0200, Paolo Bonzini wrote:
> > From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> >
> > Tiny machines optimized for fast boot time generally don't use EFI,
> > which means a random seed has to be supplied some other way. For this
> > purpose, Linux (≥5.20) supports passing a seed in the setup_data table
> > with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and
> > specialized bootloaders. The linked commit shows the upstream kernel
> > implementation.
> >
> > At Paolo's request, we don't pass these to versioned machine types ≤7.0.
>
>
> This change has also broken direct kernel measured boot with AMD SEV
> confidential virtualization.
>
> The vmlinuz that we pass in with -kernel is measured by the BIOS and
> since that gets munged with a random seed, the measurement no longer
> matches the expected measurements the person attesting boot will
> have pre-calculated.
>
> The kernel binary passed to the firmware must be 100% unchanged
> from what the user provided in order for boot measurements to
> succeed.
>
> So at the very least this codes needs to be conditionalized to
> not run when AMD SEV is active.

If you look at the v2 patch, I move all of the setup_data stuff
outside of the kernel image, so the kernel image itself doesn't get
modified. So SEV should still work.

Can you test that patch and see?

Jason

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Daniel P. Berrangé]

On Thu, Aug 04, 2022 at 02:13:41PM +0200, Jason A. Donenfeld wrote:
> Hi Daniel,
> 
> On Thu, Aug 4, 2022 at 2:01 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Jul 21, 2022 at 06:36:21PM +0200, Paolo Bonzini wrote:
> > > From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> > >
> > > Tiny machines optimized for fast boot time generally don't use EFI,
> > > which means a random seed has to be supplied some other way. For this
> > > purpose, Linux (≥5.20) supports passing a seed in the setup_data table
> > > with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and
> > > specialized bootloaders. The linked commit shows the upstream kernel
> > > implementation.
> > >
> > > At Paolo's request, we don't pass these to versioned machine types ≤7.0.
> >
> >
> > This change has also broken direct kernel measured boot with AMD SEV
> > confidential virtualization.
> >
> > The vmlinuz that we pass in with -kernel is measured by the BIOS and
> > since that gets munged with a random seed, the measurement no longer
> > matches the expected measurements the person attesting boot will
> > have pre-calculated.
> >
> > The kernel binary passed to the firmware must be 100% unchanged
> > from what the user provided in order for boot measurements to
> > succeed.
> >
> > So at the very least this codes needs to be conditionalized to
> > not run when AMD SEV is active.
> 
> If you look at the v2 patch, I move all of the setup_data stuff
> outside of the kernel image, so the kernel image itself doesn't get
> modified. So SEV should still work.
> 
> Can you test that patch and see?

It looks like the v2 patch fixes it, 'kernel' is no longer modified
and we throw away the modified 'setup' data for SEV.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|

|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 9/9] hw/i386: pass RNG seed via setup_data entry

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jul 21, 2022 at 06:36:21PM +0200, Paolo Bonzini wrote:
>> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
>> 
>> Tiny machines optimized for fast boot time generally don't use EFI,
>> which means a random seed has to be supplied some other way. For this
>> purpose, Linux (≥5.20) supports passing a seed in the setup_data table
>> with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and
>> specialized bootloaders. The linked commit shows the upstream kernel
>> implementation.
>> 
>> At Paolo's request, we don't pass these to versioned machine types ≤7.0.
>
>
> This change has also broken direct kernel measured boot with AMD SEV
> confidential virtualization.

FWIW this is why we had to introduce the dtb-randomness control knob for
ARM -M virt machines. Although we have deprecated the old dtb-kaslr-seed
knob and it has always enabled by default because the measured boot was
sufficiently new the few people working with it could just add it to
their command lines.

-- 
Alex Bennée