[PATCH for-4.10 0/5] tools/dombuilder: Fixes and improvements to grant handling

[PATCH for-4.10 0/5] tools/dombuilder: Fixes and improvements to grant handling

[Andrew Cooper]

A git tree version is available:

http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/dombuilder-gnt-v1

Andrew Cooper (5):
  tools/dombuilder: Drop more PVH v1 leftovers
  tools/dombuilder: Remove clear_page() from xc_dom_boot.c
  tools/dombuilder: Switch to using gfn terminology for console and
    xenstore rings
  tools/dombuilder: Fix asymetry when setting up console and xenstore
    rings
  tools/dombuilder: Prevent failures of xc_dom_gnttab_init()

 tools/libxc/include/xc_dom.h      |  26 ++++++--
 tools/libxc/xc_dom_arm.c          |  17 +++---
 tools/libxc/xc_dom_boot.c         | 121 +++++++++++++++++++++++---------------
 tools/libxc/xc_dom_compat_linux.c |   6 +-
 tools/libxc/xc_dom_core.c         |   8 +++
 tools/libxc/xc_dom_x86.c          |  57 ++++++++++--------
 tools/libxl/libxl_dom.c           |  51 ++++++----------
 tools/libxl/libxl_internal.h      |   1 -
 8 files changed, 165 insertions(+), 122 deletions(-)

-- 
2.1.4


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

[PATCH for-4.10 1/5] tools/dombuilder: Drop more PVH v1 leftovers

[Andrew Cooper]

alloc_magic_pages() is renamed to alloc_magic_pages_pv() to mirror its
alloc_magic_pages_hvm() counterpart.  Delete a redundant comment, introduce
some newlines clarity, and remove a logically dead allocation of shared info.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/xc_dom_x86.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index cb68efc..885ca1b 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -534,24 +534,20 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
 
 /* ------------------------------------------------------------------------ */
 
-static int alloc_magic_pages(struct xc_dom_image *dom)
+static int alloc_magic_pages_pv(struct xc_dom_image *dom)
 {
-    /* allocate special pages */
     dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
     if ( dom->start_info_pfn == INVALID_PFN )
         return -1;
+
     dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
     if ( dom->xenstore_pfn == INVALID_PFN )
         return -1;
+
     dom->console_pfn = xc_dom_alloc_page(dom, "console");
     if ( dom->console_pfn == INVALID_PFN )
         return -1;
-    if ( xc_dom_translated(dom) )
-    {
-        dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared info");
-        if ( dom->shared_info_pfn == INVALID_PFN )
-            return -1;
-    }
+
     dom->alloc_bootstack = 1;
 
     return 0;
@@ -1756,7 +1752,7 @@ static struct xc_dom_arch xc_dom_32_pae = {
     .sizeof_pfn = 4,
     .p2m_base_supported = 0,
     .arch_private_size = sizeof(struct xc_dom_image_x86),
-    .alloc_magic_pages = alloc_magic_pages,
+    .alloc_magic_pages = alloc_magic_pages_pv,
     .alloc_pgtables = alloc_pgtables_x86_32_pae,
     .alloc_p2m_list = alloc_p2m_list_x86_32,
     .setup_pgtables = setup_pgtables_x86_32_pae,
@@ -1775,7 +1771,7 @@ static struct xc_dom_arch xc_dom_64 = {
     .sizeof_pfn = 8,
     .p2m_base_supported = 1,
     .arch_private_size = sizeof(struct xc_dom_image_x86),
-    .alloc_magic_pages = alloc_magic_pages,
+    .alloc_magic_pages = alloc_magic_pages_pv,
     .alloc_pgtables = alloc_pgtables_x86_64,
     .alloc_p2m_list = alloc_p2m_list_x86_64,
     .setup_pgtables = setup_pgtables_x86_64,
-- 
2.1.4


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

Re: [PATCH for-4.10 1/5] tools/dombuilder: Drop more PVH v1 leftovers

[Roger Pau Monné]

On Thu, Oct 05, 2017 at 06:23:39PM +0000, Andrew Cooper wrote:
> alloc_magic_pages() is renamed to alloc_magic_pages_pv() to mirror its
> alloc_magic_pages_hvm() counterpart.  Delete a redundant comment, introduce
> some newlines clarity, and remove a logically dead allocation of shared info.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  tools/libxc/xc_dom_x86.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index cb68efc..885ca1b 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -534,24 +534,20 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
>  
>  /* ------------------------------------------------------------------------ */
>  
> -static int alloc_magic_pages(struct xc_dom_image *dom)
> +static int alloc_magic_pages_pv(struct xc_dom_image *dom)
>  {
> -    /* allocate special pages */
>      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
>      if ( dom->start_info_pfn == INVALID_PFN )
>          return -1;

Maybe those errors paths should set errno = ENOMEM?

In any case the patch is an improvement.

Thanks, Roger.

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

Re: [PATCH for-4.10 1/5] tools/dombuilder: Drop more PVH v1 leftovers

[Wei Liu]

On Fri, Oct 06, 2017 at 10:26:03AM +0100, Roger Pau Monné wrote:
> On Thu, Oct 05, 2017 at 06:23:39PM +0000, Andrew Cooper wrote:
> > alloc_magic_pages() is renamed to alloc_magic_pages_pv() to mirror its
> > alloc_magic_pages_hvm() counterpart.  Delete a redundant comment, introduce
> > some newlines clarity, and remove a logically dead allocation of shared info.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> > ---
> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Julien Grall <julien.grall@arm.com>
> > ---
> >  tools/libxc/xc_dom_x86.c | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> > index cb68efc..885ca1b 100644
> > --- a/tools/libxc/xc_dom_x86.c
> > +++ b/tools/libxc/xc_dom_x86.c
> > @@ -534,24 +534,20 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
> >  
> >  /* ------------------------------------------------------------------------ */
> >  
> > -static int alloc_magic_pages(struct xc_dom_image *dom)
> > +static int alloc_magic_pages_pv(struct xc_dom_image *dom)
> >  {
> > -    /* allocate special pages */
> >      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
> >      if ( dom->start_info_pfn == INVALID_PFN )
> >          return -1;
> 
> Maybe those errors paths should set errno = ENOMEM?

I believe the actual allocation function already does that.

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

Re: [PATCH for-4.10 1/5] tools/dombuilder: Drop more PVH v1 leftovers

[Roger Pau Monné]

On Fri, Oct 06, 2017 at 09:33:52AM +0000, Wei Liu wrote:
> On Fri, Oct 06, 2017 at 10:26:03AM +0100, Roger Pau Monné wrote:
> > On Thu, Oct 05, 2017 at 06:23:39PM +0000, Andrew Cooper wrote:
> > > alloc_magic_pages() is renamed to alloc_magic_pages_pv() to mirror its
> > > alloc_magic_pages_hvm() counterpart.  Delete a redundant comment, introduce
> > > some newlines clarity, and remove a logically dead allocation of shared info.
> > > 
> > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > > ---
> > > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > > CC: Wei Liu <wei.liu2@citrix.com>
> > > CC: Julien Grall <julien.grall@arm.com>
> > > ---
> > >  tools/libxc/xc_dom_x86.c | 16 ++++++----------
> > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> > > index cb68efc..885ca1b 100644
> > > --- a/tools/libxc/xc_dom_x86.c
> > > +++ b/tools/libxc/xc_dom_x86.c
> > > @@ -534,24 +534,20 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
> > >  
> > >  /* ------------------------------------------------------------------------ */
> > >  
> > > -static int alloc_magic_pages(struct xc_dom_image *dom)
> > > +static int alloc_magic_pages_pv(struct xc_dom_image *dom)
> > >  {
> > > -    /* allocate special pages */
> > >      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
> > >      if ( dom->start_info_pfn == INVALID_PFN )
> > >          return -1;
> > 
> > Maybe those errors paths should set errno = ENOMEM?
> 
> I believe the actual allocation function already does that.

Doesn't seem like xc_dom_alloc_page or xc_dom_chk_alloc_pages set
errno at all (or I'm not able to find it), but in any case it should
be set there rather than here. libxc is a disaster in this regard I'm
afraid.

Roger.

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

Re: [PATCH for-4.10 1/5] tools/dombuilder: Drop more PVH v1 leftovers

[Wei Liu]

On Fri, Oct 06, 2017 at 10:40:36AM +0100, Roger Pau Monné wrote:
> On Fri, Oct 06, 2017 at 09:33:52AM +0000, Wei Liu wrote:
> > On Fri, Oct 06, 2017 at 10:26:03AM +0100, Roger Pau Monné wrote:
> > > On Thu, Oct 05, 2017 at 06:23:39PM +0000, Andrew Cooper wrote:
> > > > alloc_magic_pages() is renamed to alloc_magic_pages_pv() to mirror its
> > > > alloc_magic_pages_hvm() counterpart.  Delete a redundant comment, introduce
> > > > some newlines clarity, and remove a logically dead allocation of shared info.
> > > > 
> > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > 
> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > > 
> > > > ---
> > > > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > > > CC: Wei Liu <wei.liu2@citrix.com>
> > > > CC: Julien Grall <julien.grall@arm.com>
> > > > ---
> > > >  tools/libxc/xc_dom_x86.c | 16 ++++++----------
> > > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> > > > index cb68efc..885ca1b 100644
> > > > --- a/tools/libxc/xc_dom_x86.c
> > > > +++ b/tools/libxc/xc_dom_x86.c
> > > > @@ -534,24 +534,20 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
> > > >  
> > > >  /* ------------------------------------------------------------------------ */
> > > >  
> > > > -static int alloc_magic_pages(struct xc_dom_image *dom)
> > > > +static int alloc_magic_pages_pv(struct xc_dom_image *dom)
> > > >  {
> > > > -    /* allocate special pages */
> > > >      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
> > > >      if ( dom->start_info_pfn == INVALID_PFN )
> > > >          return -1;
> > > 
> > > Maybe those errors paths should set errno = ENOMEM?
> > 
> > I believe the actual allocation function already does that.
> 
> Doesn't seem like xc_dom_alloc_page or xc_dom_chk_alloc_pages set
> errno at all (or I'm not able to find it), but in any case it should
> be set there rather than here. libxc is a disaster in this regard I'm
> afraid.

I misremembered. ;-)

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

Re: [PATCH for-4.10 1/5] tools/dombuilder: Drop more PVH v1 leftovers

[Wei Liu]

On Thu, Oct 05, 2017 at 07:23:39PM +0100, Andrew Cooper wrote:
> alloc_magic_pages() is renamed to alloc_magic_pages_pv() to mirror its
> alloc_magic_pages_hvm() counterpart.  Delete a redundant comment, introduce
> some newlines clarity, and remove a logically dead allocation of shared info.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH for-4.10 2/5] tools/dombuilder: Remove clear_page() from xc_dom_boot.c

[Andrew Cooper]

pfn 0 is a legitimate (albeit unlikely) frame to use for translated domains,
so skipping it is wrong.  (This behaviour appears to exists simply to cover
the fact that zero is the default value of an uninitialised field in dom.)

ARM already clears the frames at the point that the pfns are allocated,
meaning that the added clear_page() is wasteful.  Alter x86 to match ARM and
clear the page when it is allocated.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/xc_dom_arm.c  |  3 ++-
 tools/libxc/xc_dom_boot.c | 26 --------------------------
 tools/libxc/xc_dom_x86.c  |  8 ++++++++
 3 files changed, 10 insertions(+), 27 deletions(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 98200ae..2aeb287 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -91,7 +91,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
-    xc_clear_domain_page(dom->xch, dom->guest_domid, base + VUART_PFN_OFFSET);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
+
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
             dom->console_pfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 8a376d0..ce3c22e 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -62,25 +62,6 @@ static int setup_hypercall_page(struct xc_dom_image *dom)
     return rc;
 }
 
-static int clear_page(struct xc_dom_image *dom, xen_pfn_t pfn)
-{
-    xen_pfn_t dst;
-    int rc;
-
-    if ( pfn == 0 )
-        return 0;
-
-    dst = xc_dom_p2m(dom, pfn);
-    DOMPRINTF("%s: pfn 0x%" PRIpfn ", mfn 0x%" PRIpfn "",
-              __FUNCTION__, pfn, dst);
-    rc = xc_clear_domain_page(dom->xch, dom->guest_domid, dst);
-    if ( rc != 0 )
-        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
-                     "%s: xc_clear_domain_page failed (pfn 0x%" PRIpfn
-                     ", rc=%d)", __FUNCTION__, pfn, rc);
-    return rc;
-}
-
 
 /* ------------------------------------------------------------------------ */
 
@@ -222,13 +203,6 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
         if ( (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
             return rc;
 
-    if ( (rc = clear_page(dom, dom->console_pfn)) != 0 )
-        return rc;
-    if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
-        return rc;
-    if ( (rc = clear_page(dom, dom->vuart_gfn)) != 0 )
-        return rc;
-
     /* start info page */
     if ( dom->arch_hooks->start_info )
         dom->arch_hooks->start_info(dom);
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 885ca1b..0c80b59 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -543,10 +543,14 @@ static int alloc_magic_pages_pv(struct xc_dom_image *dom)
     dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
     if ( dom->xenstore_pfn == INVALID_PFN )
         return -1;
+    xc_clear_domain_page(dom->xch, dom->guest_domid,
+                         xc_dom_p2m(dom, dom->xenstore_pfn));
 
     dom->console_pfn = xc_dom_alloc_page(dom, "console");
     if ( dom->console_pfn == INVALID_PFN )
         return -1;
+    xc_clear_domain_page(dom->xch, dom->guest_domid,
+                         xc_dom_p2m(dom, dom->console_pfn));
 
     dom->alloc_bootstack = 1;
 
@@ -696,7 +700,11 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
                      special_pfn(SPECIALPAGE_IDENT_PT) << PAGE_SHIFT);
 
     dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
+
     dom->xenstore_pfn = special_pfn(SPECIALPAGE_XENSTORE);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
+
     dom->parms.virt_hypercall = -1;
 
     rc = 0;
-- 
2.1.4


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

Re: [PATCH for-4.10 2/5] tools/dombuilder: Remove clear_page() from xc_dom_boot.c

[Roger Pau Monné]

On Thu, Oct 05, 2017 at 06:23:40PM +0000, Andrew Cooper wrote:
> pfn 0 is a legitimate (albeit unlikely) frame to use for translated domains,
> so skipping it is wrong.  (This behaviour appears to exists simply to cover
> the fact that zero is the default value of an uninitialised field in dom.)
> 
> ARM already clears the frames at the point that the pfns are allocated,
> meaning that the added clear_page() is wasteful.  Alter x86 to match ARM and
> clear the page when it is allocated.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  tools/libxc/xc_dom_arm.c  |  3 ++-
>  tools/libxc/xc_dom_boot.c | 26 --------------------------
>  tools/libxc/xc_dom_x86.c  |  8 ++++++++
>  3 files changed, 10 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 98200ae..2aeb287 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -91,7 +91,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
>      xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
> -    xc_clear_domain_page(dom->xch, dom->guest_domid, base + VUART_PFN_OFFSET);
> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
> +

This seems kind of unrelated to the patch itself, not that it's wrong.
IMHO it should be split into a separate patch.

>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>              dom->console_pfn);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index 8a376d0..ce3c22e 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -62,25 +62,6 @@ static int setup_hypercall_page(struct xc_dom_image *dom)
>      return rc;
>  }
>  
> -static int clear_page(struct xc_dom_image *dom, xen_pfn_t pfn)
> -{
> -    xen_pfn_t dst;
> -    int rc;
> -
> -    if ( pfn == 0 )
> -        return 0;
> -
> -    dst = xc_dom_p2m(dom, pfn);
> -    DOMPRINTF("%s: pfn 0x%" PRIpfn ", mfn 0x%" PRIpfn "",
> -              __FUNCTION__, pfn, dst);
> -    rc = xc_clear_domain_page(dom->xch, dom->guest_domid, dst);
> -    if ( rc != 0 )
> -        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> -                     "%s: xc_clear_domain_page failed (pfn 0x%" PRIpfn
> -                     ", rc=%d)", __FUNCTION__, pfn, rc);
> -    return rc;
> -}
> -
>  
>  /* ------------------------------------------------------------------------ */
>  
> @@ -222,13 +203,6 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
>          if ( (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
>              return rc;
>  
> -    if ( (rc = clear_page(dom, dom->console_pfn)) != 0 )
> -        return rc;
> -    if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
> -        return rc;
> -    if ( (rc = clear_page(dom, dom->vuart_gfn)) != 0 )
> -        return rc;
> -
>      /* start info page */
>      if ( dom->arch_hooks->start_info )
>          dom->arch_hooks->start_info(dom);
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 885ca1b..0c80b59 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -543,10 +543,14 @@ static int alloc_magic_pages_pv(struct xc_dom_image *dom)
>      dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
>      if ( dom->xenstore_pfn == INVALID_PFN )
>          return -1;
> +    xc_clear_domain_page(dom->xch, dom->guest_domid,
> +                         xc_dom_p2m(dom, dom->xenstore_pfn));

The start info page doesn't need to be cleared because it's re-written
anyway with the data I guess.

Thanks, Roger.

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

Re: [PATCH for-4.10 2/5] tools/dombuilder: Remove clear_page() from xc_dom_boot.c

[Andrew Cooper]

On 06/10/17 10:35, Roger Pau Monné wrote:
> On Thu, Oct 05, 2017 at 06:23:40PM +0000, Andrew Cooper wrote:
>> pfn 0 is a legitimate (albeit unlikely) frame to use for translated domains,
>> so skipping it is wrong.  (This behaviour appears to exists simply to cover
>> the fact that zero is the default value of an uninitialised field in dom.)
>>
>> ARM already clears the frames at the point that the pfns are allocated,
>> meaning that the added clear_page() is wasteful.  Alter x86 to match ARM and
>> clear the page when it is allocated.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  tools/libxc/xc_dom_arm.c  |  3 ++-
>>  tools/libxc/xc_dom_boot.c | 26 --------------------------
>>  tools/libxc/xc_dom_x86.c  |  8 ++++++++
>>  3 files changed, 10 insertions(+), 27 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index 98200ae..2aeb287 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -91,7 +91,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
>>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
>>      xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
>> -    xc_clear_domain_page(dom->xch, dom->guest_domid, base + VUART_PFN_OFFSET);
>> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
>> +
> This seems kind of unrelated to the patch itself, not that it's wrong.
> IMHO it should be split into a separate patch.

It is related.  It covers the fact that I remove a clear_page() of
dom->vuart_gfn below, and its context confirms that the console and
xenstore rings are cleared on ARM as well.

>
>>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>>              dom->console_pfn);
>>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
>> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
>> index 8a376d0..ce3c22e 100644
>> --- a/tools/libxc/xc_dom_boot.c
>> +++ b/tools/libxc/xc_dom_boot.c
>> @@ -62,25 +62,6 @@ static int setup_hypercall_page(struct xc_dom_image *dom)
>>      return rc;
>>  }
>>  
>> -static int clear_page(struct xc_dom_image *dom, xen_pfn_t pfn)
>> -{
>> -    xen_pfn_t dst;
>> -    int rc;
>> -
>> -    if ( pfn == 0 )
>> -        return 0;
>> -
>> -    dst = xc_dom_p2m(dom, pfn);
>> -    DOMPRINTF("%s: pfn 0x%" PRIpfn ", mfn 0x%" PRIpfn "",
>> -              __FUNCTION__, pfn, dst);
>> -    rc = xc_clear_domain_page(dom->xch, dom->guest_domid, dst);
>> -    if ( rc != 0 )
>> -        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>> -                     "%s: xc_clear_domain_page failed (pfn 0x%" PRIpfn
>> -                     ", rc=%d)", __FUNCTION__, pfn, rc);
>> -    return rc;
>> -}
>> -
>>  
>>  /* ------------------------------------------------------------------------ */
>>  
>> @@ -222,13 +203,6 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
>>          if ( (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
>>              return rc;
>>  
>> -    if ( (rc = clear_page(dom, dom->console_pfn)) != 0 )
>> -        return rc;
>> -    if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
>> -        return rc;
>> -    if ( (rc = clear_page(dom, dom->vuart_gfn)) != 0 )
>> -        return rc;
>> -
>>      /* start info page */
>>      if ( dom->arch_hooks->start_info )
>>          dom->arch_hooks->start_info(dom);
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 885ca1b..0c80b59 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -543,10 +543,14 @@ static int alloc_magic_pages_pv(struct xc_dom_image *dom)
>>      dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
>>      if ( dom->xenstore_pfn == INVALID_PFN )
>>          return -1;
>> +    xc_clear_domain_page(dom->xch, dom->guest_domid,
>> +                         xc_dom_p2m(dom, dom->xenstore_pfn));
> The start info page doesn't need to be cleared because it's re-written
> anyway with the data I guess.

In general, guests do get zeroed RAM to begin with, but there are
certain cases where this doesn't happen (mainly by a warm reboot and
using no-bootscrub).

The rings are critical to zero, as the ring indices need to start at
zero for them to function, but yes - the start info page does get fully
written with data.

~Andrew

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

Re: [PATCH for-4.10 2/5] tools/dombuilder: Remove clear_page() from xc_dom_boot.c

[Wei Liu]

On Thu, Oct 05, 2017 at 07:23:40PM +0100, Andrew Cooper wrote:
> pfn 0 is a legitimate (albeit unlikely) frame to use for translated domains,
> so skipping it is wrong.  (This behaviour appears to exists simply to cover
> the fact that zero is the default value of an uninitialised field in dom.)
> 
> ARM already clears the frames at the point that the pfns are allocated,
> meaning that the added clear_page() is wasteful.  Alter x86 to match ARM and
> clear the page when it is allocated.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH for-4.10 3/5] tools/dombuilder: Switch to using gfn terminology for console and xenstore rings

[Andrew Cooper]

The sole use of xc_dom_translated() and xc_dom_p2m() outside of the domain
builder is for libxl_dom() to translate the console and xenstore pfns back
into useful values.  PV guest pfns are only interesting to the domain builder,
and gfns are the address space used by all other hypercalls.

Renaming the fields in xc_dom_image is deliberate, as it will cause
out-of-tree users of the dombuilder to notice the different semantics.

Correct the terminology throughout xc_dom_gnttab{_hvm,}_seed(), which are all
using gfns despite the existing variable names.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/include/xc_dom.h      | 10 +++++++--
 tools/libxc/xc_dom_arm.c          | 12 +++++------
 tools/libxc/xc_dom_boot.c         | 45 +++++++++++++++++++--------------------
 tools/libxc/xc_dom_compat_linux.c |  4 ++--
 tools/libxc/xc_dom_x86.c          | 45 ++++++++++++++++++++-------------------
 tools/libxl/libxl_dom.c           | 11 +++-------
 6 files changed, 64 insertions(+), 63 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 6e06ef1..80b4fbd 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -94,8 +94,6 @@ struct xc_dom_image {
     struct xc_dom_seg devicetree_seg;
     struct xc_dom_seg start_info_seg; /* HVMlite only */
     xen_pfn_t start_info_pfn;
-    xen_pfn_t console_pfn;
-    xen_pfn_t xenstore_pfn;
     xen_pfn_t shared_info_pfn;
     xen_pfn_t bootstack_pfn;
     xen_pfn_t pfn_alloc_end;
@@ -103,6 +101,14 @@ struct xc_dom_image {
     xen_vaddr_t bsd_symtab_start;
 
     /*
+     * Details for the toolstack-prepared rings.
+     *
+     * *_gfn fields are allocated by the domain builder.
+     */
+    xen_pfn_t console_gfn;
+    xen_pfn_t xenstore_gfn;
+
+    /*
      * initrd parameters as specified in start_info page
      * Depending on capabilities of the booted kernel this may be a virtual
      * address or a pfn. Type is neutral and large enough to hold a virtual
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 2aeb287..c7aa44a 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -84,19 +84,19 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
     if ( rc < 0 )
         return rc;
 
-    dom->console_pfn = base + CONSOLE_PFN_OFFSET;
-    dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+    dom->console_gfn = base + CONSOLE_PFN_OFFSET;
+    dom->xenstore_gfn = base + XENSTORE_PFN_OFFSET;
     dom->vuart_gfn = base + VUART_PFN_OFFSET;
 
-    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
-    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_gfn);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_gfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
 
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
-            dom->console_pfn);
+            dom->console_gfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
-            dom->xenstore_pfn);
+            dom->xenstore_gfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
             base + MEMACCESS_PFN_OFFSET);
     /* allocated by toolstack */
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index ce3c22e..a84a95e 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -257,24 +257,24 @@ static xen_pfn_t xc_dom_gnttab_setup(xc_interface *xch, domid_t domid)
 }
 
 int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
-                       xen_pfn_t console_gmfn,
-                       xen_pfn_t xenstore_gmfn,
+                       xen_pfn_t console_gfn,
+                       xen_pfn_t xenstore_gfn,
                        domid_t console_domid,
                        domid_t xenstore_domid)
 {
 
-    xen_pfn_t gnttab_gmfn;
+    xen_pfn_t gnttab_gfn;
     grant_entry_v1_t *gnttab;
 
-    gnttab_gmfn = xc_dom_gnttab_setup(xch, domid);
-    if ( gnttab_gmfn == -1 )
+    gnttab_gfn = xc_dom_gnttab_setup(xch, domid);
+    if ( gnttab_gfn == -1 )
         return -1;
 
     gnttab = xc_map_foreign_range(xch,
                                   domid,
                                   PAGE_SIZE,
                                   PROT_READ|PROT_WRITE,
-                                  gnttab_gmfn);
+                                  gnttab_gfn);
     if ( gnttab == NULL )
     {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
@@ -284,17 +284,17 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
         return -1;
     }
 
-    if ( domid != console_domid  && console_gmfn != -1)
+    if ( domid != console_domid  && console_gfn != -1 )
     {
         gnttab[GNTTAB_RESERVED_CONSOLE].flags = GTF_permit_access;
         gnttab[GNTTAB_RESERVED_CONSOLE].domid = console_domid;
-        gnttab[GNTTAB_RESERVED_CONSOLE].frame = console_gmfn;
+        gnttab[GNTTAB_RESERVED_CONSOLE].frame = console_gfn;
     }
-    if ( domid != xenstore_domid && xenstore_gmfn != -1)
+    if ( domid != xenstore_domid && xenstore_gfn != -1 )
     {
         gnttab[GNTTAB_RESERVED_XENSTORE].flags = GTF_permit_access;
         gnttab[GNTTAB_RESERVED_XENSTORE].domid = xenstore_domid;
-        gnttab[GNTTAB_RESERVED_XENSTORE].frame = xenstore_gmfn;
+        gnttab[GNTTAB_RESERVED_XENSTORE].frame = xenstore_gfn;
     }
 
     if ( munmap(gnttab, PAGE_SIZE) == -1 )
@@ -308,19 +308,19 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
 
     /* Guest shouldn't really touch its grant table until it has
      * enabled its caches. But lets be nice. */
-    xc_domain_cacheflush(xch, domid, gnttab_gmfn, 1);
+    xc_domain_cacheflush(xch, domid, gnttab_gfn, 1);
 
     return 0;
 }
 
 int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
-                           xen_pfn_t console_gpfn,
-                           xen_pfn_t xenstore_gpfn,
+                           xen_pfn_t console_gfn,
+                           xen_pfn_t xenstore_gfn,
                            domid_t console_domid,
                            domid_t xenstore_domid)
 {
     int rc;
-    xen_pfn_t scratch_gpfn;
+    xen_pfn_t scratch_gfn;
     struct xen_add_to_physmap xatp = {
         .domid = domid,
         .space = XENMAPSPACE_grant_table,
@@ -330,7 +330,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
         .domid = domid,
     };
 
-    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gpfn);
+    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gfn);
     if ( rc < 0 )
     {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
@@ -339,11 +339,11 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
                      __FUNCTION__, errno);
         return -1;
     }
-    xatp.gpfn = scratch_gpfn;
-    xrfp.gpfn = scratch_gpfn;
+    xatp.gpfn = scratch_gfn;
+    xrfp.gpfn = scratch_gfn;
 
-    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
-                  scratch_gpfn);
+    xc_dom_printf(xch, "%s: called, scratch gfn=0x%"PRI_xen_pfn, __FUNCTION__,
+                  scratch_gfn);
 
 
     rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
@@ -357,7 +357,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
     }
 
     rc = xc_dom_gnttab_seed(xch, domid,
-                            console_gpfn, xenstore_gpfn,
+                            console_gfn, xenstore_gfn,
                             console_domid, xenstore_domid);
     if (rc != 0)
     {
@@ -385,12 +385,11 @@ int xc_dom_gnttab_init(struct xc_dom_image *dom)
 {
     if ( xc_dom_translated(dom) ) {
         return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
-                                      dom->console_pfn, dom->xenstore_pfn,
+                                      dom->console_gfn, dom->xenstore_gfn,
                                       dom->console_domid, dom->xenstore_domid);
     } else {
         return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
-                                  xc_dom_p2m(dom, dom->console_pfn),
-                                  xc_dom_p2m(dom, dom->xenstore_pfn),
+                                  dom->console_gfn, dom->xenstore_gfn,
                                   dom->console_domid, dom->xenstore_domid);
     }
 }
diff --git a/tools/libxc/xc_dom_compat_linux.c b/tools/libxc/xc_dom_compat_linux.c
index c922c61..6d27ec2 100644
--- a/tools/libxc/xc_dom_compat_linux.c
+++ b/tools/libxc/xc_dom_compat_linux.c
@@ -78,8 +78,8 @@ int xc_linux_build(xc_interface *xch, uint32_t domid,
     if ( (rc = xc_dom_gnttab_init(dom)) != 0)
         goto out;
 
-    *console_mfn = xc_dom_p2m(dom, dom->console_pfn);
-    *store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
+    *console_mfn = dom->console_gfn;
+    *store_mfn = dom->xenstore_gfn;
 
  out:
     xc_dom_release(dom);
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 0c80b59..aa0ced1 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -536,21 +536,23 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
 
 static int alloc_magic_pages_pv(struct xc_dom_image *dom)
 {
+    xen_pfn_t pfn;
+
     dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
     if ( dom->start_info_pfn == INVALID_PFN )
         return -1;
 
-    dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
-    if ( dom->xenstore_pfn == INVALID_PFN )
+    pfn = xc_dom_alloc_page(dom, "xenstore");
+    if ( pfn == INVALID_PFN )
         return -1;
-    xc_clear_domain_page(dom->xch, dom->guest_domid,
-                         xc_dom_p2m(dom, dom->xenstore_pfn));
+    dom->xenstore_gfn = xc_dom_p2m(dom, pfn);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_gfn);
 
-    dom->console_pfn = xc_dom_alloc_page(dom, "console");
-    if ( dom->console_pfn == INVALID_PFN )
+    pfn = xc_dom_alloc_page(dom, "console");
+    if ( pfn == INVALID_PFN )
         return -1;
-    xc_clear_domain_page(dom->xch, dom->guest_domid,
-                         xc_dom_p2m(dom, dom->console_pfn));
+    dom->console_gfn = xc_dom_p2m(dom, pfn);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_gfn);
 
     dom->alloc_bootstack = 1;
 
@@ -612,14 +614,19 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
                                X86_HVM_NR_SPECIAL_PAGES) )
             goto error_out;
 
-    xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN,
-                     special_pfn(SPECIALPAGE_XENSTORE));
+    dom->xenstore_gfn = special_pfn(SPECIALPAGE_XENSTORE);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_gfn);
+    xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN, dom->xenstore_gfn);
+
     xc_hvm_param_set(xch, domid, HVM_PARAM_BUFIOREQ_PFN,
                      special_pfn(SPECIALPAGE_BUFIOREQ));
     xc_hvm_param_set(xch, domid, HVM_PARAM_IOREQ_PFN,
                      special_pfn(SPECIALPAGE_IOREQ));
-    xc_hvm_param_set(xch, domid, HVM_PARAM_CONSOLE_PFN,
-                     special_pfn(SPECIALPAGE_CONSOLE));
+
+    dom->console_gfn = special_pfn(SPECIALPAGE_CONSOLE);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_gfn);
+    xc_hvm_param_set(xch, domid, HVM_PARAM_CONSOLE_PFN, dom->console_gfn);
+
     xc_hvm_param_set(xch, domid, HVM_PARAM_PAGING_RING_PFN,
                      special_pfn(SPECIALPAGE_PAGING));
     xc_hvm_param_set(xch, domid, HVM_PARAM_MONITOR_RING_PFN,
@@ -699,12 +706,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     xc_hvm_param_set(xch, domid, HVM_PARAM_IDENT_PT,
                      special_pfn(SPECIALPAGE_IDENT_PT) << PAGE_SHIFT);
 
-    dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE);
-    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
-
-    dom->xenstore_pfn = special_pfn(SPECIALPAGE_XENSTORE);
-    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
-
     dom->parms.virt_hypercall = -1;
 
     rc = 0;
@@ -744,9 +745,9 @@ static int start_info_x86_32(struct xc_dom_image *dom)
     start_info->mfn_list = dom->p2m_seg.vstart;
 
     start_info->flags = dom->flags;
-    start_info->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
+    start_info->store_mfn = dom->xenstore_gfn;
     start_info->store_evtchn = dom->xenstore_evtchn;
-    start_info->console.domU.mfn = xc_dom_p2m(dom, dom->console_pfn);
+    start_info->console.domU.mfn = dom->console_gfn;
     start_info->console.domU.evtchn = dom->console_evtchn;
 
     if ( dom->ramdisk_blob )
@@ -795,9 +796,9 @@ static int start_info_x86_64(struct xc_dom_image *dom)
     }
 
     start_info->flags = dom->flags;
-    start_info->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
+    start_info->store_mfn = dom->xenstore_gfn;
     start_info->store_evtchn = dom->xenstore_evtchn;
-    start_info->console.domU.mfn = xc_dom_p2m(dom, dom->console_pfn);
+    start_info->console.domU.mfn = dom->console_gfn;
     start_info->console.domU.evtchn = dom->console_evtchn;
 
     if ( dom->ramdisk_blob )
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index ef834e6..0389a06 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -851,14 +851,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
     if (ret != 0)
         goto out;
 
-    if (xc_dom_translated(dom)) {
-        state->console_mfn = dom->console_pfn;
-        state->store_mfn = dom->xenstore_pfn;
-        state->vuart_gfn = dom->vuart_gfn;
-    } else {
-        state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
-        state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
-    }
+    state->console_mfn = dom->console_gfn;
+    state->store_mfn = dom->xenstore_gfn;
+    state->vuart_gfn = dom->vuart_gfn;
 
     ret = 0;
 out:
-- 
2.1.4


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

Re: [PATCH for-4.10 3/5] tools/dombuilder: Switch to using gfn terminology for console and xenstore rings

[Roger Pau Monné]

On Thu, Oct 05, 2017 at 06:23:41PM +0000, Andrew Cooper wrote:
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index ce3c22e..a84a95e 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -257,24 +257,24 @@ static xen_pfn_t xc_dom_gnttab_setup(xc_interface *xch, domid_t domid)
>  }
>  
>  int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
> -                       xen_pfn_t console_gmfn,
> -                       xen_pfn_t xenstore_gmfn,
> +                       xen_pfn_t console_gfn,
> +                       xen_pfn_t xenstore_gfn,
>                         domid_t console_domid,
>                         domid_t xenstore_domid)
>  {
>  
> -    xen_pfn_t gnttab_gmfn;
> +    xen_pfn_t gnttab_gfn;
>      grant_entry_v1_t *gnttab;
>  
> -    gnttab_gmfn = xc_dom_gnttab_setup(xch, domid);
> -    if ( gnttab_gmfn == -1 )
> +    gnttab_gfn = xc_dom_gnttab_setup(xch, domid);
> +    if ( gnttab_gfn == -1 )
>          return -1;
>  
>      gnttab = xc_map_foreign_range(xch,
>                                    domid,
>                                    PAGE_SIZE,
>                                    PROT_READ|PROT_WRITE,
> -                                  gnttab_gmfn);
> +                                  gnttab_gfn);
>      if ( gnttab == NULL )
>      {
>          xc_dom_panic(xch, XC_INTERNAL_ERROR,
> @@ -284,17 +284,17 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
>          return -1;
>      }
>  
> -    if ( domid != console_domid  && console_gmfn != -1)
> +    if ( domid != console_domid  && console_gfn != -1 )
                                  ^ extra space
>      {
>          gnttab[GNTTAB_RESERVED_CONSOLE].flags = GTF_permit_access;
>          gnttab[GNTTAB_RESERVED_CONSOLE].domid = console_domid;
> -        gnttab[GNTTAB_RESERVED_CONSOLE].frame = console_gmfn;
> +        gnttab[GNTTAB_RESERVED_CONSOLE].frame = console_gfn;
>      }
> -    if ( domid != xenstore_domid && xenstore_gmfn != -1)
> +    if ( domid != xenstore_domid && xenstore_gfn != -1 )
>      {
>          gnttab[GNTTAB_RESERVED_XENSTORE].flags = GTF_permit_access;
>          gnttab[GNTTAB_RESERVED_XENSTORE].domid = xenstore_domid;
> -        gnttab[GNTTAB_RESERVED_XENSTORE].frame = xenstore_gmfn;
> +        gnttab[GNTTAB_RESERVED_XENSTORE].frame = xenstore_gfn;
>      }
>  
>      if ( munmap(gnttab, PAGE_SIZE) == -1 )
> @@ -308,19 +308,19 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
>  
>      /* Guest shouldn't really touch its grant table until it has
>       * enabled its caches. But lets be nice. */
> -    xc_domain_cacheflush(xch, domid, gnttab_gmfn, 1);
> +    xc_domain_cacheflush(xch, domid, gnttab_gfn, 1);
>  
>      return 0;
>  }
>  
>  int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
> -                           xen_pfn_t console_gpfn,
> -                           xen_pfn_t xenstore_gpfn,
> +                           xen_pfn_t console_gfn,
> +                           xen_pfn_t xenstore_gfn,
>                             domid_t console_domid,
>                             domid_t xenstore_domid)
>  {
>      int rc;
> -    xen_pfn_t scratch_gpfn;
> +    xen_pfn_t scratch_gfn;
>      struct xen_add_to_physmap xatp = {
>          .domid = domid,
>          .space = XENMAPSPACE_grant_table,
> @@ -330,7 +330,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>          .domid = domid,
>      };
>  
> -    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gpfn);
> +    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gfn);
>      if ( rc < 0 )
>      {
>          xc_dom_panic(xch, XC_INTERNAL_ERROR,
> @@ -339,11 +339,11 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>                       __FUNCTION__, errno);
>          return -1;
>      }
> -    xatp.gpfn = scratch_gpfn;
> -    xrfp.gpfn = scratch_gpfn;
> +    xatp.gpfn = scratch_gfn;
> +    xrfp.gpfn = scratch_gfn;

xatp.gpfn = xrfp.gpfn = scratch_gfn;

Maybe, not important IMHO.

>  
> -    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
> -                  scratch_gpfn);
> +    xc_dom_printf(xch, "%s: called, scratch gfn=0x%"PRI_xen_pfn, __FUNCTION__,
> +                  scratch_gfn);
>  
>  
>      rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
> @@ -357,7 +357,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>      }
>  
>      rc = xc_dom_gnttab_seed(xch, domid,
> -                            console_gpfn, xenstore_gpfn,
> +                            console_gfn, xenstore_gfn,
>                              console_domid, xenstore_domid);
>      if (rc != 0)
>      {
> @@ -385,12 +385,11 @@ int xc_dom_gnttab_init(struct xc_dom_image *dom)
>  {
>      if ( xc_dom_translated(dom) ) {
>          return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
> -                                      dom->console_pfn, dom->xenstore_pfn,
> +                                      dom->console_gfn, dom->xenstore_gfn,
>                                        dom->console_domid, dom->xenstore_domid);
>      } else {
>          return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
> -                                  xc_dom_p2m(dom, dom->console_pfn),
> -                                  xc_dom_p2m(dom, dom->xenstore_pfn),
> +                                  dom->console_gfn, dom->xenstore_gfn,
>                                    dom->console_domid, dom->xenstore_domid);

return xc_dom_translated(dom) ? xc_dom_gnttab_hvm_seed : xc_dom_gnttab_seed
                                (dom->xch, dom->guest_domid, dom->console_gfn,
                                 dom->xenstore_gfn, dom->console_domid,
                                 dom->xenstore_domid);

Not sure about the best indentation here. Or that could even be hidden
inside of xc_dom_gnttab_seed, so that xc_dom_gnttab_hvm_seed can be
removed.

>      }
>  }
> diff --git a/tools/libxc/xc_dom_compat_linux.c b/tools/libxc/xc_dom_compat_linux.c
> index c922c61..6d27ec2 100644
> --- a/tools/libxc/xc_dom_compat_linux.c
> +++ b/tools/libxc/xc_dom_compat_linux.c
> @@ -78,8 +78,8 @@ int xc_linux_build(xc_interface *xch, uint32_t domid,
>      if ( (rc = xc_dom_gnttab_init(dom)) != 0)
>          goto out;
>  
> -    *console_mfn = xc_dom_p2m(dom, dom->console_pfn);
> -    *store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
> +    *console_mfn = dom->console_gfn;
> +    *store_mfn = dom->xenstore_gfn;
>  
>   out:
>      xc_dom_release(dom);
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 0c80b59..aa0ced1 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -536,21 +536,23 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
>  
>  static int alloc_magic_pages_pv(struct xc_dom_image *dom)
>  {
> +    xen_pfn_t pfn;
> +
>      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
>      if ( dom->start_info_pfn == INVALID_PFN )
>          return -1;
>  
> -    dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
> -    if ( dom->xenstore_pfn == INVALID_PFN )
> +    pfn = xc_dom_alloc_page(dom, "xenstore");
> +    if ( pfn == INVALID_PFN )
>          return -1;
> -    xc_clear_domain_page(dom->xch, dom->guest_domid,
> -                         xc_dom_p2m(dom, dom->xenstore_pfn));
> +    dom->xenstore_gfn = xc_dom_p2m(dom, pfn);
> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_gfn);
>  
> -    dom->console_pfn = xc_dom_alloc_page(dom, "console");
> -    if ( dom->console_pfn == INVALID_PFN )
> +    pfn = xc_dom_alloc_page(dom, "console");
> +    if ( pfn == INVALID_PFN )
>          return -1;
> -    xc_clear_domain_page(dom->xch, dom->guest_domid,
> -                         xc_dom_p2m(dom, dom->console_pfn));
> +    dom->console_gfn = xc_dom_p2m(dom, pfn);
> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_gfn);
>  
>      dom->alloc_bootstack = 1;
>  
> @@ -612,14 +614,19 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>                                 X86_HVM_NR_SPECIAL_PAGES) )
>              goto error_out;
>  
> -    xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN,
> -                     special_pfn(SPECIALPAGE_XENSTORE));
> +    dom->xenstore_gfn = special_pfn(SPECIALPAGE_XENSTORE);

A pre-patch to s/special_pfn/special_gfn/ would be nice :) for
coherency.

> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index ef834e6..0389a06 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -851,14 +851,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>      if (ret != 0)
>          goto out;
>  
> -    if (xc_dom_translated(dom)) {
> -        state->console_mfn = dom->console_pfn;
> -        state->store_mfn = dom->xenstore_pfn;
> -        state->vuart_gfn = dom->vuart_gfn;

This chunk should go with patch 1, it's a PVHv1 leftover also.

Thanks, Roger.

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

Re: [PATCH for-4.10 3/5] tools/dombuilder: Switch to using gfn terminology for console and xenstore rings

[Andrew Cooper]

On 06/10/17 10:57, Roger Pau Monné wrote:
> On Thu, Oct 05, 2017 at 06:23:41PM +0000, Andrew Cooper wrote:
>>  
>> -    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
>> -                  scratch_gpfn);
>> +    xc_dom_printf(xch, "%s: called, scratch gfn=0x%"PRI_xen_pfn, __FUNCTION__,
>> +                  scratch_gfn);
>>  
>>  
>>      rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
>> @@ -357,7 +357,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>>      }
>>  
>>      rc = xc_dom_gnttab_seed(xch, domid,
>> -                            console_gpfn, xenstore_gpfn,
>> +                            console_gfn, xenstore_gfn,
>>                              console_domid, xenstore_domid);
>>      if (rc != 0)
>>      {
>> @@ -385,12 +385,11 @@ int xc_dom_gnttab_init(struct xc_dom_image *dom)
>>  {
>>      if ( xc_dom_translated(dom) ) {
>>          return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
>> -                                      dom->console_pfn, dom->xenstore_pfn,
>> +                                      dom->console_gfn, dom->xenstore_gfn,
>>                                        dom->console_domid, dom->xenstore_domid);
>>      } else {
>>          return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
>> -                                  xc_dom_p2m(dom, dom->console_pfn),
>> -                                  xc_dom_p2m(dom, dom->xenstore_pfn),
>> +                                  dom->console_gfn, dom->xenstore_gfn,
>>                                    dom->console_domid, dom->xenstore_domid);
> return xc_dom_translated(dom) ? xc_dom_gnttab_hvm_seed : xc_dom_gnttab_seed
>                                 (dom->xch, dom->guest_domid, dom->console_gfn,
>                                  dom->xenstore_gfn, dom->console_domid,
>                                  dom->xenstore_domid);
>
> Not sure about the best indentation here. Or that could even be hidden
> inside of xc_dom_gnttab_seed, so that xc_dom_gnttab_hvm_seed can be
> removed.

These seed functions are also needed in isolation from the migration
code, so I can't make them local.  I considered what you suggest here,
but I'm not sure that it helps the readability.

>
>>      }
>>  }
>> diff --git a/tools/libxc/xc_dom_compat_linux.c b/tools/libxc/xc_dom_compat_linux.c
>> index c922c61..6d27ec2 100644
>> --- a/tools/libxc/xc_dom_compat_linux.c
>> +++ b/tools/libxc/xc_dom_compat_linux.c
>> @@ -78,8 +78,8 @@ int xc_linux_build(xc_interface *xch, uint32_t domid,
>>      if ( (rc = xc_dom_gnttab_init(dom)) != 0)
>>          goto out;
>>  
>> -    *console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>> -    *store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
>> +    *console_mfn = dom->console_gfn;
>> +    *store_mfn = dom->xenstore_gfn;
>>  
>>   out:
>>      xc_dom_release(dom);
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 0c80b59..aa0ced1 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -536,21 +536,23 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
>>  
>>  static int alloc_magic_pages_pv(struct xc_dom_image *dom)
>>  {
>> +    xen_pfn_t pfn;
>> +
>>      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
>>      if ( dom->start_info_pfn == INVALID_PFN )
>>          return -1;
>>  
>> -    dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
>> -    if ( dom->xenstore_pfn == INVALID_PFN )
>> +    pfn = xc_dom_alloc_page(dom, "xenstore");
>> +    if ( pfn == INVALID_PFN )
>>          return -1;
>> -    xc_clear_domain_page(dom->xch, dom->guest_domid,
>> -                         xc_dom_p2m(dom, dom->xenstore_pfn));
>> +    dom->xenstore_gfn = xc_dom_p2m(dom, pfn);
>> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_gfn);
>>  
>> -    dom->console_pfn = xc_dom_alloc_page(dom, "console");
>> -    if ( dom->console_pfn == INVALID_PFN )
>> +    pfn = xc_dom_alloc_page(dom, "console");
>> +    if ( pfn == INVALID_PFN )
>>          return -1;
>> -    xc_clear_domain_page(dom->xch, dom->guest_domid,
>> -                         xc_dom_p2m(dom, dom->console_pfn));
>> +    dom->console_gfn = xc_dom_p2m(dom, pfn);
>> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_gfn);
>>  
>>      dom->alloc_bootstack = 1;
>>  
>> @@ -612,14 +614,19 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>                                 X86_HVM_NR_SPECIAL_PAGES) )
>>              goto error_out;
>>  
>> -    xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN,
>> -                     special_pfn(SPECIALPAGE_XENSTORE));
>> +    dom->xenstore_gfn = special_pfn(SPECIALPAGE_XENSTORE);
> A pre-patch to s/special_pfn/special_gfn/ would be nice :) for
> coherency.

Sorting out all terminology is a far larger problem than I have time for
atm.  For HVM guests, pfn == gfn, so I chose not to dive down that
rabbit hole right now.

>
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index ef834e6..0389a06 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -851,14 +851,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>      if (ret != 0)
>>          goto out;
>>  
>> -    if (xc_dom_translated(dom)) {
>> -        state->console_mfn = dom->console_pfn;
>> -        state->store_mfn = dom->xenstore_pfn;
>> -        state->vuart_gfn = dom->vuart_gfn;
> This chunk should go with patch 1, it's a PVHv1 leftover also.

Sadly, no its not :(

ARM uses libxl__build_pv(), not libxl__build_hvm()

~Andrew

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

Re: [PATCH for-4.10 3/5] tools/dombuilder: Switch to using gfn terminology for console and xenstore rings

[Roger Pau Monné]

On Fri, Oct 06, 2017 at 10:03:39AM +0000, Andrew Cooper wrote:
> On 06/10/17 10:57, Roger Pau Monné wrote:
> > On Thu, Oct 05, 2017 at 06:23:41PM +0000, Andrew Cooper wrote:
> >> @@ -612,14 +614,19 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> >>                                 X86_HVM_NR_SPECIAL_PAGES) )
> >>              goto error_out;
> >>  
> >> -    xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN,
> >> -                     special_pfn(SPECIALPAGE_XENSTORE));
> >> +    dom->xenstore_gfn = special_pfn(SPECIALPAGE_XENSTORE);
> > A pre-patch to s/special_pfn/special_gfn/ would be nice :) for
> > coherency.
> 
> Sorting out all terminology is a far larger problem than I have time for
> atm.  For HVM guests, pfn == gfn, so I chose not to dive down that
> rabbit hole right now.

Heh, right.

> >
> >> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> >> index ef834e6..0389a06 100644
> >> --- a/tools/libxl/libxl_dom.c
> >> +++ b/tools/libxl/libxl_dom.c
> >> @@ -851,14 +851,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> >>      if (ret != 0)
> >>          goto out;
> >>  
> >> -    if (xc_dom_translated(dom)) {
> >> -        state->console_mfn = dom->console_pfn;
> >> -        state->store_mfn = dom->xenstore_pfn;
> >> -        state->vuart_gfn = dom->vuart_gfn;
> > This chunk should go with patch 1, it's a PVHv1 leftover also.
> 
> Sadly, no its not :(
> 
> ARM uses libxl__build_pv(), not libxl__build_hvm()

Really? That's seems very wrong^W confusing IMHO. I'm quite sure that
with the changes I've made to libxl in PVHv2 ARM could use the HVM
guest type.

With the other nits fixed (not the "xc_dom_gnttab_hvm_seed :
xc_dom_gnttab_seed" if you don't think it's helpful):

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [PATCH for-4.10 3/5] tools/dombuilder: Switch to using gfn terminology for console and xenstore rings

[Julien Grall]

Hi Roger,

On 06/10/17 11:36, Roger Pau Monné wrote:
> On Fri, Oct 06, 2017 at 10:03:39AM +0000, Andrew Cooper wrote:
>> On 06/10/17 10:57, Roger Pau Monné wrote:
>>> On Thu, Oct 05, 2017 at 06:23:41PM +0000, Andrew Cooper wrote:
>>>> @@ -612,14 +614,19 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>>>                                  X86_HVM_NR_SPECIAL_PAGES) )
>>>>               goto error_out;
>>>>   
>>>> -    xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN,
>>>> -                     special_pfn(SPECIALPAGE_XENSTORE));
>>>> +    dom->xenstore_gfn = special_pfn(SPECIALPAGE_XENSTORE);
>>> A pre-patch to s/special_pfn/special_gfn/ would be nice :) for
>>> coherency.
>>
>> Sorting out all terminology is a far larger problem than I have time for
>> atm.  For HVM guests, pfn == gfn, so I chose not to dive down that
>> rabbit hole right now.
> 
> Heh, right.
> 
>>>
>>>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>>>> index ef834e6..0389a06 100644
>>>> --- a/tools/libxl/libxl_dom.c
>>>> +++ b/tools/libxl/libxl_dom.c
>>>> @@ -851,14 +851,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>>>       if (ret != 0)
>>>>           goto out;
>>>>   
>>>> -    if (xc_dom_translated(dom)) {
>>>> -        state->console_mfn = dom->console_pfn;
>>>> -        state->store_mfn = dom->xenstore_pfn;
>>>> -        state->vuart_gfn = dom->vuart_gfn;
>>> This chunk should go with patch 1, it's a PVHv1 leftover also.
>>
>> Sadly, no its not :(
>>
>> ARM uses libxl__build_pv(), not libxl__build_hvm()
> 
> Really? That's seems very wrong^W confusing IMHO. I'm quite sure that
> with the changes I've made to libxl in PVHv2 ARM could use the HVM
> guest type.

Yes we use PV type for ARM. I think the reason was because HVM would 
create QEMU by default. So PV was more closer here. It might be worth 
having a look how we can re-use the PVH path if it helps maintaining libxl.

Although, this may imply some changes to the user? (I am thinking of 
selecting pvh).

I have created a task on Jira (XEN-102) to keep track of this idea.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-4.10 3/5] tools/dombuilder: Switch to using gfn terminology for console and xenstore rings

[Roger Pau Monné]

On Fri, Oct 06, 2017 at 02:48:48PM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On 06/10/17 11:36, Roger Pau Monné wrote:
> > On Fri, Oct 06, 2017 at 10:03:39AM +0000, Andrew Cooper wrote:
> > > On 06/10/17 10:57, Roger Pau Monné wrote:
> > > > On Thu, Oct 05, 2017 at 06:23:41PM +0000, Andrew Cooper wrote:
> > > ARM uses libxl__build_pv(), not libxl__build_hvm()
> > 
> > Really? That's seems very wrong^W confusing IMHO. I'm quite sure that
> > with the changes I've made to libxl in PVHv2 ARM could use the HVM
> > guest type.
> 
> Yes we use PV type for ARM. I think the reason was because HVM would create
> QEMU by default. So PV was more closer here. It might be worth having a look
> how we can re-use the PVH path if it helps maintaining libxl.
> 
> Although, this may imply some changes to the user? (I am thinking of
> selecting pvh).

I'm quite sure you could make the default (and only) guest type 'pvh'
or 'hvm' on ARM, so that would be transparent to the user (as it is
now).

> I have created a task on Jira (XEN-102) to keep track of this idea.

Thanks!

Let me know if you need help.

Roger.

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

Re: [PATCH for-4.10 3/5] tools/dombuilder: Switch to using gfn terminology for console and xenstore rings

[Wei Liu]

On Thu, Oct 05, 2017 at 07:23:41PM +0100, Andrew Cooper wrote:
> The sole use of xc_dom_translated() and xc_dom_p2m() outside of the domain
> builder is for libxl_dom() to translate the console and xenstore pfns back
> into useful values.  PV guest pfns are only interesting to the domain builder,
> and gfns are the address space used by all other hypercalls.
> 
> Renaming the fields in xc_dom_image is deliberate, as it will cause
> out-of-tree users of the dombuilder to notice the different semantics.
> 
> Correct the terminology throughout xc_dom_gnttab{_hvm,}_seed(), which are all
> using gfns despite the existing variable names.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH for-4.10 4/5] tools/dombuilder: Fix asymetry when setting up console and xenstore rings

[Andrew Cooper]

libxl always uses xc_dom_gnttab_init(), which internally calls
xc_dom_gnttab{_hvm,}_seed() to set up the grants point at the console and
xenstore rings.  For HVM guests, libxl then asks Xen for the information set
up previously, and calls xc_dom_gnttab_hvm_seed() a second time, which is
wasteful.  ARM construction expects libxl to have set up
dom->{console,xenstore}_evtchn earlier, so only actually functions because of
this second call.

Rationalise everything and make it consistent for all guests.

 1) Users of the domain builder are expected to provide
    dom->{console,xenstore}_{evtchn,domid} unconditionally.  This is checked
    by setting invalid values in xc_dom_allocate(), and checking in
    xc_dom_boot_image().

 2) For x86 HVM and ARM guests, the event channels are given to Xen at the
    same time as the ring gfns.  ARM already did this, but x86 is updated to
    match.  x86 PV already provides this information in the start_info page.

 3) Libxl is updated to drop all relevent functionality from
    hvm_build_set_params(), and behave consistently with PV guests when it
    comes to the handling of dom->{console,xenstore}_{evtchn,domid,gfn}.

This removes several redundant hypercalls (including a foreign mapping) from
the x86 HVM and ARM construction paths.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/include/xc_dom.h      | 12 ++++++++----
 tools/libxc/xc_dom_arm.c          |  2 +-
 tools/libxc/xc_dom_boot.c         | 36 ++++++++++++++++++++++++++++++++++++
 tools/libxc/xc_dom_compat_linux.c |  2 ++
 tools/libxc/xc_dom_core.c         |  5 +++++
 tools/libxc/xc_dom_x86.c          |  4 ++++
 tools/libxl/libxl_dom.c           | 28 ++++++++++------------------
 tools/libxl/libxl_internal.h      |  1 -
 8 files changed, 66 insertions(+), 24 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 80b4fbd..790869b 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -20,6 +20,8 @@
 #include <xenguest.h>
 
 #define INVALID_PFN ((xen_pfn_t)-1)
+#define INVALID_EVTCHN (~0u)
+#define INVALID_DOMID  (-1)
 #define X86_HVM_NR_SPECIAL_PAGES    8
 #define X86_HVM_END_SPECIAL_REGION  0xff000u
 
@@ -104,10 +106,16 @@ struct xc_dom_image {
      * Details for the toolstack-prepared rings.
      *
      * *_gfn fields are allocated by the domain builder.
+     * *_{evtchn,domid} fields must be provided by the caller.
      */
     xen_pfn_t console_gfn;
     xen_pfn_t xenstore_gfn;
 
+    unsigned int console_evtchn;
+    unsigned int xenstore_evtchn;
+    domid_t console_domid;
+    domid_t xenstore_domid;
+
     /*
      * initrd parameters as specified in start_info page
      * Depending on capabilities of the booted kernel this may be a virtual
@@ -165,10 +173,6 @@ struct xc_dom_image {
 
     /* misc xen domain config stuff */
     unsigned long flags;
-    unsigned int console_evtchn;
-    unsigned int xenstore_evtchn;
-    domid_t console_domid;
-    domid_t xenstore_domid;
     xen_pfn_t shared_info_mfn;
 
     xc_interface *xch;
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index c7aa44a..d668df1 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -99,7 +99,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
             dom->xenstore_gfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
             base + MEMACCESS_PFN_OFFSET);
-    /* allocated by toolstack */
+
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
             dom->console_evtchn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index a84a95e..8d4fefa 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -163,6 +163,39 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, xen_pfn_t pfn,
     return ptr;
 }
 
+static int xc_dom_check_required_fields(struct xc_dom_image *dom)
+{
+    int rc = 0;
+
+    if ( dom->console_evtchn == INVALID_EVTCHN )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set dom->console_evtchn", __func__);
+        rc = -1;
+    }
+    if ( dom->console_domid == INVALID_DOMID )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set dom->console_domid", __func__);
+        rc = -1;
+    }
+
+    if ( dom->xenstore_evtchn == INVALID_EVTCHN )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set dom->xenstore_evtchn", __func__);
+        rc = -1;
+    }
+    if ( dom->xenstore_domid == INVALID_DOMID )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set dom->xenstore_domid", __func__);
+        rc = -1;
+    }
+
+    return rc;
+}
+
 int xc_dom_boot_image(struct xc_dom_image *dom)
 {
     xc_dominfo_t info;
@@ -170,6 +203,9 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
 
     DOMPRINTF_CALLED(dom->xch);
 
+    if ( (rc = xc_dom_check_required_fields(dom)) != 0 )
+        return rc;
+
     /* misc stuff*/
     if ( (rc = dom->arch_hooks->bootearly(dom)) != 0 )
         return rc;
diff --git a/tools/libxc/xc_dom_compat_linux.c b/tools/libxc/xc_dom_compat_linux.c
index 6d27ec2..2ad43e4 100644
--- a/tools/libxc/xc_dom_compat_linux.c
+++ b/tools/libxc/xc_dom_compat_linux.c
@@ -61,7 +61,9 @@ int xc_linux_build(xc_interface *xch, uint32_t domid,
 
     dom->flags |= flags;
     dom->console_evtchn = console_evtchn;
+    dom->console_domid = 0;
     dom->xenstore_evtchn = store_evtchn;
+    dom->xenstore_domid = 0;
 
     if ( (rc = xc_dom_boot_xen_init(dom, xch, domid)) != 0 )
         goto out;
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index b5f316a..7087c50 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -779,6 +779,11 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
     dom->parms.elf_paddr_offset = UNSET_ADDR;
     dom->parms.p2m_base = UNSET_ADDR;
 
+    dom->console_evtchn = INVALID_EVTCHN;
+    dom->xenstore_evtchn = INVALID_EVTCHN;
+    dom->console_domid = INVALID_DOMID;
+    dom->xenstore_domid = INVALID_DOMID;
+
     dom->flags = SIF_VIRT_P2M_4TOOLS;
 
     dom->alloc_malloc += sizeof(*dom);
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index aa0ced1..05f4b80 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -617,6 +617,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     dom->xenstore_gfn = special_pfn(SPECIALPAGE_XENSTORE);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_gfn);
     xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN, dom->xenstore_gfn);
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
+                     dom->xenstore_evtchn);
 
     xc_hvm_param_set(xch, domid, HVM_PARAM_BUFIOREQ_PFN,
                      special_pfn(SPECIALPAGE_BUFIOREQ));
@@ -626,6 +628,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     dom->console_gfn = special_pfn(SPECIALPAGE_CONSOLE);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_gfn);
     xc_hvm_param_set(xch, domid, HVM_PARAM_CONSOLE_PFN, dom->console_gfn);
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
+                     dom->console_evtchn);
 
     xc_hvm_param_set(xch, domid, HVM_PARAM_PAGING_RING_PFN,
                      special_pfn(SPECIALPAGE_PAGING));
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 0389a06..fcdeef0 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -862,14 +862,10 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
 }
 
 static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
-                                libxl_domain_build_info *info,
-                                int store_evtchn, unsigned long *store_mfn,
-                                int console_evtchn, unsigned long *console_mfn,
-                                domid_t store_domid, domid_t console_domid)
+                                libxl_domain_build_info *info)
 {
     struct hvm_info_table *va_hvm;
     uint8_t *va_map, sum;
-    uint64_t str_mfn, cons_mfn;
     int i;
 
     if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
@@ -890,15 +886,6 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
         munmap(va_map, XC_PAGE_SIZE);
     }
 
-    xc_hvm_param_get(handle, domid, HVM_PARAM_STORE_PFN, &str_mfn);
-    xc_hvm_param_get(handle, domid, HVM_PARAM_CONSOLE_PFN, &cons_mfn);
-    xc_hvm_param_set(handle, domid, HVM_PARAM_STORE_EVTCHN, store_evtchn);
-    xc_hvm_param_set(handle, domid, HVM_PARAM_CONSOLE_EVTCHN, console_evtchn);
-
-    *store_mfn = str_mfn;
-    *console_mfn = cons_mfn;
-
-    xc_dom_gnttab_hvm_seed(handle, domid, *console_mfn, *store_mfn, console_domid, store_domid);
     return 0;
 }
 
@@ -1159,6 +1146,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
 
     dom->container_type = XC_DOM_HVM_CONTAINER;
 
+    dom->console_evtchn = state->console_port;
+    dom->console_domid = state->console_domid;
+    dom->xenstore_evtchn = state->store_port;
+    dom->xenstore_domid = state->store_domid;
+
     /* The params from the configuration file are in Mb, which are then
      * multiplied by 1 Kb. This was then divided off when calling
      * the old xc_hvm_build_target_mem() which then turned them to bytes.
@@ -1263,10 +1255,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
         goto out;
     }
 
-    rc = hvm_build_set_params(ctx->xch, domid, info, state->store_port,
-                               &state->store_mfn, state->console_port,
-                               &state->console_mfn, state->store_domid,
-                               state->console_domid);
+    rc = hvm_build_set_params(ctx->xch, domid, info);
     if (rc != 0) {
         LOG(ERROR, "hvm build set params failed");
         goto out;
@@ -1278,6 +1267,9 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
         goto out;
     }
 
+    state->console_mfn = dom->console_gfn;
+    state->store_mfn = dom->xenstore_gfn;
+
     xc_dom_release(dom);
     return 0;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index bcb6b0a..8a8ef4a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -115,7 +115,6 @@
 #define STUBDOM_SPECIAL_CONSOLES 3
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DOMID_XS_PATH "domid"
-#define INVALID_DOMID ~0
 
 /* Size macros. */
 #define __AC(X,Y)   (X##Y)
-- 
2.1.4


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

Re: [PATCH for-4.10 4/5] tools/dombuilder: Fix asymetry when setting up console and xenstore rings

[Wei Liu]

On Thu, Oct 05, 2017 at 07:23:42PM +0100, Andrew Cooper wrote:
> libxl always uses xc_dom_gnttab_init(), which internally calls
> xc_dom_gnttab{_hvm,}_seed() to set up the grants point at the console and
> xenstore rings.  For HVM guests, libxl then asks Xen for the information set
> up previously, and calls xc_dom_gnttab_hvm_seed() a second time, which is
> wasteful.  ARM construction expects libxl to have set up
> dom->{console,xenstore}_evtchn earlier, so only actually functions because of
> this second call.
> 
> Rationalise everything and make it consistent for all guests.
> 
>  1) Users of the domain builder are expected to provide
>     dom->{console,xenstore}_{evtchn,domid} unconditionally.  This is checked
>     by setting invalid values in xc_dom_allocate(), and checking in
>     xc_dom_boot_image().
> 
>  2) For x86 HVM and ARM guests, the event channels are given to Xen at the
>     same time as the ring gfns.  ARM already did this, but x86 is updated to
>     match.  x86 PV already provides this information in the start_info page.
> 
>  3) Libxl is updated to drop all relevent functionality from
>     hvm_build_set_params(), and behave consistently with PV guests when it
>     comes to the handling of dom->{console,xenstore}_{evtchn,domid,gfn}.
> 
> This removes several redundant hypercalls (including a foreign mapping) from
> the x86 HVM and ARM construction paths.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

With Roger's comments addressed:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH for-4.10 4/5] tools/dombuilder: Fix asymmetry when setting up console and xenstore rings

[Andrew Cooper]

libxl always uses xc_dom_gnttab_init(), which internally calls
xc_dom_gnttab{_hvm,}_seed() to set up the grants point at the console and
xenstore rings.  For HVM guests, libxl then asks Xen for the information set
up previously, and calls xc_dom_gnttab_hvm_seed() a second time, which is
wasteful.  ARM construction expects libxl to have set up
dom->{console,xenstore}_evtchn earlier, so only actually functions because of
this second call.

Rationalise everything and make it consistent for all guests.

 1) Users of the domain builder are expected to provide
    dom->{console,xenstore}_{evtchn,domid} unconditionally.  This is checked
    by setting invalid values in xc_dom_allocate(), and checking in
    xc_dom_boot_image().

 2) For x86 HVM and ARM guests, the event channels are given to Xen at the
    same time as the ring gfns.  ARM already did this, but x86 is updated to
    match.  x86 PV already provides this information in the start_info page.

 3) Libxl is updated to drop all relevant functionality from
    hvm_build_set_params(), and behave consistently with PV guests when it
    comes to the handling of dom->{console,xenstore}_{evtchn,domid,gfn}.

This removes several redundant hypercalls (including a foreign mapping) from
the x86 HVM and ARM construction paths.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/include/xc_dom.h      | 12 ++++++++----
 tools/libxc/xc_dom_arm.c          |  2 +-
 tools/libxc/xc_dom_boot.c         | 36 ++++++++++++++++++++++++++++++++++++
 tools/libxc/xc_dom_compat_linux.c |  2 ++
 tools/libxc/xc_dom_core.c         |  5 +++++
 tools/libxc/xc_dom_x86.c          |  4 ++++
 tools/libxl/libxl_dom.c           | 28 ++++++++++------------------
 tools/libxl/libxl_internal.h      |  1 -
 8 files changed, 66 insertions(+), 24 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 80b4fbd..790869b 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -20,6 +20,8 @@
 #include <xenguest.h>
 
 #define INVALID_PFN ((xen_pfn_t)-1)
+#define INVALID_EVTCHN (~0u)
+#define INVALID_DOMID  (-1)
 #define X86_HVM_NR_SPECIAL_PAGES    8
 #define X86_HVM_END_SPECIAL_REGION  0xff000u
 
@@ -104,10 +106,16 @@ struct xc_dom_image {
      * Details for the toolstack-prepared rings.
      *
      * *_gfn fields are allocated by the domain builder.
+     * *_{evtchn,domid} fields must be provided by the caller.
      */
     xen_pfn_t console_gfn;
     xen_pfn_t xenstore_gfn;
 
+    unsigned int console_evtchn;
+    unsigned int xenstore_evtchn;
+    domid_t console_domid;
+    domid_t xenstore_domid;
+
     /*
      * initrd parameters as specified in start_info page
      * Depending on capabilities of the booted kernel this may be a virtual
@@ -165,10 +173,6 @@ struct xc_dom_image {
 
     /* misc xen domain config stuff */
     unsigned long flags;
-    unsigned int console_evtchn;
-    unsigned int xenstore_evtchn;
-    domid_t console_domid;
-    domid_t xenstore_domid;
     xen_pfn_t shared_info_mfn;
 
     xc_interface *xch;
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index c7aa44a..d668df1 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -99,7 +99,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
             dom->xenstore_gfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
             base + MEMACCESS_PFN_OFFSET);
-    /* allocated by toolstack */
+
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
             dom->console_evtchn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index a84a95e..8d4fefa 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -163,6 +163,39 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, xen_pfn_t pfn,
     return ptr;
 }
 
+static int xc_dom_check_required_fields(struct xc_dom_image *dom)
+{
+    int rc = 0;
+
+    if ( dom->console_evtchn == INVALID_EVTCHN )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set dom->console_evtchn", __func__);
+        rc = -1;
+    }
+    if ( dom->console_domid == INVALID_DOMID )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set dom->console_domid", __func__);
+        rc = -1;
+    }
+
+    if ( dom->xenstore_evtchn == INVALID_EVTCHN )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set dom->xenstore_evtchn", __func__);
+        rc = -1;
+    }
+    if ( dom->xenstore_domid == INVALID_DOMID )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set dom->xenstore_domid", __func__);
+        rc = -1;
+    }
+
+    return rc;
+}
+
 int xc_dom_boot_image(struct xc_dom_image *dom)
 {
     xc_dominfo_t info;
@@ -170,6 +203,9 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
 
     DOMPRINTF_CALLED(dom->xch);
 
+    if ( (rc = xc_dom_check_required_fields(dom)) != 0 )
+        return rc;
+
     /* misc stuff*/
     if ( (rc = dom->arch_hooks->bootearly(dom)) != 0 )
         return rc;
diff --git a/tools/libxc/xc_dom_compat_linux.c b/tools/libxc/xc_dom_compat_linux.c
index 6d27ec2..2ad43e4 100644
--- a/tools/libxc/xc_dom_compat_linux.c
+++ b/tools/libxc/xc_dom_compat_linux.c
@@ -61,7 +61,9 @@ int xc_linux_build(xc_interface *xch, uint32_t domid,
 
     dom->flags |= flags;
     dom->console_evtchn = console_evtchn;
+    dom->console_domid = 0;
     dom->xenstore_evtchn = store_evtchn;
+    dom->xenstore_domid = 0;
 
     if ( (rc = xc_dom_boot_xen_init(dom, xch, domid)) != 0 )
         goto out;
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index b5f316a..7087c50 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -779,6 +779,11 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
     dom->parms.elf_paddr_offset = UNSET_ADDR;
     dom->parms.p2m_base = UNSET_ADDR;
 
+    dom->console_evtchn = INVALID_EVTCHN;
+    dom->xenstore_evtchn = INVALID_EVTCHN;
+    dom->console_domid = INVALID_DOMID;
+    dom->xenstore_domid = INVALID_DOMID;
+
     dom->flags = SIF_VIRT_P2M_4TOOLS;
 
     dom->alloc_malloc += sizeof(*dom);
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index aa0ced1..05f4b80 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -617,6 +617,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     dom->xenstore_gfn = special_pfn(SPECIALPAGE_XENSTORE);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_gfn);
     xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN, dom->xenstore_gfn);
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
+                     dom->xenstore_evtchn);
 
     xc_hvm_param_set(xch, domid, HVM_PARAM_BUFIOREQ_PFN,
                      special_pfn(SPECIALPAGE_BUFIOREQ));
@@ -626,6 +628,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     dom->console_gfn = special_pfn(SPECIALPAGE_CONSOLE);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_gfn);
     xc_hvm_param_set(xch, domid, HVM_PARAM_CONSOLE_PFN, dom->console_gfn);
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
+                     dom->console_evtchn);
 
     xc_hvm_param_set(xch, domid, HVM_PARAM_PAGING_RING_PFN,
                      special_pfn(SPECIALPAGE_PAGING));
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 0389a06..fcdeef0 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -862,14 +862,10 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
 }
 
 static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
-                                libxl_domain_build_info *info,
-                                int store_evtchn, unsigned long *store_mfn,
-                                int console_evtchn, unsigned long *console_mfn,
-                                domid_t store_domid, domid_t console_domid)
+                                libxl_domain_build_info *info)
 {
     struct hvm_info_table *va_hvm;
     uint8_t *va_map, sum;
-    uint64_t str_mfn, cons_mfn;
     int i;
 
     if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
@@ -890,15 +886,6 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
         munmap(va_map, XC_PAGE_SIZE);
     }
 
-    xc_hvm_param_get(handle, domid, HVM_PARAM_STORE_PFN, &str_mfn);
-    xc_hvm_param_get(handle, domid, HVM_PARAM_CONSOLE_PFN, &cons_mfn);
-    xc_hvm_param_set(handle, domid, HVM_PARAM_STORE_EVTCHN, store_evtchn);
-    xc_hvm_param_set(handle, domid, HVM_PARAM_CONSOLE_EVTCHN, console_evtchn);
-
-    *store_mfn = str_mfn;
-    *console_mfn = cons_mfn;
-
-    xc_dom_gnttab_hvm_seed(handle, domid, *console_mfn, *store_mfn, console_domid, store_domid);
     return 0;
 }
 
@@ -1159,6 +1146,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
 
     dom->container_type = XC_DOM_HVM_CONTAINER;
 
+    dom->console_evtchn = state->console_port;
+    dom->console_domid = state->console_domid;
+    dom->xenstore_evtchn = state->store_port;
+    dom->xenstore_domid = state->store_domid;
+
     /* The params from the configuration file are in Mb, which are then
      * multiplied by 1 Kb. This was then divided off when calling
      * the old xc_hvm_build_target_mem() which then turned them to bytes.
@@ -1263,10 +1255,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
         goto out;
     }
 
-    rc = hvm_build_set_params(ctx->xch, domid, info, state->store_port,
-                               &state->store_mfn, state->console_port,
-                               &state->console_mfn, state->store_domid,
-                               state->console_domid);
+    rc = hvm_build_set_params(ctx->xch, domid, info);
     if (rc != 0) {
         LOG(ERROR, "hvm build set params failed");
         goto out;
@@ -1278,6 +1267,9 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
         goto out;
     }
 
+    state->console_mfn = dom->console_gfn;
+    state->store_mfn = dom->xenstore_gfn;
+
     xc_dom_release(dom);
     return 0;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index bcb6b0a..8a8ef4a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -115,7 +115,6 @@
 #define STUBDOM_SPECIAL_CONSOLES 3
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DOMID_XS_PATH "domid"
-#define INVALID_DOMID ~0
 
 /* Size macros. */
 #define __AC(X,Y)   (X##Y)
-- 
2.1.4


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

Re: [PATCH for-4.10 4/5] tools/dombuilder: Fix asymmetry when setting up console and xenstore rings

[Roger Pau Monné]

On Thu, Oct 05, 2017 at 06:23:43PM +0000, Andrew Cooper wrote:
> libxl always uses xc_dom_gnttab_init(), which internally calls
> xc_dom_gnttab{_hvm,}_seed() to set up the grants point at the console and
> xenstore rings.  For HVM guests, libxl then asks Xen for the information set
> up previously, and calls xc_dom_gnttab_hvm_seed() a second time, which is
> wasteful.  ARM construction expects libxl to have set up
> dom->{console,xenstore}_evtchn earlier, so only actually functions because of
> this second call.
> 
> Rationalise everything and make it consistent for all guests.
> 
>  1) Users of the domain builder are expected to provide
>     dom->{console,xenstore}_{evtchn,domid} unconditionally.  This is checked
>     by setting invalid values in xc_dom_allocate(), and checking in
>     xc_dom_boot_image().
> 
>  2) For x86 HVM and ARM guests, the event channels are given to Xen at the
>     same time as the ring gfns.  ARM already did this, but x86 is updated to
>     match.  x86 PV already provides this information in the start_info page.
> 
>  3) Libxl is updated to drop all relevant functionality from
>     hvm_build_set_params(), and behave consistently with PV guests when it
>     comes to the handling of dom->{console,xenstore}_{evtchn,domid,gfn}.
> 
> This removes several redundant hypercalls (including a foreign mapping) from
> the x86 HVM and ARM construction paths.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

LGTM, just one nit:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  tools/libxc/include/xc_dom.h      | 12 ++++++++----
>  tools/libxc/xc_dom_arm.c          |  2 +-
>  tools/libxc/xc_dom_boot.c         | 36 ++++++++++++++++++++++++++++++++++++
>  tools/libxc/xc_dom_compat_linux.c |  2 ++
>  tools/libxc/xc_dom_core.c         |  5 +++++
>  tools/libxc/xc_dom_x86.c          |  4 ++++
>  tools/libxl/libxl_dom.c           | 28 ++++++++++------------------
>  tools/libxl/libxl_internal.h      |  1 -
>  8 files changed, 66 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 80b4fbd..790869b 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -20,6 +20,8 @@
>  #include <xenguest.h>
>  
>  #define INVALID_PFN ((xen_pfn_t)-1)
> +#define INVALID_EVTCHN (~0u)
> +#define INVALID_DOMID  (-1)

Both xl and libxl already have an INVALID_DOMID, maybe it would be
time to place this in a public header.

Oh, I see that at the end of the patch you remove the one from libxl,
so nm.

>  #define X86_HVM_NR_SPECIAL_PAGES    8
>  #define X86_HVM_END_SPECIAL_REGION  0xff000u
>  
> @@ -104,10 +106,16 @@ struct xc_dom_image {
>       * Details for the toolstack-prepared rings.
>       *
>       * *_gfn fields are allocated by the domain builder.
> +     * *_{evtchn,domid} fields must be provided by the caller.
>       */
>      xen_pfn_t console_gfn;
>      xen_pfn_t xenstore_gfn;
>  
> +    unsigned int console_evtchn;
> +    unsigned int xenstore_evtchn;
> +    domid_t console_domid;
> +    domid_t xenstore_domid;
> +
>      /*
>       * initrd parameters as specified in start_info page
>       * Depending on capabilities of the booted kernel this may be a virtual
> @@ -165,10 +173,6 @@ struct xc_dom_image {
>  
>      /* misc xen domain config stuff */
>      unsigned long flags;
> -    unsigned int console_evtchn;
> -    unsigned int xenstore_evtchn;
> -    domid_t console_domid;
> -    domid_t xenstore_domid;
>      xen_pfn_t shared_info_mfn;
>  
>      xc_interface *xch;
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index c7aa44a..d668df1 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -99,7 +99,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>              dom->xenstore_gfn);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
>              base + MEMACCESS_PFN_OFFSET);
> -    /* allocated by toolstack */
> +
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
>              dom->console_evtchn);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index a84a95e..8d4fefa 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -163,6 +163,39 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, xen_pfn_t pfn,
>      return ptr;
>  }
>  
> +static int xc_dom_check_required_fields(struct xc_dom_image *dom)
> +{
> +    int rc = 0;
> +
> +    if ( dom->console_evtchn == INVALID_EVTCHN )
> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set dom->console_evtchn", __func__);
> +        rc = -1;
> +    }
> +    if ( dom->console_domid == INVALID_DOMID )
> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set dom->console_domid", __func__);
> +        rc = -1;
> +    }
> +
> +    if ( dom->xenstore_evtchn == INVALID_EVTCHN )
> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set dom->xenstore_evtchn", __func__);
> +        rc = -1;
> +    }
> +    if ( dom->xenstore_domid == INVALID_DOMID )
> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set dom->xenstore_domid", __func__);
> +        rc = -1;
> +    }

if ( rc != 0 )
    errno = EINVAL;

Thanks Roger.

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

Re: [PATCH for-4.10 4/5] tools/dombuilder: Fix asymmetry when setting up console and xenstore rings

[Andrew Cooper]

On 06/10/17 11:17, Roger Pau Monné wrote:
> On Thu, Oct 05, 2017 at 06:23:43PM +0000, Andrew Cooper wrote:
>> libxl always uses xc_dom_gnttab_init(), which internally calls
>> xc_dom_gnttab{_hvm,}_seed() to set up the grants point at the console and
>> xenstore rings.  For HVM guests, libxl then asks Xen for the information set
>> up previously, and calls xc_dom_gnttab_hvm_seed() a second time, which is
>> wasteful.  ARM construction expects libxl to have set up
>> dom->{console,xenstore}_evtchn earlier, so only actually functions because of
>> this second call.
>>
>> Rationalise everything and make it consistent for all guests.
>>
>>  1) Users of the domain builder are expected to provide
>>     dom->{console,xenstore}_{evtchn,domid} unconditionally.  This is checked
>>     by setting invalid values in xc_dom_allocate(), and checking in
>>     xc_dom_boot_image().
>>
>>  2) For x86 HVM and ARM guests, the event channels are given to Xen at the
>>     same time as the ring gfns.  ARM already did this, but x86 is updated to
>>     match.  x86 PV already provides this information in the start_info page.
>>
>>  3) Libxl is updated to drop all relevant functionality from
>>     hvm_build_set_params(), and behave consistently with PV guests when it
>>     comes to the handling of dom->{console,xenstore}_{evtchn,domid,gfn}.
>>
>> This removes several redundant hypercalls (including a foreign mapping) from
>> the x86 HVM and ARM construction paths.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> LGTM, just one nit:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
>> ---
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  tools/libxc/include/xc_dom.h      | 12 ++++++++----
>>  tools/libxc/xc_dom_arm.c          |  2 +-
>>  tools/libxc/xc_dom_boot.c         | 36 ++++++++++++++++++++++++++++++++++++
>>  tools/libxc/xc_dom_compat_linux.c |  2 ++
>>  tools/libxc/xc_dom_core.c         |  5 +++++
>>  tools/libxc/xc_dom_x86.c          |  4 ++++
>>  tools/libxl/libxl_dom.c           | 28 ++++++++++------------------
>>  tools/libxl/libxl_internal.h      |  1 -
>>  8 files changed, 66 insertions(+), 24 deletions(-)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index 80b4fbd..790869b 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -20,6 +20,8 @@
>>  #include <xenguest.h>
>>  
>>  #define INVALID_PFN ((xen_pfn_t)-1)
>> +#define INVALID_EVTCHN (~0u)
>> +#define INVALID_DOMID  (-1)
> Both xl and libxl already have an INVALID_DOMID, maybe it would be
> time to place this in a public header.
>
> Oh, I see that at the end of the patch you remove the one from libxl,
> so nm.

It turns out that Clang objects to this particular constant.

https://travis-ci.org/andyhhp/xen/jobs/283828942

I've folded a change to use (~0), which was what libxl used.

>
>>  #define X86_HVM_NR_SPECIAL_PAGES    8
>>  #define X86_HVM_END_SPECIAL_REGION  0xff000u
>>  
>> @@ -104,10 +106,16 @@ struct xc_dom_image {
>>       * Details for the toolstack-prepared rings.
>>       *
>>       * *_gfn fields are allocated by the domain builder.
>> +     * *_{evtchn,domid} fields must be provided by the caller.
>>       */
>>      xen_pfn_t console_gfn;
>>      xen_pfn_t xenstore_gfn;
>>  
>> +    unsigned int console_evtchn;
>> +    unsigned int xenstore_evtchn;
>> +    domid_t console_domid;
>> +    domid_t xenstore_domid;
>> +
>>      /*
>>       * initrd parameters as specified in start_info page
>>       * Depending on capabilities of the booted kernel this may be a virtual
>> @@ -165,10 +173,6 @@ struct xc_dom_image {
>>  
>>      /* misc xen domain config stuff */
>>      unsigned long flags;
>> -    unsigned int console_evtchn;
>> -    unsigned int xenstore_evtchn;
>> -    domid_t console_domid;
>> -    domid_t xenstore_domid;
>>      xen_pfn_t shared_info_mfn;
>>  
>>      xc_interface *xch;
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index c7aa44a..d668df1 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -99,7 +99,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>>              dom->xenstore_gfn);
>>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
>>              base + MEMACCESS_PFN_OFFSET);
>> -    /* allocated by toolstack */
>> +
>>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
>>              dom->console_evtchn);
>>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
>> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
>> index a84a95e..8d4fefa 100644
>> --- a/tools/libxc/xc_dom_boot.c
>> +++ b/tools/libxc/xc_dom_boot.c
>> @@ -163,6 +163,39 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, xen_pfn_t pfn,
>>      return ptr;
>>  }
>>  
>> +static int xc_dom_check_required_fields(struct xc_dom_image *dom)
>> +{
>> +    int rc = 0;
>> +
>> +    if ( dom->console_evtchn == INVALID_EVTCHN )
>> +    {
>> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
>> +                     "%s: Caller didn't set dom->console_evtchn", __func__);
>> +        rc = -1;
>> +    }
>> +    if ( dom->console_domid == INVALID_DOMID )
>> +    {
>> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
>> +                     "%s: Caller didn't set dom->console_domid", __func__);
>> +        rc = -1;
>> +    }
>> +
>> +    if ( dom->xenstore_evtchn == INVALID_EVTCHN )
>> +    {
>> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
>> +                     "%s: Caller didn't set dom->xenstore_evtchn", __func__);
>> +        rc = -1;
>> +    }
>> +    if ( dom->xenstore_domid == INVALID_DOMID )
>> +    {
>> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
>> +                     "%s: Caller didn't set dom->xenstore_domid", __func__);
>> +        rc = -1;
>> +    }
> if ( rc != 0 )
>     errno = EINVAL;

Will do.

~Andrew

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

[PATCH for-4.10 5/5] tools/dombuilder: Prevent failures of xc_dom_gnttab_init()

[Andrew Cooper]

Recent changes in grant table configuration have caused calls to
xc_dom_gnttab_init() to fail if not proceeded with a call to
xc_domain_set_gnttab_limits().  This is backwards from the point of view of
3rd party dombuilder users.

Add max_{grant,maptrack}_frames parameters to struct xc_dom_image, and require
them to be set by callers using xc_dom_gnttab_init().  Libxl, which uses
xc_dom_gnttab_init() itself is updated appropriately.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/include/xc_dom.h |  4 ++++
 tools/libxc/xc_dom_boot.c    | 14 ++++++++++++++
 tools/libxc/xc_dom_core.c    |  3 +++
 tools/libxl/libxl_dom.c      | 12 ++++++------
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 790869b..8e673fb 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -116,6 +116,10 @@ struct xc_dom_image {
     domid_t console_domid;
     domid_t xenstore_domid;
 
+    /* Grant limit configuration; mandatory if calling xc_dom_gnttab_init(). */
+    unsigned int max_grant_frames;
+    unsigned int max_maptrack_frames;
+
     /*
      * initrd parameters as specified in start_info page
      * Depending on capabilities of the booted kernel this may be a virtual
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 8d4fefa..7cb9e40 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -419,6 +419,20 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
 
 int xc_dom_gnttab_init(struct xc_dom_image *dom)
 {
+    int rc;
+
+    if ( dom->max_grant_frames == -1 || dom->max_maptrack_frames == -1 )
+    {
+        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
+                     "%s: Caller didn't set grant limit information", __func__);
+        return -1;
+    }
+
+    if ( (rc = xc_domain_set_gnttab_limits(dom->xch, dom->guest_domid,
+                                           dom->max_grant_frames,
+                                           dom->max_maptrack_frames)) != 0 )
+        return rc;
+
     if ( xc_dom_translated(dom) ) {
         return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
                                       dom->console_gfn, dom->xenstore_gfn,
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 7087c50..d660651 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -784,6 +784,9 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
     dom->console_domid = INVALID_DOMID;
     dom->xenstore_domid = INVALID_DOMID;
 
+    dom->max_grant_frames = -1;
+    dom->max_maptrack_frames = -1;
+
     dom->flags = SIF_VIRT_P2M_4TOOLS;
 
     dom->alloc_malloc += sizeof(*dom);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index fcdeef0..fa5319d 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -358,12 +358,6 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
         return ERROR_FAIL;
     }
 
-    if (xc_domain_set_gnttab_limits(ctx->xch, domid, info->max_grant_frames,
-                                    info->max_maptrack_frames) != 0) {
-        LOG(ERROR, "Couldn't set grant table limits");
-        return ERROR_FAIL;
-    }
-
     /*
      * Check if the domain has any CPU or node affinity already. If not, try
      * to build up the latter via automatic NUMA placement. In fact, in case
@@ -815,6 +809,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
     dom->xenstore_domid = state->store_domid;
     dom->claim_enabled = libxl_defbool_val(info->claim_mode);
 
+    dom->max_grant_frames    = info->max_grant_frames;
+    dom->max_maptrack_frames = info->max_maptrack_frames;
+
     if (info->num_vnuma_nodes != 0) {
         unsigned int i;
 
@@ -1151,6 +1148,9 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
     dom->xenstore_evtchn = state->store_port;
     dom->xenstore_domid = state->store_domid;
 
+    dom->max_grant_frames    = info->max_grant_frames;
+    dom->max_maptrack_frames = info->max_maptrack_frames;
+
     /* The params from the configuration file are in Mb, which are then
      * multiplied by 1 Kb. This was then divided off when calling
      * the old xc_hvm_build_target_mem() which then turned them to bytes.
-- 
2.1.4


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

Re: [PATCH for-4.10 5/5] tools/dombuilder: Prevent failures of xc_dom_gnttab_init()

[Roger Pau Monné]

On Thu, Oct 05, 2017 at 06:23:44PM +0000, Andrew Cooper wrote:
> Recent changes in grant table configuration have caused calls to
> xc_dom_gnttab_init() to fail if not proceeded with a call to
> xc_domain_set_gnttab_limits().  This is backwards from the point of view of
> 3rd party dombuilder users.
> 
> Add max_{grant,maptrack}_frames parameters to struct xc_dom_image, and require
> them to be set by callers using xc_dom_gnttab_init().  Libxl, which uses
> xc_dom_gnttab_init() itself is updated appropriately.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Some nits with can be fixed while committing IMHO if required.

> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  tools/libxc/include/xc_dom.h |  4 ++++
>  tools/libxc/xc_dom_boot.c    | 14 ++++++++++++++
>  tools/libxc/xc_dom_core.c    |  3 +++
>  tools/libxl/libxl_dom.c      | 12 ++++++------
>  4 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 790869b..8e673fb 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -116,6 +116,10 @@ struct xc_dom_image {
>      domid_t console_domid;
>      domid_t xenstore_domid;
>  
> +    /* Grant limit configuration; mandatory if calling xc_dom_gnttab_init(). */
> +    unsigned int max_grant_frames;
> +    unsigned int max_maptrack_frames;
> +
>      /*
>       * initrd parameters as specified in start_info page
>       * Depending on capabilities of the booted kernel this may be a virtual
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index 8d4fefa..7cb9e40 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -419,6 +419,20 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>  
>  int xc_dom_gnttab_init(struct xc_dom_image *dom)
>  {
> +    int rc;
> +
> +    if ( dom->max_grant_frames == -1 || dom->max_maptrack_frames == -1 )

Not sure if compilers will complain about comparing an unsigned type
against a signed one. Maybe better to use ~0u?

> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set grant limit information", __func__);

errno = EINVAL;

Thanks, Roger.

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

Re: [PATCH for-4.10 5/5] tools/dombuilder: Prevent failures of xc_dom_gnttab_init()

[Andrew Cooper]

On 06/10/17 11:30, Roger Pau Monné wrote:
> On Thu, Oct 05, 2017 at 06:23:44PM +0000, Andrew Cooper wrote:
>> Recent changes in grant table configuration have caused calls to
>> xc_dom_gnttab_init() to fail if not proceeded with a call to
>> xc_domain_set_gnttab_limits().  This is backwards from the point of view of
>> 3rd party dombuilder users.
>>
>> Add max_{grant,maptrack}_frames parameters to struct xc_dom_image, and require
>> them to be set by callers using xc_dom_gnttab_init().  Libxl, which uses
>> xc_dom_gnttab_init() itself is updated appropriately.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Some nits with can be fixed while committing IMHO if required.
>
>> ---
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  tools/libxc/include/xc_dom.h |  4 ++++
>>  tools/libxc/xc_dom_boot.c    | 14 ++++++++++++++
>>  tools/libxc/xc_dom_core.c    |  3 +++
>>  tools/libxl/libxl_dom.c      | 12 ++++++------
>>  4 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index 790869b..8e673fb 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -116,6 +116,10 @@ struct xc_dom_image {
>>      domid_t console_domid;
>>      domid_t xenstore_domid;
>>  
>> +    /* Grant limit configuration; mandatory if calling xc_dom_gnttab_init(). */
>> +    unsigned int max_grant_frames;
>> +    unsigned int max_maptrack_frames;
>> +
>>      /*
>>       * initrd parameters as specified in start_info page
>>       * Depending on capabilities of the booted kernel this may be a virtual
>> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
>> index 8d4fefa..7cb9e40 100644
>> --- a/tools/libxc/xc_dom_boot.c
>> +++ b/tools/libxc/xc_dom_boot.c
>> @@ -419,6 +419,20 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>>  
>>  int xc_dom_gnttab_init(struct xc_dom_image *dom)
>>  {
>> +    int rc;
>> +
>> +    if ( dom->max_grant_frames == -1 || dom->max_maptrack_frames == -1 )
> Not sure if compilers will complain about comparing an unsigned type
> against a signed one. Maybe better to use ~0u?

The conversion of -1 to unsigned int is well defined.  I think this is
fine as is.  Wei, as maintainer, whats your say?

>
>> +    {
>> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
>> +                     "%s: Caller didn't set grant limit information", __func__);
> errno = EINVAL;

Done.

~Andrew

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

Re: [PATCH for-4.10 5/5] tools/dombuilder: Prevent failures of xc_dom_gnttab_init()

[Wei Liu]

On Fri, Oct 06, 2017 at 07:04:49PM +0100, Andrew Cooper wrote:
> On 06/10/17 11:30, Roger Pau Monné wrote:
> > On Thu, Oct 05, 2017 at 06:23:44PM +0000, Andrew Cooper wrote:
> >> Recent changes in grant table configuration have caused calls to
> >> xc_dom_gnttab_init() to fail if not proceeded with a call to
> >> xc_domain_set_gnttab_limits().  This is backwards from the point of view of
> >> 3rd party dombuilder users.
> >>
> >> Add max_{grant,maptrack}_frames parameters to struct xc_dom_image, and require
> >> them to be set by callers using xc_dom_gnttab_init().  Libxl, which uses
> >> xc_dom_gnttab_init() itself is updated appropriately.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> > Some nits with can be fixed while committing IMHO if required.
> >
> >> ---
> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> ---
> >>  tools/libxc/include/xc_dom.h |  4 ++++
> >>  tools/libxc/xc_dom_boot.c    | 14 ++++++++++++++
> >>  tools/libxc/xc_dom_core.c    |  3 +++
> >>  tools/libxl/libxl_dom.c      | 12 ++++++------
> >>  4 files changed, 27 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> >> index 790869b..8e673fb 100644
> >> --- a/tools/libxc/include/xc_dom.h
> >> +++ b/tools/libxc/include/xc_dom.h
> >> @@ -116,6 +116,10 @@ struct xc_dom_image {
> >>      domid_t console_domid;
> >>      domid_t xenstore_domid;
> >>  
> >> +    /* Grant limit configuration; mandatory if calling xc_dom_gnttab_init(). */
> >> +    unsigned int max_grant_frames;
> >> +    unsigned int max_maptrack_frames;
> >> +
> >>      /*
> >>       * initrd parameters as specified in start_info page
> >>       * Depending on capabilities of the booted kernel this may be a virtual
> >> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> >> index 8d4fefa..7cb9e40 100644
> >> --- a/tools/libxc/xc_dom_boot.c
> >> +++ b/tools/libxc/xc_dom_boot.c
> >> @@ -419,6 +419,20 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
> >>  
> >>  int xc_dom_gnttab_init(struct xc_dom_image *dom)
> >>  {
> >> +    int rc;
> >> +
> >> +    if ( dom->max_grant_frames == -1 || dom->max_maptrack_frames == -1 )
> > Not sure if compilers will complain about comparing an unsigned type
> > against a signed one. Maybe better to use ~0u?
> 
> The conversion of -1 to unsigned int is well defined.  I think this is
> fine as is.  Wei, as maintainer, whats your say?
> 

That's fine.

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

Re: [PATCH for-4.10 5/5] tools/dombuilder: Prevent failures of xc_dom_gnttab_init()

[Wei Liu]

On Thu, Oct 05, 2017 at 07:23:44PM +0100, Andrew Cooper wrote:
> Recent changes in grant table configuration have caused calls to
> xc_dom_gnttab_init() to fail if not proceeded with a call to
> xc_domain_set_gnttab_limits().  This is backwards from the point of view of
> 3rd party dombuilder users.
> 
> Add max_{grant,maptrack}_frames parameters to struct xc_dom_image, and require
> them to be set by callers using xc_dom_gnttab_init().  Libxl, which uses
> xc_dom_gnttab_init() itself is updated appropriately.
> 

Either the code as-is or this patch requires modification to dombuilder
users, so I'm not too convinced if the original code is backwards.

I'm not too fussed about how things are done as long as they continue to
work, so with Roger's comments addressed:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-4.10 5/5] tools/dombuilder: Prevent failures of xc_dom_gnttab_init()

[Andrew Cooper]

On 06/10/17 18:39, Wei Liu wrote:
> On Thu, Oct 05, 2017 at 07:23:44PM +0100, Andrew Cooper wrote:
>> Recent changes in grant table configuration have caused calls to
>> xc_dom_gnttab_init() to fail if not proceeded with a call to
>> xc_domain_set_gnttab_limits().  This is backwards from the point of view of
>> 3rd party dombuilder users.
>>
>> Add max_{grant,maptrack}_frames parameters to struct xc_dom_image, and require
>> them to be set by callers using xc_dom_gnttab_init().  Libxl, which uses
>> xc_dom_gnttab_init() itself is updated appropriately.
>>
> Either the code as-is or this patch requires modification to dombuilder
> users, so I'm not too convinced if the original code is backwards.

For dombuilder users who currently call xc_dom_gnttab_init(), it is
antisocial for them to suddenly find they need to call
xc_domain_set_gnttab_limits() before xc_dom_gnttab_init() will succeed,
and _init() can reasonably be expected to DTRT.

Whatever happens, before or after this series, all dombuilder users need
to modify their calls.

I argue that with this series in place, the caller has a far more
rational interface to use, which is consistent across guests types.

>
> I'm not too fussed about how things are done as long as they continue to
> work, so with Roger's comments addressed:
>
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks,

~Andrew

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

Re: [PATCH for-4.10 0/5] tools/dombuilder: Fixes and improvements to grant handling

[Julien Grall]

Hi Andrew,

On 05/10/17 19:23, Andrew Cooper wrote:
> A git tree version is available:
> 
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/dombuilder-gnt-v1

I have tested it on Arm64 and didn't spot any regression so far:

Tested-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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