Re: [XEN v3] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used as a rhs operand of shift operator

Re: [XEN v3] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used as a rhs operand of shift operator

[Bobby Eshleman]

On Fri, Dec 02, 2022 at 12:56:06PM +0000, Andrew Cooper wrote:
> On 02/12/2022 12:35, Ayan Kumar Halder wrote:
> > 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            |
> 
> Just FYI, I think the answer here is 56 for RISC-V.
> 
> ~Andrew
> 

Andrew got it: 56.

[XEN v3] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used as a rhs operand of shift operator

[Ayan Kumar Halder]

We want to ensure that "xenheap_bits - PAGE_SHIFT" is strictly less than
the number of bits to represent unsigned long as it is used a rhs operand
to shift mfn.
It is also important to note that the "xenheap_bits - PAGE_SHIFT" is never
used to shift an address (rather than a frame number), and going forward
then also shouldn't be (perhaps unless further precautions are taken).

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. This platform does not follow
the same code path as Arm_32.
Thus, I have introduced this change as I don't see it causing a regression on
any of the supported platforms.

Changes from v1:-
1. Changed the check from "(PADDR_BITS > BITS_PER_LONG)" to "((PADDR_BITS - PAGE_SHIFT) >= BITS_PER_LONG)"
2. Updated the commit message to explain the reason for this.

v2 :-
1. Updated the commit message.

 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..c5b8c7444f 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 - 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);
 }
-- 
2.17.1

Re: [XEN v3] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used as a rhs operand of shift operator

[Andrew Cooper]

On 02/12/2022 12:35, Ayan Kumar Halder wrote:
> 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            |

Just FYI, I think the answer here is 56 for RISC-V.

~Andrew

> | x86       |   52            |   64            |
> -------------------------------------------------
>

Re: [XEN v3] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used as a rhs operand of shift operator

[Jan Beulich]

On 02.12.2022 13:35, Ayan Kumar Halder wrote:
> We want to ensure that "xenheap_bits - PAGE_SHIFT" is strictly less than
> the number of bits to represent unsigned long as it is used a rhs operand
> to shift mfn.
> It is also important to note that the "xenheap_bits - PAGE_SHIFT" is never
> used to shift an address (rather than a frame number), and going forward
> then also shouldn't be (perhaps unless further precautions are taken).

Hmm, now you've lost why you are making the change in the first place:
The way things are before the patch is quite fine for the described
purpose. (This then also extends to the title. I should have noticed
this in v2 already, but didn't because I still had the v1 title in mind.)

Furthermore in the 2nd paragraph instead of 'the "xenheap_bits -
PAGE_SHIFT"' you mean '"xenheap_bits" alone'.

Jan

Re: [XEN v3] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used as a rhs operand of shift operator

[Julien Grall]

Hi,

On 02/12/2022 13:43, Jan Beulich wrote:
> On 02.12.2022 13:35, Ayan Kumar Halder wrote:
>> We want to ensure that "xenheap_bits - PAGE_SHIFT" is strictly less than
>> the number of bits to represent unsigned long as it is used a rhs operand
>> to shift mfn.
>> It is also important to note that the "xenheap_bits - PAGE_SHIFT" is never
>> used to shift an address (rather than a frame number), and going forward
>> then also shouldn't be (perhaps unless further precautions are taken).
> 
> Hmm, now you've lost why you are making the change in the first place:
> The way things are before the patch is quite fine for the described
> purpose. (This then also extends to the title. I should have noticed
> this in v2 already, but didn't because I still had the v1 title in mind.)
> 
> Furthermore in the 2nd paragraph instead of 'the "xenheap_bits -
> PAGE_SHIFT"' you mean '"xenheap_bits" alone'.

Let me propose a commit message:

"
xen/page_alloc: Relax the BUILD_BUG_ON() in xenheap_max_mfn()

In the near future, we are considering to use a common xen/domain heap 
for Arm 32-bit V8-R. In this setup, both PADDR_BITS and BITS_PER_LONG 
will be 32. Therefore, the condition PADDR_BITS >= BITS_PER_LONG will 
become true.

Looking at the commit that introduce the BUILD_BUG_ON (88e3ed61642b 
"x86/NUMA: make init_node_heap() respect Xen heap limit") and the 
current use, it seems this check was mainly to ensure that the shift of 
xenheap_bits is not going to be undefined (>> N for a N-bit type is 
undefined).

So far, all the shifts are using "xenheap_bits - PAGE_SHIFT". Therefore, 
the existing code should work for 32-bit architecture as long as the 
result of the substraction is below 32.

Therefore relax the BUILD_BUG_ON() to check that (PADDR_BITS - 
PAGE_SHIFT) is not equal of above BITS_PER_LONG.

Note that we would need further precaution if we ended up to use 
'xenheap_bits' alone in shifts.
"

If you end up to take my commit message, then please add my signed-off-by.

Cheers,

-- 
Julien Grall

Re: [XEN v3] Xen: Ensure "xenheap_bits - PAGE_SHIFT" can be used as a rhs operand of shift operator

[Ayan Kumar Halder]

Hi Bobby,

On 20/11/2022 17:01, Bobby Eshleman wrote:
> On Fri, Dec 02, 2022 at 12:56:06PM +0000, Andrew Cooper wrote:
>> On 02/12/2022 12:35, Ayan Kumar Halder wrote:
>>> 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            |
>> Just FYI, I think the answer here is 56 for RISC-V.
>>
>> ~Andrew
>>
> Andrew got it: 56.

Thanks Andrew, Bobby.

Bobby, I couldn't see PADDR_BITS defined anywhere. Did I miss something ?

If not, will you define PADDR_BITS somewhere in xen/arch/riscv/* ?

- Ayan