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.
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)
seems to work. The min_t() is because all callers pass in bits as a
long constant, tripping the typecheck in min().
> +#endif
>
> unsigned long domain_get_maximum_gpfn(struct domain *d);
>
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index 21158ce1812e..e4f95d8f2fc8 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -626,8 +626,9 @@ static int __init dom0_construct(const struct boot_domain
> *bd)
> initrd_mfn = paddr_to_pfn(initrd->start);
> mfn = initrd_mfn;
> count = PFN_UP(initrd_len);
> - if ( d->arch.physaddr_bitsize &&
> - ((mfn + count - 1) >> (d->arch.physaddr_bitsize - PAGE_SHIFT)) )
While you're editing this, blank line here please.
> +#ifdef CONFIG_PV32
> + if ( d->arch.pv.physaddr_bitsize &&
> + ((mfn + count - 1) >> (d->arch.pv.physaddr_bitsize -
> PAGE_SHIFT)) )
> {
> order = get_order_from_pages(count);
> page = alloc_domheap_pages(d, order, MEMF_no_scrub);
> @@ -650,6 +651,7 @@ static int __init dom0_construct(const struct boot_domain
> *bd)
> initrd->start = pfn_to_paddr(initrd_mfn);
> }
> else
> +#endif
> {
> while ( count-- )
> if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> index 9c4785c187dd..ad40a712ac5f 100644
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -230,6 +230,29 @@ 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.pv.physaddr_bitsize == 0) )
> + return bits;
> +
> + return min(d->arch.pv.physaddr_bitsize, bits);
> +}
> +
> +static void domain_set_alloc_bitsize(struct domain *d)
> +{
> + if ( !is_pv_32bit_domain(d) ||
> + (MACH2PHYS_COMPAT_NR_ENTRIES(d) >= max_page) ||
> + d->arch.pv.physaddr_bitsize > 0 )
> + return;
> +
> + d->arch.pv.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;
> +}
The single caller has just set d->arch.pv.is_32bit = true, but the
compiler can't eliminate the first condition because of the embedded
evaluate_nospec(). Nor the 3rd condition, because it can't reason about
the singleton nature of switch_compat().
Thus, it would be better inlined fully, as:
if ( MACH2PHYS_COMPAT_NR_ENTRIES(d) >= max_page )
d->arch.pv.physaddr_bitsize =
/* 2^n entries can be contained in guest's p2m mapping space */
fls(MACH2PHYS_COMPAT_NR_ENTRIES(d)) - 1 + PAGE_SHIFT;
which is rather easier to follow. Even the comment about PAGE_SHIFT is
more noise than help.
In an ideal world it ought to be in its own patch, and in principle I'm
happy to draft one with a fuller explanation if you'd prefer, but given
the repositioning of physaddr_bitsize into pv domain as wekk, it's
probably all better together in this single patch.
~Andrew
