| Message ID | 20190304204826.2414365-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | xen: avoid link error on ARM | expand |
On 3/4/19 3:47 PM, Arnd Bergmann wrote: > Building the privcmd code as a loadable module on ARM, we get > a link error due to the private cache management functions: > > ERROR: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko] undefined! Can __sync_icache_dcache be exported in arm32, just like it is in arm64? -boris
On Mon, Mar 4, 2019 at 10:19 PM Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote: > > On 3/4/19 3:47 PM, Arnd Bergmann wrote: > > Building the privcmd code as a loadable module on ARM, we get > > a link error due to the private cache management functions: > > > > ERROR: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko] undefined! > > Can __sync_icache_dcache be exported in arm32, just like it is in arm64? Russell wants all cache management operations to stay private to the kernel, as they tend to get abused by other drivers: https://lore.kernel.org/linux-arm-kernel/20181218100908.GL26090@n2100.armlinux.org.uk/ Arnd
On 04/03/2019 21:47, Arnd Bergmann wrote: > Building the privcmd code as a loadable module on ARM, we get > a link error due to the private cache management functions: > > ERROR: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko] undefined! > > Move the code into a new file that is built along with privcmd.o > but is always built-in, even when the latter is a loadable module. > > xen_remap_vma_range() may not be the best name here, if someone > comes up with a better one, let me know. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/xen/Makefile | 3 +++ > drivers/xen/mm.c | 41 +++++++++++++++++++++++++++++++++++++++++ > drivers/xen/privcmd.c | 30 +----------------------------- > include/xen/xen-ops.h | 3 +++ > 4 files changed, 48 insertions(+), 29 deletions(-) > create mode 100644 drivers/xen/mm.c > > diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile > index ad3844d9f876..7124f9e749b4 100644 > --- a/drivers/xen/Makefile > +++ b/drivers/xen/Makefile > @@ -29,6 +29,9 @@ obj-$(CONFIG_SWIOTLB_XEN) += swiotlb-xen.o > obj-$(CONFIG_XEN_MCE_LOG) += mcelog.o > obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback/ > obj-$(CONFIG_XEN_PRIVCMD) += xen-privcmd.o > +ifdef CONFIG_XEN_PRIVCMD > +obj-y += mm.o > +endif Can we avoid that ifdef in the Makefile? I'd rather have an architecture independant builtin driver added which is always included for CONFIG_XEN. This would allow to move redundant stuff from arch/*/xen/ into it (e.g. xen_vcpu_id). So: rename mm.c to xen-builtin.c, use: obj-$(CONFIG_XEN) += xen-builtin.o Juergen
On Tue, Mar 5, 2019 at 7:39 AM Juergen Gross <jgross@suse.com> wrote: > > Can we avoid that ifdef in the Makefile? > > I'd rather have an architecture independant builtin driver added which > is always included for CONFIG_XEN. This would allow to move redundant > stuff from arch/*/xen/ into it (e.g. xen_vcpu_id). > > So: rename mm.c to xen-builtin.c, use: > > obj-$(CONFIG_XEN) += xen-builtin.o Sure, I'm happy to change the naming and the Makefile logic. The way you suggested sounds fine to me, but it will make the xen code slightly bigger even if that code is not used. We could also have a silent Kconfig symbol that turns this on and still avoid the ifdef: obj-$(CONFIG_XEN_BUILTIN) += xen-builtin.o (or using any other symbol name that makes more sense than that. Let me know your preference and I'll resubmit. Arnd
On 05/03/2019 09:34, Arnd Bergmann wrote: > On Tue, Mar 5, 2019 at 7:39 AM Juergen Gross <jgross@suse.com> wrote: > >> >> Can we avoid that ifdef in the Makefile? >> >> I'd rather have an architecture independant builtin driver added which >> is always included for CONFIG_XEN. This would allow to move redundant >> stuff from arch/*/xen/ into it (e.g. xen_vcpu_id). >> >> So: rename mm.c to xen-builtin.c, use: >> >> obj-$(CONFIG_XEN) += xen-builtin.o > > Sure, I'm happy to change the naming and the Makefile logic. The way you > suggested sounds fine to me, but it will make the xen code slightly bigger > even if that code is not used. We could also have a silent Kconfig symbol > that turns this on and still avoid the ifdef: > > obj-$(CONFIG_XEN_BUILTIN) += xen-builtin.o That was my first thought. But looking through arch/[arm|x86]/xen/enlighten.c I found several global variables defined the same way. I'd like to merge those, too. So my preference is a common source for all this stuff. I'll send a followup patch to move the mentioned variables into the new source. Juergen
On Tue, Mar 5, 2019 at 10:05 AM Juergen Gross <jgross@suse.com> wrote: > > On 05/03/2019 09:34, Arnd Bergmann wrote: > > On Tue, Mar 5, 2019 at 7:39 AM Juergen Gross <jgross@suse.com> wrote: > > > >> > >> Can we avoid that ifdef in the Makefile? > >> > >> I'd rather have an architecture independant builtin driver added which > >> is always included for CONFIG_XEN. This would allow to move redundant > >> stuff from arch/*/xen/ into it (e.g. xen_vcpu_id). > >> > >> So: rename mm.c to xen-builtin.c, use: > >> > >> obj-$(CONFIG_XEN) += xen-builtin.o > > > > Sure, I'm happy to change the naming and the Makefile logic. The way you > > suggested sounds fine to me, but it will make the xen code slightly bigger > > even if that code is not used. We could also have a silent Kconfig symbol > > that turns this on and still avoid the ifdef: > > > > obj-$(CONFIG_XEN_BUILTIN) += xen-builtin.o > > That was my first thought. > > But looking through arch/[arm|x86]/xen/enlighten.c I found several > global variables defined the same way. I'd like to merge those, too. > > So my preference is a common source for all this stuff. Ok, makes sense. I've prepared that patch now and will send it after some more build testing. Arnd
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile index ad3844d9f876..7124f9e749b4 100644 --- a/drivers/xen/Makefile +++ b/drivers/xen/Makefile @@ -29,6 +29,9 @@ obj-$(CONFIG_SWIOTLB_XEN) += swiotlb-xen.o obj-$(CONFIG_XEN_MCE_LOG) += mcelog.o obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback/ obj-$(CONFIG_XEN_PRIVCMD) += xen-privcmd.o +ifdef CONFIG_XEN_PRIVCMD +obj-y += mm.o +endif obj-$(CONFIG_XEN_STUB) += xen-stub.o obj-$(CONFIG_XEN_ACPI_HOTPLUG_MEMORY) += xen-acpi-memhotplug.o obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU) += xen-acpi-cpuhotplug.o diff --git a/drivers/xen/mm.c b/drivers/xen/mm.c new file mode 100644 index 000000000000..8ad0d4900588 --- /dev/null +++ b/drivers/xen/mm.c @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Architecture independent helper functions for memory management + * + * Written by Paul Durrant <paul.durrant@citrix.com> + */ +#include <linux/mm.h> +#include <linux/export.h> + +struct remap_pfn { + struct mm_struct *mm; + struct page **pages; + pgprot_t prot; + unsigned long i; +}; + +static int remap_pfn_fn(pte_t *ptep, pgtable_t token, unsigned long addr, + void *data) +{ + struct remap_pfn *r = data; + struct page *page = r->pages[r->i]; + pte_t pte = pte_mkspecial(pfn_pte(page_to_pfn(page), r->prot)); + + set_pte_at(r->mm, addr, ptep, pte); + r->i++; + + return 0; +} + +/* Used by the privcmd module, but has to be built-in on ARM */ +int xen_remap_vma_range(struct vm_area_struct *vma, unsigned long addr, unsigned long len) +{ + struct remap_pfn r = { + .mm = vma->vm_mm, + .pages = vma->vm_private_data, + .prot = vma->vm_page_prot, + }; + + return apply_to_page_range(vma->vm_mm, addr, len, remap_pfn_fn, &r); +} +EXPORT_SYMBOL_GPL(xen_remap_vma_range); diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index b24ddac1604b..290b6aca7e1d 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -723,26 +723,6 @@ static long privcmd_ioctl_restrict(struct file *file, void __user *udata) return 0; } -struct remap_pfn { - struct mm_struct *mm; - struct page **pages; - pgprot_t prot; - unsigned long i; -}; - -static int remap_pfn_fn(pte_t *ptep, pgtable_t token, unsigned long addr, - void *data) -{ - struct remap_pfn *r = data; - struct page *page = r->pages[r->i]; - pte_t pte = pte_mkspecial(pfn_pte(page_to_pfn(page), r->prot)); - - set_pte_at(r->mm, addr, ptep, pte); - r->i++; - - return 0; -} - static long privcmd_ioctl_mmap_resource(struct file *file, void __user *udata) { struct privcmd_data *data = file->private_data; @@ -809,15 +789,7 @@ static long privcmd_ioctl_mmap_resource(struct file *file, void __user *udata) goto out; if (xen_feature(XENFEAT_auto_translated_physmap)) { - struct remap_pfn r = { - .mm = vma->vm_mm, - .pages = vma->vm_private_data, - .prot = vma->vm_page_prot, - }; - - rc = apply_to_page_range(r.mm, kdata.addr, - kdata.num << PAGE_SHIFT, - remap_pfn_fn, &r); + rc = xen_remap_vma_range(vma, kdata.addr, kdata.num << PAGE_SHIFT); } else { unsigned int domid = (xdata.flags & XENMEM_rsrc_acq_caller_owned) ? diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h index 4969817124a8..98b30c1613b2 100644 --- a/include/xen/xen-ops.h +++ b/include/xen/xen-ops.h @@ -109,6 +109,9 @@ static inline int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma, } #endif +int xen_remap_vma_range(struct vm_area_struct *vma, unsigned long addr, + unsigned long len); + /* * xen_remap_domain_gfn_array() - map an array of foreign frames by gfn * @vma: VMA to map the pages into
Building the privcmd code as a loadable module on ARM, we get a link error due to the private cache management functions: ERROR: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko] undefined! Move the code into a new file that is built along with privcmd.o but is always built-in, even when the latter is a loadable module. xen_remap_vma_range() may not be the best name here, if someone comes up with a better one, let me know. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/xen/Makefile | 3 +++ drivers/xen/mm.c | 41 +++++++++++++++++++++++++++++++++++++++++ drivers/xen/privcmd.c | 30 +----------------------------- include/xen/xen-ops.h | 3 +++ 4 files changed, 48 insertions(+), 29 deletions(-) create mode 100644 drivers/xen/mm.c -- 2.20.0