| Message ID | 20190811081247.22111-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] resource: pass a name argument to devm_request_free_mem_region | expand |
On Sun, Aug 11, 2019 at 10:12:47AM +0200, Christoph Hellwig wrote: > The kvmppc ultravisor code wants a device private memory pool that is > system wide and not attached to a device. Instead of faking up one > provide a low-level memremap_pages for it. Note that this function is > not exported, and doesn't have a cleanup routine associated with it to > discourage use from more driver like users. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > include/linux/memremap.h | 1 + > mm/memremap.c | 74 ++++++++++++++++++++++++---------------- > 2 files changed, 45 insertions(+), 30 deletions(-) > > diff --git a/include/linux/memremap.h b/include/linux/memremap.h > index 8f0013e18e14..eac23e88a94a 100644 > +++ b/include/linux/memremap.h > @@ -123,6 +123,7 @@ static inline struct vmem_altmap *pgmap_altmap(struct dev_pagemap *pgmap) > } > > #ifdef CONFIG_ZONE_DEVICE > +void *memremap_pages(struct dev_pagemap *pgmap, int nid); > void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap); > void devm_memunmap_pages(struct device *dev, struct dev_pagemap *pgmap); > struct dev_pagemap *get_dev_pagemap(unsigned long pfn, > diff --git a/mm/memremap.c b/mm/memremap.c > index 09a087ca30ff..7b7575330db4 100644 > +++ b/mm/memremap.c > @@ -137,27 +137,12 @@ static void dev_pagemap_percpu_release(struct percpu_ref *ref) > complete(&pgmap->done); > } > > -/** > - * devm_memremap_pages - remap and provide memmap backing for the given resource > - * @dev: hosting device for @res > - * @pgmap: pointer to a struct dev_pagemap > - * > - * Notes: > - * 1/ At a minimum the res and type members of @pgmap must be initialized > - * by the caller before passing it to this function > - * > - * 2/ The altmap field may optionally be initialized, in which case > - * PGMAP_ALTMAP_VALID must be set in pgmap->flags. > - * > - * 3/ The ref field may optionally be provided, in which pgmap->ref must be > - * 'live' on entry and will be killed and reaped at > - * devm_memremap_pages_release() time, or if this routine fails. > - * > - * 4/ res is expected to be a host memory range that could feasibly be > - * treated as a "System RAM" range, i.e. not a device mmio range, but > - * this is not enforced. > +/* > + * This version is not intended for system resources only, and there is no Was 'is not' what was intended here? I'm having a hard time reading this. Jason
On Sun, Aug 11, 2019 at 10:56:07PM +0000, Jason Gunthorpe wrote: > > + * This version is not intended for system resources only, and there is no > > Was 'is not' what was intended here? I'm having a hard time reading > this. s/not//g
On Sun, Aug 11, 2019 at 10:12:47AM +0200, Christoph Hellwig wrote: > The kvmppc ultravisor code wants a device private memory pool that is > system wide and not attached to a device. Instead of faking up one > provide a low-level memremap_pages for it. Note that this function is > not exported, and doesn't have a cleanup routine associated with it to > discourage use from more driver like users. The kvmppc secure pages management code will be part of kvm-hv which can be built as module too. So it would require memremap_pages() to be exported. Additionally, non-dev version of the cleanup routine devm_memremap_pages_release() or equivalent would also be requried. With device being present, put_device() used to take care of this cleanup. Regards, Bharata.
On Mon, Aug 12, 2019 at 08:20:58PM +0530, Bharata B Rao wrote: > On Sun, Aug 11, 2019 at 10:12:47AM +0200, Christoph Hellwig wrote: > > The kvmppc ultravisor code wants a device private memory pool that is > > system wide and not attached to a device. Instead of faking up one > > provide a low-level memremap_pages for it. Note that this function is > > not exported, and doesn't have a cleanup routine associated with it to > > discourage use from more driver like users. > > The kvmppc secure pages management code will be part of kvm-hv which > can be built as module too. So it would require memremap_pages() to be > exported. > > Additionally, non-dev version of the cleanup routine > devm_memremap_pages_release() or equivalent would also be requried. > With device being present, put_device() used to take care of this > cleanup. Oh well. We can add them fairly easily if we really need to, but I tried to avoid that. Can you try to see if this works non-modular for you for now until we hear more feedback from Dan?
On Mon, Aug 12, 2019 at 05:00:12PM +0200, Christoph Hellwig wrote: > On Mon, Aug 12, 2019 at 08:20:58PM +0530, Bharata B Rao wrote: > > On Sun, Aug 11, 2019 at 10:12:47AM +0200, Christoph Hellwig wrote: > > > The kvmppc ultravisor code wants a device private memory pool that is > > > system wide and not attached to a device. Instead of faking up one > > > provide a low-level memremap_pages for it. Note that this function is > > > not exported, and doesn't have a cleanup routine associated with it to > > > discourage use from more driver like users. > > > > The kvmppc secure pages management code will be part of kvm-hv which > > can be built as module too. So it would require memremap_pages() to be > > exported. > > > > Additionally, non-dev version of the cleanup routine > > devm_memremap_pages_release() or equivalent would also be requried. > > With device being present, put_device() used to take care of this > > cleanup. > > Oh well. We can add them fairly easily if we really need to, but I > tried to avoid that. Can you try to see if this works non-modular > for you for now until we hear more feedback from Dan? Yes, this patchset works non-modular and with kvm-hv as module, it works with devm_memremap_pages_release() and release_mem_region() in the cleanup path. The cleanup path will be required in the non-modular case too for proper recovery from failures. Regards, Bharata.
On Tue, Aug 13, 2019 at 10:26:11AM +0530, Bharata B Rao wrote: > Yes, this patchset works non-modular and with kvm-hv as module, it > works with devm_memremap_pages_release() and release_mem_region() in the > cleanup path. The cleanup path will be required in the non-modular > case too for proper recovery from failures. Can you check if the version here: git://git.infradead.org/users/hch/misc.git pgmap-remove-dev Gitweb: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/pgmap-remove-dev works for you fully before I resend? > > Regards, > Bharata. ---end quoted text---
On Wed, Aug 14, 2019 at 08:11:50AM +0200, Christoph Hellwig wrote: > On Tue, Aug 13, 2019 at 10:26:11AM +0530, Bharata B Rao wrote: > > Yes, this patchset works non-modular and with kvm-hv as module, it > > works with devm_memremap_pages_release() and release_mem_region() in the > > cleanup path. The cleanup path will be required in the non-modular > > case too for proper recovery from failures. > > Can you check if the version here: > > git://git.infradead.org/users/hch/misc.git pgmap-remove-dev > > Gitweb: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/pgmap-remove-dev > > works for you fully before I resend? Yes, this works for us. This and migrate-vma-cleanup series helps to really simplify the kvmppc secure pages management code. Thanks. Regards, Bharata.
On Wed, Aug 14, 2019 at 02:28:26PM +0530, Bharata B Rao wrote: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/pgmap-remove-dev > > > > works for you fully before I resend? > > Yes, this works for us. This and migrate-vma-cleanup series helps to > really simplify the kvmppc secure pages management code. Thanks. Thanks. I'm going to resend it once we've made a bit of progress on the migrate_vma series that I resent this morning. There are a few more lose ends in this area with implications for the driver API, so I might have a few more patches for you to test in a bit.
diff --git a/include/linux/memremap.h b/include/linux/memremap.h index 8f0013e18e14..eac23e88a94a 100644 --- a/include/linux/memremap.h +++ b/include/linux/memremap.h @@ -123,6 +123,7 @@ static inline struct vmem_altmap *pgmap_altmap(struct dev_pagemap *pgmap) } #ifdef CONFIG_ZONE_DEVICE +void *memremap_pages(struct dev_pagemap *pgmap, int nid); void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap); void devm_memunmap_pages(struct device *dev, struct dev_pagemap *pgmap); struct dev_pagemap *get_dev_pagemap(unsigned long pfn, diff --git a/mm/memremap.c b/mm/memremap.c index 09a087ca30ff..7b7575330db4 100644 --- a/mm/memremap.c +++ b/mm/memremap.c @@ -137,27 +137,12 @@ static void dev_pagemap_percpu_release(struct percpu_ref *ref) complete(&pgmap->done); } -/** - * devm_memremap_pages - remap and provide memmap backing for the given resource - * @dev: hosting device for @res - * @pgmap: pointer to a struct dev_pagemap - * - * Notes: - * 1/ At a minimum the res and type members of @pgmap must be initialized - * by the caller before passing it to this function - * - * 2/ The altmap field may optionally be initialized, in which case - * PGMAP_ALTMAP_VALID must be set in pgmap->flags. - * - * 3/ The ref field may optionally be provided, in which pgmap->ref must be - * 'live' on entry and will be killed and reaped at - * devm_memremap_pages_release() time, or if this routine fails. - * - * 4/ res is expected to be a host memory range that could feasibly be - * treated as a "System RAM" range, i.e. not a device mmio range, but - * this is not enforced. +/* + * This version is not intended for system resources only, and there is no + * way to clean up the resource acquisitions. If you need to clean up you + * probably want dev_memremap_pages below. */ -void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) +void *memremap_pages(struct dev_pagemap *pgmap, int nid) { struct resource *res = &pgmap->res; struct dev_pagemap *conflict_pgmap; @@ -168,7 +153,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) .altmap = pgmap_altmap(pgmap), }; pgprot_t pgprot = PAGE_KERNEL; - int error, nid, is_ram; + int error, is_ram; bool need_devmap_managed = true; switch (pgmap->type) { @@ -223,7 +208,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) conflict_pgmap = get_dev_pagemap(PHYS_PFN(res->start), NULL); if (conflict_pgmap) { - dev_WARN(dev, "Conflicting mapping in same section\n"); + WARN(1, "Conflicting mapping in same section\n"); put_dev_pagemap(conflict_pgmap); error = -ENOMEM; goto err_array; @@ -231,7 +216,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) conflict_pgmap = get_dev_pagemap(PHYS_PFN(res->end), NULL); if (conflict_pgmap) { - dev_WARN(dev, "Conflicting mapping in same section\n"); + WARN(1, "Conflicting mapping in same section\n"); put_dev_pagemap(conflict_pgmap); error = -ENOMEM; goto err_array; @@ -252,7 +237,6 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) if (error) goto err_array; - nid = dev_to_node(dev); if (nid < 0) nid = numa_mem_id(); @@ -308,12 +292,6 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) PHYS_PFN(res->start), PHYS_PFN(resource_size(res)), pgmap); percpu_ref_get_many(pgmap->ref, pfn_end(pgmap) - pfn_first(pgmap)); - - error = devm_add_action_or_reset(dev, devm_memremap_pages_release, - pgmap); - if (error) - return ERR_PTR(error); - return __va(res->start); err_add_memory: @@ -328,6 +306,42 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) devmap_managed_enable_put(); return ERR_PTR(error); } + +/** + * devm_memremap_pages - remap and provide memmap backing for the given resource + * @dev: hosting device for @res + * @pgmap: pointer to a struct dev_pagemap + * + * Notes: + * 1/ At a minimum the res and type members of @pgmap must be initialized + * by the caller before passing it to this function + * + * 2/ The altmap field may optionally be initialized, in which case + * PGMAP_ALTMAP_VALID must be set in pgmap->flags. + * + * 3/ The ref field may optionally be provided, in which pgmap->ref must be + * 'live' on entry and will be killed and reaped at + * devm_memremap_pages_release() time, or if this routine fails. + * + * 4/ res is expected to be a host memory range that could feasibly be + * treated as a "System RAM" range, i.e. not a device mmio range, but + * this is not enforced. + */ +void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) +{ + int error; + void *ret; + + ret = memremap_pages(pgmap, dev_to_node(dev)); + if (IS_ERR(ret)) + return ret; + + error = devm_add_action_or_reset(dev, devm_memremap_pages_release, + pgmap); + if (error) + return ERR_PTR(error); + return ret; +} EXPORT_SYMBOL_GPL(devm_memremap_pages); void devm_memunmap_pages(struct device *dev, struct dev_pagemap *pgmap)
The kvmppc ultravisor code wants a device private memory pool that is system wide and not attached to a device. Instead of faking up one provide a low-level memremap_pages for it. Note that this function is not exported, and doesn't have a cleanup routine associated with it to discourage use from more driver like users. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/memremap.h | 1 + mm/memremap.c | 74 ++++++++++++++++++++++++---------------- 2 files changed, 45 insertions(+), 30 deletions(-)