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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2025 21:43, Grygorii Strashko wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> 
> 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