Mailing List Archive: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

[PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

Oct 4, 2012, 8:11 AM

Post #1 of 7 (188 views)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
arch/arm/xen/enlighten.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index ba5cc13..9956af5 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -9,6 +9,7 @@
#include <xen/platform_pci.h>
#include <xen/xenbus.h>
#include <xen/page.h>
+#include <xen/xen-ops.h>
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include <linux/interrupt.h>
@@ -18,6 +19,8 @@
#include <linux/of_irq.h>
#include <linux/of_address.h>

+#include <linux/mm.h>
+
struct start_info _xen_start_info;
struct start_info *xen_start_info = &_xen_start_info;
EXPORT_SYMBOL_GPL(xen_start_info);
@@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);

static __read_mostly int xen_events_irq = -1;

+/* map fgmfn of domid to lpfn in the current domain */
+static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
+ unsigned int domid)
+{
+ int rc;
+ struct xen_add_to_physmap xatp = {
+ .domid = DOMID_SELF,
+ .u.foreign_domid = domid,
+ .space = XENMAPSPACE_gmfn_foreign,
+ .idx = fgmfn,
+ .gpfn = lpfn,
+ };
+
+ rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
+ if (rc) {
+ pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
+ rc, lpfn, fgmfn);
+ return 1;
+ }
+ return 0;
+}
+
+struct remap_data {
+ unsigned long fgmfn; /* foreign domain's gmfn */
+ pgprot_t prot;
+ domid_t domid;
+ struct vm_area_struct *vma;
+ struct xen_remap_mfn_info *info;
+};
+
+static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
+ void *data)
+{
+ struct remap_data *info = data;
+ struct xen_remap_mfn_info *savp = info->info;
+ struct page *page = savp->pi_paga[savp->pi_next_todo++];
+ unsigned long pfn = page_to_pfn(page);
+ pte_t pte = pfn_pte(pfn, info->prot);
+
+ if (map_foreign_page(pfn, info->fgmfn, info->domid))
+ return -EFAULT;
+ set_pte_at(info->vma->vm_mm, addr, ptep, pte);
+
+ return 0;
+}
+
int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
unsigned long addr,
unsigned long mfn, int nr,
- pgprot_t prot, unsigned domid)
+ pgprot_t prot, unsigned domid,
+ struct xen_remap_mfn_info *info)
{
- return -ENOSYS;
+ int err;
+ struct remap_data data;
+
+ /* TBD: Batching, current sole caller only does page at a time */
+ if (nr > 1)
+ return -EINVAL;
+
+ data.fgmfn = mfn;
+ data.prot = prot;
+ data.domid = domid;
+ data.vma = vma;
+ data.info = info;
+ err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
+ remap_pte_fn, &data);
+ return err;
}
EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);

+/* Returns: Number of pages unmapped */
+int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
+ struct xen_remap_mfn_info *info)
+{
+ int count = 0;
+
+ while (info->pi_next_todo--) {
+ struct xen_remove_from_physmap xrp;
+ unsigned long rc, pfn;
+
+ pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);
+
+ xrp.domid = DOMID_SELF;
+ xrp.gpfn = pfn;
+ rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
+ if (rc) {
+ pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
+ pfn, rc);
+ return count;
+ }
+ count++;
+ }
+ return count;
+}
+EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
+
/*
* see Documentation/devicetree/bindings/arm/xen.txt for the
* documentation of the Xen Device Tree format.
--
1.7.2.5

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings. [ In reply to ]

konrad.wilk at oracle

Oct 4, 2012, 8:24 AM

Post #2 of 7 (180 views)

Permalink

On Thu, Oct 04, 2012 at 04:11:52PM +0100, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> arch/arm/xen/enlighten.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index ba5cc13..9956af5 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -9,6 +9,7 @@
> #include <xen/platform_pci.h>
> #include <xen/xenbus.h>
> #include <xen/page.h>
> +#include <xen/xen-ops.h>
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <linux/interrupt.h>
> @@ -18,6 +19,8 @@
> #include <linux/of_irq.h>
> #include <linux/of_address.h>
>
> +#include <linux/mm.h>
> +
> struct start_info _xen_start_info;
> struct start_info *xen_start_info = &_xen_start_info;
> EXPORT_SYMBOL_GPL(xen_start_info);
> @@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>
> static __read_mostly int xen_events_irq = -1;
>
> +/* map fgmfn of domid to lpfn in the current domain */

<laughs> Say that really fast a couple of times :-)

Any way we can explain it a bit more of what each acronym means?

> +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> + unsigned int domid)
> +{
> + int rc;
> + struct xen_add_to_physmap xatp = {
> + .domid = DOMID_SELF,
> + .u.foreign_domid = domid,
> + .space = XENMAPSPACE_gmfn_foreign,
> + .idx = fgmfn,
> + .gpfn = lpfn,
> + };
> +
> + rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> + if (rc) {
> + pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",

How about 'Failed to map pfn (0x%lx) to mfn (0x%lx), rc: %d\n" ?

> + rc, lpfn, fgmfn);
> + return 1;
> + }
> + return 0;
> +}
> +
> +struct remap_data {
> + unsigned long fgmfn; /* foreign domain's gmfn */

Shouldn't this be called now 'xen_pfn_t' or something.

> + pgprot_t prot;
> + domid_t domid;
> + struct vm_area_struct *vma;
> + struct xen_remap_mfn_info *info;
> +};
> +
> +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> + void *data)
> +{
> + struct remap_data *info = data;
> + struct xen_remap_mfn_info *savp = info->info;
> + struct page *page = savp->pi_paga[savp->pi_next_todo++];
> + unsigned long pfn = page_to_pfn(page);
> + pte_t pte = pfn_pte(pfn, info->prot);
> +
> + if (map_foreign_page(pfn, info->fgmfn, info->domid))
> + return -EFAULT;
> + set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> +
> + return 0;
> +}
> +
> int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> unsigned long addr,
> unsigned long mfn, int nr,
> - pgprot_t prot, unsigned domid)
> + pgprot_t prot, unsigned domid,
> + struct xen_remap_mfn_info *info)
> {
> - return -ENOSYS;
> + int err;
> + struct remap_data data;
> +
> + /* TBD: Batching, current sole caller only does page at a time */
> + if (nr > 1)

Lets also wrap it with WARN_ON?

> + return -EINVAL;
> +
> + data.fgmfn = mfn;
> + data.prot = prot;
> + data.domid = domid;
> + data.vma = vma;
> + data.info = info;
> + err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> + remap_pte_fn, &data);
> + return err;
> }
> EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>
> +/* Returns: Number of pages unmapped */
> +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> + struct xen_remap_mfn_info *info)
> +{
> + int count = 0;
> +
> + while (info->pi_next_todo--) {
> + struct xen_remove_from_physmap xrp;
> + unsigned long rc, pfn;
> +
> + pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);

Won't this miss the first pi_next_todo? You did the 'pi_next_todo--' so
will the compiler decrement it here in this operation or will it do
when it gets to the 'do' logic of the loop?

> +
> + xrp.domid = DOMID_SELF;
> + xrp.gpfn = pfn;
> + rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);

'rc' is 'unsigned long'. Is that right? You don't want it to be 'int'?

> + if (rc) {
> + pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> + pfn, rc);
> + return count;
> + }
> + count++;
> + }
> + return count;
> +}
> +EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> +
> /*
> * see Documentation/devicetree/bindings/arm/xen.txt for the
> * documentation of the Xen Device Tree format.
> --
> 1.7.2.5

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings. [ In reply to ]

Ian.Campbell at citrix

Oct 4, 2012, 8:48 AM

Post #3 of 7 (186 views)

Permalink

On Thu, 2012-10-04 at 16:24 +0100, Konrad Rzeszutek Wilk wrote:
> On Thu, Oct 04, 2012 at 04:11:52PM +0100, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > arch/arm/xen/enlighten.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
> > 1 files changed, 92 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index ba5cc13..9956af5 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -9,6 +9,7 @@
> > #include <xen/platform_pci.h>
> > #include <xen/xenbus.h>
> > #include <xen/page.h>
> > +#include <xen/xen-ops.h>
> > #include <asm/xen/hypervisor.h>
> > #include <asm/xen/hypercall.h>
> > #include <linux/interrupt.h>
> > @@ -18,6 +19,8 @@
> > #include <linux/of_irq.h>
> > #include <linux/of_address.h>
> >
> > +#include <linux/mm.h>
> > +
> > struct start_info _xen_start_info;
> > struct start_info *xen_start_info = &_xen_start_info;
> > EXPORT_SYMBOL_GPL(xen_start_info);
> > @@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
> >
> > static __read_mostly int xen_events_irq = -1;
> >
> > +/* map fgmfn of domid to lpfn in the current domain */
>
> <laughs> Say that really fast a couple of times :-)
>
> Any way we can explain it a bit more of what each acronym means?

The names come from the x86 PVH version, which has the comment:
/* Map foreign gmfn, fgmfn, to local pfn, lpfn. This for the user space
* creating new guest on PVH dom0 and needs to map domU pages.
*/
Is that preferable? (modulo removing the PVH bit)

>
> > +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> > + unsigned int domid)
> > +{
> > + int rc;
> > + struct xen_add_to_physmap xatp = {
> > + .domid = DOMID_SELF,
> > + .u.foreign_domid = domid,
> > + .space = XENMAPSPACE_gmfn_foreign,
> > + .idx = fgmfn,
> > + .gpfn = lpfn,
> > + };
> > +
> > + rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> > + if (rc) {
> > + pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
>
> How about 'Failed to map pfn (0x%lx) to mfn (0x%lx), rc: %d\n" ?

Sure, need to slip foreign domid in there somewhere now I look at it.

x86 PVH has basically the same print BTW.

>
> > + rc, lpfn, fgmfn);
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > +struct remap_data {
> > + unsigned long fgmfn; /* foreign domain's gmfn */
>
> Shouldn't this be called now 'xen_pfn_t' or something.

xen_pfn_t is needed at the hypervisor interface layer, I'm not so sure
about kernel internal use. I guess it can't hurt?

>
> > + pgprot_t prot;
> > + domid_t domid;
> > + struct vm_area_struct *vma;
> > + struct xen_remap_mfn_info *info;
> > +};
> > +
> > +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> > + void *data)
> > +{
> > + struct remap_data *info = data;
> > + struct xen_remap_mfn_info *savp = info->info;
> > + struct page *page = savp->pi_paga[savp->pi_next_todo++];
> > + unsigned long pfn = page_to_pfn(page);
> > + pte_t pte = pfn_pte(pfn, info->prot);
> > +
> > + if (map_foreign_page(pfn, info->fgmfn, info->domid))
> > + return -EFAULT;
> > + set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> > +
> > + return 0;
> > +}
> > +
> > int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> > unsigned long addr,
> > unsigned long mfn, int nr,
> > - pgprot_t prot, unsigned domid)
> > + pgprot_t prot, unsigned domid,
> > + struct xen_remap_mfn_info *info)
> > {
> > - return -ENOSYS;
> > + int err;
> > + struct remap_data data;
> > +
> > + /* TBD: Batching, current sole caller only does page at a time */
> > + if (nr > 1)
>
> Lets also wrap it with WARN_ON?

ACK, needs doing on x86 PVH too then.

>
> > + return -EINVAL;
> > +
> > + data.fgmfn = mfn;
> > + data.prot = prot;
> > + data.domid = domid;
> > + data.vma = vma;
> > + data.info = info;
> > + err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> > + remap_pte_fn, &data);
> > + return err;
> > }
> > EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> >
> > +/* Returns: Number of pages unmapped */
> > +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> > + struct xen_remap_mfn_info *info)
> > +{
> > + int count = 0;
> > +
> > + while (info->pi_next_todo--) {
> > + struct xen_remove_from_physmap xrp;
> > + unsigned long rc, pfn;
> > +
> > + pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);
>
> Won't this miss the first pi_next_todo? You did the 'pi_next_todo--' so
> will the compiler decrement it here in this operation or will it do
> when it gets to the 'do' logic of the loop?

It's a post decrement so if pi_next_todo == 1 then the expression in the
while will see 1 (true) but inside the loop we see zero. So we end up
processing elements N-1..0 of the array which is correct.

This is the same as on x86 PVH, so if I'm wrong then that is too.

I mentioned in the PVH thread this morning that I think this interface
should drop pi_next_todo and have a simple for loop based on the number
of pages between vm_start...vm_end here.

>
> > +
> > + xrp.domid = DOMID_SELF;
> > + xrp.gpfn = pfn;
> > + rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
>
> 'rc' is 'unsigned long'. Is that right? You don't want it to be 'int'?

Hypercalls return unsigned long these days, I think it was considered a
mistake that some didn't. See <25744:47080c965937> in the hypervisor
tree.

Oh, wait, we are both wrong -- it's a long. I'll fix that...

>
> > + if (rc) {
> > + pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> > + pfn, rc);
> > + return count;
> > + }
> > + count++;
> > + }
> > + return count;
> > +}
> > +EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> > +
> > /*
> > * see Documentation/devicetree/bindings/arm/xen.txt for the
> > * documentation of the Xen Device Tree format.
> > --
> > 1.7.2.5

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings. [ In reply to ]

konrad.wilk at oracle

Oct 4, 2012, 8:54 AM

Post #4 of 7 (182 views)

Permalink

On Thu, Oct 04, 2012 at 04:48:38PM +0100, Ian Campbell wrote:
> On Thu, 2012-10-04 at 16:24 +0100, Konrad Rzeszutek Wilk wrote:
> > On Thu, Oct 04, 2012 at 04:11:52PM +0100, Ian Campbell wrote:
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > > arch/arm/xen/enlighten.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
> > > 1 files changed, 92 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > > index ba5cc13..9956af5 100644
> > > --- a/arch/arm/xen/enlighten.c
> > > +++ b/arch/arm/xen/enlighten.c
> > > @@ -9,6 +9,7 @@
> > > #include <xen/platform_pci.h>
> > > #include <xen/xenbus.h>
> > > #include <xen/page.h>
> > > +#include <xen/xen-ops.h>
> > > #include <asm/xen/hypervisor.h>
> > > #include <asm/xen/hypercall.h>
> > > #include <linux/interrupt.h>
> > > @@ -18,6 +19,8 @@
> > > #include <linux/of_irq.h>
> > > #include <linux/of_address.h>
> > >
> > > +#include <linux/mm.h>
> > > +
> > > struct start_info _xen_start_info;
> > > struct start_info *xen_start_info = &_xen_start_info;
> > > EXPORT_SYMBOL_GPL(xen_start_info);
> > > @@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
> > >
> > > static __read_mostly int xen_events_irq = -1;
> > >
> > > +/* map fgmfn of domid to lpfn in the current domain */
> >
> > <laughs> Say that really fast a couple of times :-)
> >
> > Any way we can explain it a bit more of what each acronym means?
>
> The names come from the x86 PVH version, which has the comment:
> /* Map foreign gmfn, fgmfn, to local pfn, lpfn. This for the user space
> * creating new guest on PVH dom0 and needs to map domU pages.
> */
> Is that preferable? (modulo removing the PVH bit)

<nods>
>
> >
> > > +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> > > + unsigned int domid)
> > > +{
> > > + int rc;
> > > + struct xen_add_to_physmap xatp = {
> > > + .domid = DOMID_SELF,
> > > + .u.foreign_domid = domid,
> > > + .space = XENMAPSPACE_gmfn_foreign,
> > > + .idx = fgmfn,
> > > + .gpfn = lpfn,
> > > + };
> > > +
> > > + rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> > > + if (rc) {
> > > + pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
> >
> > How about 'Failed to map pfn (0x%lx) to mfn (0x%lx), rc: %d\n" ?
>
> Sure, need to slip foreign domid in there somewhere now I look at it.
>
> x86 PVH has basically the same print BTW.

OK, lets address that as well in the next review of the patchset.
>
> >
> > > + rc, lpfn, fgmfn);
> > > + return 1;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +struct remap_data {
> > > + unsigned long fgmfn; /* foreign domain's gmfn */
> >
> > Shouldn't this be called now 'xen_pfn_t' or something.
>
> xen_pfn_t is needed at the hypervisor interface layer, I'm not so sure
> about kernel internal use. I guess it can't hurt?

My thoughts.. as somebody might be wondering why here is unsigned long
but other places it is not.
>
> >
> > > + pgprot_t prot;
> > > + domid_t domid;
> > > + struct vm_area_struct *vma;
> > > + struct xen_remap_mfn_info *info;
> > > +};
> > > +
> > > +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> > > + void *data)
> > > +{
> > > + struct remap_data *info = data;
> > > + struct xen_remap_mfn_info *savp = info->info;
> > > + struct page *page = savp->pi_paga[savp->pi_next_todo++];
> > > + unsigned long pfn = page_to_pfn(page);
> > > + pte_t pte = pfn_pte(pfn, info->prot);
> > > +
> > > + if (map_foreign_page(pfn, info->fgmfn, info->domid))
> > > + return -EFAULT;
> > > + set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> > > unsigned long addr,
> > > unsigned long mfn, int nr,
> > > - pgprot_t prot, unsigned domid)
> > > + pgprot_t prot, unsigned domid,
> > > + struct xen_remap_mfn_info *info)
> > > {
> > > - return -ENOSYS;
> > > + int err;
> > > + struct remap_data data;
> > > +
> > > + /* TBD: Batching, current sole caller only does page at a time */
> > > + if (nr > 1)
> >
> > Lets also wrap it with WARN_ON?
>
> ACK, needs doing on x86 PVH too then.
>
> >
> > > + return -EINVAL;
> > > +
> > > + data.fgmfn = mfn;
> > > + data.prot = prot;
> > > + data.domid = domid;
> > > + data.vma = vma;
> > > + data.info = info;
> > > + err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> > > + remap_pte_fn, &data);
> > > + return err;
> > > }
> > > EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> > >
> > > +/* Returns: Number of pages unmapped */
> > > +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> > > + struct xen_remap_mfn_info *info)
> > > +{
> > > + int count = 0;
> > > +
> > > + while (info->pi_next_todo--) {
> > > + struct xen_remove_from_physmap xrp;
> > > + unsigned long rc, pfn;
> > > +
> > > + pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);
> >
> > Won't this miss the first pi_next_todo? You did the 'pi_next_todo--' so
> > will the compiler decrement it here in this operation or will it do
> > when it gets to the 'do' logic of the loop?
>
> It's a post decrement so if pi_next_todo == 1 then the expression in the
> while will see 1 (true) but inside the loop we see zero. So we end up
> processing elements N-1..0 of the array which is correct.

OK. That is what I wanted to make sure about.
>
> This is the same as on x86 PVH, so if I'm wrong then that is too.
>
> I mentioned in the PVH thread this morning that I think this interface
> should drop pi_next_todo and have a simple for loop based on the number
> of pages between vm_start...vm_end here.

Yeah, I think we need to do that. I understand Mukesh's desire to have
an easy searchable name for variables, but that 'pi' just makes me
think of bathroom, then of 3.1415, and then I have to actually recall
really hard why it is called 'pi' .. and I still can't remember why.
>
> >
> > > +
> > > + xrp.domid = DOMID_SELF;
> > > + xrp.gpfn = pfn;
> > > + rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> >
> > 'rc' is 'unsigned long'. Is that right? You don't want it to be 'int'?
>
> Hypercalls return unsigned long these days, I think it was considered a
> mistake that some didn't. See <25744:47080c965937> in the hypervisor
> tree.
>
> Oh, wait, we are both wrong -- it's a long. I'll fix that...
>
> >
> > > + if (rc) {
> > > + pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> > > + pfn, rc);
> > > + return count;
> > > + }
> > > + count++;
> > > + }
> > > + return count;
> > > +}
> > > +EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> > > +
> > > /*
> > > * see Documentation/devicetree/bindings/arm/xen.txt for the
> > > * documentation of the Xen Device Tree format.
> > > --
> > > 1.7.2.5
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings. [ In reply to ]

stefano.stabellini at eu

Oct 5, 2012, 6:36 AM

Post #5 of 7 (177 views)

Permalink

On Thu, 4 Oct 2012, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> arch/arm/xen/enlighten.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index ba5cc13..9956af5 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -9,6 +9,7 @@
> #include <xen/platform_pci.h>
> #include <xen/xenbus.h>
> #include <xen/page.h>
> +#include <xen/xen-ops.h>
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <linux/interrupt.h>
> @@ -18,6 +19,8 @@
> #include <linux/of_irq.h>
> #include <linux/of_address.h>
>
> +#include <linux/mm.h>
> +
> struct start_info _xen_start_info;
> struct start_info *xen_start_info = &_xen_start_info;
> EXPORT_SYMBOL_GPL(xen_start_info);
> @@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>
> static __read_mostly int xen_events_irq = -1;
>
> +/* map fgmfn of domid to lpfn in the current domain */
> +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> + unsigned int domid)
> +{
> + int rc;
> + struct xen_add_to_physmap xatp = {
> + .domid = DOMID_SELF,
> + .u.foreign_domid = domid,
> + .space = XENMAPSPACE_gmfn_foreign,
> + .idx = fgmfn,
> + .gpfn = lpfn,
> + };
> +
> + rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> + if (rc) {
> + pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
> + rc, lpfn, fgmfn);
> + return 1;
> + }
> + return 0;
> +}
> +
> +struct remap_data {
> + unsigned long fgmfn; /* foreign domain's gmfn */
> + pgprot_t prot;
> + domid_t domid;
> + struct vm_area_struct *vma;
> + struct xen_remap_mfn_info *info;
> +};
> +
> +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> + void *data)
> +{
> + struct remap_data *info = data;
> + struct xen_remap_mfn_info *savp = info->info;
> + struct page *page = savp->pi_paga[savp->pi_next_todo++];
> + unsigned long pfn = page_to_pfn(page);
> + pte_t pte = pfn_pte(pfn, info->prot);
> +
> + if (map_foreign_page(pfn, info->fgmfn, info->domid))
> + return -EFAULT;
> + set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> +
> + return 0;
> +}
> +
> int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> unsigned long addr,
> unsigned long mfn, int nr,
> - pgprot_t prot, unsigned domid)
> + pgprot_t prot, unsigned domid,
> + struct xen_remap_mfn_info *info)
> {
> - return -ENOSYS;
> + int err;
> + struct remap_data data;
> +
> + /* TBD: Batching, current sole caller only does page at a time */
> + if (nr > 1)
> + return -EINVAL;

It looks like that this implementation of xen_remap_domain_mfn_range is
capable of handling nr > 1, so why this return -EINVAL?

> + data.fgmfn = mfn;
> + data.prot = prot;
> + data.domid = domid;
> + data.vma = vma;
> + data.info = info;
> + err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> + remap_pte_fn, &data);
> + return err;
> }
> EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>
> +/* Returns: Number of pages unmapped */
> +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> + struct xen_remap_mfn_info *info)
> +{
> + int count = 0;
> +
> + while (info->pi_next_todo--) {
> + struct xen_remove_from_physmap xrp;
> + unsigned long rc, pfn;
> +
> + pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);
> +
> + xrp.domid = DOMID_SELF;
> + xrp.gpfn = pfn;
> + rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> + if (rc) {
> + pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> + pfn, rc);
> + return count;
> + }
> + count++;
> + }
> + return count;
> +}
> +EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> +
> /*
> * see Documentation/devicetree/bindings/arm/xen.txt for the
> * documentation of the Xen Device Tree format.
> --
> 1.7.2.5
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings. [ In reply to ]

Ian.Campbell at citrix

Oct 5, 2012, 6:42 AM

Post #6 of 7 (173 views)

Permalink

On Fri, 2012-10-05 at 14:36 +0100, Stefano Stabellini wrote:
[.... snip dozens of pointless lines -- please trim your quotes...]

> > - return -ENOSYS;
> > + int err;
> > + struct remap_data data;
> > +
> > + /* TBD: Batching, current sole caller only does page at a time */
> > + if (nr > 1)
> > + return -EINVAL;
>
> It looks like that this implementation of xen_remap_domain_mfn_range is
> capable of handling nr > 1, so why this return -EINVAL?

Hrm, yes. I think I just blindly copied that from the pvh
implementation.

[...]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings. [ In reply to ]

mukesh.rathor at oracle

Oct 8, 2012, 5:59 PM

Post #7 of 7 (171 views)

Permalink

On Fri, 5 Oct 2012 14:42:30 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Fri, 2012-10-05 at 14:36 +0100, Stefano Stabellini wrote:
> [.... snip dozens of pointless lines -- please trim your quotes...]
>
> > > - return -ENOSYS;
> > > + int err;
> > > + struct remap_data data;
> > > +
> > > + /* TBD: Batching, current sole caller only does page at
> > > a time */
> > > + if (nr > 1)
> > > + return -EINVAL;
> >
> > It looks like that this implementation of
> > xen_remap_domain_mfn_range is capable of handling nr > 1, so why
> > this return -EINVAL?
>
> Hrm, yes. I think I just blindly copied that from the pvh
> implementation.
>
> [...]

PVH was using a different mechanism earlier, pfn from high up memory
address space. So it was doing one at a time. It can be removed now.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel