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

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
> Boris Ostrovsky
> Sent: 19 October 2017 18:45
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; x86@xxxxxxxxxx; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: Juergen Gross <jgross@xxxxxxxx>; Thomas Gleixner
> <tglx@xxxxxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>; H. Peter Anvin
> <hpa@xxxxxxxxx>
> 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@xxxxxxxxxx>
> > ---
> > Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > Cc: Juergen Gross <jgross@xxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > ---
> >  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.

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

  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@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel