From: David Woodhouse <dwmw@amazon.co.uk>
The creation of dom0 can be relatively self-contained. Shift it into
a separate function and simplify __start_xen() a little bit.
This is a cleanup in its own right, but will be even more desireable
when live update provides an alternative path through __start_xen()
that doesn't involve creating a new dom0 at all.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
xen/arch/x86/setup.c | 169 +++++++++++++++++++++++--------------------
1 file changed, 92 insertions(+), 77 deletions(-)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 10209e6bfb..9d86722ecd 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -678,6 +678,92 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
return n;
}
+static struct domain * __init create_dom0(const module_t *image,
+ unsigned long headroom,
+ module_t *initrd, char *kextra,
+ char *loader)
+{
+ struct xen_domctl_createdomain dom0_cfg = {
+ .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
+ .max_evtchn_port = -1,
+ .max_grant_frames = -1,
+ .max_maptrack_frames = -1,
+ };
+ struct domain *d;
+ char *cmdline;
+
+ if ( opt_dom0_pvh )
+ {
+ dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
+ ((hvm_hap_supported() && !opt_dom0_shadow) ?
+ XEN_DOMCTL_CDF_hap : 0));
+
+ dom0_cfg.arch.emulation_flags |=
+ XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
+ }
+ dom0_cfg.max_vcpus = dom0_max_vcpus();
+
+ if ( iommu_enabled )
+ dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+
+ /* Create initial domain 0. */
+ d = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
+ if ( IS_ERR(d) || (alloc_dom0_vcpu0(d) == NULL) )
+ panic("Error creating domain 0\n");
+
+ /* Grab the DOM0 command line. */
+ cmdline = (char *)(image->string ? __va(image->string) : NULL);
+ if ( (cmdline != NULL) || (kextra != NULL) )
+ {
+ static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
+
+ cmdline = cmdline_cook(cmdline, loader);
+ safe_strcpy(dom0_cmdline, cmdline);
+
+ if ( kextra != NULL )
+ /* kextra always includes exactly one leading space. */
+ safe_strcat(dom0_cmdline, kextra);
+
+ /* Append any extra parameters. */
+ if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
+ safe_strcat(dom0_cmdline, " noapic");
+ if ( (strlen(acpi_param) == 0) && acpi_disabled )
+ {
+ printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
+ safe_strcpy(acpi_param, "off");
+ }
+ if ( (strlen(acpi_param) != 0) && !strstr(dom0_cmdline, "acpi=") )
+ {
+ safe_strcat(dom0_cmdline, " acpi=");
+ safe_strcat(dom0_cmdline, acpi_param);
+ }
+
+ cmdline = dom0_cmdline;
+ }
+
+ /*
+ * Temporarily clear SMAP in CR4 to allow user-accesses in construct_dom0().
+ * This saves a large number of corner cases interactions with
+ * copy_from_user().
+ */
+ if ( cpu_has_smap )
+ {
+ cr4_pv32_mask &= ~X86_CR4_SMAP;
+ write_cr4(read_cr4() & ~X86_CR4_SMAP);
+ }
+
+ if ( construct_dom0(d, image, headroom, initrd, cmdline) != 0 )
+ panic("Could not construct domain 0\n");
+
+ if ( cpu_has_smap )
+ {
+ write_cr4(read_cr4() | X86_CR4_SMAP);
+ cr4_pv32_mask |= X86_CR4_SMAP;
+ }
+
+ return d;
+}
+
/* How much of the directmap is prebuilt at compile time. */
#define PREBUILT_MAP_LIMIT (1 << L2_PAGETABLE_SHIFT)
@@ -697,12 +783,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
.parity = 'n',
.stop_bits = 1
};
- struct xen_domctl_createdomain dom0_cfg = {
- .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
- .max_evtchn_port = -1,
- .max_grant_frames = -1,
- .max_maptrack_frames = -1,
- };
const char *hypervisor_name;
/* Critical region without IDT or TSS. Any fault is deadly! */
@@ -1740,58 +1820,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
init_guest_cpuid();
init_guest_msr_policy();
- if ( opt_dom0_pvh )
- {
- dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
- ((hvm_hap_supported() && !opt_dom0_shadow) ?
- XEN_DOMCTL_CDF_hap : 0));
-
- dom0_cfg.arch.emulation_flags |=
- XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
- }
- dom0_cfg.max_vcpus = dom0_max_vcpus();
-
- if ( iommu_enabled )
- dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
-
- /* Create initial domain 0. */
- dom0 = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
- if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
- panic("Error creating domain 0\n");
-
- /* Grab the DOM0 command line. */
- cmdline = (char *)(mod[0].string ? __va(mod[0].string) : NULL);
- if ( (cmdline != NULL) || (kextra != NULL) )
- {
- static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
-
- cmdline = cmdline_cook(cmdline, loader);
- safe_strcpy(dom0_cmdline, cmdline);
-
- if ( kextra != NULL )
- /* kextra always includes exactly one leading space. */
- safe_strcat(dom0_cmdline, kextra);
-
- /* Append any extra parameters. */
- if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
- safe_strcat(dom0_cmdline, " noapic");
- if ( (strlen(acpi_param) == 0) && acpi_disabled )
- {
- printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
- safe_strcpy(acpi_param, "off");
- }
- if ( (strlen(acpi_param) != 0) && !strstr(dom0_cmdline, "acpi=") )
- {
- safe_strcat(dom0_cmdline, " acpi=");
- safe_strcat(dom0_cmdline, acpi_param);
- }
-
- cmdline = dom0_cmdline;
- }
-
if ( xen_cpuidle )
xen_processor_pmbits |= XEN_PROCESSOR_PM_CX;
+ printk("%sNX (Execute Disable) protection %sactive\n",
+ cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
+ cpu_has_nx ? "" : "not ");
+
initrdidx = find_first_bit(module_map, mbi->mods_count);
if ( initrdidx < mbi->mods_count )
initrd = mod + initrdidx;
@@ -1801,34 +1836,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
"Multiple initrd candidates, picking module #%u\n",
initrdidx);
- /*
- * Temporarily clear SMAP in CR4 to allow user-accesses in construct_dom0().
- * This saves a large number of corner cases interactions with
- * copy_from_user().
- */
- if ( cpu_has_smap )
- {
- cr4_pv32_mask &= ~X86_CR4_SMAP;
- write_cr4(read_cr4() & ~X86_CR4_SMAP);
- }
-
- printk("%sNX (Execute Disable) protection %sactive\n",
- cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
- cpu_has_nx ? "" : "not ");
-
/*
* We're going to setup domain0 using the module(s) that we stashed safely
* above our heap. The second module, if present, is an initrd ramdisk.
*/
- if ( construct_dom0(dom0, mod, modules_headroom, initrd, cmdline) != 0 )
+ dom0 = create_dom0(mod, modules_headroom, initrd, kextra, loader);
+ if ( dom0 == NULL )
panic("Could not set up DOM0 guest OS\n");
- if ( cpu_has_smap )
- {
- write_cr4(read_cr4() | X86_CR4_SMAP);
- cr4_pv32_mask |= X86_CR4_SMAP;
- }
-
heap_init_late();
init_trace_bufs();
--
2.21.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 01/02/2020 00:33, David Woodhouse wrote:
> if ( xen_cpuidle )
> xen_processor_pmbits |= XEN_PROCESSOR_PM_CX;
>
> + printk("%sNX (Execute Disable) protection %sactive\n",
> + cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
> + cpu_has_nx ? "" : "not ");
> +
The placement of printk shouldn't matter but the change feels a bit
out-of-context. Would you mind to explain it in the commit message?
> initrdidx = find_first_bit(module_map, mbi->mods_count);
> if ( initrdidx < mbi->mods_count )
> initrd = mod + initrdidx;
> @@ -1801,34 +1836,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> "Multiple initrd candidates, picking module #%u\n",
> initrdidx);
>
> - /*
> - * Temporarily clear SMAP in CR4 to allow user-accesses in construct_dom0().
> - * This saves a large number of corner cases interactions with
> - * copy_from_user().
> - */
> - if ( cpu_has_smap )
> - {
> - cr4_pv32_mask &= ~X86_CR4_SMAP;
> - write_cr4(read_cr4() & ~X86_CR4_SMAP);
> - }
> -
> - printk("%sNX (Execute Disable) protection %sactive\n",
> - cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
> - cpu_has_nx ? "" : "not ");
> -
[...]
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Mon, 2020-02-03 at 14:28 +0000, Julien Grall wrote:
> The placement of printk shouldn't matter but the change feels a bit
> out-of-context. Would you mind to explain it in the commit message?
I didn't really intend to move the printk up; what I intended to do was
move the setting of 'initrd' down, so that it's right before the
create_dom0() call that it is preparing for. Which is purely cosmetic
for now, and more practical later when all that goes into a single
conditional for the non-live-update boot (as shown below).
Will update the commit message to note this; thanks.
printk("%sNX (Execute Disable) protection %sactive\n",
cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
cpu_has_nx ? "" : "not ");
if ( lu_breadcrumb_phys )
{
dom0 = lu_restore_domains(&lu_stream);
if ( dom0 == NULL )
panic("No DOM0 found in live update data\n");
lu_stream_free(&lu_stream);
}
else
{
initrdidx = find_first_bit(module_map, mbi->mods_count);
if ( initrdidx < mbi->mods_count )
initrd = mod + initrdidx;
if ( bitmap_weight(module_map, mbi->mods_count) > 1 )
printk(XENLOG_WARNING
"Multiple initrd candidates, picking module #%u\n",
initrdidx);
/*
* We're going to setup domain0 using the module(s) that we
* stashed safely above our heap. The second module, if
* present, is an initrd ramdisk.
*/
dom0 = create_dom0(mod, modules_headroom, initrd, kextra, loader);
if ( dom0 == NULL )
panic("Could not set up DOM0 guest OS\n");
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 01.02.2020 01:33, David Woodhouse wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -678,6 +678,92 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
> return n;
> }
>
> +static struct domain * __init create_dom0(const module_t *image,
> + unsigned long headroom,
> + module_t *initrd, char *kextra,
> + char *loader)
Can any of these last three be pointer-to-const?
> +{
> + struct xen_domctl_createdomain dom0_cfg = {
> + .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> + .max_evtchn_port = -1,
> + .max_grant_frames = -1,
> + .max_maptrack_frames = -1,
> + };
> + struct domain *d;
> + char *cmdline;
> +
> + if ( opt_dom0_pvh )
> + {
> + dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
> + ((hvm_hap_supported() && !opt_dom0_shadow) ?
> + XEN_DOMCTL_CDF_hap : 0));
> +
> + dom0_cfg.arch.emulation_flags |=
> + XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
> + }
> + dom0_cfg.max_vcpus = dom0_max_vcpus();
Can this not be part of the initializer now?
> + if ( iommu_enabled )
> + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> +
> + /* Create initial domain 0. */
> + d = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
> + if ( IS_ERR(d) || (alloc_dom0_vcpu0(d) == NULL) )
> + panic("Error creating domain 0\n");
> +
> + /* Grab the DOM0 command line. */
> + cmdline = (char *)(image->string ? __va(image->string) : NULL);
Is this cast needed? (I know you're only moving the code, but some
easy cleanup would be nice anyway.)
> + if ( (cmdline != NULL) || (kextra != NULL) )
Similarly here you may want to consider shortening to
if ( cmdline || kextra )
At least one more similar case further down.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, 2020-02-21 at 18:06 +0100, Jan Beulich wrote:
> On 01.02.2020 01:33, David Woodhouse wrote:
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -678,6 +678,92 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
> > return n;
> > }
> >
> > +static struct domain * __init create_dom0(const module_t *image,
> > + unsigned long headroom,
> > + module_t *initrd, char *kextra,
> > + char *loader)
>
> Can any of these last three be pointer-to-const?
I suppose kextra can. The other two are passed on to construct_dom0(),
which could perhaps be changed to take const pointers but that's a
separate cleanup.
> > +{
> > + struct xen_domctl_createdomain dom0_cfg = {
> > + .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> > + .max_evtchn_port = -1,
> > + .max_grant_frames = -1,
> > + .max_maptrack_frames = -1,
> > + };
> > + struct domain *d;
> > + char *cmdline;
> > +
> > + if ( opt_dom0_pvh )
> > + {
> > + dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
> > + ((hvm_hap_supported() && !opt_dom0_shadow) ?
> > + XEN_DOMCTL_CDF_hap : 0));
> > +
> > + dom0_cfg.arch.emulation_flags |=
> > + XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
> > + }
> > + dom0_cfg.max_vcpus = dom0_max_vcpus();
>
> Can this not be part of the initializer now?
Yes, I suppose it can. Fixed.
> > + if ( iommu_enabled )
> > + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> > +
> > + /* Create initial domain 0. */
> > + d = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
> > + if ( IS_ERR(d) || (alloc_dom0_vcpu0(d) == NULL) )
> > + panic("Error creating domain 0\n");
> > +
> > + /* Grab the DOM0 command line. */
> > + cmdline = (char *)(image->string ? __va(image->string) : NULL);
>
> Is this cast needed? (I know you're only moving the code, but some
> easy cleanup would be nice anyway.)
>
> > + if ( (cmdline != NULL) || (kextra != NULL) )
>
> Similarly here you may want to consider shortening to
>
> if ( cmdline || kextra )
>
> At least one more similar case further down.
Makes sense. Done too; thanks.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2026 Red Hat, Inc.