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
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
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
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 | > ------------------------------------------------- >
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.
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
© 2016 - 2024 Red Hat, Inc.