On 26.12.2024 17:57, Daniel P. Smith wrote:
> @@ -759,9 +758,10 @@ static int __init pvh_load_kernel(
> /* Free temporary buffers. */
> free_boot_modules();
>
> - if ( cmdline != NULL )
> + if ( bd->cmdline != NULL )
> {
> - rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1,
> v);
> + rc = hvm_copy_to_guest_phys(
> + last_addr, bd->cmdline, strlen(bd->cmdline) + 1, v);
Nit: Indentation. The anchor point for this kind of increased indentation
is the function name being called, so you want to add one more blank. (It
is not N times the usual indentation of 4, until "it looks okay".)
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -11,6 +11,8 @@
> #define __XEN_X86_BOOTDOMAIN_H__
>
> struct boot_domain {
> + const char *cmdline;
> +
> domid_t domid;
>
> struct boot_module *kernel;
I can see why in the earlier patch you added domid at the top. But cmdline?
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -975,10 +975,29 @@ static unsigned int __init copy_bios_e820(struct
> e820entry *map, unsigned int li
> return n;
> }
>
> -static struct domain *__init create_dom0(struct boot_info *bi)
> +static size_t __init domain_cmdline_size(
> + struct boot_info *bi, struct boot_domain *bd)
> {
> - static char __initdata cmdline[MAX_GUEST_CMDLINE];
> + size_t s = bi->kextra ? strlen(bi->kextra) : 0;
> +
> + s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
> +
> + if ( s == 0 )
> + return s;
> +
> + /*
> + * Certain parameters from the Xen command line may be added to the dom0
> + * command line. Add additional space for the possible cases along with
> one
> + * extra char to hold \0.
> + */
> + s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;
See below; I question this all being necessary for PVH Dom0.
> @@ -1018,39 +1037,52 @@ static struct domain *__init create_dom0(struct
> boot_info *bi)
> panic("Error creating d%uv0\n", bd->domid);
>
> /* Grab the DOM0 command line. */
> - if ( bd->kernel->cmdline_pa || bi->kextra )
> + if ( (bd->kernel->cmdline_pa &&
> + ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
> + bi->kextra )
> {
> - if ( bd->kernel->cmdline_pa )
> - safe_strcpy(cmdline,
> - cmdline_cook(__va(bd->kernel->cmdline_pa),
> bi->loader));
> + size_t cmdline_size = domain_cmdline_size(bi, bd);
> +
> + if ( cmdline_size )
> + {
> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
> + panic("Error allocating cmdline buffer for %pd\n", d);
>
> - if ( bi->kextra )
> - /* kextra always includes exactly one leading space. */
> - safe_strcat(cmdline, bi->kextra);
> + if ( bd->kernel->cmdline_pa )
> + strlcpy(cmdline,
> + cmdline_cook(__va(bd->kernel->cmdline_pa),
> bi->loader),
> + cmdline_size);
>
> - /* Append any extra parameters. */
> - if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> - safe_strcat(cmdline, " noapic");
> + if ( bi->kextra )
> + /* kextra always includes exactly one leading space. */
> + strlcat(cmdline, bi->kextra, cmdline_size);
>
> - if ( (strlen(acpi_param) == 0) && acpi_disabled )
> - {
> - printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> - safe_strcpy(acpi_param, "off");
> - }
> + /* Append any extra parameters. */
> + if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> + strlcat(cmdline, " noapic", cmdline_size);
Roger - this isn't going to work very well with PVH Dom0, is it?
> - if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> - {
> - safe_strcat(cmdline, " acpi=");
> - safe_strcat(cmdline, acpi_param);
> - }
> + if ( (strlen(acpi_param) == 0) && acpi_disabled )
Not sure whether the compiler will do that transformation anyway, but this
check looks odd to me. Why not simply check whether the first char is the
nul one?
> + {
> + printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> + safe_strcpy(acpi_param, "off");
> + }
Here I'm doubtful, too, when it comes to PVH Dom0. If Xen's even works in
this mode anymore at all, I don't think we can sensibly start a PVH Dom0
then.
(This is leaving aside that all of this is Linux-centric anyway.)
> - bd->kernel->cmdline_pa = __pa(cmdline);
> + if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> + {
> + strlcat(cmdline, " acpi=", cmdline_size);
> + strlcat(cmdline, acpi_param, cmdline_size);
> + }
(This, btw, won't work quite well when acpi= is specified more than once
on the command line.)
> + bd->kernel->cmdline_pa = 0;
> + bd->cmdline = cmdline;
> + }
> }
>
> bd->d = d;
> if ( construct_dom0(bd) != 0 )
> panic("Could not construct domain 0\n");
>
> + xfree(cmdline);
Leaving bd->cmdline dangling?
Jan
