[PATCH] of: unmap memory regions in /memreserve node

[PATCH] of: unmap memory regions in /memreserve node

[Calvin Zhang]

Reserved memory regions in /memreserve node aren't and shouldn't
be referenced elsewhere. So mark them no-map to skip direct mapping
for them.

Signed-off-by: Calvin Zhang <calvinzhang.cool@gmail.com>
---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bdca35284ceb..9e88cc8445f6 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -638,7 +638,7 @@ void __init early_init_fdt_scan_reserved_mem(void)
 		fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
 		if (!size)
 			break;
-		early_init_dt_reserve_memory_arch(base, size, false);
+		early_init_dt_reserve_memory_arch(base, size, true);
 	}
 
 	fdt_scan_reserved_mem();
-- 
2.30.2

Re: [PATCH] of: unmap memory regions in /memreserve node

[Rob Herring]

+linuxppc-dev

On Wed, Nov 24, 2021 at 09:33:47PM +0800, Calvin Zhang wrote:
> Reserved memory regions in /memreserve node aren't and shouldn't
> be referenced elsewhere. So mark them no-map to skip direct mapping
> for them.

I suspect this has a high chance of breaking some platform. There's no 
rule a region can't be accessed.

> 
> Signed-off-by: Calvin Zhang <calvinzhang.cool@gmail.com>
> ---
>  drivers/of/fdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index bdca35284ceb..9e88cc8445f6 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -638,7 +638,7 @@ void __init early_init_fdt_scan_reserved_mem(void)
>  		fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
>  		if (!size)
>  			break;
> -		early_init_dt_reserve_memory_arch(base, size, false);
> +		early_init_dt_reserve_memory_arch(base, size, true);
>  	}
>  
>  	fdt_scan_reserved_mem();
> -- 
> 2.30.2
> 
> 

Re: [PATCH] of: unmap memory regions in /memreserve node

[Mark Rutland]

On Tue, Nov 30, 2021 at 04:43:31PM -0600, Rob Herring wrote:
> +linuxppc-dev
> 
> On Wed, Nov 24, 2021 at 09:33:47PM +0800, Calvin Zhang wrote:
> > Reserved memory regions in /memreserve node aren't and shouldn't
> > be referenced elsewhere. So mark them no-map to skip direct mapping
> > for them.
> 
> I suspect this has a high chance of breaking some platform. There's no 
> rule a region can't be accessed.

The subtlety is that the region shouldn't be explicitly accessed (e.g.
modified), but the OS is permitted to have the region mapped. In ePAPR this is
described as:

   This requirement is necessary because the client program is permitted to map
   memory with storage attributes specified as not Write Through Required, not
   Caching Inhibited, and Memory Coherence Required (i.e., WIMG = 0b001x), and
   VLE=0 where supported. The client program may use large virtual pages that
   contain reserved memory. However, the client program may not modify reserved
   memory, so the boot program may perform accesses to reserved memory as Write
   Through Required where conflicting values for this storage attribute are
   architecturally permissible.

Historically arm64 relied upon this for spin-table to work, and I *think* we
might not need that any more I agree that there's a high chance this will break
something (especially on 16K or 64K page size kernels), so I'd prefer to leave
it as-is.

If someone requires no-map behaviour, they should use a /reserved-memory entry
with a no-map property, which will work today and document their requirement
explicitly.

Thanks,
Mark.

> > Signed-off-by: Calvin Zhang <calvinzhang.cool@gmail.com>
> > ---
> >  drivers/of/fdt.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index bdca35284ceb..9e88cc8445f6 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -638,7 +638,7 @@ void __init early_init_fdt_scan_reserved_mem(void)
> >  		fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
> >  		if (!size)
> >  			break;
> > -		early_init_dt_reserve_memory_arch(base, size, false);
> > +		early_init_dt_reserve_memory_arch(base, size, true);
> >  	}
> >  
> >  	fdt_scan_reserved_mem();
> > -- 
> > 2.30.2
> > 
> > 

Re: [PATCH] of: unmap memory regions in /memreserve node

[Michael Ellerman]

Mark Rutland <mark.rutland@arm.com> writes:
> On Tue, Nov 30, 2021 at 04:43:31PM -0600, Rob Herring wrote:
>> +linuxppc-dev
 
Sorry missed this until now ...

>> On Wed, Nov 24, 2021 at 09:33:47PM +0800, Calvin Zhang wrote:
>> > Reserved memory regions in /memreserve node aren't and shouldn't
>> > be referenced elsewhere. So mark them no-map to skip direct mapping
>> > for them.
>> 
>> I suspect this has a high chance of breaking some platform. There's no 
>> rule a region can't be accessed.
>
> The subtlety is that the region shouldn't be explicitly accessed (e.g.
> modified),

I think "modified" is the key there, reserved means Linux doesn't use
the range for its own data, but may still read from whatever is in the
range.

On some platforms the initrd will be marked as reserved, which Linux
obviously needs to read from.

> but the OS is permitted to have the region mapped. In ePAPR this is
> described as:
>
>    This requirement is necessary because the client program is permitted to map
>    memory with storage attributes specified as not Write Through Required, not
>    Caching Inhibited, and Memory Coherence Required (i.e., WIMG = 0b001x), and
>    VLE=0 where supported. The client program may use large virtual pages that
>    contain reserved memory. However, the client program may not modify reserved
>    memory, so the boot program may perform accesses to reserved memory as Write
>    Through Required where conflicting values for this storage attribute are
>    architecturally permissible.
>
> Historically arm64 relied upon this for spin-table to work, and I *think* we
> might not need that any more I agree that there's a high chance this will break
> something (especially on 16K or 64K page size kernels), so I'd prefer to leave
> it as-is.

Yeah I agree. On powerpc we still use large pages for the linear mapping
(direct map), so reserved regions will be incidentally mapped as
described above.

> If someone requires no-map behaviour, they should use a /reserved-memory entry
> with a no-map property, which will work today and document their requirement
> explicitly.

+1.

cheers