Re: [PATCH v2 09/12] lib: test_hmm add module param for zone device type

["Sierra Guiza, Alejandro (Alex)" <alex.sierra@xxxxxxx>]

On 9/20/2021 3:53 AM, Alistair Popple wrote:
> On Tuesday, 14 September 2021 2:16:01 AM AEST Alex Sierra wrote:
> > In order to configure device public in test_hmm, two module parameters
> > should be passed, which correspond to the SP start address of each
> > device (2) spm_addr_dev0 & spm_addr_dev1. If no parameters are passed,
> > private device type is configured.
> It's a pity that testing this seems to require some amount of special setup to
> test. Is there a way this could be made to work on a more standard setup
> similar to how DEVICE_PRIVATE is tested?
Hi Alistair
We tried to do it as simpler as possible. Unfortunately, there are two main
requirements to register dev memory as DEVICE_PUBLIC type. This memory must
NOT be accessed by any memory allocator (SLAB, SLOB, SLUB) plus, it has 
to be
CPU coherently accessed.  We also want to avoid aliasing the same PFNs for
different page types (regular system memory and DEVICE_PUBLIC). So we don't
want the reserved memory to be part of the kernel's memory map before we 
call
memremap_pages. A transparent way of doing it, without any special HW, was
setting a portion of system memory as SPM (Special purpose memory). And use
this as our “device fake” memory.

Regards,
Alex Sierra

> > Signed-off-by: Alex Sierra <alex.sierra@xxxxxxx>
> > ---
> > v5:
> > Remove devmem->pagemap.type = MEMORY_DEVICE_PRIVATE at
> > dmirror_allocate_chunk that was forcing to configure pagemap.type
> > to MEMORY_DEVICE_PRIVATE
> >
> > v6:
> > Check for null pointers for resource and memremap references
> > at dmirror_allocate_chunk
> >
> > v7:
> > Due to patch dropped from these patch series "kernel: resource:
> > lookup_resource as exported symbol", lookup_resource was not longer a
> > callable function. This was used in public device configuration, to
> > get start and end addresses, to create pgmap->range struct. This
> > information is now taken directly from the spm_addr_devX parameters and
> > the fixed size DEVMEM_CHUNK_SIZE.
> > ---
> >   lib/test_hmm.c      | 66 +++++++++++++++++++++++++++++++--------------
> >   lib/test_hmm_uapi.h |  1 +
> >   2 files changed, 47 insertions(+), 20 deletions(-)
> >
> > diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> > index 3cd91ca31dd7..ef27e355738a 100644
> > --- a/lib/test_hmm.c
> > +++ b/lib/test_hmm.c
> > @@ -33,6 +33,16 @@
> >   #define DEVMEM_CHUNK_SIZE		(256 * 1024 * 1024U)
> >   #define DEVMEM_CHUNKS_RESERVE		16
> >   
> > +static unsigned long spm_addr_dev0;
> > +module_param(spm_addr_dev0, long, 0644);
> > +MODULE_PARM_DESC(spm_addr_dev0,
> > +		"Specify start address for SPM (special purpose memory) used for device 0. By setting this Generic device type will be used. Make sure spm_addr_dev1 is set too");
> > +
> > +static unsigned long spm_addr_dev1;
> > +module_param(spm_addr_dev1, long, 0644);
> > +MODULE_PARM_DESC(spm_addr_dev1,
> > +		"Specify start address for SPM (special purpose memory) used for device 1. By setting this Generic device type will be used. Make sure spm_addr_dev0 is set too");
> > +
> >   static const struct dev_pagemap_ops dmirror_devmem_ops;
> >   static const struct mmu_interval_notifier_ops dmirror_min_ops;
> >   static dev_t dmirror_dev;
> > @@ -450,11 +460,11 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
> >   	return ret;
> >   }
> >   
> > -static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
> > +static int dmirror_allocate_chunk(struct dmirror_device *mdevice,
> >   				   struct page **ppage)
> >   {
> >   	struct dmirror_chunk *devmem;
> > -	struct resource *res;
> > +	struct resource *res = NULL;
> >   	unsigned long pfn;
> >   	unsigned long pfn_first;
> >   	unsigned long pfn_last;
> > @@ -462,17 +472,29 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
> >   
> >   	devmem = kzalloc(sizeof(*devmem), GFP_KERNEL);
> >   	if (!devmem)
> > -		return false;
> > +		return -ENOMEM;
> >   
> > -	res = request_free_mem_region(&iomem_resource, DEVMEM_CHUNK_SIZE,
> > -				      "hmm_dmirror");
> > -	if (IS_ERR(res))
> > -		goto err_devmem;
> > +	if (!spm_addr_dev0 && !spm_addr_dev1) {
> > +		res = request_free_mem_region(&iomem_resource, DEVMEM_CHUNK_SIZE,
> > +					      "hmm_dmirror");
> > +		if (IS_ERR_OR_NULL(res))
> > +			goto err_devmem;
> > +		devmem->pagemap.range.start = res->start;
> > +		devmem->pagemap.range.end = res->end;
> > +		devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
> > +		mdevice->zone_device_type = HMM_DMIRROR_MEMORY_DEVICE_PRIVATE;
> > +	} else if (spm_addr_dev0 && spm_addr_dev1) {
> > +		devmem->pagemap.range.start = MINOR(mdevice->cdevice.dev) ?
> > +							spm_addr_dev0 :
> > +							spm_addr_dev1;
> > +		devmem->pagemap.range.end = devmem->pagemap.range.start +
> > +					    DEVMEM_CHUNK_SIZE - 1;
> > +		devmem->pagemap.type = MEMORY_DEVICE_PUBLIC;
> > +		mdevice->zone_device_type = HMM_DMIRROR_MEMORY_DEVICE_PUBLIC;
> > +	} else {
> > +		pr_err("Both spm_addr_dev parameters should be set\n");
> > +	}
> >   
> > -	mdevice->zone_device_type = HMM_DMIRROR_MEMORY_DEVICE_PRIVATE;
> > -	devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
> > -	devmem->pagemap.range.start = res->start;
> > -	devmem->pagemap.range.end = res->end;
> >   	devmem->pagemap.nr_range = 1;
> >   	devmem->pagemap.ops = &dmirror_devmem_ops;
> >   	devmem->pagemap.owner = mdevice;
> > @@ -493,10 +515,14 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
> >   		mdevice->devmem_capacity = new_capacity;
> >   		mdevice->devmem_chunks = new_chunks;
> >   	}
> > -
> >   	ptr = memremap_pages(&devmem->pagemap, numa_node_id());
> > -	if (IS_ERR(ptr))
> > +	if (IS_ERR_OR_NULL(ptr)) {
> > +		if (ptr)
> > +			ret = PTR_ERR(ptr);
> > +		else
> > +			ret = -EFAULT;
> >   		goto err_release;
> > +	}
> >   
> >   	devmem->mdevice = mdevice;
> >   	pfn_first = devmem->pagemap.range.start >> PAGE_SHIFT;
> > @@ -529,7 +555,8 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice,
> >   
> >   err_release:
> >   	mutex_unlock(&mdevice->devmem_lock);
> > -	release_mem_region(devmem->pagemap.range.start, range_len(&devmem->pagemap.range));
> > +	if (res)
> > +		release_mem_region(devmem->pagemap.range.start, range_len(&devmem->pagemap.range));
> >   err_devmem:
> >   	kfree(devmem);
> >   
> > @@ -1097,10 +1124,8 @@ static int dmirror_device_init(struct dmirror_device *mdevice, int id)
> >   	if (ret)
> >   		return ret;
> >   
> > -	/* Build a list of free ZONE_DEVICE private struct pages */
> > -	dmirror_allocate_chunk(mdevice, NULL);
> > -
> > -	return 0;
> > +	/* Build a list of free ZONE_DEVICE struct pages */
> > +	return dmirror_allocate_chunk(mdevice, NULL);
> >   }
> >   
> >   static void dmirror_device_remove(struct dmirror_device *mdevice)
> > @@ -1113,8 +1138,9 @@ static void dmirror_device_remove(struct dmirror_device *mdevice)
> >   				mdevice->devmem_chunks[i];
> >   
> >   			memunmap_pages(&devmem->pagemap);
> > -			release_mem_region(devmem->pagemap.range.start,
> > -					   range_len(&devmem->pagemap.range));
> > +			if (devmem->pagemap.type == MEMORY_DEVICE_PRIVATE)
> > +				release_mem_region(devmem->pagemap.range.start,
> > +						   range_len(&devmem->pagemap.range));
> >   			kfree(devmem);
> >   		}
> >   		kfree(mdevice->devmem_chunks);
> > diff --git a/lib/test_hmm_uapi.h b/lib/test_hmm_uapi.h
> > index ee88701793d5..00259d994410 100644
> > --- a/lib/test_hmm_uapi.h
> > +++ b/lib/test_hmm_uapi.h
> > @@ -65,6 +65,7 @@ enum {
> >   enum {
> >   	/* 0 is reserved to catch uninitialized type fields */
> >   	HMM_DMIRROR_MEMORY_DEVICE_PRIVATE = 1,
> > +	HMM_DMIRROR_MEMORY_DEVICE_PUBLIC,
> >   };
> >   
> >   #endif /* _LIB_TEST_HMM_UAPI_H */