RE: [Xen-devel] [PATCH] x86/xen: support priv-mapping in an HVM tools domain

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Boris Ostrovsky
> Sent: 20 October 2017 16:09
> To: Paul Durrant <Paul.Durrant@citrix.com>; x86@kernel.org; xen-
> devel@lists.xenproject.org; linux-kernel@vger.kernel.org
> Cc: Juergen Gross <jgross@suse.com>; Thomas Gleixner
> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin
> <hpa@zytor.com>
> Subject: Re: [Xen-devel] [PATCH] x86/xen: support priv-mapping in an HVM
> tools domain
> 
> On 10/20/2017 04:35 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> >> Boris Ostrovsky
> >> Sent: 19 October 2017 18:45
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; x86@kernel.org; xen-
> >> devel@lists.xenproject.org; linux-kernel@vger.kernel.org
> >> Cc: Juergen Gross <jgross@suse.com>; Thomas Gleixner
> >> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin
> >> <hpa@zytor.com>
> >> Subject: Re: [Xen-devel] [PATCH] x86/xen: support priv-mapping in an
> HVM
> >> tools domain
> >>
> >> On 10/19/2017 11:26 AM, 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. A check is also added
> >>> to xen_remap_domain_gfn_range() to fail with -EOPNOTSUPP since this
> >>> should not be used in an HVM tools domain.
> >>>
> >>> 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 | 14 ++++++++++++--
> >>>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> >>> index 3e15345abfe7..d33e7dbe3129 100644
> >>> --- a/arch/x86/xen/mmu.c
> >>> +++ b/arch/x86/xen/mmu.c
> >>> @@ -172,6 +172,9 @@ int xen_remap_domain_gfn_range(struct
> >> vm_area_struct *vma,
> >>>  			       pgprot_t prot, unsigned domid,
> >>>  			       struct page **pages)
> >>>  {
> >>> +	if (xen_feature(XENFEAT_auto_translated_physmap))
> >>> +		return -EOPNOTSUPP;
> >>> +
> >> This is never called on XENFEAT_auto_translated_physmap domains,
> there
> >> is a check in privcmd_ioctl_mmap() for that.
> > Yes, that's true but it seems like the wrong place for such a check. I could
> remove that one it you'd prefer.
> 
> I actually think that perhaps we could wrap privcmd_ioctl_mmap() with
> "#ifdef CONFIG_XEN_PV" (#else return -ENOSYS) and move
> xen_remap_domain_gfn_range() to mmu_pv.c. We can then remove it from
> ARM
> code too.
> 
> >
> >>>  	return do_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid,
> >> pages);
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
> >>> @@ -182,6 +185,10 @@ int xen_remap_domain_gfn_array(struct
> >> vm_area_struct *vma,
> >>>  			       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);
> >>> +
> >> So how did this work before? In fact, I don't see any callers of
> >> xen_xlate_{re|un}map_gfn_range().
> > I assume mean 'array' for the map since there is no
> xen_xlate_remap_gfn_range() function. I'm not quite sure what you're
> asking? Without this patch the mmu code in an x86 domain simply assumes
> the domain is PV... the xlate code is currently only used via the arm mmu
> code (where it clearly knows it's not PV). AFAICS this Is just a straightforward
> buggy assumption in the x86 code.
> 
> Looks like this was originally intended for dom0 PVH and was removed by
> 063334f. So it should indeed be restored.
> 

Ok, I'll re-work the patch with your suggestion re xen_remap_domain_gfn_range() and send a v2.

Thanks,

  Paul

> 
> -boris
> 
> >
> >   Paul
> >
> >> -boris
> >>
> >>
> >>>  	/* 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".
> >>> @@ -193,9 +200,12 @@
> >> 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;
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xen.org
> >> https://lists.xen.org/xen-devel
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel