On Wed, May 28, 2025 at 11:02:40AM -0500, Aaron Rainbolt wrote:
> On Wed, 28 May 2025 17:12:19 +0200 Daniel Kiper <dki...@net-space.pl> wrote:
>
> > First of all, next time please add Xen-devel ML to recipients too...
>
> Apologies, didn't realize that list would be appropriate. I will add
> them in the next version of the patch.
Thank you!
> > On Wed, Apr 23, 2025 at 09:47:48PM -0500, Aaron Rainbolt wrote:
> > > Enable GRUB to parse the Xen command line for parameters, and expose
> > > certain of those parameters to the GRUB config file (or rescue
> > > shell) as environment variables.
> >
> > Please make the commit message more verbose, i.e., explain why this
> > feature it is needed, how it works, etc.
> >
> > > ---
> > > grub-core/Makefile.core.def | 2 +
> > > grub-core/kern/i386/xen/pvh.c | 16 ++
> > > grub-core/kern/main.c | 12 ++
> > > grub-core/kern/xen/cmdline.c | 270
> > > ++++++++++++++++++++++++++++++++++ include/grub/xen.h |
> > > 2 +
> >
> > I think this feature should have a description in the GRUB docs.
> >
> > > 5 files changed, 302 insertions(+)
> > > create mode 100644 grub-core/kern/xen/cmdline.c
> > >
> > > diff --git a/grub-core/Makefile.core.def
> > > b/grub-core/Makefile.core.def index f70e02e69..79e681a7b 100644
> > > --- a/grub-core/Makefile.core.def
> > > +++ b/grub-core/Makefile.core.def
> > > @@ -248,6 +248,7 @@ kernel = {
> > > xen = term/xen/console.c;
> > > xen = disk/xen/xendisk.c;
> > > xen = commands/boot.c;
> > > + xen = kern/xen/cmdline.c;
> > >
> > > i386_xen_pvh = commands/boot.c;
> > > i386_xen_pvh = disk/xen/xendisk.c;
> > > @@ -255,6 +256,7 @@ kernel = {
> > > i386_xen_pvh = kern/i386/xen/tsc.c;
> > > i386_xen_pvh = kern/i386/xen/pvh.c;
> > > i386_xen_pvh = kern/xen/init.c;
> > > + i386_xen_pvh = kern/xen/cmdline.c;
> > > i386_xen_pvh = term/xen/console.c;
> > >
> > > ia64_efi = kern/ia64/efi/startup.S;
> > > diff --git a/grub-core/kern/i386/xen/pvh.c
> > > b/grub-core/kern/i386/xen/pvh.c index 91fbca859..9fb048082 100644
> > > --- a/grub-core/kern/i386/xen/pvh.c
> > > +++ b/grub-core/kern/i386/xen/pvh.c
> > > @@ -352,6 +352,22 @@ grub_xen_setup_pvh (void)
> > > grub_xen_mm_init_regions ();
> > >
> > > grub_rsdp_addr = pvh_start_info->rsdp_paddr;
> > > + int xen_cmdline_fits = 0;
> >
> > We have bool type in the GRUB.
> >
> > > + char *xen_cmdline = (char *) pvh_start_info->cmdline_paddr;
> >
> > const char?
> >
> > And please add an empty line here.
> >
> > > + for (int i = 0; i < 1024; i++)
> >
> > 1024? Where this number come from? And I suggest using constants where
> > possible.
>
> 1024 is the maximum Xen command line length specified by
> include/xen/xen.h. I think I can just use the MAX_GUEST_CMDLINE define
> set by that file here, is that acceptable?
OK...
> > Additionally, please do not mix code with variable definitions. Define
> > all variables at the beginning of a function or block.
>
> Will do, but this should probably be added to the coding style
> documentation at some point in the future. (It looks like a lot of the
> issues with this patch are coding style issues that I could have
> avoided had the details been documented.)
Yeah, that is good point...
[...]
> > > +void
> > > +grub_parse_xen_cmdline (void)
> > > +{
> > > + word_list = grub_malloc (0);
> >
> > word_list = NULL;
>
> I was going off of the documentation in `man 3 malloc`, which states
> "if size is 0, then malloc() returns a unique pointer value that can
> later be successfully passed to free()`. Looking closer at the memory
> management code, I see `grub_free()` can handle a null pointer, but
POSIX free() accepts NULL too...
[...]
> > > + for (grub_size_t i = 0; i < grub_strlen (cmdline); i++)
> >
> > Is cmdline always NUL terminated? Should not we impose a (sensible)
> > command line length limit in the GRUB code?
>
> If GRUB is started in a PVH VM, `grub_xen_setup_pvh` will check the
> command line to ensure it is NUL-terminated before loading it into the
> `grub_xen_start_page_addr->cmd_line`. In a PV VM, this struct is
> populated by Xen itself and GRUB simply grabs a pointer to it that
> Xen passes it, so I guess in theory a Xen bug could result in this
> command line not being NUL-terminated. That would be a pretty severe
> bug on Xen's part, but I suppose it would be good to double-check just
> in case.
Yep!
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
