Re: [XEN v1] Xen: Enable compilation when PADDR_BITS == BITS_PER_LONG

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 01/12/2022 10:26, Julien Grall wrote:
> Hi Ayan,
Hi Julien,

I have a question.

> On 01/12/2022 10:03, Ayan Kumar Halder wrote:
> > It is possible for a pointer to represent physical memory of the same 
> > size.
> > In other words, a 32 bit pointer can represent 32 bit addressable 
> > physical
> > memory.
> > Thus, issue a compilation failure only when the count of physical 
> > address bits
> > is greater than BITS_PER_LONG (ie count of bits in void*).
> I am having difficult to understand how this description is related to 
> the BUILD_BUG_ON(). AFAIU, it is used to check that xenheap_bits can 
> be used in shift.
> If the unsigned long is 32-bit, then a shift of 32 could be undefined. 
> Looking at the current use, the shift are used with "xenheap_bits - 
> PAGE_SHIFT". So as long as PAGE_SHIFT is not 0, you would be fine.
Ack
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> >
> > Currently this change will not have any impact on the existing 
> > architectures.
> > The following table illustrates PADDR_BITS vs BITS_PER_LONG of 
> > different archs
> > ------------------------------------------------
> > | Arch      |   PADDR_BITS    |   BITS_PER_LONG |
> > ------------------------------------------------
> > | Arm_64    |   48            |   64            |
> > | Arm_32    |   40            |   32            |
> > | RISCV_64  |   Don't know    |   64            |
> > | x86       |   52            |   64            |
> > -------------------------------------------------
> The Arm_32 line is a bit confusing because one would wonder why we 
> haven't seen this issue yet. So I think you want to clarify that the 
> code path is not used by Arm32.
Do you want this clarification and the above/below explanation to be a 
part of the commit message ?
I will then send out a v2 with the fix.

- Ayan

> > However, this will change when we introduce a platform (For eg 
> > Cortex-R52) which
> > supports 32 bit physical address and BITS_PER_LONG.
> > Thus, I have introduced this change as I don't see it causing a 
> > regression on
> > any of the supported platforms.
> >
> >   xen/common/page_alloc.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 62afb07bc6..cd390a0956 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2245,7 +2245,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
> >   {
> >       ASSERT(!first_node_initialised);
> >       ASSERT(!xenheap_bits);
> > -    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
> > +    BUILD_BUG_ON(PADDR_BITS > BITS_PER_LONG);
> Based on the above, I think this wants to be "(PADDR_BITS - 
> PAGE_SHIFT) >= BITS_PER_LONG)".
Ack
> >       xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
> >       printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
> >   }
> Cheers,