Re: [PATCH RFC v2 15/27] arm64: mte: Check that tag storage blocks are in the same zone

[Hyesoo Yu]

Hi~

On Thu, Nov 30, 2023 at 12:00:11PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Wed, Nov 29, 2023 at 05:57:44PM +0900, Hyesoo Yu wrote:
> > On Sun, Nov 19, 2023 at 04:57:09PM +0000, Alexandru Elisei wrote:
> > > alloc_contig_range() requires that the requested pages are in the same
> > > zone. Check that this is indeed the case before initializing the tag
> > > storage blocks.
> > > 
> > > Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> > > ---
> > >  arch/arm64/kernel/mte_tag_storage.c | 33 +++++++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kernel/mte_tag_storage.c b/arch/arm64/kernel/mte_tag_storage.c
> > > index 8b9bedf7575d..fd63430d4dc0 100644
> > > --- a/arch/arm64/kernel/mte_tag_storage.c
> > > +++ b/arch/arm64/kernel/mte_tag_storage.c
> > > @@ -265,6 +265,35 @@ void __init mte_tag_storage_init(void)
> > >  	}
> > >  }
> > >  
> > > +/* alloc_contig_range() requires all pages to be in the same zone. */
> > > +static int __init mte_tag_storage_check_zone(void)
> > > +{
> > > +	struct range *tag_range;
> > > +	struct zone *zone;
> > > +	unsigned long pfn;
> > > +	u32 block_size;
> > > +	int i, j;
> > > +
> > > +	for (i = 0; i < num_tag_regions; i++) {
> > > +		block_size = tag_regions[i].block_size;
> > > +		if (block_size == 1)
> > > +			continue;
> > > +
> > > +		tag_range = &tag_regions[i].tag_range;
> > > +		for (pfn = tag_range->start; pfn <= tag_range->end; pfn += block_size) {
> > > +			zone = page_zone(pfn_to_page(pfn));
> > 
> > Hello.
> > 
> > Since the blocks within the tag_range must all be in the same zone, can we move the "page_zone"
> > out of the loop ?
> `
> Hmm.. why do you say that the pages in a tag_range must be in the same
> zone? I am not very familiar with how the memory management code puts pages
> into zones, but I would imagine that pages in a tag range straddling the
> 4GB limit (so, let's say, from 3GB to 5GB) will end up in both ZONE_DMA and
> ZONE_NORMAL.
> 
> Thanks,
> Alex
> 

Oh, I see that reserve_tag_storage only calls alloc_contig_rnage in units of block_size,
I thought it could be called for the entire range the page needed at once.
(Maybe it could be a bit faster ? It doesn't seem like unnecessary drain and
other operation are repeated.)

If we use the cma code when activating the tag storage, it will be error if the
entire area of tag region is not in the same zone, so there should be a constraint
that it must be in the same zone when defining the tag region on device tree.

Thanks,
Regards.

> > 
> > Thanks,
> > Regards.
> > 
> > > +			for (j = 1; j < block_size; j++) {
> > > +				if (page_zone(pfn_to_page(pfn + j)) != zone) {
> > > +					pr_err("Tag storage block pages in different zones");
> > > +					return -EINVAL;
> > > +				}
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	 return 0;
> > > +}
> > > +
> > >  static int __init mte_tag_storage_activate_regions(void)
> > >  {
> > >  	phys_addr_t dram_start, dram_end;
> > > @@ -321,6 +350,10 @@ static int __init mte_tag_storage_activate_regions(void)
> > >  		goto out_disabled;
> > >  	}
> > >  
> > > +	ret = mte_tag_storage_check_zone();
> > > +	if (ret)
> > > +		goto out_disabled;
> > > +
> > >  	for (i = 0; i < num_tag_regions; i++) {
> > >  		tag_range = &tag_regions[i].tag_range;
> > >  		for (pfn = tag_range->start; pfn <= tag_range->end; pfn += pageblock_nr_pages)
> > > -- 
> > > 2.42.1
> > > 
> > > 
> 
> 
>