Re: [PATCH] x86: mmap kernel for direct kernel boot

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Nov 07, 2026 at 01:59:12PM -0500, John Snow wrote:
> On Sat, Nov 7, 2026 at 8:24 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Nov 06, 2026 at 01:02:33PM -0500, John Snow wrote:
> > > On Wed, Oct 21, 2026 at 12:02 PM John Snow <jsnow@redhat.com> wrote:
> > > >
> > > > On Wed, Oct 21, 2026 at 2:31 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > > >
> > > > > Reduce memory footprint by mapping the kernel into memory instead of
> > > > > reading into allocated memory in the common case (no -dtb $file on the
> > > > > qemu command line).
> > > > >
> > > > > Cc: John Snow <jsnow@redhat.com>
> > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > >
> > > > Tested-by: John Snow <jsnow@redhat.com>
> > > >
> > >
> > > Ping, is someone going to pick up this patch?
> > >
> > > --js
> >
> > First we are in freeze. So no rush.
> 
> Patch was on-list well before freeze.

Yes but it has to be in a maintainer tree with pull on list, not just
patch on list.

> > Here's some ai based review - the arguments make sense to me.
> >
> > 1. No length validation on the mapping. kernel_size comes from
> > get_file_size() (fstat at open time), but the patch never checks
> > g_mapped_file_get_length() == kernel_size. If the file shrinks between
> > the stat and the mmap (or is a weird file), you get an OOB read / SIGBUS
> > instead of the old clean fread() failed exit. Further, if the
> > file is truncated later (QEMU running, guest hasn't read the kernel
> > through fw_cfg yet), guest reads hit SIGBUS and kill QEMU. fread copies
> > up front, so this race didn't exist before.
> 
> Looks like we do use this function when loading the initrd, easy
> enough to fix here.
> 
> >
> > 2. Files that are readable but not mappable. fread works on anything
> > stdio can read; mmap fails on some FUSE mounts, files with outstanding
> > mandatory/lease locks, etc. g_mapped_file_new failing exits QEMU, so a
> > previously booting config on such a filesystem stops booting.
> >
> > 3. Windows share modes. stdio fopen("rb") opens with permissive sharing,
> > while gio's file mapping opens the file with a more restrictive share
> > mode. A kernel image that another process still holds open for writing
> > (download in progress, installer, antivirus scanning with exclusive
> > handle) currently boots and would now fail to map. Windows is a
> > supported platform, so this counts.
> 
> OK, if 2 and 3 are valid, it sounds like we just want a fallback to
> fread. Mind you, we already use this technique for initrd images, so I
> don't think we are doing anything fundamentally wacky here that QEMU
> doesn't already do.

Better safe I guess.

> >
> > 4. Torn reads on concurrent modification. With fread you snapshot the
> > file once; with a private mapping, pages faulted after an external write
> > see new content, so a guest could get a mix of old and new kernel bytes.
> > Only matters for someone rewriting the image under a running QEMU, which
> > is already dubious.
> 
> I wouldn't worry about this; it's the same as "Well, what if you
> modify a .qcow2 under a running QEMU?" Well, don't do that.

OK.

> >
> >
> > I would not worry about torn reads too much. But failure seems
> > theoretically legit.  It's hard to know this does not break any existing
> > configs.  It might be safer to just add an option.
> 
> I think matching initrd loading behavior is all the justification we
> need for treating the kernel identically.

I'd say the reverse, let's fix initrd loading)

> >
> > Also, pls include the observed gain in the commit log.
> 
> Observed gain is that if you are loading a 19 MiB kernel, this change
> allows that memory to be mapped and allocated as RssFile memory
> instead of RssAnon memory. This means that if you run multiple QEMU
> instances using the same kernel, that memory is shared and excluded
> from USS instead of being charged per-instance.

So do this? Run many QEMU's and measure a practical gain?

> >
> >
> > > > Using this patch, I shifted about 19MiB of resources from RssAnon to
> > > > RssFile. This theoretically allows sharing resources between multiple
> > > > QEMU instances in the trivial case using -kernel and -initrd. This
> > > > matches how we already load initrd.
> > > >
> > > > --js
> > > >
> > > > > ---
> > > > >  hw/i386/x86-common.c | 25 ++++++++++++++++++++-----
> > > > >  1 file changed, 20 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> > > > > index fa18bf1aeceb..25e141aa8537 100644
> > > > > --- a/hw/i386/x86-common.c
> > > > > +++ b/hw/i386/x86-common.c
> > > > > @@ -911,16 +911,31 @@ void x86_load_linux(X86MachineState *x86ms,
> > > > >      }
> > > > >
> > > > >      setup  = g_malloc(setup_size);
> > > > > -    kernel = g_malloc(kernel_size);
> > > > >      fseek(f, 0, SEEK_SET);
> > > > >      if (fread(setup, 1, setup_size, f) != setup_size) {
> > > > >          fprintf(stderr, "fread() failed\n");
> > > > >          exit(1);
> > > > >      }
> > > > > -    fseek(f, 0, SEEK_SET);
> > > > > -    if (fread(kernel, 1, kernel_size, f) != kernel_size) {
> > > > > -        fprintf(stderr, "fread() failed\n");
> > > > > -        exit(1);
> > > > > +    if (dtb_filename) {
> > > > > +        /* read kernel into allocated memory, so we can realloc for the dtb */
> > > > > +        kernel = g_malloc(kernel_size);
> > > > > +        fseek(f, 0, SEEK_SET);
> > > > > +        if (fread(kernel, 1, kernel_size, f) != kernel_size) {
> > > > > +            fprintf(stderr, "fread() failed\n");
> > > > > +            exit(1);
> > > > > +        }
> > > > > +    } else {
> > > > > +        /* mmap kernel */
> > > > > +        GMappedFile *mapped_file;
> > > > > +        GError *gerr = NULL;
> > > > > +
> > > > > +        mapped_file = g_mapped_file_new(kernel_filename, false, &gerr);
> > > > > +        if (!mapped_file) {
> > > > > +            fprintf(stderr, "qemu: error reading kernel %s: %s\n",
> > > > > +                    kernel_filename, gerr->message);
> > > > > +            exit(1);
> > > > > +        }
> > > > > +        kernel = (uint8_t *)g_mapped_file_get_contents(mapped_file);
> > > > >      }
> > > > >      fclose(f);
> > > > >
> > > > > --
> > > > > 2.56.0
> > > > >
> >