Xen: Enable compilation when PADDR_BITS == BITS_PER_LONG

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

Posted by Ayan Kumar Halder 3 years, 2 months ago

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*).

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

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            |
-------------------------------------------------

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);
     xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
     printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
 }
-- 
2.17.1

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

Posted by Julien Grall 3 years, 2 months ago

Hi Ayan,

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.

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> 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.

> 
> 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)".

>       xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
>       printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
>   }

Cheers,

-- 
Julien Grall

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

Posted by Ayan Kumar Halder 3 years, 2 months ago

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@amd.com>
>> ---
>>
>> 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,
>

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

Posted by Julien Grall 3 years, 2 months ago


On 01/12/2022 12:12, Ayan Kumar Halder wrote:
> 
> 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@amd.com>
>>> ---
>>>
>>> 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 ?

No I don't think this will be relevant in the final commit message.

Cheers,

-- 
Julien Grall

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

Posted by Jan Beulich 3 years, 2 months ago

On 01.12.2022 11:26, Julien Grall wrote:
> On 01/12/2022 10:03, Ayan Kumar Halder wrote:
>> --- 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)".

+1, fwiw.

Jan