Re: [PATCH RFC v2 11/27] arm64: mte: Reserve tag storage memory

[Hyesoo Yu]

Hi, 

I'm sorry for the late response, I was on vacation.

On Sun, Dec 03, 2023 at 12:14:30PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Wed, Nov 29, 2023 at 05:44:24PM +0900, Hyesoo Yu wrote:
> > Hello.
> > 
> > On Sun, Nov 19, 2023 at 04:57:05PM +0000, Alexandru Elisei wrote:
> > > Allow the kernel to get the size and location of the MTE tag storage
> > > regions from the DTB. This memory is marked as reserved for now.
> > > 
> > > The DTB node for the tag storage region is defined as:
> > > 
> > >         tags0: tag-storage@8f8000000 {
> > >                 compatible = "arm,mte-tag-storage";
> > >                 reg = <0x08 0xf8000000 0x00 0x4000000>;
> > >                 block-size = <0x1000>;
> > >                 memory = <&memory0>;	// Associated tagged memory node
> > >         };
> > >
> > 
> > How about using compatible = "shared-dma-pool" like below ?
> > 
> > &reserved_memory {
> > 	tags0: tag0@8f8000000 {
> > 		compatible = "arm,mte-tag-storage";
> >         	reg = <0x08 0xf8000000 0x00 0x4000000>;
> > 	};
> > }
> > 
> > tag-storage {
> >         compatible = "arm,mte-tag-storage";
> > 	memory-region = <&tag>;
> >         memory = <&memory0>;
> > 	block-size = <0x1000>;
> > }
> > 
> > And then, the activation of CMA would be performed in the CMA code.
> > We just can get the region information from memory-region and allocate it directly
> > like alloc_contig_range, take_page_off_buddy. It seems like we can remove a lots of code.
>

Sorry, that example was my mistake. Actually I wanted to write like this. 

&reserved_memory {
	tags0: tag0@8f8000000 {
		compatible = "shared-dma-pool";
        	reg = <0x08 0xf8000000 0x00 0x4000000>;
		reusable;
	};
}

tag-storage {
        compatible = "arm,mte-tag-storage";
 	memory-region = <&tag>;
        memory = <&memory0>;
	block-size = <0x1000>;
}


> Played with reserved_mem a bit. I don't think that's the correct path
> forward.
> 
> The location of the tag storage is a hardware property, independent of how
> Linux is configured.
> 
> early_init_fdt_scan_reserved_mem() is called from arm64_memblock_init(),
> **after** the kernel enforces an upper address for various reasons. One of
> the reasons can be that it's been compiled with 39 bits VA.
> 

I'm not sure about this part. What is the upper address enforced by the kernel ?
Where can I check the code ? Do you means that memblock_end_of_DRAM() ?  

> After early_init_fdt_scan_reserved_mem() returns, the kernel sets the
> maximum address, stored in the variable "high_memory".
>
> What can happen is that tag storage is present at an address above the
> maximum addressable by the kernel, and the CMA code will trigger an
> unrecovrable page fault.
> 
> I was able to trigger this with the dts change:
> 
> diff --git a/arch/arm64/boot/dts/arm/fvp-base-revc.dts b/arch/arm64/boot/dts/arm/fvp-base-revc.dts
> index 60472d65a355..201359d014e4 100644
> --- a/arch/arm64/boot/dts/arm/fvp-base-revc.dts
> +++ b/arch/arm64/boot/dts/arm/fvp-base-revc.dts
> @@ -183,6 +183,13 @@ vram: vram@18000000 {
>                         reg = <0x00000000 0x18000000 0 0x00800000>;
>                         no-map;
>                 };
> +
> +
> +               linux,cma {
> +                       compatible = "shared-dma-pool";
> +                       reg = <0x100 0x0 0x00 0x4000000>;
> +                       reusable;
> +               };
>         };
> 
>         gic: interrupt-controller@2f000000 {
> 
> And the error I got:
> 
> [    0.000000] Reserved memory: created CMA memory pool at 0x0000010000000000, size 64 MiB
> [    0.000000] OF: reserved mem: initialized node linux,cma, compatible id shared-dma-pool
> [    0.000000] OF: reserved mem: 0x0000010000000000..0x0000010003ffffff (65536 KiB) map reusable linux,cma
> [..]
> [    0.793193] WARNING: CPU: 0 PID: 1 at mm/cma.c:111 cma_init_reserved_areas+0xa8/0x378
> [..]
> [    0.806945] Unable to handle kernel paging request at virtual address 00000001fe000000
> [    0.807277] Mem abort info:
> [    0.807277]   ESR = 0x0000000096000005
> [    0.807693]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    0.808110]   SET = 0, FnV = 0
> [    0.808443]   EA = 0, S1PTW = 0
> [    0.808526]   FSC = 0x05: level 1 translation fault
> [    0.808943] Data abort info:
> [    0.808943]   ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
> [    0.809360]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [    0.809776]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [    0.810221] [00000001fe000000] user address but active_mm is swapper
> [..]
> [    0.820887] Call trace:
> [    0.821027]  cma_init_reserved_areas+0xc4/0x378
> [    0.821443]  do_one_initcall+0x7c/0x1c0
> [    0.821860]  kernel_init_freeable+0x1bc/0x284
> [    0.822277]  kernel_init+0x24/0x1dc
> [    0.822693]  ret_from_fork+0x10/0x20
> [    0.823554] Code: 9127a29a cb813321 d37ae421 8b030020 (f8636822)
> [    0.823554] ---[ end trace 0000000000000000 ]---
> [    0.824360] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [    0.824443] SMP: stopping secondary CPUs
> [    0.825193] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
> 
> Should reserved mem check if the reserved memory is actually addressable by
> the kernel if it's not "no-map"? Should cma fail gracefully if
> !pfn_valid(base_pfn)? Shold early_init_fdt_scan_reserved_mem() be moved
> because arm64_bootmem_init()? I don't have the answer to any of those. And
> I got a kernel panic because the kernel cannot address that memory (39 bits
> VA). I don't know what would happen if the upper limit is reduced for
> another reason.
> 

My answer may not be accurate because I don't understand what this upper limit is.
Is this a problem caused by the tag storage area not being included in the memory node ?

The reason for not including it in the memory node is to enable static mte when dmte
initilization fails, right ? I think I missed that. I thought the tag storage is included
in the memory node and registered as cma.

> What I think should happen:
> 
> 1. Add the tag storage memory before any limits are enforced by
> arm64_bootmem_init().
>
> 2. Call cma_declare_contiguous_nid() after arm64_bootmem_init(), because
> the function will check the memory limit.
> 
> 3. Have an arch initcall that checks that the CMA regions corresponding to
> the tag storage have been activated successfully (cma_init_reserved_areas()
> is a core initcall). If not, then don't enable tag storage.
> 
> How does that sound to you?
> 
> Thanks,
> Alex
> 

I think this is a good way to utilize the cma code !

Thanks,
Regards.

> > > +	ret = tag_storage_of_flat_read_u32(node, "block-size", &block_size_bytes);
> > > +	if (ret || block_size_bytes == 0) {
> > > +		pr_err("Invalid or missing 'block-size' property");
> > > +		return -EINVAL;
> > > +	}
> > > +	region->block_size = get_block_size_pages(block_size_bytes);
> > > +	if (range_len(tag_range) % region->block_size != 0) {
> > > +		pr_err("Tag storage region size 0x%llx is not a multiple of block size %u",
> > > +		       PFN_PHYS(range_len(tag_range)), region->block_size);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > 
> > I was confused about the variable "block_size", The block size declared in the device tree is
> > in bytes, but the actual block size used is in pages. I think the term "block_size" can cause
> > confusion as it might be interpreted as bytes. If possible, I suggest changing the term "block_size"
> > to something more readable, such as "block_nr_pages" (This is just a example!)
> > 
> > Thanks,
> > Regards.
>>

What do you think about this ?

Thanks,
Regards.

> > > +	ret = tag_storage_of_flat_read_u32(mem_node, "numa-node-id", &nid);
> > > +	if (ret)
> > > +		nid = numa_node_id();
> > > +
> > > +	ret = memblock_add_node(PFN_PHYS(tag_range->start), PFN_PHYS(range_len(tag_range)),
> > > +				nid, MEMBLOCK_NONE);
> > > +	if (ret) {
> > > +		pr_err("Error adding tag memblock (%d)", ret);
> > > +		return ret;
> > > +	}
> > > +	memblock_reserve(PFN_PHYS(tag_range->start), PFN_PHYS(range_len(tag_range)));
> > > +
> > > +	pr_info("Found tag storage region 0x%llx-0x%llx, block size %u pages",
> > > +		PFN_PHYS(tag_range->start), PFN_PHYS(tag_range->end), region->block_size);
> > > +
> > > +	num_tag_regions++;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void __init mte_tag_storage_init(void)
> > > +{
> > > +	struct range *tag_range;
> > > +	int i, ret;
> > > +
> > > +	ret = of_scan_flat_dt(fdt_init_tag_storage, NULL);
> > > +	if (ret) {
> > > +		for (i = 0; i < num_tag_regions; i++) {
> > > +			tag_range = &tag_regions[i].tag_range;
> > > +			memblock_remove(PFN_PHYS(tag_range->start), PFN_PHYS(range_len(tag_range)));
> > > +		}
> > > +		num_tag_regions = 0;
> > > +		pr_info("MTE tag storage region management disabled");
> > > +	}
> > > +}
> > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > > index 417a8a86b2db..1b77138c1aa5 100644
> > > --- a/arch/arm64/kernel/setup.c
> > > +++ b/arch/arm64/kernel/setup.c
> > > @@ -42,6 +42,7 @@
> > >  #include <asm/cpufeature.h>
> > >  #include <asm/cpu_ops.h>
> > >  #include <asm/kasan.h>
> > > +#include <asm/mte_tag_storage.h>
> > >  #include <asm/numa.h>
> > >  #include <asm/scs.h>
> > >  #include <asm/sections.h>
> > > @@ -342,6 +343,12 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
> > >  			   FW_BUG "Booted with MMU enabled!");
> > >  	}
> > >  
> > > +	/*
> > > +	 * Must be called before memory limits are enforced by
> > > +	 * arm64_memblock_init().
> > > +	 */
> > > +	mte_tag_storage_init();
> > > +
> > >  	arm64_memblock_init();
> > >  
> > >  	paging_init();
> > > -- 
> > > 2.42.1
> > > 
> > > 
> 
> 
>