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

[dmkhn@xxxxxxxxx]

On Thu, Aug 21, 2025 at 12:29:23PM +0200, Alejandro Vallejo wrote:
> 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.

Thanks

> 
> Cheers,
> Alejandro