Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Thu Aug 21, 2025 at 9:16 AM CEST, Jan Beulich wrote:
> On 20.08.2025 01:58, dmkhn@xxxxxxxxx wrote:
>> On Thu, Aug 14, 2025 at 09:11:11AM +0200, Jan Beulich wrote:
>>> On 13.08.2025 00:30, dmkhn@xxxxxxxxx wrote:
>>>> From: Denis Mukhin <dmukhin@xxxxxxxx>
>>>>
>>>> Currently, there are two different domain ID allocation implementations:
>>>>
>>>>   1) Sequential IDs allocation in dom0less Arm code based on 
>>>> max_init_domid;
>>>>
>>>>   2) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
>>>>      max_init_domid (both Arm and x86).
>>>>
>>>> The domain ID allocation covers dom0 or late hwdom, predefined domains,
>>>> post-boot domains, excluding Xen system domains (domid >=
>>>> DOMID_FIRST_RESERVED).
>>>>
>>>> It makes sense to have a common helper code for such task across 
>>>> architectures
>>>> (Arm and x86) and between dom0less / toolstack domU allocation.
>>>>
>>>> Note, fixing dependency on max_init_domid is out of scope of this patch.
>>>>
>>>> Wrap the domain ID allocation as an arch-independent function 
>>>> domid_alloc() in
>>>> new common/domid.c based on the bitmap.
>>>>
>>>> Allocation algorithm:
>>>> - If an explicit domain ID is provided, verify its availability and use it 
>>>> if
>>>>   ID is not used;
>>>> - If DOMID_INVALID is provided, search the range 
>>>> [1..DOMID_FIRST_RESERVED-1],
>>>>   starting from the last used ID.
>>>>   Implementation guarantees that two consecutive calls will never return 
>>>> the
>>>>   same ID. ID#0 is reserved for the first boot domain (currently, dom0) and
>>>>   excluded from the allocation range.
>>>>
>>>> Remove is_free_domid() helper as it is not needed now.
>>>>
>>>> No functional change intended.
>>>>
>>>> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
>>>> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
>>>> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
>>>> ---
>>>> Changes since v15:
>>>> - fixup for check after the first pass in the bitarray in domid_alloc()
>>>> - trivial renaming for the local variable in domid_alloc()
>>>> - kept Julien's R-b, added Alejandro's R-b
>>>
>>> Just to mention: My take is that this kind of a fix ought to invalidate all
>>> earlier R-b. It's not just a cosmetic change, after all.
>> 
>> Sorry for the hiccup here, did not mean to overrule the review process.
>> 
>> My bold assumption was that in case of small fixups like this it is
>> satisfactory to carry over previous acks.
>
> Acks may be okay to keep, but imo R-b need dropping when an actual bug was
> fixed.

I don't know. Unless the bugfix involves a change in the code with wide reaching
consequences I'd say it's reasonable to keep them. But that's something for you
(the committers) to decide, and this just my .02 cents.

> Irrespective of how severe the bug was.

It's not so much about the severity (imo), as the behavioural differences
involved in the fixup. In this case (afaics?) it's a straight s/==/>=/, which is
self-contained and has no wide-reaching side effects at all.

>
>> I asked (matrix) both Julien and Alejandro to re-review and confirm.
>
> While good to ask, that's of limited use. It'll be impossible later on to
> figure whether such a confirmation was given. Decisions (and acks and alike
> effectively fall into that category) need to be on the list, to be able to
> locate them later on.
>
> Jan

He meant he reached out to ask for an in-list confirmation. As far as I'm
concerned, my R-by still holds.

Cheers,
Alejandro