On 26.11.2025 21:43, Grygorii Strashko wrote:
> From: Grygorii Strashko <[email protected]>
>
> The d->arch.physaddr_bitsize field is used only by PV32 code, so:
>
> - move domain_set_alloc_bitsize() function into PV32 code and clean up
> unused domain_set_alloc_bitsize() defines from other arches
This could be two separate patches, which likely would be pretty uncontroversial
(and hence be able to go in quickly): One to drop the entirely unused #define-s
for non-x86, and the other to move the x86 function so it can become static in
the sole file using it.
The former patch may want to have several Fixes: tags (as to being Misra
violations), as from all I can see there had never been a need to introduce
those #define-s. Even in 3.2.x they were already confined to x86-only code.
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -314,10 +314,10 @@ struct arch_domain
>
> #ifdef CONFIG_PV32
> unsigned int hv_compat_vstart;
> -#endif
>
> /* Maximum physical-address bitwidth supported by this guest. */
> unsigned int physaddr_bitsize;
> +#endif
I'm not happy to see the anomaly here being widened. PV-only items really
belong in struct pv_domain.
> --- a/xen/arch/x86/include/asm/mm.h
> +++ b/xen/arch/x86/include/asm/mm.h
> @@ -619,9 +619,6 @@ 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);
> -
> unsigned long domain_get_maximum_gpfn(struct domain *d);
>
> /* Definition of an mm lock: spinlock with extra fields for debugging */
> @@ -659,4 +656,11 @@ static inline bool arch_mfns_in_directmap(unsigned long
> mfn, unsigned long nr)
> return (mfn + nr) <= (virt_to_mfn(eva - 1) + 1);
> }
>
> +#ifdef CONFIG_PV32
> +unsigned int _domain_clamp_alloc_bitsize(const struct domain *d,
> + unsigned int bits);
This introduces a (C language spec) naming violation - identifiers with
this kind of a name a reserved for file-scope use.
> +#define domain_clamp_alloc_bitsize(d, bits)
> \
> + _domain_clamp_alloc_bitsize((d), (bits))
Nit: No need for the inner parentheses; they only hamper readability.
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -230,6 +230,27 @@ unsigned long pv_make_cr4(const struct vcpu *v)
> }
>
> #ifdef CONFIG_PV32
> +unsigned int _domain_clamp_alloc_bitsize(const struct domain *d,
> + unsigned int bits)
> +{
> + if ( (d == NULL) || (d->arch.physaddr_bitsize == 0) )
> + return bits;
> + return min(d->arch.physaddr_bitsize, bits);
> +}
While moving, please make minimal style adjustments: Add the missing blank
line before the final "return" here, and ...
> +static void domain_set_alloc_bitsize(struct domain *d)
> +{
> + if ( !is_pv_32bit_domain(d) ||
> + (MACH2PHYS_COMPAT_NR_ENTRIES(d) >= max_page) ||
> + d->arch.physaddr_bitsize > 0 )
> + return;
> + d->arch.physaddr_bitsize =
> + /* 2^n entries can be contained in guest's p2m mapping space */
> + fls(MACH2PHYS_COMPAT_NR_ENTRIES(d)) - 1
> + /* 2^n pages -> 2^(n+PAGE_SHIFT) bits */
> + + PAGE_SHIFT;
> +}
... add a blank line between the two statements here. In the former function
please further consider switching == NULL and == 0 to our more common use of !.
Jan
