Re: [PATCH v2 3/3] xen/x86: move d->arch.physaddr_bitsize field handling to pv32

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2025 00:09, Andrew Cooper wrote:
> On 27/11/2025 10:12 pm, Grygorii Strashko wrote:
>> diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
>> index 17ca6666a34e..128115442ecc 100644
>> --- a/xen/arch/x86/include/asm/mm.h
>> +++ b/xen/arch/x86/include/asm/mm.h
>> @@ -619,9 +619,11 @@ void __iomem *ioremap_wc(paddr_t pa, size_t len);
>>  
>>  extern int memory_add(unsigned long spfn, unsigned long epfn, unsigned int 
>> pxm);
>>  
>> -void domain_set_alloc_bitsize(struct domain *d);
>> -unsigned int domain_clamp_alloc_bitsize(struct domain *d, unsigned int 
>> bits);
>> +#ifdef CONFIG_PV32
>> +unsigned int domain_clamp_alloc_bitsize(const struct domain *d,
>> +                                        unsigned int bits);
> 
> Do not convert this, or any other domains/vcpus you see, to const.  I
> realise you have been given conflicting information on this point, but
> the maintainers as a whole agreed to not const pointers to complex
> objects in the general case because of the churn it creates, and the
> repeated examples of MISRA violations people have inserted to work
> around the fact it shouldn't have been a const pointer to start with.

While moot here when indeed converted to a macro, as you suggest below,
I don't recall "maintainers as a whole agreed" on this. For one there
are predicate-like functions where I think even you agreed their
parameters can be pointer-to-const. The case here isn't very far from
predicate-like, in particular ...

> That aside, I think clamp wants to be a static inline here.  (Except it
> can't be, so it needs to be a macro).
> 
> It's currently a concrete function call to very simple piece of logic,
> where the callers have options to eliminate it entirely in the d = NULL
> case if they could only see in.
> 
> #define domain_clamp_alloc_bitsize(d, bits)                             \
>     (((d) && (d)->arch.pv.physaddr_bitsize)                             \
>      ? min_t(uint32_t, (d)->arch.pv.physaddr_bitsize, bits) : bits)

... this representation alone demonstrates that what d points to is only
ever read.

Yes, there are cases where the situation is more complex, and where I can
see how my pov may not be shared by others. Yet still - can you point me
to a written form of the supposed agreement among maintainers? Imo
something like this, which has been controversial for a long time, really
needs recording in ./CODING_STYLE or docs/process/coding-best-practices.pandoc.
And then preferably in a shape acceptable to everyone (i.e. no outright
"never").

Jan