Re: [PATCH] xen/arm: check max_init_domid validity

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi MIchal,

> On 28 Feb 2023, at 12:10, Michal Orzel <michal.orzel@xxxxxxx> 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@xxxxxxx>
>> ---
>> 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