Re: [MINI-OS PATCH 12/19] mini-os: kexec: add support for handing over some memory across kexec

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-07-11 03:49, Jürgen Groß wrote:
> On 11.07.25 01:21, Jason Andryuk wrote:
> > On 2025-07-02 04:12, Juergen Gross wrote:
> > > Especially for support of Xenstore-stubdom live update some memory must
> > > be handed over to the new kernel without moving it around: as the
> > > 9pfs device used for storing and retrieving the state of Xenstore
> > > needs to be kept operational across kexec (it can't be reopened due to
> > > Xenstore not being available without access to the device), the ring
> > > pages need to be accessible via active grants by the backend all the
> > > time.
> > >
> > > Add the basic support for that by reserving a pre-defined number of
> > > memory pages at the top of the memory. This memory area will be
> > > handed over to the new kernel via specifying it as a module in
> > > struct hvm_start_info.
> > >
> > > The contents of the memory area are described via a generic table of
> > > contents in the last page of the memory.
> > >
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > diff --git a/Config.mk b/Config.mk
> > > index b9675e61..0e4e86d8 100644
> > > --- a/Config.mk
> > > +++ b/Config.mk
> > > @@ -220,6 +220,8 @@ CONFIG-$(lwip) += CONFIG_LWIP
> > >   $(foreach i,$(CONFIG-y),$(eval $(i) ?= y))
> > >   $(foreach i,$(CONFIG-n),$(eval $(i) ?= n))
> > > +CONFIG-val-$(CONFIG_KEXEC) += CONFIG_KEXEC_MODULE_PAGES
> > > +
> > I don't know Makefiles well enough to review the preceding patch.  
> > This doesn't seem to be used?
> It is.
Oh, so setting CONFIG-val-y, through the Makefile magic, creates
-DCONFIG_KEXEC_MODULE_PAGES=$n

I said I didn't know Makefiles well :)

> > >   $(foreach i,$(CONFIG-val-y),$(eval $(i) ?= 0))
> > >   CONFIG-x += CONFIG_LIBXS
> > > +    module_ptr = (void *)(mod + mod_size - (unsigned long)PAGE_SIZE);
> > > +
> > > +    /* Check eye catcher. */
> > > +    if ( memcmp(module_ptr->eye_catcher, KEXECMOD_EYECATCHER,
> > > +                sizeof(module_ptr->eye_catcher)) )
> > > +        return NULL;
> > > +    if ( module_ptr->n_pages != (mod_size >> PAGE_SHIFT) - 1 )
> > > +        return NULL;
> > > +
> > > +    return module_ptr;
> > > +}
> > > +#define min(a, b) ((a) < (b) ? (a) : (b))
> > > +void kexec_module(unsigned long start_pfn, unsigned long max_pfn)
> > > +{
> > > +    /* Reuse already existing kexec module. */
> > > +    mod_ptr = kexec_check_module();
> > > +    if ( !mod_ptr && CONFIG_KEXEC_MODULE_PAGES )
> > What if CONFIG_KEXEC_MODULE_PAGES changes between the old and the new 
> > stubdom?
> Right now this wouldn't really matter. The CONFIG value is used
> only for sizing the module if we are not started via kexec().
When I wrote this I was thinking of:
A -- kexec --> B -- kexec --> C

A: CONFIG_KEXEC_MODULE_PAGES=4
B: CONFIG_KEXEC_MODULE_PAGES=6

When B is running, it has the 4 pages from A, but it expects 6 available 
for its use....
> > > diff --git a/include/kexec.h b/include/kexec.h
> > > index b89c3000..0200005f 100644
> > > --- a/include/kexec.h
> > > +++ b/include/kexec.h
> > > @@ -2,6 +2,48 @@
> > >   #define _KEXEC_H
> > >   #include <mini-os/elf.h>
> > > +/*
> > > + * Kexec module used to hand over memory across kexec().
> > > + *
> > > + * This is an ABI which should be modified only in a compatible way.
> > > + * struct kexec_module is located at the start of the last page of 
> > > the module.
> > Why is kexec_module, which seems like a header, placed in the last page?
> In case we ever need to grow the module (e.g. when adding new data
> to it and with that having to handle different CONFIG_KEXEC_MODULE_PAGES
> values between old and new build), it will be easier, as the module will
> normally be located at the end of the usable memory, so we wouldn't have
> to move the struct kexec_module around.
.. and this avoids that issue since the extra pages can be grabbed 
without moving struct kexec_module.
> > > + *
> > > + * The module can contain data/pages of multiple users. Each user 
> > > has an own
> > > + * record which layout is depending on the user. Records are linked 
> > > via a table
> > > + * of record offsets.
> > > + *
> > > + * All admin data (struct kexec_module, record offset table and 
> > > records) must
> > > + * fit into the last page of the module.
> > > + */
> > > +struct kexec_module {
> > > +    uint8_t eye_catcher[8];
> > > +#define KEXECMOD_EYECATCHER "KexecMem"
> > > +    uint16_t n_pages;       /* Number of allocatable pages in the 
> > > module.    */
> > > +    uint16_t n_records;     /* Size of record table (max. 
> > > 255).              */
> > > +#define KEXECMOD_REC_MAX    255
> > > +    uint16_t recs_off;      /* Offset to record table from start of 
> > > page.    */
> > > +                            /* The record table is an array 
> > > of               */
> > > +                            /* struct 
> > > kexec_module_rec.                      */
> > > +    uint8_t pg2rec[];       /* One entry per allocatable module 
> > > page, value  */
> > > +                            /* is record number (starting from 0) 
> > > associated */
> > > +                            /* with it. Free pages have value 
> > > 255.           */
> > This reads like it is 1 page per record...
> No, this means that each used page is associated with a record, but
> there can be [0 ... n_pages] associated with each record.
Maybe:
"Mapping of module pages to associated module record.  Allocated pages 
are indicated by their record number (starting from 0).  Free pages have 
value 255."?
> > > +#define KEXECMOD_PG_FREE    255
> > > +};
> > > +
> > > +struct kexec_module_rec {
> > > +    uint16_t offset;        /* Offset to record from start of 
> > > page.          */
> > ... but then why would you need an offset?  How do you identify which 
> > "page" or do you mean from the start of the module?
> The record itself needs to contain the data for finding the pages
> associated with it. See patch 18 for the details of the 9pfs record
> added there.
>
> It will use all 17 allocatable pages from the module (1 for the main
> 9pfs shared interface page plus 16 for the shared rings), while the
> record itself will hold the grant reference of the 9pfs interface
> page, which serves as the key for locating the page itself and the
> ring pages.
I was missing the overall view.  I think I have it now from reading 
patch 13.
From kexec_mod_start, we have:
(CONFIG_KEXEC_MODULE_PAGES - 1) pages (allocated as pages - referenced 
by pg2rec)
struct kexec_module *mod_ptr is the start of the final page.
pg2rec[n_pages]
struct kexec_module_rec mod_recs[n_records]
< extra data for each mod_recs found by .offset and .size >

I didn't realize the mod_recs were separate from their "extra data", and 
extra data took up the end of the.  If that's correct, I think it would 
be helpful to describe the overall layout.
Thanks,
Jason

> > > +    uint8_t type;           /* Type of 
> > > record.                               */
> > > +#define KEXECMOD_REC_NONE   0
> > > +    uint8_t size;           /* Size of 
> > > record.                               */
> > > +};
> > > +
> > > +#ifndef CONFIG_KEXEC_MODULE_PAGES
> > > +#define CONFIG_KEXEC_MODULE_PAGES 0
> > > +#endif
> > > +
> > > +extern unsigned long kexec_mod_start;
> > > +extern struct kexec_module *mod_ptr;
> > > +
> > >   /* One element of kexec actions (last element must have action 
> > > KEXEC_CALL): */
> > >   struct kexec_action {
> > >       enum {