Re: Arm: AArch32: Need suggestions to support 32 bit physical addresses

[Andre Przywara <andre.przywara@xxxxxxx>]

On Wed, 30 Nov 2022 08:09:53 +0100
Jan Beulich <jbeulich@xxxxxxxx> wrote:

Hi Ayan,

> On 29.11.2022 19:18, Ayan Kumar Halder wrote:
> > On 29/11/2022 15:52, Julien Grall wrote:  
> >> On 29/11/2022 16:23, Ayan Kumar Halder wrote:  
> >>> On 29/11/2022 14:52, Julien Grall wrote:  
> >>>> On 29/11/2022 14:57, Ayan Kumar Halder wrote:  
> >>> --- a/xen/common/page_alloc.c
> >>> +++ b/xen/common/page_alloc.c
> >>> @@ -2245,7 +2245,9 @@ void __init xenheap_max_mfn(unsigned long mfn)
> >>>   {
> >>>       ASSERT(!first_node_initialised);
> >>>       ASSERT(!xenheap_bits);
> >>> +#ifndef CONFIG_AARCH32_V8R
> >>>       BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
> >>> +#endif  
> >>
> >> BUILD_BUG_ON() are used to indicate that the code would fall over the 
> >> check pass. I can't find the justification for this change in the 
> >> commit message.  
> > 
> > I had a look at the following commit which introduced this, but I 
> > couldn't get the explaination for this.
> > 
> > commit 88e3ed61642bb393458acc7a9bd2f96edc337190
> > Author: Jan Beulich <jbeulich@xxxxxxxx>
> > Date:   Tue Sep 1 14:02:57 2015 +0200
> > 
> > @Jan :- Do you know why BUILD_BUG_ON() was introduced ?  
> 
> You've cut too much context - the next line explains this all by itself,
> I think:
> 
>     xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
> 
> Clearly addresses used for the Xen heap need to be representable in an
> unsigned long (which we assume to be the same size as void *).

So I am wondering why you hit that code for your port in the first case?
If you check, then ARM32 won't pass this, because PADDR_BITS is 40, while
a long is still 32 bits.
So digging deeper this is code for the case when we want to map the entire
physical memory into Xen (the Xen heap, or direct map in Linux terms).
Which we cannot do for ARM32, and that's why the code is protected by
!CONFIG_SEPARATE_XENHEAP, which is forced to 1 for CONFIG_ARM_32 (in a
hacked up way, btw).

So I think you must just force the same thing for your port, then this
code will never be compiled.

Does that make sense?

Cheers,
Andre

> But I'm afraid there's further context missing for your question: Why
> would that construct be a problem in your case? Is it just that you'd
> need it to be > rather than the >= that's used presently? If so, why
> do you add an #ifdef rather than dealing with the (apparent) off-by-1?
> (I say "apparent" because I haven't checked whether the >= is really
> depended upon anywhere.)
> 
> Jan