[PATCH] Arm: drop assertion from page_is_ram_type()

[PATCH] Arm: drop assertion from page_is_ram_type()

[Jan Beulich]

Its uses in offline_page() and query_page_offline() make it reachable on
Arm, as long as XEN_SYSCTL_page_offline_op doesn't have any Arm-specific
code added. It being reachable was even mentioned in the commit
introducing it, claiming it "clearly shouldn't be called on ARM just
yet".

Fixes: 214c4cd94a80 ("xen: arm: stub page_is_ram_type")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -66,7 +66,6 @@ int steal_page(
 
 int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
 {
-    ASSERT_UNREACHABLE();
     return 0;
 }
 

Re: [PATCH] Arm: drop assertion from page_is_ram_type()

[Julien Grall]

Hi Jan,

On 11/08/2025 14:01, Jan Beulich wrote:
> Its uses in offline_page() and query_page_offline() make it reachable on
> Arm, as long as XEN_SYSCTL_page_offline_op doesn't have any Arm-specific
> code added. It being reachable was even mentioned in the commit
> introducing it, claiming it "clearly shouldn't be called on ARM just
> yet".

So I agree that the function can be reached. But then I don't think the 
function can simply return 0.

The name is too generic enough that someone may end up to use it in 
common code and there will be no signal to the user that the function 
will not properly indicate a RAM page on Arm.

I can think of two possible approaches:

1/ Implement properly page_is_ram_type(). We don't have an e820, but we 
could mimick it using the memory banks we stored.
2/ Rename page_is_ram_type() to page_offlinable() (or similar) so it is 
clear that the common use is for offlining.

The latter might be the simplest.

Cheers,

-- 
Julien Grall

Re: [PATCH] Arm: drop assertion from page_is_ram_type()

[Jan Beulich]

On 11.08.2025 19:24, Julien Grall wrote:
> On 11/08/2025 14:01, Jan Beulich wrote:
>> Its uses in offline_page() and query_page_offline() make it reachable on
>> Arm, as long as XEN_SYSCTL_page_offline_op doesn't have any Arm-specific
>> code added. It being reachable was even mentioned in the commit
>> introducing it, claiming it "clearly shouldn't be called on ARM just
>> yet".
> 
> So I agree that the function can be reached. But then I don't think the 
> function can simply return 0.
> 
> The name is too generic enough that someone may end up to use it in 
> common code and there will be no signal to the user that the function 
> will not properly indicate a RAM page on Arm.
> 
> I can think of two possible approaches:
> 
> 1/ Implement properly page_is_ram_type(). We don't have an e820, but we 
> could mimick it using the memory banks we stored.
> 2/ Rename page_is_ram_type() to page_offlinable() (or similar) so it is 
> clear that the common use is for offlining.
> 
> The latter might be the simplest.

I consider page_offlinable() potentially ambiguous: When you offline a PCI
device, you offline its MMIO as well, for example. Or, much like RAM, some
page in, say, a video device's framebuffer may have gone bad. One might
think that such would then also be covered by that function. So minimally
page_offlinable_ram() or page_is_offlinable_ram(), I think. That would
then have the benefit of allowing an avenue towards x86 also making its
checking more precise, as certain RAM ranges can't possibly be offlined.
If that's fine with you, I can enlarge the patch accordingly.

Jan

Re: [PATCH] Arm: drop assertion from page_is_ram_type()

[Julien Grall]

Hi Jan,

On 12/08/2025 07:29, Jan Beulich wrote:
> On 11.08.2025 19:24, Julien Grall wrote:
>> On 11/08/2025 14:01, Jan Beulich wrote:
>>> Its uses in offline_page() and query_page_offline() make it reachable on
>>> Arm, as long as XEN_SYSCTL_page_offline_op doesn't have any Arm-specific
>>> code added. It being reachable was even mentioned in the commit
>>> introducing it, claiming it "clearly shouldn't be called on ARM just
>>> yet".
>>
>> So I agree that the function can be reached. But then I don't think the
>> function can simply return 0.
>>
>> The name is too generic enough that someone may end up to use it in
>> common code and there will be no signal to the user that the function
>> will not properly indicate a RAM page on Arm.
>>
>> I can think of two possible approaches:
>>
>> 1/ Implement properly page_is_ram_type(). We don't have an e820, but we
>> could mimick it using the memory banks we stored.
>> 2/ Rename page_is_ram_type() to page_offlinable() (or similar) so it is
>> clear that the common use is for offlining.
>>
>> The latter might be the simplest.
> 
> I consider page_offlinable() potentially ambiguous: When you offline a PCI
> device, you offline its MMIO as well, for example. Or, much like RAM, some
> page in, say, a video device's framebuffer may have gone bad. One might
> think that such would then also be covered by that function. So minimally
> page_offlinable_ram() or page_is_offlinable_ram(), I think. That would
> then have the benefit of allowing an avenue towards x86 also making its
> checking more precise, as certain RAM ranges can't possibly be offlined.
> If that's fine with you, I can enlarge the patch accordingly.

I would be fine with either name you proposed.

Cheers,

-- 
Julien Grall