* [PATCH] x86: mmap kernel for direct kernel boot @ 2026-10-21 6:31 Gerd Hoffmann 2026-10-21 16:02 ` John Snow 0 siblings, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2026-10-21 6:31 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Paolo Bonzini, Richard Henderson, Gerd Hoffmann, John Snow 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> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: mmap kernel for direct kernel boot 2026-10-21 6:31 [PATCH] x86: mmap kernel for direct kernel boot Gerd Hoffmann @ 2026-10-21 16:02 ` John Snow 2026-11-06 18:02 ` John Snow 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2026-10-21 16:02 UTC (permalink / raw) To: Gerd Hoffmann Cc: qemu-devel, Michael S. Tsirkin, Paolo Bonzini, Richard Henderson 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> 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: mmap kernel for direct kernel boot 2026-10-21 16:02 ` John Snow @ 2026-11-06 18:02 ` John Snow 2026-11-07 13:12 ` Michael S. Tsirkin 2026-11-07 13:24 ` Michael S. Tsirkin 0 siblings, 2 replies; 7+ messages in thread From: John Snow @ 2026-11-06 18:02 UTC (permalink / raw) To: Gerd Hoffmann Cc: qemu-devel, Michael S. Tsirkin, Paolo Bonzini, Richard Henderson 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 > 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: mmap kernel for direct kernel boot 2026-11-06 18:02 ` John Snow @ 2026-11-07 13:12 ` Michael S. Tsirkin 2026-11-07 13:24 ` Michael S. Tsirkin 1 sibling, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2026-11-07 13:12 UTC (permalink / raw) To: John Snow; +Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Richard Henderson 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 ok > > 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: mmap kernel for direct kernel boot 2026-11-06 18:02 ` John Snow 2026-11-07 13:12 ` Michael S. Tsirkin @ 2026-11-07 13:24 ` Michael S. Tsirkin 2026-11-07 18:59 ` John Snow 1 sibling, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2026-11-07 13:24 UTC (permalink / raw) To: John Snow; +Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Richard Henderson 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. 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. 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. 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 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. Also, pls include the observed gain in the commit log. > > 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: mmap kernel for direct kernel boot 2026-11-07 13:24 ` Michael S. Tsirkin @ 2026-11-07 18:59 ` John Snow 2026-11-08 0:09 ` Michael S. Tsirkin 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2026-11-07 18:59 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Richard Henderson 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. > 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. > > 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. > > > 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. > > 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. > > > > > 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 > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: mmap kernel for direct kernel boot 2026-11-07 18:59 ` John Snow @ 2026-11-08 0:09 ` Michael S. Tsirkin 0 siblings, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2026-11-08 0:09 UTC (permalink / raw) To: John Snow; +Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Richard Henderson 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 > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-11-08 0:10 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-10-21 6:31 [PATCH] x86: mmap kernel for direct kernel boot Gerd Hoffmann 2026-10-21 16:02 ` John Snow 2026-11-06 18:02 ` John Snow 2026-11-07 13:12 ` Michael S. Tsirkin 2026-11-07 13:24 ` Michael S. Tsirkin 2026-11-07 18:59 ` John Snow 2026-11-08 0:09 ` Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.