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