| Message ID | 20200201003303.2363081-8-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Early cleanups and bug fixes in preparation for live update | expand |
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,
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"); }
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
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.
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();