[PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Julien Grall]

The code to initialize the grant table in libxc uses
xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
frame and to initialize it.

This solution has two major issues:
    - The check of the return of xc_domain_maximum_gpfn is buggy because
    xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
    Which is never catch with ( pfn <= 0 ).
    - The guest memory layout maybe filled up to the end, i.e
    xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
    hardware limitation.

Futhermore, on ARM, xc_domain_maximum_gpfn() is not implemented and
return -ENOSYS. This will make libxc to use always the same PFN which
may colapse with an already mapped region (see xen/include/public/arch-arm.h
for the layout).

This patch only address the problem for ARM, the x86 version use the same
behavior (ie xc_domain_maximum_gpfn() + 1), as I'm not familiar with Xen x86.

A new function xc_core_arch_get_scratch_gpfn is introduced to be able to
choose the gpfn per architecture.

For the ARM version, we use the GUEST_GNTTAB_GUEST which is the base of
the region by the guest to map the grant table. At the build time,
nothing is mapped there.

At the same time correctly check the return of xc_domain_maximum_gpfn
for x86.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>

---
    This patch is candidate for backport to Xen 4.5. With the support of
    MMIO passthrough, we may end up to erase an MMIO region.

    I don't think it's required for Xen 4.4.

    I chose to take this appproach after the discussion on implementing
    XENMEM_maximum_gpfn on ARM (https://patches.linaro.org/32894/).

    This patch has only been built tested on x86 and the same behavior
    has been kept (i.e xc_domain_maximum_gpfn() + 1). I would be happy
    if someone for x86 world is looking for a possible solution.

    Changes in v2:
        - x86: The +1 was missing to get the scratch pfn
        - Fix mispelling in comment in xc_core_arch_get_gpfn for ARM
---
 tools/libxc/xc_core.h     |  3 +++
 tools/libxc/xc_core_arm.c | 17 +++++++++++++++++
 tools/libxc/xc_core_x86.c | 17 +++++++++++++++++
 tools/libxc/xc_dom_boot.c | 18 ++++++++++++------
 4 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_core.h b/tools/libxc/xc_core.h
index 10cbfca..5867030 100644
--- a/tools/libxc/xc_core.h
+++ b/tools/libxc/xc_core.h
@@ -148,6 +148,9 @@ int xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width,
                                   shared_info_any_t *live_shinfo,
                                   xen_pfn_t **live_p2m, unsigned long *pfnp);
 
+int xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
+                                  xen_pfn_t *gpfn);
+
 
 #if defined (__i386__) || defined (__x86_64__)
 # include "xc_core_x86.h"
diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
index 2fbcf3f..16508e7 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -96,6 +96,23 @@ xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_do
     return xc_core_arch_map_p2m_rw(xch, dinfo, info,
                                    live_shinfo, live_p2m, pfnp, 1);
 }
+
+int
+xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
+                              xen_pfn_t *gpfn)
+{
+    /*
+     * The Grant Table region space is not used until the guest is
+     * booting. Use the first page for the scratch pfn.
+     */
+    XC_BUILD_BUG_ON(GUEST_GNTTAB_SIZE < XC_PAGE_SIZE);
+
+    *gpfn = GUEST_GNTTAB_BASE >> XC_PAGE_SHIFT;
+
+    return 0;
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index f05060a..fac99ec 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -205,6 +205,23 @@ xc_core_arch_map_p2m_writable(xc_interface *xch, unsigned int guest_width, xc_do
     return xc_core_arch_map_p2m_rw(xch, dinfo, info,
                                    live_shinfo, live_p2m, pfnp, 1);
 }
+
+int
+xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
+                              xen_pfn_t *gpfn)
+{
+    int rc;
+
+    rc = xc_domain_maximum_gpfn(xch, domid);
+
+    if ( rc <= 0 )
+        return rc;
+
+    *gpfn = rc + 1;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index f0a1c64..a141eb5 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -33,6 +33,7 @@
 
 #include "xg_private.h"
 #include "xc_dom.h"
+#include "xc_core.h"
 #include <xen/hvm/params.h>
 #include <xen/grant_table.h>
 
@@ -365,7 +366,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
                            domid_t xenstore_domid)
 {
     int rc;
-    xen_pfn_t max_gfn;
+    xen_pfn_t scratch_gpfn;
     struct xen_add_to_physmap xatp = {
         .domid = domid,
         .space = XENMAPSPACE_grant_table,
@@ -375,16 +376,21 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
         .domid = domid,
     };
 
-    max_gfn = xc_domain_maximum_gpfn(xch, domid);
-    if ( max_gfn <= 0 ) {
+    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gpfn);
+    if ( rc < 0 )
+    {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
-                     "%s: failed to get max gfn "
+                     "%s: failed to get a scratch gfn "
                      "[errno=%d]\n",
                      __FUNCTION__, errno);
         return -1;
     }
-    xatp.gpfn = max_gfn + 1;
-    xrfp.gpfn = max_gfn + 1;
+    xatp.gpfn = scratch_gpfn;
+    xrfp.gpfn = scratch_gpfn;
+
+    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
+                  scratch_gpfn);
+
 
     rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
     if ( rc != 0 )
-- 
2.1.4


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

Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Andrew Cooper]

On 21/01/15 13:25, Julien Grall wrote:
> The code to initialize the grant table in libxc uses
> xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
> frame and to initialize it.
>
> This solution has two major issues:
>     - The check of the return of xc_domain_maximum_gpfn is buggy because
>     xen_pfn_t is unsigned and in case of an error -ERRNO is returned.
>     Which is never catch with ( pfn <= 0 ).
>     - The guest memory layout maybe filled up to the end, i.e
>     xc_domain_maximum_gpfn() + 1 gives either 0 or an invalid PFN due to
>     hardware limitation.
>
> Futhermore, on ARM, xc_domain_maximum_gpfn() is not implemented and
> return -ENOSYS. This will make libxc to use always the same PFN which
> may colapse with an already mapped region (see xen/include/public/arch-arm.h
> for the layout).
>
> This patch only address the problem for ARM, the x86 version use the same
> behavior (ie xc_domain_maximum_gpfn() + 1), as I'm not familiar with Xen x86.
>
> A new function xc_core_arch_get_scratch_gpfn is introduced to be able to
> choose the gpfn per architecture.
>
> For the ARM version, we use the GUEST_GNTTAB_GUEST which is the base of
> the region by the guest to map the grant table. At the build time,
> nothing is mapped there.
>
> At the same time correctly check the return of xc_domain_maximum_gpfn
> for x86.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

For the x86 side of things, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

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

Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Ian Campbell]

On Wed, 2015-01-21 at 16:42 +0000, Andrew Cooper wrote:
> On 21/01/15 13:25, Julien Grall wrote:
> > The code to initialize the grant table in libxc uses
> > xc_domain_maximum_gpfn() + 1 to get a guest pfn for mapping the grant
> > frame and to initialize it.
> >[...]
> For the x86 side of things, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Acked the tools+arm side + applied.

Ian, Julien has suggested this for backport to 4.5 but not 4.4.

Ian.

Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping"):
> Ian, Julien has suggested this for backport to 4.5 but not 4.4.

Queued, thanks.

Ian.

[PATCH] tools/libxc: Don't leave scratch_pfn uninitialised if the domain has no memory

[Andrew Cooper]

c/s 5b5c40c0d1 "libxc: introduce a per architecture scratch pfn for temporary
grant mapping" accidentally an issue whereby there were two paths out of
xc_core_arch_get_scratch_gpfn() which returned 0, but only one of which
assigned a value to the gpfn parameter.

xc_domain_maximum_gpfn() can validly return 0, at which point gpfn 1 is a
valid scratch page to use.

In addition, widen rc before adding 1 and possibly overflowing.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Julien Grall <julien.grall@linaro.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_core_x86.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index fac99ec..d8846f1 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -214,10 +214,10 @@ xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
 
     rc = xc_domain_maximum_gpfn(xch, domid);
 
-    if ( rc <= 0 )
+    if ( rc < 0 )
         return rc;
 
-    *gpfn = rc + 1;
+    *gpfn = (xen_pfn_t)rc + 1;
 
     return 0;
 }
-- 
1.7.10.4

Re: [PATCH] tools/libxc: Don't leave scratch_pfn uninitialised if the domain has no memory

[Julien Grall]

Hi Andrew,

On 28/01/15 15:52, Andrew Cooper wrote:
> c/s 5b5c40c0d1 "libxc: introduce a per architecture scratch pfn for temporary
> grant mapping" accidentally an issue whereby there were two paths out of
> xc_core_arch_get_scratch_gpfn() which returned 0, but only one of which
> assigned a value to the gpfn parameter.
> xc_domain_maximum_gpfn() can validly return 0, at which point gpfn 1 is a
> valid scratch page to use.

The original version was considering rc = 0 as an error. Should not we
keep the same behavior?

Regards,

> In addition, widen rc before adding 1 and possibly overflowing.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Julien Grall <julien.grall@linaro.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/xc_core_x86.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
> index fac99ec..d8846f1 100644
> --- a/tools/libxc/xc_core_x86.c
> +++ b/tools/libxc/xc_core_x86.c
> @@ -214,10 +214,10 @@ xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
>  
>      rc = xc_domain_maximum_gpfn(xch, domid);
>  
> -    if ( rc <= 0 )
> +    if ( rc < 0 )
>          return rc;
>  
> -    *gpfn = rc + 1;
> +    *gpfn = (xen_pfn_t)rc + 1;
>  
>      return 0;
>  }
> 


-- 
Julien Grall

Re: [PATCH] tools/libxc: Don't leave scratch_pfn uninitialised if the domain has no memory

[Andrew Cooper]

On 29/01/2015 18:35, Julien Grall wrote:
> Hi Andrew,
>
> On 28/01/15 15:52, Andrew Cooper wrote:
>> c/s 5b5c40c0d1 "libxc: introduce a per architecture scratch pfn for temporary
>> grant mapping" accidentally an issue whereby there were two paths out of
>> xc_core_arch_get_scratch_gpfn() which returned 0, but only one of which
>> assigned a value to the gpfn parameter.
>> xc_domain_maximum_gpfn() can validly return 0, at which point gpfn 1 is a
>> valid scratch page to use.
> The original version was considering rc = 0 as an error. Should not we
> keep the same behavior?
>
> Regards,

The difference between this code and the original is that the original
returned two bits of information in its return value, whereas this has
return value and a parameter it fills in.

Independent of whether 0 should be a success or failure, the existing
caller used 0 as a success case and used an uninitialised piece of stack
as a scratch pfn.

As stated in the commit message, I believe that if 0 is the max memory
so far, 1 is a valid scratch pfn to use, making 0 from the hypercall a
valid success case.

~Andrew

Re: [PATCH] tools/libxc: Don't leave scratch_pfn uninitialised if the domain has no memory

[Ian Campbell]

On Wed, 2015-01-28 at 15:52 +0000, Andrew Cooper wrote:
> c/s 5b5c40c0d1 "libxc: introduce a per architecture scratch pfn for temporary
> grant mapping" accidentally an issue whereby there were two paths out of
> xc_core_arch_get_scratch_gpfn() which returned 0, but only one of which
> assigned a value to the gpfn parameter.
> 
> xc_domain_maximum_gpfn() can validly return 0, at which point gpfn 1 is a
> valid scratch page to use.
> 
> In addition, widen rc before adding 1 and possibly overflowing.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Julien Grall <julien.grall@linaro.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>

Acked + applied, thanks.

Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping"):
> Ian Campbell writes ("Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping"):
> > Ian, Julien has suggested this for backport to 4.5 but not 4.4.
> 
> Queued, thanks.

Backported to 4.5.

Ian.

Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping

[Andrew Cooper]

On 16/02/15 14:49, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping"):
>> Ian Campbell writes ("Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping"):
>>> Ian, Julien has suggested this for backport to 4.5 but not 4.4.
>> Queued, thanks.
> Backported to 4.5.
>
> Ian.

You need to also backport 5b0447f647b1031595d24a8a50b362726c887d12 which
is a bugfix to this changeset.

~Andrew

Re: [PATCH] tools/libxc: Don't leave scratch_pfn uninitialised if the domain has no memory [and 3 more messages]

[Ian Jackson]

Andrew Cooper writes ("[PATCH] tools/libxc: Don't leave scratch_pfn uninitialised if the domain has no memory"):
> c/s 5b5c40c0d1 "libxc: introduce a per architecture scratch pfn for temporary
> grant mapping" accidentally an issue whereby there were two paths out of
> xc_core_arch_get_scratch_gpfn() which returned 0, but only one of which
> assigned a value to the gpfn parameter.

Andrew Cooper writes ("Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping"):
> On 16/02/15 14:49, Ian Jackson wrote:
> > Ian Jackson writes ("Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping"):
> >> Ian Campbell writes ("Re: [PATCH v2] libxc: introduce a per architecture scratch pfn for temporary grant mapping"):
> >>> Ian, Julien has suggested this for backport to 4.5 but not 4.4.
> >> Queued, thanks.
> > Backported to 4.5.
> >
> > Ian.
> 
> You need to also backport 5b0447f647b1031595d24a8a50b362726c887d12 which
> is a bugfix to this changeset.

This is a reference to "tools/libxc: Don't leave scratch_pfn
uninitialised if the domain has no memory", as above.

I have backported that too.

thanks,
Ian.