[PATCH v2] xen: support priv-mapping in an HVM tools domain

[PATCH v2] xen: support priv-mapping in an HVM tools domain

[Paul Durrant]

If the domain has XENFEAT_auto_translated_physmap then use of the PV-
specific HYPERVISOR_mmu_update hypercall is clearly incorrect.

This patch adds checks in xen_remap_domain_gfn_array() and
xen_unmap_domain_gfn_array() which call through to the approprate
xlate_mmu function if the feature is present.

This patch also moves xen_remap_domain_gfn_range() into the PV-only MMU
code and #ifdefs the (only) calling code in privcmd accordingly.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/xen/mmu.c    | 36 +++++++++++++++++-------------------
 arch/x86/xen/mmu_pv.c | 11 +++++++++++
 drivers/xen/privcmd.c | 17 +++++++++++++----
 include/xen/xen-ops.h |  7 +++++++
 4 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 3e15345abfe7..01837c36e293 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -91,12 +91,12 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, pgtable_t token,
 	return 0;
 }
 
-static int do_remap_gfn(struct vm_area_struct *vma,
-			unsigned long addr,
-			xen_pfn_t *gfn, int nr,
-			int *err_ptr, pgprot_t prot,
-			unsigned domid,
-			struct page **pages)
+int xen_remap_gfn(struct vm_area_struct *vma,
+		  unsigned long addr,
+		  xen_pfn_t *gfn, int nr,
+		  int *err_ptr, pgprot_t prot,
+		  unsigned int domid,
+		  struct page **pages)
 {
 	int err = 0;
 	struct remap_data rmd;
@@ -166,36 +166,34 @@ static int do_remap_gfn(struct vm_area_struct *vma,
 	return err < 0 ? err : mapped;
 }
 
-int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
-			       unsigned long addr,
-			       xen_pfn_t gfn, int nr,
-			       pgprot_t prot, unsigned domid,
-			       struct page **pages)
-{
-	return do_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid, pages);
-}
-EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
-
 int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
 			       unsigned long addr,
 			       xen_pfn_t *gfn, int nr,
 			       int *err_ptr, pgprot_t prot,
 			       unsigned domid, struct page **pages)
 {
+	if (xen_feature(XENFEAT_auto_translated_physmap))
+		return xen_xlate_remap_gfn_array(vma, addr, gfn, nr, err_ptr,
+						 prot, domid, pages);
+
 	/* We BUG_ON because it's a programmer error to pass a NULL err_ptr,
 	 * and the consequences later is quite hard to detect what the actual
 	 * cause of "wrong memory was mapped in".
 	 */
 	BUG_ON(err_ptr == NULL);
-	return do_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid, pages);
+	return xen_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid,
+			     pages);
 }
 EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_array);
 
 /* Returns: 0 success */
 int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
-			       int numpgs, struct page **pages)
+			       int nr, struct page **pages)
 {
-	if (!pages || !xen_feature(XENFEAT_auto_translated_physmap))
+	if (xen_feature(XENFEAT_auto_translated_physmap))
+		return xen_xlate_unmap_gfn_range(vma, nr, pages);
+
+	if (!pages)
 		return 0;
 
 	return -EINVAL;
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 71495f1a86d7..4974d8a6c2b4 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -2670,3 +2670,14 @@ phys_addr_t paddr_vmcoreinfo_note(void)
 		return __pa(vmcoreinfo_note);
 }
 #endif /* CONFIG_KEXEC_CORE */
+
+int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
+			       unsigned long addr,
+			       xen_pfn_t gfn, int nr,
+			       pgprot_t prot, unsigned int domid,
+			       struct page **pages)
+{
+	return xen_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid,
+			     pages);
+}
+EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index feca75b07fdd..b58a1719b606 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -215,6 +215,8 @@ static int traverse_pages_block(unsigned nelem, size_t size,
 	return ret;
 }
 
+#ifdef CONFIG_XEN_PV
+
 struct mmap_gfn_state {
 	unsigned long va;
 	struct vm_area_struct *vma;
@@ -261,10 +263,6 @@ static long privcmd_ioctl_mmap(struct file *file, void __user *udata)
 	LIST_HEAD(pagelist);
 	struct mmap_gfn_state state;
 
-	/* We only support privcmd_ioctl_mmap_batch for auto translated. */
-	if (xen_feature(XENFEAT_auto_translated_physmap))
-		return -ENOSYS;
-
 	if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
 		return -EFAULT;
 
@@ -312,6 +310,17 @@ static long privcmd_ioctl_mmap(struct file *file, void __user *udata)
 	return rc;
 }
 
+#else
+
+static long privcmd_ioctl_mmap(struct file *file, void __user *udata)
+{
+	/* We only support privcmd_ioctl_mmap for PV. */
+	return -ENOSYS;
+}
+
+
+#endif /* CONFIG_XEN_PV */
+
 struct mmap_batch_state {
 	domid_t domain;
 	unsigned long va;
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 218e6aae5433..663a9a06b762 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -60,6 +60,10 @@ static inline void xen_destroy_contiguous_region(phys_addr_t pstart,
 
 struct vm_area_struct;
 
+int xen_remap_gfn(struct vm_area_struct *vma, unsigned long addr,
+		  xen_pfn_t *gfn, int nr, int *err_ptr, pgprot_t prot,
+		  unsigned int domid, struct page **pages);
+
 /*
  * xen_remap_domain_gfn_array() - map an array of foreign frames
  * @vma:     VMA to map the pages into
@@ -84,6 +88,7 @@ int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
 			       unsigned domid,
 			       struct page **pages);
 
+#ifdef CONFIG_XEN_PV
 /* xen_remap_domain_gfn_range() - map a range of foreign frames
  * @vma:     VMA to map the pages into
  * @addr:    Address at which to map the pages
@@ -101,6 +106,8 @@ int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
 			       xen_pfn_t gfn, int nr,
 			       pgprot_t prot, unsigned domid,
 			       struct page **pages);
+#endif
+
 int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
 			       int numpgs, struct page **pages);
 int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
-- 
2.11.0

Re: [PATCH v2] xen: support priv-mapping in an HVM tools domain

[Juergen Gross]

On 01/11/17 12:31, Paul Durrant wrote:
> If the domain has XENFEAT_auto_translated_physmap then use of the PV-
> specific HYPERVISOR_mmu_update hypercall is clearly incorrect.
> 
> This patch adds checks in xen_remap_domain_gfn_array() and
> xen_unmap_domain_gfn_array() which call through to the approprate
> xlate_mmu function if the feature is present.
> 
> This patch also moves xen_remap_domain_gfn_range() into the PV-only MMU
> code and #ifdefs the (only) calling code in privcmd accordingly.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
>  arch/x86/xen/mmu.c    | 36 +++++++++++++++++-------------------
>  arch/x86/xen/mmu_pv.c | 11 +++++++++++
>  drivers/xen/privcmd.c | 17 +++++++++++++----
>  include/xen/xen-ops.h |  7 +++++++
>  4 files changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index 3e15345abfe7..01837c36e293 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -91,12 +91,12 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, pgtable_t token,
>  	return 0;
>  }
>  
> -static int do_remap_gfn(struct vm_area_struct *vma,
> -			unsigned long addr,
> -			xen_pfn_t *gfn, int nr,
> -			int *err_ptr, pgprot_t prot,
> -			unsigned domid,
> -			struct page **pages)
> +int xen_remap_gfn(struct vm_area_struct *vma,
> +		  unsigned long addr,
> +		  xen_pfn_t *gfn, int nr,
> +		  int *err_ptr, pgprot_t prot,
> +		  unsigned int domid,
> +		  struct page **pages)
>  {
>  	int err = 0;
>  	struct remap_data rmd;
> @@ -166,36 +166,34 @@ static int do_remap_gfn(struct vm_area_struct *vma,
>  	return err < 0 ? err : mapped;
>  }
>  
> -int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
> -			       unsigned long addr,
> -			       xen_pfn_t gfn, int nr,
> -			       pgprot_t prot, unsigned domid,
> -			       struct page **pages)
> -{
> -	return do_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid, pages);
> -}
> -EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
> -
>  int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       xen_pfn_t *gfn, int nr,
>  			       int *err_ptr, pgprot_t prot,
>  			       unsigned domid, struct page **pages)
>  {
> +	if (xen_feature(XENFEAT_auto_translated_physmap))
> +		return xen_xlate_remap_gfn_array(vma, addr, gfn, nr, err_ptr,
> +						 prot, domid, pages);
> +
>  	/* We BUG_ON because it's a programmer error to pass a NULL err_ptr,
>  	 * and the consequences later is quite hard to detect what the actual
>  	 * cause of "wrong memory was mapped in".
>  	 */
>  	BUG_ON(err_ptr == NULL);
> -	return do_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid, pages);
> +	return xen_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid,
> +			     pages);
>  }
>  EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_array);
>  
>  /* Returns: 0 success */
>  int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
> -			       int numpgs, struct page **pages)
> +			       int nr, struct page **pages)
>  {
> -	if (!pages || !xen_feature(XENFEAT_auto_translated_physmap))
> +	if (xen_feature(XENFEAT_auto_translated_physmap))
> +		return xen_xlate_unmap_gfn_range(vma, nr, pages);
> +
> +	if (!pages)
>  		return 0;
>  
>  	return -EINVAL;
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 71495f1a86d7..4974d8a6c2b4 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -2670,3 +2670,14 @@ phys_addr_t paddr_vmcoreinfo_note(void)
>  		return __pa(vmcoreinfo_note);
>  }
>  #endif /* CONFIG_KEXEC_CORE */
> +
> +int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
> +			       unsigned long addr,
> +			       xen_pfn_t gfn, int nr,
> +			       pgprot_t prot, unsigned int domid,
> +			       struct page **pages)
> +{
> +	return xen_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid,
> +			     pages);
> +}
> +EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index feca75b07fdd..b58a1719b606 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -215,6 +215,8 @@ static int traverse_pages_block(unsigned nelem, size_t size,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_XEN_PV
> +
>  struct mmap_gfn_state {
>  	unsigned long va;
>  	struct vm_area_struct *vma;
> @@ -261,10 +263,6 @@ static long privcmd_ioctl_mmap(struct file *file, void __user *udata)
>  	LIST_HEAD(pagelist);
>  	struct mmap_gfn_state state;
>  
> -	/* We only support privcmd_ioctl_mmap_batch for auto translated. */
> -	if (xen_feature(XENFEAT_auto_translated_physmap))
> -		return -ENOSYS;
> -

CONFIG_XEN_PV doesn't mean we don't support running as HVM guest.

I don't think you can remove this test, nor do I think it makes sense
to put privcmd_ioctl_mmap() inside an #ifdef.

You should rather add something like privcmd_ioctl_mmap_pv() with all
the PV-specific functionality and call this here. For !CONFIG_XEN_PV
it can e.g. just BUG().


Juergen

RE: [PATCH v2] xen: support priv-mapping in an HVM tools domain

[Paul Durrant]

> -----Original Message-----
> From: Juergen Gross [mailto:jgross@suse.com]
> Sent: 01 November 2017 13:40
> To: Paul Durrant <Paul.Durrant@citrix.com>; x86@kernel.org; xen-
> devel@lists.xenproject.org; linux-kernel@vger.kernel.org
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>; Thomas Gleixner
> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin
> <hpa@zytor.com>
> Subject: Re: [PATCH v2] xen: support priv-mapping in an HVM tools domain
> 
> On 01/11/17 12:31, Paul Durrant wrote:
> > If the domain has XENFEAT_auto_translated_physmap then use of the PV-
> > specific HYPERVISOR_mmu_update hypercall is clearly incorrect.
> >
> > This patch adds checks in xen_remap_domain_gfn_array() and
> > xen_unmap_domain_gfn_array() which call through to the approprate
> > xlate_mmu function if the feature is present.
> >
> > This patch also moves xen_remap_domain_gfn_range() into the PV-only
> MMU
> > code and #ifdefs the (only) calling code in privcmd accordingly.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > ---
> >  arch/x86/xen/mmu.c    | 36 +++++++++++++++++-------------------
> >  arch/x86/xen/mmu_pv.c | 11 +++++++++++
> >  drivers/xen/privcmd.c | 17 +++++++++++++----
> >  include/xen/xen-ops.h |  7 +++++++
> >  4 files changed, 48 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> > index 3e15345abfe7..01837c36e293 100644
> > --- a/arch/x86/xen/mmu.c
> > +++ b/arch/x86/xen/mmu.c
> > @@ -91,12 +91,12 @@ static int remap_area_mfn_pte_fn(pte_t *ptep,
> pgtable_t token,
> >  	return 0;
> >  }
> >
> > -static int do_remap_gfn(struct vm_area_struct *vma,
> > -			unsigned long addr,
> > -			xen_pfn_t *gfn, int nr,
> > -			int *err_ptr, pgprot_t prot,
> > -			unsigned domid,
> > -			struct page **pages)
> > +int xen_remap_gfn(struct vm_area_struct *vma,
> > +		  unsigned long addr,
> > +		  xen_pfn_t *gfn, int nr,
> > +		  int *err_ptr, pgprot_t prot,
> > +		  unsigned int domid,
> > +		  struct page **pages)
> >  {
> >  	int err = 0;
> >  	struct remap_data rmd;
> > @@ -166,36 +166,34 @@ static int do_remap_gfn(struct vm_area_struct
> *vma,
> >  	return err < 0 ? err : mapped;
> >  }
> >
> > -int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
> > -			       unsigned long addr,
> > -			       xen_pfn_t gfn, int nr,
> > -			       pgprot_t prot, unsigned domid,
> > -			       struct page **pages)
> > -{
> > -	return do_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid,
> pages);
> > -}
> > -EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
> > -
> >  int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
> >  			       unsigned long addr,
> >  			       xen_pfn_t *gfn, int nr,
> >  			       int *err_ptr, pgprot_t prot,
> >  			       unsigned domid, struct page **pages)
> >  {
> > +	if (xen_feature(XENFEAT_auto_translated_physmap))
> > +		return xen_xlate_remap_gfn_array(vma, addr, gfn, nr,
> err_ptr,
> > +						 prot, domid, pages);
> > +
> >  	/* We BUG_ON because it's a programmer error to pass a NULL
> err_ptr,
> >  	 * and the consequences later is quite hard to detect what the actual
> >  	 * cause of "wrong memory was mapped in".
> >  	 */
> >  	BUG_ON(err_ptr == NULL);
> > -	return do_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid,
> pages);
> > +	return xen_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid,
> > +			     pages);
> >  }
> >  EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_array);
> >
> >  /* Returns: 0 success */
> >  int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
> > -			       int numpgs, struct page **pages)
> > +			       int nr, struct page **pages)
> >  {
> > -	if (!pages || !xen_feature(XENFEAT_auto_translated_physmap))
> > +	if (xen_feature(XENFEAT_auto_translated_physmap))
> > +		return xen_xlate_unmap_gfn_range(vma, nr, pages);
> > +
> > +	if (!pages)
> >  		return 0;
> >
> >  	return -EINVAL;
> > diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> > index 71495f1a86d7..4974d8a6c2b4 100644
> > --- a/arch/x86/xen/mmu_pv.c
> > +++ b/arch/x86/xen/mmu_pv.c
> > @@ -2670,3 +2670,14 @@ phys_addr_t paddr_vmcoreinfo_note(void)
> >  		return __pa(vmcoreinfo_note);
> >  }
> >  #endif /* CONFIG_KEXEC_CORE */
> > +
> > +int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
> > +			       unsigned long addr,
> > +			       xen_pfn_t gfn, int nr,
> > +			       pgprot_t prot, unsigned int domid,
> > +			       struct page **pages)
> > +{
> > +	return xen_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid,
> > +			     pages);
> > +}
> > +EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index feca75b07fdd..b58a1719b606 100644
> > --- a/drivers/xen/privcmd.c
> > +++ b/drivers/xen/privcmd.c
> > @@ -215,6 +215,8 @@ static int traverse_pages_block(unsigned nelem,
> size_t size,
> >  	return ret;
> >  }
> >
> > +#ifdef CONFIG_XEN_PV
> > +
> >  struct mmap_gfn_state {
> >  	unsigned long va;
> >  	struct vm_area_struct *vma;
> > @@ -261,10 +263,6 @@ static long privcmd_ioctl_mmap(struct file *file,
> void __user *udata)
> >  	LIST_HEAD(pagelist);
> >  	struct mmap_gfn_state state;
> >
> > -	/* We only support privcmd_ioctl_mmap_batch for auto translated.
> */
> > -	if (xen_feature(XENFEAT_auto_translated_physmap))
> > -		return -ENOSYS;
> > -
> 
> CONFIG_XEN_PV doesn't mean we don't support running as HVM guest.
> 
> I don't think you can remove this test, nor do I think it makes sense
> to put privcmd_ioctl_mmap() inside an #ifdef.

Ok, I can leave the test in, but I only put the function inside the ifdef at Boris's request (unless I misinterpreted what he meant).

> 
> You should rather add something like privcmd_ioctl_mmap_pv() with all
> the PV-specific functionality and call this here. For !CONFIG_XEN_PV
> it can e.g. just BUG().
> 

Personally, I prefer my original patch where all the changes were kept inside mmu.c. It was much neater.

  Paul

> 
> Juergen

Re: [PATCH v2] xen: support priv-mapping in an HVM tools domain

[Juergen Gross]

On 01/11/17 14:45, Paul Durrant wrote:
>> -----Original Message-----
>> From: Juergen Gross [mailto:jgross@suse.com]
>> Sent: 01 November 2017 13:40
>> To: Paul Durrant <Paul.Durrant@citrix.com>; x86@kernel.org; xen-
>> devel@lists.xenproject.org; linux-kernel@vger.kernel.org
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>; Thomas Gleixner
>> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin
>> <hpa@zytor.com>
>> Subject: Re: [PATCH v2] xen: support priv-mapping in an HVM tools domain
>>
>> On 01/11/17 12:31, Paul Durrant wrote:
>>> If the domain has XENFEAT_auto_translated_physmap then use of the PV-
>>> specific HYPERVISOR_mmu_update hypercall is clearly incorrect.
>>>
>>> This patch adds checks in xen_remap_domain_gfn_array() and
>>> xen_unmap_domain_gfn_array() which call through to the approprate
>>> xlate_mmu function if the feature is present.
>>>
>>> This patch also moves xen_remap_domain_gfn_range() into the PV-only
>> MMU
>>> code and #ifdefs the (only) calling code in privcmd accordingly.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>> ---
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: Juergen Gross <jgross@suse.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>> ---
>>>  arch/x86/xen/mmu.c    | 36 +++++++++++++++++-------------------
>>>  arch/x86/xen/mmu_pv.c | 11 +++++++++++
>>>  drivers/xen/privcmd.c | 17 +++++++++++++----
>>>  include/xen/xen-ops.h |  7 +++++++
>>>  4 files changed, 48 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
>>> index 3e15345abfe7..01837c36e293 100644
>>> --- a/arch/x86/xen/mmu.c
>>> +++ b/arch/x86/xen/mmu.c
>>> @@ -91,12 +91,12 @@ static int remap_area_mfn_pte_fn(pte_t *ptep,
>> pgtable_t token,
>>>  	return 0;
>>>  }
>>>
>>> -static int do_remap_gfn(struct vm_area_struct *vma,
>>> -			unsigned long addr,
>>> -			xen_pfn_t *gfn, int nr,
>>> -			int *err_ptr, pgprot_t prot,
>>> -			unsigned domid,
>>> -			struct page **pages)
>>> +int xen_remap_gfn(struct vm_area_struct *vma,
>>> +		  unsigned long addr,
>>> +		  xen_pfn_t *gfn, int nr,
>>> +		  int *err_ptr, pgprot_t prot,
>>> +		  unsigned int domid,
>>> +		  struct page **pages)
>>>  {
>>>  	int err = 0;
>>>  	struct remap_data rmd;
>>> @@ -166,36 +166,34 @@ static int do_remap_gfn(struct vm_area_struct
>> *vma,
>>>  	return err < 0 ? err : mapped;
>>>  }
>>>
>>> -int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
>>> -			       unsigned long addr,
>>> -			       xen_pfn_t gfn, int nr,
>>> -			       pgprot_t prot, unsigned domid,
>>> -			       struct page **pages)
>>> -{
>>> -	return do_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid,
>> pages);
>>> -}
>>> -EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
>>> -
>>>  int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
>>>  			       unsigned long addr,
>>>  			       xen_pfn_t *gfn, int nr,
>>>  			       int *err_ptr, pgprot_t prot,
>>>  			       unsigned domid, struct page **pages)
>>>  {
>>> +	if (xen_feature(XENFEAT_auto_translated_physmap))
>>> +		return xen_xlate_remap_gfn_array(vma, addr, gfn, nr,
>> err_ptr,
>>> +						 prot, domid, pages);
>>> +
>>>  	/* We BUG_ON because it's a programmer error to pass a NULL
>> err_ptr,
>>>  	 * and the consequences later is quite hard to detect what the actual
>>>  	 * cause of "wrong memory was mapped in".
>>>  	 */
>>>  	BUG_ON(err_ptr == NULL);
>>> -	return do_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid,
>> pages);
>>> +	return xen_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid,
>>> +			     pages);
>>>  }
>>>  EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_array);
>>>
>>>  /* Returns: 0 success */
>>>  int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
>>> -			       int numpgs, struct page **pages)
>>> +			       int nr, struct page **pages)
>>>  {
>>> -	if (!pages || !xen_feature(XENFEAT_auto_translated_physmap))
>>> +	if (xen_feature(XENFEAT_auto_translated_physmap))
>>> +		return xen_xlate_unmap_gfn_range(vma, nr, pages);
>>> +
>>> +	if (!pages)
>>>  		return 0;
>>>
>>>  	return -EINVAL;
>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>> index 71495f1a86d7..4974d8a6c2b4 100644
>>> --- a/arch/x86/xen/mmu_pv.c
>>> +++ b/arch/x86/xen/mmu_pv.c
>>> @@ -2670,3 +2670,14 @@ phys_addr_t paddr_vmcoreinfo_note(void)
>>>  		return __pa(vmcoreinfo_note);
>>>  }
>>>  #endif /* CONFIG_KEXEC_CORE */
>>> +
>>> +int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
>>> +			       unsigned long addr,
>>> +			       xen_pfn_t gfn, int nr,
>>> +			       pgprot_t prot, unsigned int domid,
>>> +			       struct page **pages)
>>> +{
>>> +	return xen_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid,
>>> +			     pages);
>>> +}
>>> +EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
>>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
>>> index feca75b07fdd..b58a1719b606 100644
>>> --- a/drivers/xen/privcmd.c
>>> +++ b/drivers/xen/privcmd.c
>>> @@ -215,6 +215,8 @@ static int traverse_pages_block(unsigned nelem,
>> size_t size,
>>>  	return ret;
>>>  }
>>>
>>> +#ifdef CONFIG_XEN_PV
>>> +
>>>  struct mmap_gfn_state {
>>>  	unsigned long va;
>>>  	struct vm_area_struct *vma;
>>> @@ -261,10 +263,6 @@ static long privcmd_ioctl_mmap(struct file *file,
>> void __user *udata)
>>>  	LIST_HEAD(pagelist);
>>>  	struct mmap_gfn_state state;
>>>
>>> -	/* We only support privcmd_ioctl_mmap_batch for auto translated.
>> */
>>> -	if (xen_feature(XENFEAT_auto_translated_physmap))
>>> -		return -ENOSYS;
>>> -
>>
>> CONFIG_XEN_PV doesn't mean we don't support running as HVM guest.
>>
>> I don't think you can remove this test, nor do I think it makes sense
>> to put privcmd_ioctl_mmap() inside an #ifdef.
> 
> Ok, I can leave the test in, but I only put the function inside the ifdef at Boris's request (unless I misinterpreted what he meant).
> 
>>
>> You should rather add something like privcmd_ioctl_mmap_pv() with all
>> the PV-specific functionality and call this here. For !CONFIG_XEN_PV
>> it can e.g. just BUG().
>>
> 
> Personally, I prefer my original patch where all the changes were kept inside mmu.c. It was much neater.

Hmm, this was sent (and commented) during my vacation, so I didn't look
at it very thoroughly with Boris having done so.

TBH I like V1 better, too.

Boris, do you feel strong about the #ifdef part?


Juergen

Re: [PATCH v2] xen: support priv-mapping in an HVM tools domain

[Boris Ostrovsky]

On 11/01/2017 11:37 AM, Juergen Gross wrote:
>
> TBH I like V1 better, too.
>
> Boris, do you feel strong about the #ifdef part?

Having looked at what this turned into I now like V1 better too ;-)

Sorry, Paul.


-boris

RE: [PATCH v2] xen: support priv-mapping in an HVM tools domain

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 01 November 2017 18:19
> To: Juergen Gross <jgross@suse.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; x86@kernel.org; xen-
> devel@lists.xenproject.org; linux-kernel@vger.kernel.org
> Cc: Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar
> <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>
> Subject: Re: [PATCH v2] xen: support priv-mapping in an HVM tools domain
> 
> On 11/01/2017 11:37 AM, Juergen Gross wrote:
> >
> > TBH I like V1 better, too.
> >
> > Boris, do you feel strong about the #ifdef part?
> 
> Having looked at what this turned into I now like V1 better too ;-)
> 
> Sorry, Paul.

That's ok. Are you happy with v1 as-is or do you want me to submit a v3 with any tweaks?

  Paul

> 
> 
> -boris

Re: [PATCH v2] xen: support priv-mapping in an HVM tools domain

[Boris Ostrovsky]

On 11/02/2017 05:30 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> Sent: 01 November 2017 18:19
>> To: Juergen Gross <jgross@suse.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>; x86@kernel.org; xen-
>> devel@lists.xenproject.org; linux-kernel@vger.kernel.org
>> Cc: Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar
>> <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>
>> Subject: Re: [PATCH v2] xen: support priv-mapping in an HVM tools domain
>>
>> On 11/01/2017 11:37 AM, Juergen Gross wrote:
>>> TBH I like V1 better, too.
>>>
>>> Boris, do you feel strong about the #ifdef part?
>> Having looked at what this turned into I now like V1 better too ;-)
>>
>> Sorry, Paul.
> That's ok. Are you happy with v1 as-is or do you want me to submit a v3 with any tweaks?

V1:

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>