| Message ID | 20210308115610.48203-1-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Prevent Dom0 to be loaded when using dom0less | expand |
Hi Luca, On 08/03/2021 11:56, Luca Fancellu wrote: > This patch prevents the dom0 to be loaded skipping its > building and going forward to build domUs when the dom0 > kernel is not found and at least one domU is present. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> I have received 3 versions of this patch, can you clarify I should review? If the modification is just the CC list, then we usually add RESEND in the subject. For all the other modifications, we bump the version (vX in suject) and provide a changelog. Cheers,
> On 8 Mar 2021, at 11:59, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 08/03/2021 11:56, Luca Fancellu wrote: >> This patch prevents the dom0 to be loaded skipping its >> building and going forward to build domUs when the dom0 >> kernel is not found and at least one domU is present. >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > I have received 3 versions of this patch, can you clarify I should review? Hi Julien, I’m really sorry for the noise, because of a problem in my configuration I sent twice the same patch and it comes with a Change-Id: , this is the correct one without the Change-Id. > > If the modification is just the CC list, then we usually add RESEND in the subject. For all the other modifications, we bump the version (vX in suject) and provide a changelog. Sure, next time I will add something to the changelog. > > Cheers, > > -- > Julien Grall > Cheers, Luca
Hi Luca, On 08/03/2021 11:56, Luca Fancellu wrote: > This patch prevents the dom0 to be loaded skipping its > building and going forward to build domUs when the dom0 > kernel is not found and at least one domU is present. As you are skipping dom0, the domid 0 will not be usable for another domain. I can see a few issues: 1) The first domU created will now be considered as the hardware domain (see domain_create()). 2) There are still a few hardcoded use of d->domain_id == 0 in the codebase (I could spot at least on in the RTDS code). 3) Not all the code seems to be able to cope with hardware_domain is NULL (although most of it looks to be only reachable by x86)? 4) is_hardware_domain() will return true when passing NULL. It is not clear whether one may pass NULL here. For 2), ideally this needs to be fixed. But we may also want to reserve domid 0 just for sanity. For 3) and 4), you will need to go through the code and check the usage. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > xen/arch/arm/setup.c | 83 +++++++++++++++++++++++++++++++------------- > 1 file changed, 59 insertions(+), 24 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 2532ec9739..6d169ff6ce 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -794,6 +794,35 @@ static void __init setup_mm(void) > } > #endif > > +static bool __init is_dom0less_mode(void) > +{ > + struct bootmodules *mods = &bootinfo.modules; > + struct bootmodule *mod; > + unsigned int i; > + bool dom0found = false; > + bool domUfound = false; > + > + /* Look into the bootmodules */ > + for ( i = 0 ; i < mods->nr_mods ; i++ ) > + { > + mod = &mods->module[i]; > + /* Find if dom0 and domU kernels are present */ > + if ( mod->kind == BOOTMOD_KERNEL ) > + { > + if ( mod->domU == false ) > + dom0found = true; > + else > + domUfound = true; > + } > + } > + > + /* > + * If there is no dom0 kernel but at least one domU, then we are in > + * dom0less mode > + */ > + return ( !dom0found && domUfound ); > +} Should the documentation be updated to reflect this change? > + > size_t __read_mostly dcache_line_bytes; > > /* C entry point for boot CPU */ > @@ -804,7 +833,7 @@ void __init start_xen(unsigned long boot_phys_offset, > int cpus, i; > const char *cmdline; > struct bootmodule *xen_bootmodule; > - struct domain *dom0; > + struct domain *dom0 = NULL; > struct xen_domctl_createdomain dom0_cfg = { > .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, > .max_evtchn_port = -1, > @@ -964,28 +993,33 @@ void __init start_xen(unsigned long boot_phys_offset, > apply_alternatives_all(); > enable_errata_workarounds(); > > - /* Create initial domain 0. */ > - /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > - dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; > - /* > - * Xen vGIC supports a maximum of 992 interrupt lines. > - * 32 are substracted to cover local IRQs. > - */ > - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; > - if ( gic_number_lines() > 992 ) > - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); > - dom0_cfg.arch.tee_type = tee_get_type(); > - dom0_cfg.max_vcpus = dom0_max_vcpus(); > - > - if ( iommu_enabled ) > - dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; > - > - dom0 = domain_create(0, &dom0_cfg, true); > - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) > - panic("Error creating domain 0\n"); > - > - if ( construct_dom0(dom0) != 0) > - panic("Could not set up DOM0 guest OS\n"); > + if ( !is_dom0less_mode() ) > + { > + /* Create initial domain 0. */ > + /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > + dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; > + /* > + * Xen vGIC supports a maximum of 992 interrupt lines. > + * 32 are substracted to cover local IRQs. > + */ > + dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; > + if ( gic_number_lines() > 992 ) > + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); > + dom0_cfg.arch.tee_type = tee_get_type(); > + dom0_cfg.max_vcpus = dom0_max_vcpus(); > + > + if ( iommu_enabled ) > + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; > + > + dom0 = domain_create(0, &dom0_cfg, true); > + if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) > + panic("Error creating domain 0\n"); > + > + if ( construct_dom0(dom0) != 0) > + panic("Could not set up DOM0 guest OS\n"); > + } It always felt a bit strange the dom0 creation is partly happening in setup.c when for domU everythink will happen in domain_build.c. Woule you be able to create a patch that will first move the code in a new function (maybe create_dom0())? The function would return NULL in case of an error or the domain. > + else > + printk(XENLOG_INFO "Xen dom0less mode detected\n"); > > heap_init_late(); > > @@ -1003,7 +1037,8 @@ void __init start_xen(unsigned long boot_phys_offset, > if ( acpi_disabled ) > create_domUs(); > > - domain_unpause_by_systemcontroller(dom0); > + if ( dom0 ) > + domain_unpause_by_systemcontroller(dom0); > > /* Switch on to the dynamically allocated stack for the idle vcpu > * since the static one we're running on is about to be freed. */ > Cheers,
On 08.03.2021 15:12, Julien Grall wrote: > On 08/03/2021 11:56, Luca Fancellu wrote: >> This patch prevents the dom0 to be loaded skipping its >> building and going forward to build domUs when the dom0 >> kernel is not found and at least one domU is present. > > As you are skipping dom0, the domid 0 will not be usable for another > domain. I can see a few issues: > 1) The first domU created will now be considered as the hardware > domain (see domain_create()). > 2) There are still a few hardcoded use of d->domain_id == 0 in the > codebase (I could spot at least on in the RTDS code). > 3) Not all the code seems to be able to cope with hardware_domain is > NULL (although most of it looks to be only reachable by x86)? > 4) is_hardware_domain() will return true when passing NULL. It is > not clear whether one may pass NULL here. > > For 2), ideally this needs to be fixed. But we may also want to reserve > domid 0 just for sanity. +1 to reserving ID zero in such a case. Jan
On Mon, 8 Mar 2021, Jan Beulich wrote: > On 08.03.2021 15:12, Julien Grall wrote: > > On 08/03/2021 11:56, Luca Fancellu wrote: > >> This patch prevents the dom0 to be loaded skipping its > >> building and going forward to build domUs when the dom0 > >> kernel is not found and at least one domU is present. > > > > As you are skipping dom0, the domid 0 will not be usable for another > > domain. I can see a few issues: > > 1) The first domU created will now be considered as the hardware > > domain (see domain_create()). > > 2) There are still a few hardcoded use of d->domain_id == 0 in the > > codebase (I could spot at least on in the RTDS code). > > 3) Not all the code seems to be able to cope with hardware_domain is > > NULL (although most of it looks to be only reachable by x86)? > > 4) is_hardware_domain() will return true when passing NULL. It is > > not clear whether one may pass NULL here. > > > > For 2), ideally this needs to be fixed. But we may also want to reserve > > domid 0 just for sanity. > > +1 to reserving ID zero in such a case. I agree too
On Mon, 8 Mar 2021, Luca Fancellu wrote: > This patch prevents the dom0 to be loaded skipping its > building and going forward to build domUs when the dom0 > kernel is not found and at least one domU is present. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > xen/arch/arm/setup.c | 83 +++++++++++++++++++++++++++++++------------- > 1 file changed, 59 insertions(+), 24 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 2532ec9739..6d169ff6ce 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -794,6 +794,35 @@ static void __init setup_mm(void) > } > #endif > > +static bool __init is_dom0less_mode(void) > +{ > + struct bootmodules *mods = &bootinfo.modules; > + struct bootmodule *mod; > + unsigned int i; > + bool dom0found = false; > + bool domUfound = false; > + > + /* Look into the bootmodules */ > + for ( i = 0 ; i < mods->nr_mods ; i++ ) > + { > + mod = &mods->module[i]; > + /* Find if dom0 and domU kernels are present */ > + if ( mod->kind == BOOTMOD_KERNEL ) > + { > + if ( mod->domU == false ) > + dom0found = true; > + else > + domUfound = true; > + } > + } > + > + /* > + * If there is no dom0 kernel but at least one domU, then we are in > + * dom0less mode > + */ > + return ( !dom0found && domUfound ); > +} This code looks correct to me. Julien's suggestion of updating the docs and moving dom0 creation to domain_build.c are good ones. And also I think reserving domain_id 0 is really important to avoid errors and should be done as part of this patch too. > size_t __read_mostly dcache_line_bytes; > > /* C entry point for boot CPU */ > @@ -804,7 +833,7 @@ void __init start_xen(unsigned long boot_phys_offset, > int cpus, i; > const char *cmdline; > struct bootmodule *xen_bootmodule; > - struct domain *dom0; > + struct domain *dom0 = NULL; > struct xen_domctl_createdomain dom0_cfg = { > .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, > .max_evtchn_port = -1, > @@ -964,28 +993,33 @@ void __init start_xen(unsigned long boot_phys_offset, > apply_alternatives_all(); > enable_errata_workarounds(); > > - /* Create initial domain 0. */ > - /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > - dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; > - /* > - * Xen vGIC supports a maximum of 992 interrupt lines. > - * 32 are substracted to cover local IRQs. > - */ > - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; > - if ( gic_number_lines() > 992 ) > - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); > - dom0_cfg.arch.tee_type = tee_get_type(); > - dom0_cfg.max_vcpus = dom0_max_vcpus(); > - > - if ( iommu_enabled ) > - dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; > - > - dom0 = domain_create(0, &dom0_cfg, true); > - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) > - panic("Error creating domain 0\n"); > - > - if ( construct_dom0(dom0) != 0) > - panic("Could not set up DOM0 guest OS\n"); > + if ( !is_dom0less_mode() ) > + { > + /* Create initial domain 0. */ > + /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > + dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; > + /* > + * Xen vGIC supports a maximum of 992 interrupt lines. > + * 32 are substracted to cover local IRQs. > + */ > + dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; > + if ( gic_number_lines() > 992 ) > + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); > + dom0_cfg.arch.tee_type = tee_get_type(); > + dom0_cfg.max_vcpus = dom0_max_vcpus(); > + > + if ( iommu_enabled ) > + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; > + > + dom0 = domain_create(0, &dom0_cfg, true); > + if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) > + panic("Error creating domain 0\n"); > + > + if ( construct_dom0(dom0) != 0) > + panic("Could not set up DOM0 guest OS\n"); > + } > + else > + printk(XENLOG_INFO "Xen dom0less mode detected\n"); > > heap_init_late(); > > @@ -1003,7 +1037,8 @@ void __init start_xen(unsigned long boot_phys_offset, > if ( acpi_disabled ) > create_domUs(); > > - domain_unpause_by_systemcontroller(dom0); > + if ( dom0 ) > + domain_unpause_by_systemcontroller(dom0); > > /* Switch on to the dynamically allocated stack for the idle vcpu > * since the static one we're running on is about to be freed. */ > -- > 2.17.1 >
Hi all, > On 8 Mar 2021, at 14:12, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 08/03/2021 11:56, Luca Fancellu wrote: >> This patch prevents the dom0 to be loaded skipping its >> building and going forward to build domUs when the dom0 >> kernel is not found and at least one domU is present. > > As you are skipping dom0, the domid 0 will not be usable for another domain. I can see a few issues: > 1) The first domU created will now be considered as the hardware domain (see domain_create()). > 2) There are still a few hardcoded use of d->domain_id == 0 in the codebase (I could spot at least on in the RTDS code). > 3) Not all the code seems to be able to cope with hardware_domain is NULL (although most of it looks to be only reachable by x86)? > 4) is_hardware_domain() will return true when passing NULL. It is not clear whether one may pass NULL here. > > For 2), ideally this needs to be fixed. But we may also want to reserve domid 0 just for sanity. > > For 3) and 4), you will need to go through the code and check the usage. I’m investigating these points, but I agree with you all that domid 0 should be reserved. > >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> xen/arch/arm/setup.c | 83 +++++++++++++++++++++++++++++++------------- >> 1 file changed, 59 insertions(+), 24 deletions(-) >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 2532ec9739..6d169ff6ce 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -794,6 +794,35 @@ static void __init setup_mm(void) >> } >> #endif >> +static bool __init is_dom0less_mode(void) >> +{ >> + struct bootmodules *mods = &bootinfo.modules; >> + struct bootmodule *mod; >> + unsigned int i; >> + bool dom0found = false; >> + bool domUfound = false; >> + >> + /* Look into the bootmodules */ >> + for ( i = 0 ; i < mods->nr_mods ; i++ ) >> + { >> + mod = &mods->module[i]; >> + /* Find if dom0 and domU kernels are present */ >> + if ( mod->kind == BOOTMOD_KERNEL ) >> + { >> + if ( mod->domU == false ) >> + dom0found = true; >> + else >> + domUfound = true; >> + } >> + } >> + >> + /* >> + * If there is no dom0 kernel but at least one domU, then we are in >> + * dom0less mode >> + */ >> + return ( !dom0found && domUfound ); >> +} > Should the documentation be updated to reflect this change? Sure I will update the documentation in the v2 patch > >> + >> size_t __read_mostly dcache_line_bytes; >> /* C entry point for boot CPU */ >> @@ -804,7 +833,7 @@ void __init start_xen(unsigned long boot_phys_offset, >> int cpus, i; >> const char *cmdline; >> struct bootmodule *xen_bootmodule; >> - struct domain *dom0; >> + struct domain *dom0 = NULL; >> struct xen_domctl_createdomain dom0_cfg = { >> .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, >> .max_evtchn_port = -1, >> @@ -964,28 +993,33 @@ void __init start_xen(unsigned long boot_phys_offset, >> apply_alternatives_all(); >> enable_errata_workarounds(); >> - /* Create initial domain 0. */ >> - /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >> - dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >> - /* >> - * Xen vGIC supports a maximum of 992 interrupt lines. >> - * 32 are substracted to cover local IRQs. >> - */ >> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >> - if ( gic_number_lines() > 992 ) >> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >> - dom0_cfg.arch.tee_type = tee_get_type(); >> - dom0_cfg.max_vcpus = dom0_max_vcpus(); >> - >> - if ( iommu_enabled ) >> - dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; >> - >> - dom0 = domain_create(0, &dom0_cfg, true); >> - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) >> - panic("Error creating domain 0\n"); >> - >> - if ( construct_dom0(dom0) != 0) >> - panic("Could not set up DOM0 guest OS\n"); >> + if ( !is_dom0less_mode() ) >> + { >> + /* Create initial domain 0. */ >> + /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >> + dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >> + /* >> + * Xen vGIC supports a maximum of 992 interrupt lines. >> + * 32 are substracted to cover local IRQs. >> + */ >> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >> + if ( gic_number_lines() > 992 ) >> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >> + dom0_cfg.arch.tee_type = tee_get_type(); >> + dom0_cfg.max_vcpus = dom0_max_vcpus(); >> + >> + if ( iommu_enabled ) >> + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; >> + >> + dom0 = domain_create(0, &dom0_cfg, true); >> + if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) >> + panic("Error creating domain 0\n"); >> + >> + if ( construct_dom0(dom0) != 0) >> + panic("Could not set up DOM0 guest OS\n"); >> + } > > It always felt a bit strange the dom0 creation is partly happening in setup.c when for domU everythink will happen in domain_build.c. > > Woule you be able to create a patch that will first move the code in a new function (maybe create_dom0())? The function would return NULL in case of an error or the domain. Yes I will create a new patch with this change and I will put on top the v2 dom0less patch > >> + else >> + printk(XENLOG_INFO "Xen dom0less mode detected\n"); >> heap_init_late(); >> @@ -1003,7 +1037,8 @@ void __init start_xen(unsigned long boot_phys_offset, >> if ( acpi_disabled ) >> create_domUs(); >> - domain_unpause_by_systemcontroller(dom0); >> + if ( dom0 ) >> + domain_unpause_by_systemcontroller(dom0); >> /* Switch on to the dynamically allocated stack for the idle vcpu >> * since the static one we're running on is about to be freed. */ > > Cheers, > > -- > Julien Grall Thank you for your feedbacks. Cheers, Luca
Hi Luca, On 12/03/2021 09:38, Luca Fancellu wrote: >>> + >>> size_t __read_mostly dcache_line_bytes; >>> /* C entry point for boot CPU */ >>> @@ -804,7 +833,7 @@ void __init start_xen(unsigned long boot_phys_offset, >>> int cpus, i; >>> const char *cmdline; >>> struct bootmodule *xen_bootmodule; >>> - struct domain *dom0; >>> + struct domain *dom0 = NULL; >>> struct xen_domctl_createdomain dom0_cfg = { >>> .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, >>> .max_evtchn_port = -1, >>> @@ -964,28 +993,33 @@ void __init start_xen(unsigned long boot_phys_offset, >>> apply_alternatives_all(); >>> enable_errata_workarounds(); >>> - /* Create initial domain 0. */ >>> - /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>> - dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>> - /* >>> - * Xen vGIC supports a maximum of 992 interrupt lines. >>> - * 32 are substracted to cover local IRQs. >>> - */ >>> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>> - if ( gic_number_lines() > 992 ) >>> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>> - dom0_cfg.arch.tee_type = tee_get_type(); >>> - dom0_cfg.max_vcpus = dom0_max_vcpus(); >>> - >>> - if ( iommu_enabled ) >>> - dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; >>> - >>> - dom0 = domain_create(0, &dom0_cfg, true); >>> - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) >>> - panic("Error creating domain 0\n"); >>> - >>> - if ( construct_dom0(dom0) != 0) >>> - panic("Could not set up DOM0 guest OS\n"); >>> + if ( !is_dom0less_mode() ) >>> + { >>> + /* Create initial domain 0. */ >>> + /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>> + dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>> + /* >>> + * Xen vGIC supports a maximum of 992 interrupt lines. >>> + * 32 are substracted to cover local IRQs. >>> + */ >>> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>> + if ( gic_number_lines() > 992 ) >>> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>> + dom0_cfg.arch.tee_type = tee_get_type(); >>> + dom0_cfg.max_vcpus = dom0_max_vcpus(); >>> + >>> + if ( iommu_enabled ) >>> + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; >>> + >>> + dom0 = domain_create(0, &dom0_cfg, true); >>> + if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) >>> + panic("Error creating domain 0\n"); >>> + >>> + if ( construct_dom0(dom0) != 0) >>> + panic("Could not set up DOM0 guest OS\n"); >>> + } >> >> It always felt a bit strange the dom0 creation is partly happening in setup.c when for domU everythink will happen in domain_build.c. >> >> Woule you be able to create a patch that will first move the code in a new function (maybe create_dom0())? The function would return NULL in case of an error or the domain. > > Yes I will create a new patch with this change and I will put on top the v2 dom0less patch I think it would be better to put it first. This will avoid some churn if the code movmement comes second (you would first indent and then move the code). Cheers,
Hi, I’ve checked the common code and the arm part, I can confirm that the domid 0 is never allocated even if the domain 0 is not present, here the only places where domain_create(…) is called using a variable value: 1) xen/arch/arm/domain_build.c d = domain_create(++max_init_domid, &d_cfg, false); Where max_init_domid has value 0 and it is defined in setup.c 2) xen/common/domctl.c d = domain_create(dom, &op->u.createdomain, false); For me seems that the dom variable won’t take the 0 value, if someone could give another feedback it would be great. On every other part where domain_create(…) is used, it is called with a constant value different from 0. For the hardware_domain being NULL and not handled in some situation, it seems that it’s not directly related to this patch, but I can handle it on a next serie, from a quick look it seems that many cases can be handled by checking if the domain is NULL in is_hardware_domain(…). So, if the community agree, I can push a v2 patch with all the discussed things (moving dom0 creation code) Cheers, Luca > On 12 Mar 2021, at 11:31, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 12/03/2021 09:38, Luca Fancellu wrote: >>>> + >>>> size_t __read_mostly dcache_line_bytes; >>>> /* C entry point for boot CPU */ >>>> @@ -804,7 +833,7 @@ void __init start_xen(unsigned long boot_phys_offset, >>>> int cpus, i; >>>> const char *cmdline; >>>> struct bootmodule *xen_bootmodule; >>>> - struct domain *dom0; >>>> + struct domain *dom0 = NULL; >>>> struct xen_domctl_createdomain dom0_cfg = { >>>> .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, >>>> .max_evtchn_port = -1, >>>> @@ -964,28 +993,33 @@ void __init start_xen(unsigned long boot_phys_offset, >>>> apply_alternatives_all(); >>>> enable_errata_workarounds(); >>>> - /* Create initial domain 0. */ >>>> - /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>>> - dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>>> - /* >>>> - * Xen vGIC supports a maximum of 992 interrupt lines. >>>> - * 32 are substracted to cover local IRQs. >>>> - */ >>>> - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>>> - if ( gic_number_lines() > 992 ) >>>> - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>>> - dom0_cfg.arch.tee_type = tee_get_type(); >>>> - dom0_cfg.max_vcpus = dom0_max_vcpus(); >>>> - >>>> - if ( iommu_enabled ) >>>> - dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; >>>> - >>>> - dom0 = domain_create(0, &dom0_cfg, true); >>>> - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) >>>> - panic("Error creating domain 0\n"); >>>> - >>>> - if ( construct_dom0(dom0) != 0) >>>> - panic("Could not set up DOM0 guest OS\n"); >>>> + if ( !is_dom0less_mode() ) >>>> + { >>>> + /* Create initial domain 0. */ >>>> + /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >>>> + dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >>>> + /* >>>> + * Xen vGIC supports a maximum of 992 interrupt lines. >>>> + * 32 are substracted to cover local IRQs. >>>> + */ >>>> + dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; >>>> + if ( gic_number_lines() > 992 ) >>>> + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); >>>> + dom0_cfg.arch.tee_type = tee_get_type(); >>>> + dom0_cfg.max_vcpus = dom0_max_vcpus(); >>>> + >>>> + if ( iommu_enabled ) >>>> + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; >>>> + >>>> + dom0 = domain_create(0, &dom0_cfg, true); >>>> + if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) >>>> + panic("Error creating domain 0\n"); >>>> + >>>> + if ( construct_dom0(dom0) != 0) >>>> + panic("Could not set up DOM0 guest OS\n"); >>>> + } >>> >>> It always felt a bit strange the dom0 creation is partly happening in setup.c when for domU everythink will happen in domain_build.c. >>> >>> Woule you be able to create a patch that will first move the code in a new function (maybe create_dom0())? The function would return NULL in case of an error or the domain. >> Yes I will create a new patch with this change and I will put on top the v2 dom0less patch > > I think it would be better to put it first. This will avoid some churn if the code movmement comes second (you would first indent and then move the code). > > Cheers, > > -- > Julien Grall
On 17/03/2021 17:04, Luca Fancellu wrote: > Hi, Hi Luca, > I’ve checked the common code and the arm part, I can confirm that the domid 0 is never allocated even if the domain 0 is not present, here the only places where domain_create(…) is called using a variable value: Thanks for checking it! > 1) xen/arch/arm/domain_build.c > d = domain_create(++max_init_domid, &d_cfg, false); > Where max_init_domid has value 0 and it is defined in setup.c We might want to add a comment on top of this code to explain why the '++a' rather than 'a++'. > > 2) xen/common/domctl.c > d = domain_create(dom, &op->u.createdomain, false); > For me seems that the dom variable won’t take the 0 value, if someone could give another feedback it would be great. > > On every other part where domain_create(…) is used, it is called with a constant value different from 0. I agree with the analysis. However, I feel this is fragile because we rely on the caller to never pass 0. But it looks like domain_create() doesn't check if the ID is already used. So it would already be possible to overwrite hardware_domain. Therefore, this can probably be deffered. > > For the hardware_domain being NULL and not handled in some situation, it seems that it’s not directly related to this patch, but I can handle it on a next serie, from a quick look it seems that many cases can be handled by checking if the domain is NULL in is_hardware_domain(…). Before this series, it is not possible to have hardware_domain == NULL at runtime because dom0 is always created. Cheers,
Hi Julien, I will create a new serie with all the improvements we have discussed. Thank you for your time. Cheers, Luca > On 18 Mar 2021, at 11:13, Julien Grall <julien@xen.org> wrote: > > > > On 17/03/2021 17:04, Luca Fancellu wrote: >> Hi, > > Hi Luca, > >> I’ve checked the common code and the arm part, I can confirm that the domid 0 is never allocated even if the domain 0 is not present, here the only places where domain_create(…) is called using a variable value: > > Thanks for checking it! > > >> 1) xen/arch/arm/domain_build.c >> d = domain_create(++max_init_domid, &d_cfg, false); >> Where max_init_domid has value 0 and it is defined in setup.c > > We might want to add a comment on top of this code to explain why the '++a' rather than 'a++'. > >> 2) xen/common/domctl.c >> d = domain_create(dom, &op->u.createdomain, false); >> For me seems that the dom variable won’t take the 0 value, if someone could give another feedback it would be great. >> On every other part where domain_create(…) is used, it is called with a constant value different from 0. > > I agree with the analysis. However, I feel this is fragile because we rely on the caller to never pass 0. But it looks like domain_create() doesn't check if the ID is already used. So it would already be possible to overwrite hardware_domain. > > Therefore, this can probably be deffered. > >> For the hardware_domain being NULL and not handled in some situation, it seems that it’s not directly related to this patch, but I can handle it on a next serie, from a quick look it seems that many cases can be handled by checking if the domain is NULL in is_hardware_domain(…). > > Before this series, it is not possible to have hardware_domain == NULL at runtime because dom0 is always created. > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 2532ec9739..6d169ff6ce 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -794,6 +794,35 @@ static void __init setup_mm(void) } #endif +static bool __init is_dom0less_mode(void) +{ + struct bootmodules *mods = &bootinfo.modules; + struct bootmodule *mod; + unsigned int i; + bool dom0found = false; + bool domUfound = false; + + /* Look into the bootmodules */ + for ( i = 0 ; i < mods->nr_mods ; i++ ) + { + mod = &mods->module[i]; + /* Find if dom0 and domU kernels are present */ + if ( mod->kind == BOOTMOD_KERNEL ) + { + if ( mod->domU == false ) + dom0found = true; + else + domUfound = true; + } + } + + /* + * If there is no dom0 kernel but at least one domU, then we are in + * dom0less mode + */ + return ( !dom0found && domUfound ); +} + size_t __read_mostly dcache_line_bytes; /* C entry point for boot CPU */ @@ -804,7 +833,7 @@ void __init start_xen(unsigned long boot_phys_offset, int cpus, i; const char *cmdline; struct bootmodule *xen_bootmodule; - struct domain *dom0; + struct domain *dom0 = NULL; struct xen_domctl_createdomain dom0_cfg = { .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, .max_evtchn_port = -1, @@ -964,28 +993,33 @@ void __init start_xen(unsigned long boot_phys_offset, apply_alternatives_all(); enable_errata_workarounds(); - /* Create initial domain 0. */ - /* The vGIC for DOM0 is exactly emulating the hardware GIC */ - dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; - /* - * Xen vGIC supports a maximum of 992 interrupt lines. - * 32 are substracted to cover local IRQs. - */ - dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; - if ( gic_number_lines() > 992 ) - printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); - dom0_cfg.arch.tee_type = tee_get_type(); - dom0_cfg.max_vcpus = dom0_max_vcpus(); - - if ( iommu_enabled ) - dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; - - dom0 = domain_create(0, &dom0_cfg, true); - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) - panic("Error creating domain 0\n"); - - if ( construct_dom0(dom0) != 0) - panic("Could not set up DOM0 guest OS\n"); + if ( !is_dom0less_mode() ) + { + /* Create initial domain 0. */ + /* The vGIC for DOM0 is exactly emulating the hardware GIC */ + dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; + /* + * Xen vGIC supports a maximum of 992 interrupt lines. + * 32 are substracted to cover local IRQs. + */ + dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; + if ( gic_number_lines() > 992 ) + printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n"); + dom0_cfg.arch.tee_type = tee_get_type(); + dom0_cfg.max_vcpus = dom0_max_vcpus(); + + if ( iommu_enabled ) + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; + + dom0 = domain_create(0, &dom0_cfg, true); + if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) + panic("Error creating domain 0\n"); + + if ( construct_dom0(dom0) != 0) + panic("Could not set up DOM0 guest OS\n"); + } + else + printk(XENLOG_INFO "Xen dom0less mode detected\n"); heap_init_late(); @@ -1003,7 +1037,8 @@ void __init start_xen(unsigned long boot_phys_offset, if ( acpi_disabled ) create_domUs(); - domain_unpause_by_systemcontroller(dom0); + if ( dom0 ) + domain_unpause_by_systemcontroller(dom0); /* Switch on to the dynamically allocated stack for the idle vcpu * since the static one we're running on is about to be freed. */
This patch prevents the dom0 to be loaded skipping its building and going forward to build domUs when the dom0 kernel is not found and at least one domU is present. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/setup.c | 83 +++++++++++++++++++++++++++++++------------- 1 file changed, 59 insertions(+), 24 deletions(-)