[PATCH] xen: avoid link error on ARM

[PATCH] xen: avoid link error on ARM

[Arnd Bergmann]

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
 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
-- 
2.20.0

Re: [PATCH] xen: avoid link error on ARM

[Boris Ostrovsky]

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

Re: [PATCH] xen: avoid link error on ARM

[Arnd Bergmann]

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

Re: [PATCH] xen: avoid link error on ARM

[Juergen Gross]

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

Re: [PATCH] xen: avoid link error on ARM

[Arnd Bergmann]

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

Re: [PATCH] xen: avoid link error on ARM

[Juergen Gross]

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

Re: [PATCH] xen: avoid link error on ARM

[Arnd Bergmann]

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