From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932583AbdKANps (ORCPT ); Wed, 1 Nov 2017 09:45:48 -0400 Received: from smtp.citrix.com.au ([103.14.252.240]:53659 "EHLO SMTP.CITRIX.COM.AU" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752143AbdKANpq (ORCPT ); Wed, 1 Nov 2017 09:45:46 -0400 X-IronPort-AV: E=Sophos;i="5.44,327,1505779200"; d="scan'208";a="106600683" From: Paul Durrant To: "'Juergen Gross'" , "x86@kernel.org" , "xen-devel@lists.xenproject.org" , "linux-kernel@vger.kernel.org" CC: Boris Ostrovsky , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" Subject: RE: [PATCH v2] xen: support priv-mapping in an HVM tools domain Thread-Topic: [PATCH v2] xen: support priv-mapping in an HVM tools domain Thread-Index: AQHTUwT91PcMajTDMk6KKQoCSvx+IKL/duOAgAARXpA= Date: Wed, 1 Nov 2017 13:45:08 +0000 Message-ID: <30616b0c528248d3bc23bcb53311c1e2@AMSPEX02CL03.citrite.net> References: <20171101113136.1844-1-paul.durrant@citrix.com> In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id vA1DjrZP032469 > -----Original Message----- > From: Juergen Gross [mailto:jgross@suse.com] > Sent: 01 November 2017 13:40 > To: Paul Durrant ; x86@kernel.org; xen- > devel@lists.xenproject.org; linux-kernel@vger.kernel.org > Cc: Boris Ostrovsky ; Thomas Gleixner > ; Ingo Molnar ; H. Peter Anvin > > 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 > > --- > > Cc: Boris Ostrovsky > > Cc: Juergen Gross > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > --- > > 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