| Message ID | 0cf2013e5e6018cae300c39fb65ed526eac5c35c.1677511937.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: check max_init_domid validity | expand |
Hi Bertrand, On 28/02/2023 09:08, Bertrand Marquis wrote: > > > Before trying to create a dom0less guest, check that max_init_domid > increment will generate a valid domain ID, lower than > DOMID_FIRST_RESERVED. > > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> > --- > xen/arch/arm/domain_build.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index edca23b986d2..9707eb7b1bb1 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -3879,6 +3879,9 @@ void __init create_domUs(void) > if ( !dt_device_is_compatible(node, "xen,domain") ) > continue; > > + if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED ) > + panic("No more domain IDs available\n"); Here are some of my thoughts: 1. The check if domid is >= DOMID_FIRST_RESERVED is used in quite a lot of places in the Xen code. We might want to introduce a global function for that purpose instead of repeating this check all over the codebase. 2. This check is something that could be moved to be generic. At the moment we do have an ASSERT with is_system_domain in domain_create. I know domain_create can be called for domids in special range so this would need to be thought through. 3. The placement of this check at the top of the function before starting to parse dt properties might be problematic in the future if we decide to allow specifying static domids for dom0less domUs. In a static configuration, most of the time, we do not have xenstore (either because of lack of xenstore support or because of lack of dom0). AFAIKT, in Xen a domain can get to know its domid only through xenstore (DOMID_SELF is not working in all the cases). Also, in a static configuration, it makes the life of an integrator easy to know all the domids upfront to easily set up some communication, grant tables, etc. Let me know your thoughts. ~Michal
Hi MIchal, > On 28 Feb 2023, at 12:10, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Bertrand, > > On 28/02/2023 09:08, Bertrand Marquis wrote: >> >> >> Before trying to create a dom0less guest, check that max_init_domid >> increment will generate a valid domain ID, lower than >> DOMID_FIRST_RESERVED. >> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >> --- >> xen/arch/arm/domain_build.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index edca23b986d2..9707eb7b1bb1 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -3879,6 +3879,9 @@ void __init create_domUs(void) >> if ( !dt_device_is_compatible(node, "xen,domain") ) >> continue; >> >> + if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED ) >> + panic("No more domain IDs available\n"); > Here are some of my thoughts: > 1. The check if domid is >= DOMID_FIRST_RESERVED is used in quite a lot of > places in the Xen code. We might want to introduce a global function for that purpose > instead of repeating this check all over the codebase. We could introduce something but looking at the code i think that the first thing to do would be to use is_system_domain where possible (ie when there is a domain structure) and then cleanup a bit domctl.c where there are some double check of DOMID_FIRST_RESERVED (in the hypercall code and in is_free_domid). Once that is done, there would be a lot less usage of this. > > 2. This check is something that could be moved to be generic. At the moment we do have > an ASSERT with is_system_domain in domain_create. I know domain_create can be called for > domids in special range so this would need to be thought through. I do not think that domain_create is the right place to have this check as it is correct to call it to create system domains. > > 3. The placement of this check at the top of the function before starting to parse dt properties > might be problematic in the future if we decide to allow specifying static domids for dom0less domUs. > In a static configuration, most of the time, we do not have xenstore (either because of lack of xenstore > support or because of lack of dom0). AFAIKT, in Xen a domain can get to know its domid only through xenstore > (DOMID_SELF is not working in all the cases). Also, in a static configuration, it makes the life of an integrator > easy to know all the domids upfront to easily set up some communication, grant tables, etc. Right now the idea is to fail as early as possible to prevent doing any operation that is not needed. Having a way to statically define the dom id in configuration does make sense and the check will have to be modified once the support for this will be added. > > Let me know your thoughts. There is some improvement possible in the overall code but the goal here is just to solve a possible issue so this patch could be merged and other changes could be done in a following patch if wanted. Cheers Bertrand > > ~Michal
On Tue, 28 Feb 2023, Bertrand Marquis wrote: > Before trying to create a dom0less guest, check that max_init_domid > increment will generate a valid domain ID, lower than > DOMID_FIRST_RESERVED. > > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/domain_build.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index edca23b986d2..9707eb7b1bb1 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -3879,6 +3879,9 @@ void __init create_domUs(void) > if ( !dt_device_is_compatible(node, "xen,domain") ) > continue; > > + if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED ) > + panic("No more domain IDs available\n"); > + > if ( dt_find_property(node, "xen,static-mem", NULL) ) > flags |= CDF_staticmem; > > -- > 2.25.1 >
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index edca23b986d2..9707eb7b1bb1 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -3879,6 +3879,9 @@ void __init create_domUs(void) if ( !dt_device_is_compatible(node, "xen,domain") ) continue; + if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED ) + panic("No more domain IDs available\n"); + if ( dt_find_property(node, "xen,static-mem", NULL) ) flags |= CDF_staticmem;
Before trying to create a dom0less guest, check that max_init_domid increment will generate a valid domain ID, lower than DOMID_FIRST_RESERVED. Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> --- xen/arch/arm/domain_build.c | 3 +++ 1 file changed, 3 insertions(+)