[PATCH 0/7] xen: More assorted improvements to domain creation

[PATCH 0/7] xen: More assorted improvements to domain creation

[Andrew Cooper]

This series is in preparation for passing more parameters via
XEN_DOMCTL_createdomain.  It is hypervisor side cleanup, with a couple of
related tangents.  The toolstack side of this work is forthcoming.

This series has been compile tested on all architecture, and functionally
tested on x86.

Andrew Cooper (7):
  xen/domain: Drop DOMCRF_dummy
  xen/domain: Drop all DOMCRF_* constants
  RFC arm/domain: Reject invalid combinations of domain creation flags
  x86/domain: Remove unused parameters from {hvm,pv}_domain_initialise()
  x86/domain: Optimise the order of actions in arch_domain_create()
  xen/domain: Pass the full domctl_createdomain struct to create_domain()
  xen/mm: Clean up share_xen_page_with_guest() API

 xen/arch/arm/domain.c             |  20 ++---
 xen/arch/arm/mm.c                 |  19 ++---
 xen/arch/arm/setup.c              |   8 +-
 xen/arch/x86/domain.c             | 159 ++++++++++++++++++--------------------
 xen/arch/x86/domain_page.c        |   3 +-
 xen/arch/x86/hvm/dom0_build.c     |   2 +-
 xen/arch/x86/hvm/hvm.c            |   3 +-
 xen/arch/x86/hvm/vmx/vmx.c        |   2 +-
 xen/arch/x86/mm.c                 |  26 +++----
 xen/arch/x86/mm/shadow/common.c   |   2 +-
 xen/arch/x86/pv/domain.c          |   3 +-
 xen/arch/x86/pv/shim.c            |   6 +-
 xen/arch/x86/setup.c              |  18 +++--
 xen/arch/x86/time.c               |   4 +-
 xen/arch/x86/x86_64/mm.c          |  16 ++--
 xen/common/domain.c               |  34 +++++---
 xen/common/domctl.c               |  20 +----
 xen/common/schedule.c             |   2 +-
 xen/common/trace.c                |   9 +--
 xen/common/xenoprof.c             |   3 +-
 xen/include/asm-arm/grant_table.h |   3 +-
 xen/include/asm-arm/mm.h          |   7 --
 xen/include/asm-x86/grant_table.h |   6 +-
 xen/include/asm-x86/hvm/hvm.h     |   3 +-
 xen/include/asm-x86/mm.h          |   8 --
 xen/include/asm-x86/pv/domain.h   |  11 +--
 xen/include/xen/domain.h          |   4 +-
 xen/include/xen/mm.h              |  14 ++++
 xen/include/xen/sched.h           |  32 +++-----
 29 files changed, 197 insertions(+), 250 deletions(-)

-- 
2.1.4


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

[PATCH 1/7] xen/domain: Drop DOMCRF_dummy

[Andrew Cooper]

At the moment, there is a tight coupling between the domid and the use of
DOMCRF_dummy.  Instead of using DOMCRF_dummy, base the one relevent decision
on domid alone.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c       |  6 +++---
 xen/arch/x86/mm.c       |  6 +++---
 xen/common/domain.c     |  3 ++-
 xen/include/xen/sched.h | 11 +++++++----
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 3c328e2..ce83f69 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -520,7 +520,7 @@ void __init arch_init_memory(void)
      * Any Xen-heap pages that we will allow to be mapped will have
      * their domain field set to dom_xen.
      */
-    dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
+    dom_xen = domain_create(DOMID_XEN, 0, 0, NULL);
     BUG_ON(IS_ERR(dom_xen));
 
     /*
@@ -528,14 +528,14 @@ void __init arch_init_memory(void)
      * This domain owns I/O pages that are within the range of the page_info
      * array. Mappings occur at the priv of the caller.
      */
-    dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0, NULL);
+    dom_io = domain_create(DOMID_IO, 0, 0, NULL);
     BUG_ON(IS_ERR(dom_io));
 
     /*
      * Initialise our COW domain.
      * This domain owns sharable pages.
      */
-    dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
+    dom_cow = domain_create(DOMID_COW, 0, 0, NULL);
     BUG_ON(IS_ERR(dom_cow));
 }
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 9b55944..c275d4b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -271,7 +271,7 @@ void __init arch_init_memory(void)
      * Hidden PCI devices will also be associated with this domain
      * (but be [partly] controlled by Dom0 nevertheless).
      */
-    dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
+    dom_xen = domain_create(DOMID_XEN, 0, 0, NULL);
     BUG_ON(IS_ERR(dom_xen));
     INIT_LIST_HEAD(&dom_xen->arch.pdev_list);
 
@@ -280,14 +280,14 @@ void __init arch_init_memory(void)
      * This domain owns I/O pages that are within the range of the page_info
      * array. Mappings occur at the priv of the caller.
      */
-    dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0, NULL);
+    dom_io = domain_create(DOMID_IO, 0, 0, NULL);
     BUG_ON(IS_ERR(dom_io));
 
     /*
      * Initialise our COW domain.
      * This domain owns sharable pages.
      */
-    dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
+    dom_cow = domain_create(DOMID_COW, 0, 0, NULL);
     BUG_ON(IS_ERR(dom_cow));
 
     /*
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 219a3e3..cd39a58 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -312,7 +312,8 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     rangeset_domain_initialise(d);
     init_status |= INIT_rangeset;
 
-    if ( domcr_flags & DOMCRF_dummy )
+    /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
+    if ( is_system_domain(d) && !is_idle_domain(d) )
         return d;
 
     if ( !is_idle_domain(d) )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 39f9386..aa5729b 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -491,9 +491,15 @@ extern spinlock_t domlist_update_lock;
 extern rcu_read_lock_t domlist_read_lock;
 
 extern struct vcpu *idle_vcpu[NR_CPUS];
+
 #define is_idle_domain(d) ((d)->domain_id == DOMID_IDLE)
 #define is_idle_vcpu(v)   (is_idle_domain((v)->domain))
 
+static inline bool is_system_domain(const struct domain *d)
+{
+    return d->domain_id >= DOMID_FIRST_RESERVED;
+}
+
 #define DOMAIN_DESTROYED (1u << 31) /* assumes atomic_t is >= 32 bits */
 #define put_domain(_d) \
   if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d)
@@ -531,7 +537,7 @@ void domain_update_node_affinity(struct domain *d);
 
 /*
  * Create a domain: the configuration is only necessary for real domain
- * (i.e !DOMCRF_dummy, excluded idle domain).
+ * (domid < DOMID_FIRST_RESERVED).
  */
 struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
                              uint32_t ssidref,
@@ -546,9 +552,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
                         by tboot */
 #define _DOMCRF_s3_integrity  2
 #define DOMCRF_s3_integrity   (1U<<_DOMCRF_s3_integrity)
- /* DOMCRF_dummy: Create a dummy domain (not scheduled; not on domain list) */
-#define _DOMCRF_dummy         3
-#define DOMCRF_dummy          (1U<<_DOMCRF_dummy)
  /* DOMCRF_oos_off: dont use out-of-sync optimization for shadow page tables */
 #define _DOMCRF_oos_off         4
 #define DOMCRF_oos_off          (1U<<_DOMCRF_oos_off)
-- 
2.1.4


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

Re: [PATCH 1/7] xen/domain: Drop DOMCRF_dummy

[Wei Liu]

On Fri, Mar 09, 2018 at 01:18:36PM +0000, Andrew Cooper wrote:
> At the moment, there is a tight coupling between the domid and the use of
> DOMCRF_dummy.  Instead of using DOMCRF_dummy, base the one relevent decision
> on domid alone.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 1/7] xen/domain: Drop DOMCRF_dummy

[Jan Beulich]

>>> On 09.03.18 at 15:12, <wei.liu2@citrix.com> wrote:
> On Fri, Mar 09, 2018 at 01:18:36PM +0000, Andrew Cooper wrote:
>> At the moment, there is a tight coupling between the domid and the use of
>> DOMCRF_dummy.  Instead of using DOMCRF_dummy, base the one relevent decision
>> on domid alone.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 1/7] xen/domain: Drop DOMCRF_dummy

[Julien Grall]

Hi Andrew,

On 03/09/2018 01:18 PM, Andrew Cooper wrote:
> At the moment, there is a tight coupling between the domid and the use of
> DOMCRF_dummy.  Instead of using DOMCRF_dummy, base the one relevent decision

NIT: s/relevent/relevant/

> on domid alone.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Cheers,

> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/mm.c       |  6 +++---
>   xen/arch/x86/mm.c       |  6 +++---
>   xen/common/domain.c     |  3 ++-
>   xen/include/xen/sched.h | 11 +++++++----
>   4 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 3c328e2..ce83f69 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -520,7 +520,7 @@ void __init arch_init_memory(void)
>        * Any Xen-heap pages that we will allow to be mapped will have
>        * their domain field set to dom_xen.
>        */
> -    dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
> +    dom_xen = domain_create(DOMID_XEN, 0, 0, NULL);
>       BUG_ON(IS_ERR(dom_xen));
>   
>       /*
> @@ -528,14 +528,14 @@ void __init arch_init_memory(void)
>        * This domain owns I/O pages that are within the range of the page_info
>        * array. Mappings occur at the priv of the caller.
>        */
> -    dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0, NULL);
> +    dom_io = domain_create(DOMID_IO, 0, 0, NULL);
>       BUG_ON(IS_ERR(dom_io));
>   
>       /*
>        * Initialise our COW domain.
>        * This domain owns sharable pages.
>        */
> -    dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
> +    dom_cow = domain_create(DOMID_COW, 0, 0, NULL);
>       BUG_ON(IS_ERR(dom_cow));
>   }
>   
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 9b55944..c275d4b 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -271,7 +271,7 @@ void __init arch_init_memory(void)
>        * Hidden PCI devices will also be associated with this domain
>        * (but be [partly] controlled by Dom0 nevertheless).
>        */
> -    dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
> +    dom_xen = domain_create(DOMID_XEN, 0, 0, NULL);
>       BUG_ON(IS_ERR(dom_xen));
>       INIT_LIST_HEAD(&dom_xen->arch.pdev_list);
>   
> @@ -280,14 +280,14 @@ void __init arch_init_memory(void)
>        * This domain owns I/O pages that are within the range of the page_info
>        * array. Mappings occur at the priv of the caller.
>        */
> -    dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0, NULL);
> +    dom_io = domain_create(DOMID_IO, 0, 0, NULL);
>       BUG_ON(IS_ERR(dom_io));
>   
>       /*
>        * Initialise our COW domain.
>        * This domain owns sharable pages.
>        */
> -    dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
> +    dom_cow = domain_create(DOMID_COW, 0, 0, NULL);
>       BUG_ON(IS_ERR(dom_cow));
>   
>       /*
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 219a3e3..cd39a58 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -312,7 +312,8 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>       rangeset_domain_initialise(d);
>       init_status |= INIT_rangeset;
>   
> -    if ( domcr_flags & DOMCRF_dummy )
> +    /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
> +    if ( is_system_domain(d) && !is_idle_domain(d) )
>           return d;
>   
>       if ( !is_idle_domain(d) )
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 39f9386..aa5729b 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -491,9 +491,15 @@ extern spinlock_t domlist_update_lock;
>   extern rcu_read_lock_t domlist_read_lock;
>   
>   extern struct vcpu *idle_vcpu[NR_CPUS];
> +
>   #define is_idle_domain(d) ((d)->domain_id == DOMID_IDLE)
>   #define is_idle_vcpu(v)   (is_idle_domain((v)->domain))
>   
> +static inline bool is_system_domain(const struct domain *d)
> +{
> +    return d->domain_id >= DOMID_FIRST_RESERVED;
> +}
> +
>   #define DOMAIN_DESTROYED (1u << 31) /* assumes atomic_t is >= 32 bits */
>   #define put_domain(_d) \
>     if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d)
> @@ -531,7 +537,7 @@ void domain_update_node_affinity(struct domain *d);
>   
>   /*
>    * Create a domain: the configuration is only necessary for real domain
> - * (i.e !DOMCRF_dummy, excluded idle domain).
> + * (domid < DOMID_FIRST_RESERVED).
>    */
>   struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>                                uint32_t ssidref,
> @@ -546,9 +552,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>                           by tboot */
>   #define _DOMCRF_s3_integrity  2
>   #define DOMCRF_s3_integrity   (1U<<_DOMCRF_s3_integrity)
> - /* DOMCRF_dummy: Create a dummy domain (not scheduled; not on domain list) */
> -#define _DOMCRF_dummy         3
> -#define DOMCRF_dummy          (1U<<_DOMCRF_dummy)
>    /* DOMCRF_oos_off: dont use out-of-sync optimization for shadow page tables */
>   #define _DOMCRF_oos_off         4
>   #define DOMCRF_oos_off          (1U<<_DOMCRF_oos_off)
> 

-- 
Julien Grall

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

[PATCH 2/7] xen/domain: Drop all DOMCRF_* constants

[Andrew Cooper]

With DOMCRF_dummy removed, all remaining DOMCRF_* idenitcally match their
DOMCTL counterparts.  Avoid having a conversion between two different bit
layouts, and use the DOMCTL_CDF_* constants everywhere.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/domain.c           |  4 ++--
 xen/arch/x86/mm/shadow/common.c |  2 +-
 xen/arch/x86/setup.c            |  6 +++---
 xen/common/domain.c             |  4 ++--
 xen/common/domctl.c             | 16 ++--------------
 xen/include/xen/sched.h         | 16 ----------------
 6 files changed, 10 insertions(+), 38 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 69679a6..36555ea 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -435,7 +435,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( config == NULL && !is_idle_domain(d) )
         return -EINVAL;
 
-    d->arch.s3_integrity = !!(domcr_flags & DOMCRF_s3_integrity);
+    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
 
     INIT_LIST_HEAD(&d->arch.pdev_list);
 
@@ -497,7 +497,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         /* Need to determine if HAP is enabled before initialising paging */
         if ( is_hvm_domain(d) )
             d->arch.hvm_domain.hap_enabled =
-                hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
+                hvm_funcs.hap_supported && (domcr_flags & XEN_DOMCTL_CDF_hap);
 
         if ( (rc = paging_domain_init(d, domcr_flags)) != 0 )
             goto fail;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 20ded3e..1cf7b28 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -62,7 +62,7 @@ int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
     d->arch.paging.shadow.oos_active = 0;
-    d->arch.paging.shadow.oos_off = (domcr_flags & DOMCRF_oos_off) ?  1 : 0;
+    d->arch.paging.shadow.oos_off = domcr_flags & XEN_DOMCTL_CDF_oos_off;
 #endif
     d->arch.paging.shadow.pagetable_dying_op = 0;
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ac530ec..a6dc5df 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -660,7 +660,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 {
     char *memmap_type = NULL;
     char *cmdline, *kextra, *loader;
-    unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
+    unsigned int initrdidx, domcr_flags = XEN_DOMCTL_CDF_s3_integrity;
     multiboot_info_t *mbi;
     module_t *mod;
     unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
@@ -1632,9 +1632,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     if ( dom0_pvh )
     {
-        domcr_flags |= DOMCRF_hvm |
+        domcr_flags |= XEN_DOMCTL_CDF_hvm_guest |
                        ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
-                         DOMCRF_hap : 0);
+                         XEN_DOMCTL_CDF_hap : 0);
         config.emulation_flags = XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC;
     }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index cd39a58..582e3e5 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -304,7 +304,7 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     if ( !zalloc_cpumask_var(&d->dirty_cpumask) )
         goto fail;
 
-    if ( domcr_flags & DOMCRF_hvm )
+    if ( domcr_flags & XEN_DOMCTL_CDF_hvm_guest )
         d->guest_type = guest_type_hvm;
     else
         d->guest_type = guest_type_pv;
@@ -331,7 +331,7 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
             hardware_domain = d;
         }
 
-        if ( domcr_flags & DOMCRF_xs_domain )
+        if ( domcr_flags & XEN_DOMCTL_CDF_xs_domain )
         {
             d->is_xenstore = 1;
             d->disable_migrate = 1;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 50f7422..a73e1a4 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -498,7 +498,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     {
         domid_t        dom;
         static domid_t rover = 0;
-        unsigned int domcr_flags;
 
         ret = -EINVAL;
         if ( (op->u.createdomain.flags &
@@ -533,19 +532,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             rover = dom;
         }
 
-        domcr_flags = 0;
-        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hvm_guest )
-            domcr_flags |= DOMCRF_hvm;
-        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hap )
-            domcr_flags |= DOMCRF_hap;
-        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_s3_integrity )
-            domcr_flags |= DOMCRF_s3_integrity;
-        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_oos_off )
-            domcr_flags |= DOMCRF_oos_off;
-        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
-            domcr_flags |= DOMCRF_xs_domain;
-
-        d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
+        d = domain_create(dom, op->u.createdomain.flags,
+                          op->u.createdomain.ssidref,
                           &op->u.createdomain.config);
         if ( IS_ERR(d) )
         {
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index aa5729b..9af78ac 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -542,22 +542,6 @@ void domain_update_node_affinity(struct domain *d);
 struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
                              uint32_t ssidref,
                              struct xen_arch_domainconfig *config);
- /* DOMCRF_hvm: Create an HVM domain, as opposed to a PV domain. */
-#define _DOMCRF_hvm           0
-#define DOMCRF_hvm            (1U<<_DOMCRF_hvm)
- /* DOMCRF_hap: Create a domain with hardware-assisted paging. */
-#define _DOMCRF_hap           1
-#define DOMCRF_hap            (1U<<_DOMCRF_hap)
- /* DOMCRF_s3_integrity: Create a domain with tboot memory integrity protection
-                        by tboot */
-#define _DOMCRF_s3_integrity  2
-#define DOMCRF_s3_integrity   (1U<<_DOMCRF_s3_integrity)
- /* DOMCRF_oos_off: dont use out-of-sync optimization for shadow page tables */
-#define _DOMCRF_oos_off         4
-#define DOMCRF_oos_off          (1U<<_DOMCRF_oos_off)
- /* DOMCRF_xs_domain: xenstore domain */
-#define _DOMCRF_xs_domain       5
-#define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
 
 /*
  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
-- 
2.1.4


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

Re: [PATCH 2/7] xen/domain: Drop all DOMCRF_* constants

[Wei Liu]

On Fri, Mar 09, 2018 at 01:18:37PM +0000, Andrew Cooper wrote:
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 50f7422..a73e1a4 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -498,7 +498,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      {
>          domid_t        dom;
>          static domid_t rover = 0;
> -        unsigned int domcr_flags;
>  
>          ret = -EINVAL;
>          if ( (op->u.createdomain.flags &
> @@ -533,19 +532,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              rover = dom;
>          }
>  
> -        domcr_flags = 0;
> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hvm_guest )
> -            domcr_flags |= DOMCRF_hvm;
> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hap )
> -            domcr_flags |= DOMCRF_hap;
> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_s3_integrity )
> -            domcr_flags |= DOMCRF_s3_integrity;
> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_oos_off )
> -            domcr_flags |= DOMCRF_oos_off;
> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
> -            domcr_flags |= DOMCRF_xs_domain;
> -

Please consider adding a test to reject flags Xen doesn't recognise.

The rest looks OK.

Wei.

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

Re: [PATCH 2/7] xen/domain: Drop all DOMCRF_* constants

[Andrew Cooper]

On 09/03/18 14:12, Wei Liu wrote:
> On Fri, Mar 09, 2018 at 01:18:37PM +0000, Andrew Cooper wrote:
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 50f7422..a73e1a4 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -498,7 +498,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>      {
>>          domid_t        dom;
>>          static domid_t rover = 0;
>> -        unsigned int domcr_flags;
>>  
>>          ret = -EINVAL;
>>          if ( (op->u.createdomain.flags &
>> @@ -533,19 +532,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>              rover = dom;
>>          }
>>  
>> -        domcr_flags = 0;
>> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hvm_guest )
>> -            domcr_flags |= DOMCRF_hvm;
>> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hap )
>> -            domcr_flags |= DOMCRF_hap;
>> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_s3_integrity )
>> -            domcr_flags |= DOMCRF_s3_integrity;
>> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_oos_off )
>> -            domcr_flags |= DOMCRF_oos_off;
>> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
>> -            domcr_flags |= DOMCRF_xs_domain;
>> -
> Please consider adding a test to reject flags Xen doesn't recognise.

There already is, between these two hunks. :)

~Andrew

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

Re: [PATCH 2/7] xen/domain: Drop all DOMCRF_* constants

[Wei Liu]

On Fri, Mar 09, 2018 at 02:14:48PM +0000, Andrew Cooper wrote:
> On 09/03/18 14:12, Wei Liu wrote:
> > On Fri, Mar 09, 2018 at 01:18:37PM +0000, Andrew Cooper wrote:
> >> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> >> index 50f7422..a73e1a4 100644
> >> --- a/xen/common/domctl.c
> >> +++ b/xen/common/domctl.c
> >> @@ -498,7 +498,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >>      {
> >>          domid_t        dom;
> >>          static domid_t rover = 0;
> >> -        unsigned int domcr_flags;
> >>  
> >>          ret = -EINVAL;
> >>          if ( (op->u.createdomain.flags &
> >> @@ -533,19 +532,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >>              rover = dom;
> >>          }
> >>  
> >> -        domcr_flags = 0;
> >> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hvm_guest )
> >> -            domcr_flags |= DOMCRF_hvm;
> >> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hap )
> >> -            domcr_flags |= DOMCRF_hap;
> >> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_s3_integrity )
> >> -            domcr_flags |= DOMCRF_s3_integrity;
> >> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_oos_off )
> >> -            domcr_flags |= DOMCRF_oos_off;
> >> -        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
> >> -            domcr_flags |= DOMCRF_xs_domain;
> >> -
> > Please consider adding a test to reject flags Xen doesn't recognise.
> 
> There already is, between these two hunks. :)
> 

Yes, indeed.

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

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

Re: [PATCH 2/7] xen/domain: Drop all DOMCRF_* constants

[Jan Beulich]

>>> On 09.03.18 at 15:16, <wei.liu2@citrix.com> wrote:
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 2/7] xen/domain: Drop all DOMCRF_* constants

[Julien Grall]

Hi Andre,

On 03/09/2018 01:18 PM, Andrew Cooper wrote:
> With DOMCRF_dummy removed, all remaining DOMCRF_* idenitcally match their

NIT: s/idenitcally/identically/

> DOMCTL counterparts.  Avoid having a conversion between two different bit
> layouts, and use the DOMCTL_CDF_* constants everywhere.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Cheers,

-- 
Julien Grall

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

[PATCH 3/7] RFC arm/domain: Reject invalid combinations of domain creation flags

[Andrew Cooper]

ARM guests are HVM and have hardware assisted paging.  There are no PV guests
or shadow paging, and all other creation flags are x86 specific.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

RFC.  This is untested, but I noticed it when putting together the preceeding
patch.  There is a moderate chance that this will cause things to explode
because of how libxl handles ARM guest construction, but something along these
lines is the right thing to do.
---
 xen/arch/arm/domain.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8de4c0a..291c282 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -585,6 +585,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( is_idle_domain(d) )
         return 0;
 
+    if ( domcr_flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
+        return -EINVAL;
+
     ASSERT(config != NULL);
 
     /* p2m_init relies on some value initialized by the IOMMU subsystem */
-- 
2.1.4


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

Re: [PATCH 3/7] RFC arm/domain: Reject invalid combinations of domain creation flags

[Julien Grall]

Hi Andrew,

On 03/09/2018 01:18 PM, Andrew Cooper wrote:
> ARM guests are HVM and have hardware assisted paging.  There are no PV guests
> or shadow paging, and all other creation flags are x86 specific.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> RFC.  This is untested, but I noticed it when putting together the preceeding
> patch.  There is a moderate chance that this will cause things to explode
> because of how libxl handles ARM guest construction, but something along these
> lines is the right thing to do.

Tools and hypervisor are considering ARM guests as PV. So this patch is 
going to break boot. There are an action (XEN-102) to move ARM guests to 
behave more like PVH from the tools POV. I am not sure when I will have 
time to look at it thought.

For the time being, I am wondering if we could override the flags for 
Arm in the toolstack?

Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/7] RFC arm/domain: Reject invalid combinations of domain creation flags

[Wei Liu]

On Sun, Mar 11, 2018 at 07:59:16PM +0000, Julien Grall wrote:
> Hi Andrew,
> 
> On 03/09/2018 01:18 PM, Andrew Cooper wrote:
> > ARM guests are HVM and have hardware assisted paging.  There are no PV guests
> > or shadow paging, and all other creation flags are x86 specific.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Stefano Stabellini <sstabellini@kernel.org>
> > CC: Julien Grall <julien.grall@arm.com>
> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > 
> > RFC.  This is untested, but I noticed it when putting together the preceeding
> > patch.  There is a moderate chance that this will cause things to explode
> > because of how libxl handles ARM guest construction, but something along these
> > lines is the right thing to do.
> 
> Tools and hypervisor are considering ARM guests as PV. So this patch is
> going to break boot. There are an action (XEN-102) to move ARM guests to
> behave more like PVH from the tools POV. I am not sure when I will have time
> to look at it thought.
> 
> For the time being, I am wondering if we could override the flags for Arm in
> the toolstack?
> 

Is that necessary? I don't think the rest of this series will break ARM
at first glance.

Wei.

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

Re: [PATCH 3/7] RFC arm/domain: Reject invalid combinations of domain creation flags

[Julien Grall]

Hi,

On 12/03/18 16:32, Wei Liu wrote:
> On Sun, Mar 11, 2018 at 07:59:16PM +0000, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 03/09/2018 01:18 PM, Andrew Cooper wrote:
>>> ARM guests are HVM and have hardware assisted paging.  There are no PV guests
>>> or shadow paging, and all other creation flags are x86 specific.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>
>>> RFC.  This is untested, but I noticed it when putting together the preceeding
>>> patch.  There is a moderate chance that this will cause things to explode
>>> because of how libxl handles ARM guest construction, but something along these
>>> lines is the right thing to do.
>>
>> Tools and hypervisor are considering ARM guests as PV. So this patch is
>> going to break boot. There are an action (XEN-102) to move ARM guests to
>> behave more like PVH from the tools POV. I am not sure when I will have time
>> to look at it thought.
>>
>> For the time being, I am wondering if we could override the flags for Arm in
>> the toolstack?
>>
> 
> Is that necessary? I don't think the rest of this series will break ARM
> at first glance.

AFAICT, the rest of the series will not break ARM. So I think it would 
be fine to just drop this patch for the time being.
Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/7] RFC arm/domain: Reject invalid combinations of domain creation flags

[Andrew Cooper]

On 13/03/18 14:42, Julien Grall wrote:
> Hi,
>
> On 12/03/18 16:32, Wei Liu wrote:
>> On Sun, Mar 11, 2018 at 07:59:16PM +0000, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 03/09/2018 01:18 PM, Andrew Cooper wrote:
>>>> ARM guests are HVM and have hardware assisted paging.  There are no
>>>> PV guests
>>>> or shadow paging, and all other creation flags are x86 specific.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>>
>>>> RFC.  This is untested, but I noticed it when putting together the
>>>> preceeding
>>>> patch.  There is a moderate chance that this will cause things to
>>>> explode
>>>> because of how libxl handles ARM guest construction, but something
>>>> along these
>>>> lines is the right thing to do.
>>>
>>> Tools and hypervisor are considering ARM guests as PV. So this patch is
>>> going to break boot. There are an action (XEN-102) to move ARM
>>> guests to
>>> behave more like PVH from the tools POV. I am not sure when I will
>>> have time
>>> to look at it thought.
>>>
>>> For the time being, I am wondering if we could override the flags
>>> for Arm in
>>> the toolstack?
>>>
>>
>> Is that necessary? I don't think the rest of this series will break ARM
>> at first glance.
>
> AFAICT, the rest of the series will not break ARM. So I think it would
> be fine to just drop this patch for the time being.

I'll drop the patch for now.  This was just meant to get the problem on
peoples radar.

~Andrew

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

[PATCH 4/7] x86/domain: Remove unused parameters from {hvm, pv}_domain_initialise()

[Andrew Cooper]

Neither domcr_flags nor config are used on either side.  Drop them, making
{hvm,pv}_domain_initialise() symmetric with all the other domain/vcpu
initialise/destroy calls.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/domain.c           |  4 ++--
 xen/arch/x86/hvm/hvm.c          |  3 +--
 xen/arch/x86/pv/domain.c        |  3 +--
 xen/include/asm-x86/hvm/hvm.h   |  3 +--
 xen/include/asm-x86/pv/domain.h | 11 +++--------
 5 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 36555ea..81ee992 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -538,7 +538,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
 
     if ( is_hvm_domain(d) )
     {
-        if ( (rc = hvm_domain_initialise(d, domcr_flags, config)) != 0 )
+        if ( (rc = hvm_domain_initialise(d)) != 0 )
             goto fail;
     }
     else if ( is_idle_domain(d) )
@@ -553,7 +553,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     }
     else
     {
-        if ( (rc = pv_domain_initialise(d, domcr_flags, config)) != 0 )
+        if ( (rc = pv_domain_initialise(d)) != 0 )
             goto fail;
     }
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4618664..b3a6e1f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -568,8 +568,7 @@ static int hvm_print_line(
     return X86EMUL_OKAY;
 }
 
-int hvm_domain_initialise(struct domain *d, unsigned long domcr_flags,
-                          struct xen_arch_domainconfig *config)
+int hvm_domain_initialise(struct domain *d)
 {
     unsigned int nr_gsis;
     int rc;
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 2c784fb..01c62e2 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -177,8 +177,7 @@ void pv_domain_destroy(struct domain *d)
 }
 
 
-int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
-                         struct xen_arch_domainconfig *config)
+int pv_domain_initialise(struct domain *d)
 {
     static const struct arch_csw pv_csw = {
         .from = paravirt_ctxt_switch_from,
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 9aa6c72..2376ed6 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -240,8 +240,7 @@ extern s8 hvm_port80_allowed;
 extern const struct hvm_function_table *start_svm(void);
 extern const struct hvm_function_table *start_vmx(void);
 
-int hvm_domain_initialise(struct domain *d, unsigned long domcr_flags,
-                          struct xen_arch_domainconfig *config);
+int hvm_domain_initialise(struct domain *d);
 void hvm_domain_relinquish_resources(struct domain *d);
 void hvm_domain_destroy(struct domain *d);
 void hvm_domain_soft_reset(struct domain *d);
diff --git a/xen/include/asm-x86/pv/domain.h b/xen/include/asm-x86/pv/domain.h
index acdf140..5e34176 100644
--- a/xen/include/asm-x86/pv/domain.h
+++ b/xen/include/asm-x86/pv/domain.h
@@ -26,8 +26,7 @@
 void pv_vcpu_destroy(struct vcpu *v);
 int pv_vcpu_initialise(struct vcpu *v);
 void pv_domain_destroy(struct domain *d);
-int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
-                         struct xen_arch_domainconfig *config);
+int pv_domain_initialise(struct domain *d);
 
 #else  /* !CONFIG_PV */
 
@@ -36,12 +35,8 @@ int pv_domain_initialise(struct domain *d, unsigned int domcr_flags,
 static inline void pv_vcpu_destroy(struct vcpu *v) {}
 static inline int pv_vcpu_initialise(struct vcpu *v) { return -EOPNOTSUPP; }
 static inline void pv_domain_destroy(struct domain *d) {}
-static inline int pv_domain_initialise(struct domain *d,
-                                       unsigned int domcr_flags,
-                                       struct xen_arch_domainconfig *config);
-{
-    return -EOPNOTSUPP;
-}
+static inline int pv_domain_initialise(struct domain *d) { return -EOPNOTSUPP; }
+
 #endif	/* CONFIG_PV */
 
 void paravirt_ctxt_switch_from(struct vcpu *v);
-- 
2.1.4


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

Re: [PATCH 4/7] x86/domain: Remove unused parameters from {hvm, pv}_domain_initialise()

[Wei Liu]

On Fri, Mar 09, 2018 at 01:18:39PM +0000, Andrew Cooper wrote:
> Neither domcr_flags nor config are used on either side.  Drop them, making
> {hvm,pv}_domain_initialise() symmetric with all the other domain/vcpu
> initialise/destroy calls.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 4/7] x86/domain: Remove unused parameters from {hvm, pv}_domain_initialise()

[Jan Beulich]

>>> On 09.03.18 at 15:13, <wei.liu2@citrix.com> wrote:
> On Fri, Mar 09, 2018 at 01:18:39PM +0000, Andrew Cooper wrote:
>> Neither domcr_flags nor config are used on either side.  Drop them, making
>> {hvm,pv}_domain_initialise() symmetric with all the other domain/vcpu
>> initialise/destroy calls.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 4/7] x86/domain: Remove unused parameters from {hvm, pv}_domain_initialise()

[Roger Pau Monné]

On Fri, Mar 09, 2018 at 01:18:39PM +0000, Andrew Cooper wrote:
> Neither domcr_flags nor config are used on either side.  Drop them, making
> {hvm,pv}_domain_initialise() symmetric with all the other domain/vcpu
> initialise/destroy calls.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/domain.c           |  4 ++--
>  xen/arch/x86/hvm/hvm.c          |  3 +--
>  xen/arch/x86/pv/domain.c        |  3 +--
>  xen/include/asm-x86/hvm/hvm.h   |  3 +--
>  xen/include/asm-x86/pv/domain.h | 11 +++--------
>  5 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 36555ea..81ee992 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -538,7 +538,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>  
>      if ( is_hvm_domain(d) )
>      {
> -        if ( (rc = hvm_domain_initialise(d, domcr_flags, config)) != 0 )
> +        if ( (rc = hvm_domain_initialise(d)) != 0 )
>              goto fail;
>      }
>      else if ( is_idle_domain(d) )
> @@ -553,7 +553,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>      }
>      else
>      {
> -        if ( (rc = pv_domain_initialise(d, domcr_flags, config)) != 0 )
> +        if ( (rc = pv_domain_initialise(d)) != 0 )
>              goto fail;
>      }

Maybe this could be:

if ( is_idle_domain(d) )
...
else
{
    rc = is_hvm_domain(d) ? hvm_domain_initialise(d)
                          : pv_domain_initialise(d);
    if ( rc )
        goto fail;
}

But that's maybe out of the scope of this patch.

Thanks, Roger.

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

Re: [PATCH 4/7] x86/domain: Remove unused parameters from {hvm, pv}_domain_initialise()

[Andrew Cooper]

On 13/03/18 12:05, Roger Pau Monné wrote:
> Maybe this could be:
>
> if ( is_idle_domain(d) )
> ...
> else
> {
>     rc = is_hvm_domain(d) ? hvm_domain_initialise(d)
>                           : pv_domain_initialise(d);
>     if ( rc )
>         goto fail;
> }
>
> But that's maybe out of the scope of this patch.

I'd actually like to reconsider our use of this design pattern.

As far as I can tell from some XTF examples, it forces the use of a
function pointer rather than an "if call else call" which in turn
forcibly out-of-lines static inline stubs, and prevents LTO from merging
a cross TU call into its sole caller.

While the code does does look slightly neater as a result, I get the
feeling that

if ( is_hvm_domain(d) )
    rc = hvm_domain_initialise(d);
else
    rc = pv_domain_initialise(d);

is far easier for the compiler to optimise when the opportunities arise.

~Andrew

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

Re: [PATCH 4/7] x86/domain: Remove unused parameters from {hvm, pv}_domain_initialise()

[Jan Beulich]

>>> On 15.03.18 at 21:09, <andrew.cooper3@citrix.com> wrote:
> On 13/03/18 12:05, Roger Pau Monné wrote:
>> Maybe this could be:
>>
>> if ( is_idle_domain(d) )
>> ...
>> else
>> {
>>     rc = is_hvm_domain(d) ? hvm_domain_initialise(d)
>>                           : pv_domain_initialise(d);
>>     if ( rc )
>>         goto fail;
>> }
>>
>> But that's maybe out of the scope of this patch.
> 
> I'd actually like to reconsider our use of this design pattern.
> 
> As far as I can tell from some XTF examples, it forces the use of a
> function pointer rather than an "if call else call" which in turn
> forcibly out-of-lines static inline stubs, and prevents LTO from merging
> a cross TU call into its sole caller.

Where's the function pointer coming from? I could see that happening
with

     rc = (is_hvm_domain(d) ? hvm_domain_initialise
                            : pv_domain_initialise)(d);

which I think Roger has been using variants of in a few other
patches of his (and which I've been debating with myself whether
to comment on while reviewing those patches).

Jan

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

[PATCH 5/7] x86/domain: Optimise the order of actions in arch_domain_create()

[Andrew Cooper]

The only relevent initialisation for the idle domain is the context switch and
poisoned pointers.  Collect these bits together early in the function and exit
when complete (although as a consequence, the e820 and vtsc lock
initialisation are moved forwards).  This allows us to remove subsequent
is_idle_domain() checks and unindent most of the logic.

Furthermore, we no longer call these functions for the idle domain:
 * mapcache_domain_init() and tsc_set_info() were previously guarded against
   the idle domain, and have had their guards turned into ASSERT()s.
 * pit_init() is implicitly guarded by has_vpit().
 * psr_domain_init() no longer allocates a socket array.

Finally, two changes are introduced for the benefit of the following patch:
 * For PV hardware domains, or XEN_X86_EMU_PIT into emflags rather than into
   config->emulation_flags, to facilitating config becoming const.
 * References to domcr_flags are moved until after the idle early exist, to
   facilitiate them being unavailable for system domains.

No practical change in behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/domain.c      | 152 ++++++++++++++++++++++-----------------------
 xen/arch/x86/domain_page.c |   3 +-
 xen/arch/x86/time.c        |   4 +-
 3 files changed, 78 insertions(+), 81 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 81ee992..48dc2b9 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -430,20 +430,37 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
     bool paging_initialised = false;
+    uint32_t emflags;
     int rc;
 
-    if ( config == NULL && !is_idle_domain(d) )
-        return -EINVAL;
-
-    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
-
     INIT_LIST_HEAD(&d->arch.pdev_list);
 
     d->arch.relmem = RELMEM_not_started;
     INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
 
-    if ( d->domain_id && !is_idle_domain(d) &&
-         cpu_has_amd_erratum(&boot_cpu_data, AMD_ERRATUM_121) )
+    spin_lock_init(&d->arch.e820_lock);
+    spin_lock_init(&d->arch.vtsc_lock);
+
+    /* Minimal initialisation for the idle domain. */
+    if ( unlikely(is_idle_domain(d)) )
+    {
+        static const struct arch_csw idle_csw = {
+            .from = paravirt_ctxt_switch_from,
+            .to   = paravirt_ctxt_switch_to,
+            .tail = continue_idle_domain,
+        };
+
+        d->arch.ctxt_switch = &idle_csw;
+
+        d->arch.cpuid = ZERO_BLOCK_PTR; /* Catch stray misuses. */
+        d->arch.msr = ZERO_BLOCK_PTR;
+
+        return 0;
+    }
+    else if ( !config )
+        return -EINVAL;
+
+    if ( d->domain_id && cpu_has_amd_erratum(&boot_cpu_data, AMD_ERRATUM_121) )
     {
         if ( !opt_allow_unsafe )
         {
@@ -456,83 +473,69 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                d->domain_id);
     }
 
-    if ( is_idle_domain(d) )
-    {
-        d->arch.emulation_flags = 0;
-        d->arch.cpuid = ZERO_BLOCK_PTR; /* Catch stray misuses. */
-        d->arch.msr = ZERO_BLOCK_PTR;
-    }
-    else
-    {
-        uint32_t emflags;
+    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
 
-        if ( is_hardware_domain(d) && is_pv_domain(d) )
-            config->emulation_flags |= XEN_X86_EMU_PIT;
+    emflags = config->emulation_flags;
 
-        emflags = config->emulation_flags;
-        if ( emflags & ~XEN_X86_EMU_ALL )
-        {
-            printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x\n",
-                   d->domain_id, emflags);
-            return -EINVAL;
-        }
+    if ( is_hardware_domain(d) && is_pv_domain(d) )
+        emflags |= XEN_X86_EMU_PIT;
 
-        if ( !emulation_flags_ok(d, emflags) )
-        {
-            printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation "
-                   "with the current selection of emulators: %#x\n",
-                   d->domain_id, is_hvm_domain(d) ? "HVM" : "PV", emflags);
-            return -EOPNOTSUPP;
-        }
-        d->arch.emulation_flags = emflags;
+    if ( emflags & ~XEN_X86_EMU_ALL )
+    {
+        printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x\n",
+               d->domain_id, emflags);
+        return -EINVAL;
     }
 
-    mapcache_domain_init(d);
+    if ( !emulation_flags_ok(d, emflags) )
+    {
+        printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation "
+               "with the current selection of emulators: %#x\n",
+               d->domain_id, is_hvm_domain(d) ? "HVM" : "PV", emflags);
+        return -EOPNOTSUPP;
+    }
+    d->arch.emulation_flags = emflags;
 
     HYPERVISOR_COMPAT_VIRT_START(d) =
         is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
 
-    if ( !is_idle_domain(d) )
-    {
-        /* Need to determine if HAP is enabled before initialising paging */
-        if ( is_hvm_domain(d) )
-            d->arch.hvm_domain.hap_enabled =
-                hvm_funcs.hap_supported && (domcr_flags & XEN_DOMCTL_CDF_hap);
+    /* Need to determine if HAP is enabled before initialising paging */
+    if ( is_hvm_domain(d) )
+        d->arch.hvm_domain.hap_enabled =
+            hvm_funcs.hap_supported && (domcr_flags & XEN_DOMCTL_CDF_hap);
 
-        if ( (rc = paging_domain_init(d, domcr_flags)) != 0 )
-            goto fail;
-        paging_initialised = 1;
+    if ( (rc = paging_domain_init(d, domcr_flags)) != 0 )
+        goto fail;
+    paging_initialised = true;
 
-        if ( (rc = init_domain_cpuid_policy(d)) )
-            goto fail;
+    if ( (rc = init_domain_cpuid_policy(d)) )
+        goto fail;
 
-        if ( (rc = init_domain_msr_policy(d)) )
-            goto fail;
+    if ( (rc = init_domain_msr_policy(d)) )
+        goto fail;
 
-        d->arch.ioport_caps = 
-            rangeset_new(d, "I/O Ports", RANGESETF_prettyprint_hex);
-        rc = -ENOMEM;
-        if ( d->arch.ioport_caps == NULL )
-            goto fail;
+    d->arch.ioport_caps =
+        rangeset_new(d, "I/O Ports", RANGESETF_prettyprint_hex);
+    rc = -ENOMEM;
+    if ( d->arch.ioport_caps == NULL )
+        goto fail;
 
-        /*
-         * The shared_info machine address must fit in a 32-bit field within a
-         * 32-bit guest's start_info structure. Hence we specify MEMF_bits(32).
-         */
-        if ( (d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32))) == NULL )
-            goto fail;
+    /*
+     * The shared_info machine address must fit in a 32-bit field within a
+     * 32-bit guest's start_info structure. Hence we specify MEMF_bits(32).
+     */
+    if ( (d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32))) == NULL )
+        goto fail;
 
-        clear_page(d->shared_info);
-        share_xen_page_with_guest(
-            virt_to_page(d->shared_info), d, XENSHARE_writable);
+    clear_page(d->shared_info);
+    share_xen_page_with_guest(
+        virt_to_page(d->shared_info), d, XENSHARE_writable);
 
-        if ( (rc = init_domain_irq_mapping(d)) != 0 )
-            goto fail;
+    if ( (rc = init_domain_irq_mapping(d)) != 0 )
+        goto fail;
 
-        if ( (rc = iommu_domain_init(d)) != 0 )
-            goto fail;
-    }
-    spin_lock_init(&d->arch.e820_lock);
+    if ( (rc = iommu_domain_init(d)) != 0 )
+        goto fail;
 
     psr_domain_init(d);
 
@@ -541,25 +544,18 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         if ( (rc = hvm_domain_initialise(d)) != 0 )
             goto fail;
     }
-    else if ( is_idle_domain(d) )
+    else if ( is_pv_domain(d) )
     {
-        static const struct arch_csw idle_csw = {
-            .from = paravirt_ctxt_switch_from,
-            .to   = paravirt_ctxt_switch_to,
-            .tail = continue_idle_domain,
-        };
+        mapcache_domain_init(d);
 
-        d->arch.ctxt_switch = &idle_csw;
-    }
-    else
-    {
         if ( (rc = pv_domain_initialise(d)) != 0 )
             goto fail;
     }
+    else
+        ASSERT_UNREACHABLE(); /* Not HVM and not PV? */
 
     /* initialize default tsc behavior in case tools don't */
     tsc_set_info(d, TSC_MODE_DEFAULT, 0UL, 0, 0);
-    spin_lock_init(&d->arch.vtsc_lock);
 
     /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
     pit_init(d, cpu_khz);
diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
index 3432a85..b5780f2 100644
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -236,8 +236,7 @@ int mapcache_domain_init(struct domain *d)
     struct mapcache_domain *dcache = &d->arch.pv_domain.mapcache;
     unsigned int bitmap_pages;
 
-    if ( !is_pv_domain(d) || is_idle_domain(d) )
-        return 0;
+    ASSERT(is_pv_domain(d));
 
 #ifdef NDEBUG
     if ( !mem_hotplug && max_page <= PFN_DOWN(__pa(HYPERVISOR_VIRT_END - 1)) )
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 1a6fde6..84c1c0c 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2124,7 +2124,9 @@ void tsc_set_info(struct domain *d,
                   uint32_t tsc_mode, uint64_t elapsed_nsec,
                   uint32_t gtsc_khz, uint32_t incarnation)
 {
-    if ( is_idle_domain(d) || is_hardware_domain(d) )
+    ASSERT(!is_system_domain(d));
+
+    if ( is_hardware_domain(d) )
     {
         d->arch.vtsc = 0;
         return;
-- 
2.1.4


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

Re: [PATCH 5/7] x86/domain: Optimise the order of actions in arch_domain_create()

[Wei Liu]

On Fri, Mar 09, 2018 at 01:18:40PM +0000, Andrew Cooper wrote:
> The only relevent initialisation for the idle domain is the context switch and
> poisoned pointers.  Collect these bits together early in the function and exit
> when complete (although as a consequence, the e820 and vtsc lock
> initialisation are moved forwards).  This allows us to remove subsequent
> is_idle_domain() checks and unindent most of the logic.
> 
> Furthermore, we no longer call these functions for the idle domain:
>  * mapcache_domain_init() and tsc_set_info() were previously guarded against
>    the idle domain, and have had their guards turned into ASSERT()s.
>  * pit_init() is implicitly guarded by has_vpit().
>  * psr_domain_init() no longer allocates a socket array.
> 
> Finally, two changes are introduced for the benefit of the following patch:
>  * For PV hardware domains, or XEN_X86_EMU_PIT into emflags rather than into
>    config->emulation_flags, to facilitating config becoming const.
>  * References to domcr_flags are moved until after the idle early exist, to
>    facilitiate them being unavailable for system domains.
> 
> No practical change in behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 5/7] x86/domain: Optimise the order of actions in arch_domain_create()

[Jan Beulich]

>>> On 09.03.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -430,20 +430,37 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>                         struct xen_arch_domainconfig *config)
>  {
>      bool paging_initialised = false;
> +    uint32_t emflags;
>      int rc;
>  
> -    if ( config == NULL && !is_idle_domain(d) )
> -        return -EINVAL;
> -
> -    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
> -
>      INIT_LIST_HEAD(&d->arch.pdev_list);
>  
>      d->arch.relmem = RELMEM_not_started;
>      INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
>  
> -    if ( d->domain_id && !is_idle_domain(d) &&
> -         cpu_has_amd_erratum(&boot_cpu_data, AMD_ERRATUM_121) )
> +    spin_lock_init(&d->arch.e820_lock);
> +    spin_lock_init(&d->arch.vtsc_lock);
> +
> +    /* Minimal initialisation for the idle domain. */
> +    if ( unlikely(is_idle_domain(d)) )
> +    {
> +        static const struct arch_csw idle_csw = {
> +            .from = paravirt_ctxt_switch_from,
> +            .to   = paravirt_ctxt_switch_to,
> +            .tail = continue_idle_domain,
> +        };
> +
> +        d->arch.ctxt_switch = &idle_csw;
> +
> +        d->arch.cpuid = ZERO_BLOCK_PTR; /* Catch stray misuses. */
> +        d->arch.msr = ZERO_BLOCK_PTR;
> +
> +        return 0;
> +    }
> +    else if ( !config )

May I suggest to avoid the "else" here? Other than that and with
Wei's R-b
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 5/7] x86/domain: Optimise the order of actions in arch_domain_create()

[Andrew Cooper]

On 09/03/18 16:54, Jan Beulich wrote:
>>>> On 09.03.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -430,20 +430,37 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>                         struct xen_arch_domainconfig *config)
>>  {
>>      bool paging_initialised = false;
>> +    uint32_t emflags;
>>      int rc;
>>  
>> -    if ( config == NULL && !is_idle_domain(d) )
>> -        return -EINVAL;
>> -
>> -    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
>> -
>>      INIT_LIST_HEAD(&d->arch.pdev_list);
>>  
>>      d->arch.relmem = RELMEM_not_started;
>>      INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
>>  
>> -    if ( d->domain_id && !is_idle_domain(d) &&
>> -         cpu_has_amd_erratum(&boot_cpu_data, AMD_ERRATUM_121) )
>> +    spin_lock_init(&d->arch.e820_lock);
>> +    spin_lock_init(&d->arch.vtsc_lock);
>> +
>> +    /* Minimal initialisation for the idle domain. */
>> +    if ( unlikely(is_idle_domain(d)) )
>> +    {
>> +        static const struct arch_csw idle_csw = {
>> +            .from = paravirt_ctxt_switch_from,
>> +            .to   = paravirt_ctxt_switch_to,
>> +            .tail = continue_idle_domain,
>> +        };
>> +
>> +        d->arch.ctxt_switch = &idle_csw;
>> +
>> +        d->arch.cpuid = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>> +        d->arch.msr = ZERO_BLOCK_PTR;
>> +
>> +        return 0;
>> +    }
>> +    else if ( !config )
> May I suggest to avoid the "else" here?

I've gone with

...
        return 0;
    }

    if ( !config )
    {
        /* Only IDLE is allowed with no config. */
        ASSERT_UNREACHABLE();
        return -EINVAL;
    }
...

For runtime safety and debug sanity.

~Andrew

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

Re: [PATCH 5/7] x86/domain: Optimise the order of actions in arch_domain_create()

[Jan Beulich]

>>> On 15.03.18 at 21:15, <andrew.cooper3@citrix.com> wrote:
> On 09/03/18 16:54, Jan Beulich wrote:
>>>>> On 09.03.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -430,20 +430,37 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>>                         struct xen_arch_domainconfig *config)
>>>  {
>>>      bool paging_initialised = false;
>>> +    uint32_t emflags;
>>>      int rc;
>>>  
>>> -    if ( config == NULL && !is_idle_domain(d) )
>>> -        return -EINVAL;
>>> -
>>> -    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
>>> -
>>>      INIT_LIST_HEAD(&d->arch.pdev_list);
>>>  
>>>      d->arch.relmem = RELMEM_not_started;
>>>      INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
>>>  
>>> -    if ( d->domain_id && !is_idle_domain(d) &&
>>> -         cpu_has_amd_erratum(&boot_cpu_data, AMD_ERRATUM_121) )
>>> +    spin_lock_init(&d->arch.e820_lock);
>>> +    spin_lock_init(&d->arch.vtsc_lock);
>>> +
>>> +    /* Minimal initialisation for the idle domain. */
>>> +    if ( unlikely(is_idle_domain(d)) )
>>> +    {
>>> +        static const struct arch_csw idle_csw = {
>>> +            .from = paravirt_ctxt_switch_from,
>>> +            .to   = paravirt_ctxt_switch_to,
>>> +            .tail = continue_idle_domain,
>>> +        };
>>> +
>>> +        d->arch.ctxt_switch = &idle_csw;
>>> +
>>> +        d->arch.cpuid = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>>> +        d->arch.msr = ZERO_BLOCK_PTR;
>>> +
>>> +        return 0;
>>> +    }
>>> +    else if ( !config )
>> May I suggest to avoid the "else" here?
> 
> I've gone with
> 
> ...
>         return 0;
>     }
> 
>     if ( !config )
>     {
>         /* Only IDLE is allowed with no config. */
>         ASSERT_UNREACHABLE();
>         return -EINVAL;
>     }
> ...
> 
> For runtime safety and debug sanity.

Ah, yes, that's even better.

Jan


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

Re: [PATCH 5/7] x86/domain: Optimise the order of actions in arch_domain_create()

[Roger Pau Monné]

On Fri, Mar 09, 2018 at 01:18:40PM +0000, Andrew Cooper wrote:
> The only relevent initialisation for the idle domain is the context switch and
           ^ relevant
> poisoned pointers.  Collect these bits together early in the function and exit
> when complete (although as a consequence, the e820 and vtsc lock
> initialisation are moved forwards).  This allows us to remove subsequent
> is_idle_domain() checks and unindent most of the logic.
> 
> Furthermore, we no longer call these functions for the idle domain:
>  * mapcache_domain_init() and tsc_set_info() were previously guarded against
>    the idle domain, and have had their guards turned into ASSERT()s.
>  * pit_init() is implicitly guarded by has_vpit().
>  * psr_domain_init() no longer allocates a socket array.
> 
> Finally, two changes are introduced for the benefit of the following patch:
>  * For PV hardware domains, or XEN_X86_EMU_PIT into emflags rather than into
>    config->emulation_flags, to facilitating config becoming const.
>  * References to domcr_flags are moved until after the idle early exist, to
                                                                    ^ exit
>    facilitiate them being unavailable for system domains.
> 
> No practical change in behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/domain.c      | 152 ++++++++++++++++++++++-----------------------
>  xen/arch/x86/domain_page.c |   3 +-
>  xen/arch/x86/time.c        |   4 +-
>  3 files changed, 78 insertions(+), 81 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 81ee992..48dc2b9 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -430,20 +430,37 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>                         struct xen_arch_domainconfig *config)
>  {
>      bool paging_initialised = false;
> +    uint32_t emflags;
>      int rc;
>  
> -    if ( config == NULL && !is_idle_domain(d) )
> -        return -EINVAL;
> -
> -    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
> -
>      INIT_LIST_HEAD(&d->arch.pdev_list);
>  
>      d->arch.relmem = RELMEM_not_started;
>      INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
>  
> -    if ( d->domain_id && !is_idle_domain(d) &&
> -         cpu_has_amd_erratum(&boot_cpu_data, AMD_ERRATUM_121) )
> +    spin_lock_init(&d->arch.e820_lock);
> +    spin_lock_init(&d->arch.vtsc_lock);
> +
> +    /* Minimal initialisation for the idle domain. */
> +    if ( unlikely(is_idle_domain(d)) )
> +    {
> +        static const struct arch_csw idle_csw = {
> +            .from = paravirt_ctxt_switch_from,
> +            .to   = paravirt_ctxt_switch_to,
> +            .tail = continue_idle_domain,
> +        };
> +
> +        d->arch.ctxt_switch = &idle_csw;
> +
> +        d->arch.cpuid = ZERO_BLOCK_PTR; /* Catch stray misuses. */
> +        d->arch.msr = ZERO_BLOCK_PTR;
> +
> +        return 0;
> +    }
> +    else if ( !config )

Unneeded else.

Thanks, Roger.

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

[PATCH 6/7] xen/domain: Pass the full domctl_createdomain struct to create_domain()

[Andrew Cooper]

In future patches, the structure will be extended with further information,
and this is far cleaner than adding extra parameters.

One minor tweak is that the setting of guest_type needs to be deferred until
config is known-good to dereference, but this doesn't result in any changed
behaviour as system domains never used to pass XEN_DOMCTL_CDF_hvm_guest.

Also for completeness, move the setting of d->handle into the tail of
domain_create() where it more logically should live.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/domain.c    | 16 ++++++++--------
 xen/arch/arm/mm.c        |  6 +++---
 xen/arch/arm/setup.c     |  8 ++++----
 xen/arch/x86/domain.c    | 12 ++++++------
 xen/arch/x86/mm.c        |  6 +++---
 xen/arch/x86/setup.c     | 18 +++++++++++-------
 xen/common/domain.c      | 31 ++++++++++++++++++++-----------
 xen/common/domctl.c      |  8 +-------
 xen/common/schedule.c    |  2 +-
 xen/include/xen/domain.h |  4 ++--
 xen/include/xen/sched.h  |  5 ++---
 11 files changed, 61 insertions(+), 55 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 291c282..4b45fad 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -573,8 +573,8 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
     v->arch.hcr_el2 |= HCR_RW;
 }
 
-int arch_domain_create(struct domain *d, unsigned int domcr_flags,
-                       struct xen_arch_domainconfig *config)
+int arch_domain_create(struct domain *d,
+                       struct xen_domctl_createdomain *config)
 {
     int rc, count = 0;
 
@@ -585,7 +585,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( is_idle_domain(d) )
         return 0;
 
-    if ( domcr_flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
+    if ( config->flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
         return -EINVAL;
 
     ASSERT(config != NULL);
@@ -605,18 +605,18 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     share_xen_page_with_guest(
         virt_to_page(d->shared_info), d, XENSHARE_writable);
 
-    switch ( config->gic_version )
+    switch ( config->config.gic_version )
     {
     case XEN_DOMCTL_CONFIG_GIC_NATIVE:
         switch ( gic_hw_version () )
         {
         case GIC_V2:
-            config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
+            config->config.gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
             d->arch.vgic.version = GIC_V2;
             break;
 
         case GIC_V3:
-            config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
+            config->config.gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
             d->arch.vgic.version = GIC_V3;
             break;
 
@@ -644,10 +644,10 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 )
         goto fail;
 
-    if ( (rc = domain_vgic_init(d, config->nr_spis)) != 0 )
+    if ( (rc = domain_vgic_init(d, config->config.nr_spis)) != 0 )
         goto fail;
 
-    if ( (rc = domain_vtimer_init(d, config)) != 0 )
+    if ( (rc = domain_vtimer_init(d, &config->config)) != 0 )
         goto fail;
 
     update_domain_wallclock_time(d);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ce83f69..a09bea2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -520,7 +520,7 @@ void __init arch_init_memory(void)
      * Any Xen-heap pages that we will allow to be mapped will have
      * their domain field set to dom_xen.
      */
-    dom_xen = domain_create(DOMID_XEN, 0, 0, NULL);
+    dom_xen = domain_create(DOMID_XEN, NULL);
     BUG_ON(IS_ERR(dom_xen));
 
     /*
@@ -528,14 +528,14 @@ void __init arch_init_memory(void)
      * This domain owns I/O pages that are within the range of the page_info
      * array. Mappings occur at the priv of the caller.
      */
-    dom_io = domain_create(DOMID_IO, 0, 0, NULL);
+    dom_io = domain_create(DOMID_IO, NULL);
     BUG_ON(IS_ERR(dom_io));
 
     /*
      * Initialise our COW domain.
      * This domain owns sharable pages.
      */
-    dom_cow = domain_create(DOMID_COW, 0, 0, NULL);
+    dom_cow = domain_create(DOMID_COW, NULL);
     BUG_ON(IS_ERR(dom_cow));
 }
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4627366..b17797d 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -693,7 +693,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     const char *cmdline;
     struct bootmodule *xen_bootmodule;
     struct domain *dom0;
-    struct xen_arch_domainconfig config;
+    struct xen_domctl_createdomain dom0_cfg = {};
 
     dcache_line_bytes = read_dcache_line_bytes();
 
@@ -840,10 +840,10 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     /* Create initial domain 0. */
     /* The vGIC for DOM0 is exactly emulating the hardware GIC */
-    config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
-    config.nr_spis = gic_number_lines() - 32;
+    dom0_cfg.config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
+    dom0_cfg.config.nr_spis = gic_number_lines() - 32;
 
-    dom0 = domain_create(0, 0, 0, &config);
+    dom0 = domain_create(0, &dom0_cfg);
     if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
             panic("Error creating domain 0");
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 48dc2b9..12d0766 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -426,8 +426,8 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
     return true;
 }
 
-int arch_domain_create(struct domain *d, unsigned int domcr_flags,
-                       struct xen_arch_domainconfig *config)
+int arch_domain_create(struct domain *d,
+                       struct xen_domctl_createdomain *config)
 {
     bool paging_initialised = false;
     uint32_t emflags;
@@ -473,9 +473,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                d->domain_id);
     }
 
-    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
+    d->arch.s3_integrity = config->flags & XEN_DOMCTL_CDF_s3_integrity;
 
-    emflags = config->emulation_flags;
+    emflags = config->config.emulation_flags;
 
     if ( is_hardware_domain(d) && is_pv_domain(d) )
         emflags |= XEN_X86_EMU_PIT;
@@ -502,9 +502,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     /* Need to determine if HAP is enabled before initialising paging */
     if ( is_hvm_domain(d) )
         d->arch.hvm_domain.hap_enabled =
-            hvm_funcs.hap_supported && (domcr_flags & XEN_DOMCTL_CDF_hap);
+            hvm_funcs.hap_supported && (config->flags & XEN_DOMCTL_CDF_hap);
 
-    if ( (rc = paging_domain_init(d, domcr_flags)) != 0 )
+    if ( (rc = paging_domain_init(d, config->flags)) != 0 )
         goto fail;
     paging_initialised = true;
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c275d4b..1d4e396 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -271,7 +271,7 @@ void __init arch_init_memory(void)
      * Hidden PCI devices will also be associated with this domain
      * (but be [partly] controlled by Dom0 nevertheless).
      */
-    dom_xen = domain_create(DOMID_XEN, 0, 0, NULL);
+    dom_xen = domain_create(DOMID_XEN, NULL);
     BUG_ON(IS_ERR(dom_xen));
     INIT_LIST_HEAD(&dom_xen->arch.pdev_list);
 
@@ -280,14 +280,14 @@ void __init arch_init_memory(void)
      * This domain owns I/O pages that are within the range of the page_info
      * array. Mappings occur at the priv of the caller.
      */
-    dom_io = domain_create(DOMID_IO, 0, 0, NULL);
+    dom_io = domain_create(DOMID_IO, NULL);
     BUG_ON(IS_ERR(dom_io));
 
     /*
      * Initialise our COW domain.
      * This domain owns sharable pages.
      */
-    dom_cow = domain_create(DOMID_COW, 0, 0, NULL);
+    dom_cow = domain_create(DOMID_COW, NULL);
     BUG_ON(IS_ERR(dom_cow));
 
     /*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a6dc5df..a4405b4 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -660,7 +660,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 {
     char *memmap_type = NULL;
     char *cmdline, *kextra, *loader;
-    unsigned int initrdidx, domcr_flags = XEN_DOMCTL_CDF_s3_integrity;
+    unsigned int initrdidx;
     multiboot_info_t *mbi;
     module_t *mod;
     unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
@@ -671,7 +671,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         .parity    = 'n',
         .stop_bits = 1
     };
-    struct xen_arch_domainconfig config = { .emulation_flags = 0 };
+    struct xen_domctl_createdomain dom0_cfg = {
+        .flags = XEN_DOMCTL_CDF_s3_integrity,
+    };
 
     /* Critical region without IDT or TSS.  Any fault is deadly! */
 
@@ -1632,14 +1634,16 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     if ( dom0_pvh )
     {
-        domcr_flags |= XEN_DOMCTL_CDF_hvm_guest |
-                       ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
-                         XEN_DOMCTL_CDF_hap : 0);
-        config.emulation_flags = XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC;
+        dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm_guest |
+                           ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
+                            XEN_DOMCTL_CDF_hap : 0));
+
+        dom0_cfg.config.emulation_flags =
+            XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC;
     }
 
     /* Create initial domain 0. */
-    dom0 = domain_create(get_initial_domain_id(), domcr_flags, 0, &config);
+    dom0 = domain_create(get_initial_domain_id(), &dom0_cfg);
     if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
         panic("Error creating domain 0");
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 582e3e5..b00cc1f 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -260,9 +260,8 @@ static int __init parse_extra_guest_irqs(const char *s)
 }
 custom_param("extra_guest_irqs", parse_extra_guest_irqs);
 
-struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
-                             uint32_t ssidref,
-                             struct xen_arch_domainconfig *config)
+struct domain *domain_create(domid_t domid,
+                             struct xen_domctl_createdomain *config)
 {
     struct domain *d, **pd, *old_hwdom = NULL;
     enum { INIT_xsm = 1u<<0, INIT_watchdog = 1u<<1, INIT_rangeset = 1u<<2,
@@ -274,6 +273,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
 
     d->domain_id = domid;
 
+    /* Debug sanity. */
+    ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
+
     TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
 
     lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
@@ -304,11 +306,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     if ( !zalloc_cpumask_var(&d->dirty_cpumask) )
         goto fail;
 
-    if ( domcr_flags & XEN_DOMCTL_CDF_hvm_guest )
-        d->guest_type = guest_type_hvm;
-    else
-        d->guest_type = guest_type_pv;
-
     rangeset_domain_initialise(d);
     init_status |= INIT_rangeset;
 
@@ -318,6 +315,11 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
 
     if ( !is_idle_domain(d) )
     {
+        if ( config->flags & XEN_DOMCTL_CDF_hvm_guest )
+            d->guest_type = guest_type_hvm;
+        else
+            d->guest_type = guest_type_pv;
+
         watchdog_domain_init(d);
         init_status |= INIT_watchdog;
 
@@ -331,7 +333,7 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
             hardware_domain = d;
         }
 
-        if ( domcr_flags & XEN_DOMCTL_CDF_xs_domain )
+        if ( config->flags & XEN_DOMCTL_CDF_xs_domain )
         {
             d->is_xenstore = 1;
             d->disable_migrate = 1;
@@ -342,7 +344,7 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         if ( !d->iomem_caps || !d->irq_caps )
             goto fail;
 
-        if ( (err = xsm_domain_create(XSM_HOOK, d, ssidref)) != 0 )
+        if ( (err = xsm_domain_create(XSM_HOOK, d, config->ssidref)) != 0 )
             goto fail;
 
         d->controller_pause_count = 1;
@@ -373,7 +375,7 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
             goto fail;
     }
 
-    if ( (err = arch_domain_create(d, domcr_flags, config)) != 0 )
+    if ( (err = arch_domain_create(d, config)) != 0 )
         goto fail;
     init_status |= INIT_arch;
 
@@ -385,6 +387,11 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         if ( (err = late_hwdom_init(d)) != 0 )
             goto fail;
 
+        /*
+         * Must not fail beyond this point, as our caller doesn't know whether
+         * the domain has been entered into domain_list or not.
+         */
+
         spin_lock(&domlist_update_lock);
         pd = &domain_list; /* NB. domain_list maintained in order of domid. */
         for ( pd = &domain_list; *pd != NULL; pd = &(*pd)->next_in_list )
@@ -395,6 +402,8 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         rcu_assign_pointer(*pd, d);
         rcu_assign_pointer(domain_hash[DOMAIN_HASH(domid)], d);
         spin_unlock(&domlist_update_lock);
+
+        memcpy(d->handle, config->handle, sizeof(d->handle));
     }
 
     return d;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index a73e1a4..9b7bc08 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -532,9 +532,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             rover = dom;
         }
 
-        d = domain_create(dom, op->u.createdomain.flags,
-                          op->u.createdomain.ssidref,
-                          &op->u.createdomain.config);
+        d = domain_create(dom, &op->u.createdomain);
         if ( IS_ERR(d) )
         {
             ret = PTR_ERR(d);
@@ -543,10 +541,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         }
 
         ret = 0;
-
-        memcpy(d->handle, op->u.createdomain.handle,
-               sizeof(xen_domain_handle_t));
-
         op->domain = d->domain_id;
         copyback = 1;
         d = NULL;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 64524f4..b3c2660 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1734,7 +1734,7 @@ void __init scheduler_init(void)
         sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
     }
 
-    idle_domain = domain_create(DOMID_IDLE, 0, 0, NULL);
+    idle_domain = domain_create(DOMID_IDLE, NULL);
     BUG_ON(IS_ERR(idle_domain));
     idle_domain->vcpu = idle_vcpu;
     idle_domain->max_vcpus = nr_cpu_ids;
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index eb62f1d..177cb35 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -55,8 +55,8 @@ void vcpu_destroy(struct vcpu *v);
 int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset);
 void unmap_vcpu_info(struct vcpu *v);
 
-int arch_domain_create(struct domain *d, unsigned int domcr_flags,
-                       struct xen_arch_domainconfig *config);
+int arch_domain_create(struct domain *d,
+                       struct xen_domctl_createdomain *config);
 
 void arch_domain_destroy(struct domain *d);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 9af78ac..c15c39e 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -539,9 +539,8 @@ void domain_update_node_affinity(struct domain *d);
  * Create a domain: the configuration is only necessary for real domain
  * (domid < DOMID_FIRST_RESERVED).
  */
-struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
-                             uint32_t ssidref,
-                             struct xen_arch_domainconfig *config);
+struct domain *domain_create(domid_t domid,
+                             struct xen_domctl_createdomain *config);
 
 /*
  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
-- 
2.1.4


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

Re: [PATCH 6/7] xen/domain: Pass the full domctl_createdomain struct to create_domain()

[Wei Liu]

On Fri, Mar 09, 2018 at 01:18:41PM +0000, Andrew Cooper wrote:
> In future patches, the structure will be extended with further information,
> and this is far cleaner than adding extra parameters.
> 
> One minor tweak is that the setting of guest_type needs to be deferred until
> config is known-good to dereference, but this doesn't result in any changed
> behaviour as system domains never used to pass XEN_DOMCTL_CDF_hvm_guest.
> 
> Also for completeness, move the setting of d->handle into the tail of
> domain_create() where it more logically should live.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 6/7] xen/domain: Pass the full domctl_createdomain struct to create_domain()

[Jan Beulich]

>>> On 09.03.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -426,8 +426,8 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>      return true;
>  }
>  
> -int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> -                       struct xen_arch_domainconfig *config)
> +int arch_domain_create(struct domain *d,
> +                       struct xen_domctl_createdomain *config)

Is there any reason for this to not be const? There's no write now
afaics, and I can't imagine you wanting to add one later on.

> @@ -1632,14 +1634,16 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      if ( dom0_pvh )
>      {
> -        domcr_flags |= XEN_DOMCTL_CDF_hvm_guest |
> -                       ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
> -                         XEN_DOMCTL_CDF_hap : 0);
> -        config.emulation_flags = XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC;
> +        dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm_guest |
> +                           ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
> +                            XEN_DOMCTL_CDF_hap : 0));
> +
> +        dom0_cfg.config.emulation_flags =
> +            XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC;

Would you mind making this |= for ease of future changes?

Other than these
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 6/7] xen/domain: Pass the full domctl_createdomain struct to create_domain()

[Andrew Cooper]

On 09/03/18 17:00, Jan Beulich wrote:
>>>> On 09.03.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -426,8 +426,8 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>>      return true;
>>  }
>>  
>> -int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>> -                       struct xen_arch_domainconfig *config)
>> +int arch_domain_create(struct domain *d,
>> +                       struct xen_domctl_createdomain *config)
> Is there any reason for this to not be const? There's no write now
> afaics, and I can't imagine you wanting to add one later on.

I originally planned to make them const, but the ARM side passes data
back to the toolstack, and the prototype is (rightfully) common.

>
>> @@ -1632,14 +1634,16 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>  
>>      if ( dom0_pvh )
>>      {
>> -        domcr_flags |= XEN_DOMCTL_CDF_hvm_guest |
>> -                       ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
>> -                         XEN_DOMCTL_CDF_hap : 0);
>> -        config.emulation_flags = XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC;
>> +        dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm_guest |
>> +                           ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
>> +                            XEN_DOMCTL_CDF_hap : 0));
>> +
>> +        dom0_cfg.config.emulation_flags =
>> +            XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC;
> Would you mind making this |= for ease of future changes?

Certainly.

>
> Other than these
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
>


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

Re: [PATCH 6/7] xen/domain: Pass the full domctl_createdomain struct to create_domain()

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 03/09/18 6:06 PM >>>
>On 09/03/18 17:00, Jan Beulich wrote:
>>>>> On 09.03.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -426,8 +426,8 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>>>      return true;
>>>  }
>>>  
>>> -int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>> -                       struct xen_arch_domainconfig *config)
>>> +int arch_domain_create(struct domain *d,
>>> +                       struct xen_domctl_createdomain *config)
>> Is there any reason for this to not be const? There's no write now
>> afaics, and I can't imagine you wanting to add one later on.
>
>I originally planned to make them const, but the ARM side passes data
>back to the toolstack, and the prototype is (rightfully) common.

Oh, I see - certainly a little odd, but that's the way it is then.

Jan


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

Re: [PATCH 6/7] xen/domain: Pass the full domctl_createdomain struct to create_domain()

[Julien Grall]

Hi Andrew,

On 03/09/2018 01:18 PM, Andrew Cooper wrote:
> In future patches, the structure will be extended with further information,
> and this is far cleaner than adding extra parameters.
> 
> One minor tweak is that the setting of guest_type needs to be deferred until
> config is known-good to dereference, but this doesn't result in any changed
> behaviour as system domains never used to pass XEN_DOMCTL_CDF_hvm_guest.
> 
> Also for completeness, move the setting of d->handle into the tail of
> domain_create() where it more logically should live.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Cheers,

> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/domain.c    | 16 ++++++++--------
>   xen/arch/arm/mm.c        |  6 +++---
>   xen/arch/arm/setup.c     |  8 ++++----
>   xen/arch/x86/domain.c    | 12 ++++++------
>   xen/arch/x86/mm.c        |  6 +++---
>   xen/arch/x86/setup.c     | 18 +++++++++++-------
>   xen/common/domain.c      | 31 ++++++++++++++++++++-----------
>   xen/common/domctl.c      |  8 +-------
>   xen/common/schedule.c    |  2 +-
>   xen/include/xen/domain.h |  4 ++--
>   xen/include/xen/sched.h  |  5 ++---
>   11 files changed, 61 insertions(+), 55 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 291c282..4b45fad 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -573,8 +573,8 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
>       v->arch.hcr_el2 |= HCR_RW;
>   }
>   
> -int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> -                       struct xen_arch_domainconfig *config)
> +int arch_domain_create(struct domain *d,
> +                       struct xen_domctl_createdomain *config)
>   {
>       int rc, count = 0;
>   
> @@ -585,7 +585,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>       if ( is_idle_domain(d) )
>           return 0;
>   
> -    if ( domcr_flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
> +    if ( config->flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
>           return -EINVAL;
>   
>       ASSERT(config != NULL);
> @@ -605,18 +605,18 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>       share_xen_page_with_guest(
>           virt_to_page(d->shared_info), d, XENSHARE_writable);
>   
> -    switch ( config->gic_version )
> +    switch ( config->config.gic_version )
>       {
>       case XEN_DOMCTL_CONFIG_GIC_NATIVE:
>           switch ( gic_hw_version () )
>           {
>           case GIC_V2:
> -            config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
> +            config->config.gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
>               d->arch.vgic.version = GIC_V2;
>               break;
>   
>           case GIC_V3:
> -            config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
> +            config->config.gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
>               d->arch.vgic.version = GIC_V3;
>               break;
>   
> @@ -644,10 +644,10 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>       if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 )
>           goto fail;
>   
> -    if ( (rc = domain_vgic_init(d, config->nr_spis)) != 0 )
> +    if ( (rc = domain_vgic_init(d, config->config.nr_spis)) != 0 )
>           goto fail;
>   
> -    if ( (rc = domain_vtimer_init(d, config)) != 0 )
> +    if ( (rc = domain_vtimer_init(d, &config->config)) != 0 )
>           goto fail;
>   
>       update_domain_wallclock_time(d);
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index ce83f69..a09bea2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -520,7 +520,7 @@ void __init arch_init_memory(void)
>        * Any Xen-heap pages that we will allow to be mapped will have
>        * their domain field set to dom_xen.
>        */
> -    dom_xen = domain_create(DOMID_XEN, 0, 0, NULL);
> +    dom_xen = domain_create(DOMID_XEN, NULL);
>       BUG_ON(IS_ERR(dom_xen));
>   
>       /*
> @@ -528,14 +528,14 @@ void __init arch_init_memory(void)
>        * This domain owns I/O pages that are within the range of the page_info
>        * array. Mappings occur at the priv of the caller.
>        */
> -    dom_io = domain_create(DOMID_IO, 0, 0, NULL);
> +    dom_io = domain_create(DOMID_IO, NULL);
>       BUG_ON(IS_ERR(dom_io));
>   
>       /*
>        * Initialise our COW domain.
>        * This domain owns sharable pages.
>        */
> -    dom_cow = domain_create(DOMID_COW, 0, 0, NULL);
> +    dom_cow = domain_create(DOMID_COW, NULL);
>       BUG_ON(IS_ERR(dom_cow));
>   }
>   
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 4627366..b17797d 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -693,7 +693,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       const char *cmdline;
>       struct bootmodule *xen_bootmodule;
>       struct domain *dom0;
> -    struct xen_arch_domainconfig config;
> +    struct xen_domctl_createdomain dom0_cfg = {};
>   
>       dcache_line_bytes = read_dcache_line_bytes();
>   
> @@ -840,10 +840,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>   
>       /* Create initial domain 0. */
>       /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> -    config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> -    config.nr_spis = gic_number_lines() - 32;
> +    dom0_cfg.config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> +    dom0_cfg.config.nr_spis = gic_number_lines() - 32;
>   
> -    dom0 = domain_create(0, 0, 0, &config);
> +    dom0 = domain_create(0, &dom0_cfg);
>       if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>               panic("Error creating domain 0");
>   
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 48dc2b9..12d0766 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -426,8 +426,8 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>       return true;
>   }
>   
> -int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> -                       struct xen_arch_domainconfig *config)
> +int arch_domain_create(struct domain *d,
> +                       struct xen_domctl_createdomain *config)
>   {
>       bool paging_initialised = false;
>       uint32_t emflags;
> @@ -473,9 +473,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>                  d->domain_id);
>       }
>   
> -    d->arch.s3_integrity = domcr_flags & XEN_DOMCTL_CDF_s3_integrity;
> +    d->arch.s3_integrity = config->flags & XEN_DOMCTL_CDF_s3_integrity;
>   
> -    emflags = config->emulation_flags;
> +    emflags = config->config.emulation_flags;
>   
>       if ( is_hardware_domain(d) && is_pv_domain(d) )
>           emflags |= XEN_X86_EMU_PIT;
> @@ -502,9 +502,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>       /* Need to determine if HAP is enabled before initialising paging */
>       if ( is_hvm_domain(d) )
>           d->arch.hvm_domain.hap_enabled =
> -            hvm_funcs.hap_supported && (domcr_flags & XEN_DOMCTL_CDF_hap);
> +            hvm_funcs.hap_supported && (config->flags & XEN_DOMCTL_CDF_hap);
>   
> -    if ( (rc = paging_domain_init(d, domcr_flags)) != 0 )
> +    if ( (rc = paging_domain_init(d, config->flags)) != 0 )
>           goto fail;
>       paging_initialised = true;
>   
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index c275d4b..1d4e396 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -271,7 +271,7 @@ void __init arch_init_memory(void)
>        * Hidden PCI devices will also be associated with this domain
>        * (but be [partly] controlled by Dom0 nevertheless).
>        */
> -    dom_xen = domain_create(DOMID_XEN, 0, 0, NULL);
> +    dom_xen = domain_create(DOMID_XEN, NULL);
>       BUG_ON(IS_ERR(dom_xen));
>       INIT_LIST_HEAD(&dom_xen->arch.pdev_list);
>   
> @@ -280,14 +280,14 @@ void __init arch_init_memory(void)
>        * This domain owns I/O pages that are within the range of the page_info
>        * array. Mappings occur at the priv of the caller.
>        */
> -    dom_io = domain_create(DOMID_IO, 0, 0, NULL);
> +    dom_io = domain_create(DOMID_IO, NULL);
>       BUG_ON(IS_ERR(dom_io));
>   
>       /*
>        * Initialise our COW domain.
>        * This domain owns sharable pages.
>        */
> -    dom_cow = domain_create(DOMID_COW, 0, 0, NULL);
> +    dom_cow = domain_create(DOMID_COW, NULL);
>       BUG_ON(IS_ERR(dom_cow));
>   
>       /*
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a6dc5df..a4405b4 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -660,7 +660,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>   {
>       char *memmap_type = NULL;
>       char *cmdline, *kextra, *loader;
> -    unsigned int initrdidx, domcr_flags = XEN_DOMCTL_CDF_s3_integrity;
> +    unsigned int initrdidx;
>       multiboot_info_t *mbi;
>       module_t *mod;
>       unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
> @@ -671,7 +671,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>           .parity    = 'n',
>           .stop_bits = 1
>       };
> -    struct xen_arch_domainconfig config = { .emulation_flags = 0 };
> +    struct xen_domctl_createdomain dom0_cfg = {
> +        .flags = XEN_DOMCTL_CDF_s3_integrity,
> +    };
>   
>       /* Critical region without IDT or TSS.  Any fault is deadly! */
>   
> @@ -1632,14 +1634,16 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>   
>       if ( dom0_pvh )
>       {
> -        domcr_flags |= XEN_DOMCTL_CDF_hvm_guest |
> -                       ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
> -                         XEN_DOMCTL_CDF_hap : 0);
> -        config.emulation_flags = XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC;
> +        dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm_guest |
> +                           ((hvm_funcs.hap_supported && !opt_dom0_shadow) ?
> +                            XEN_DOMCTL_CDF_hap : 0));
> +
> +        dom0_cfg.config.emulation_flags =
> +            XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC;
>       }
>   
>       /* Create initial domain 0. */
> -    dom0 = domain_create(get_initial_domain_id(), domcr_flags, 0, &config);
> +    dom0 = domain_create(get_initial_domain_id(), &dom0_cfg);
>       if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>           panic("Error creating domain 0");
>   
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 582e3e5..b00cc1f 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -260,9 +260,8 @@ static int __init parse_extra_guest_irqs(const char *s)
>   }
>   custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>   
> -struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
> -                             uint32_t ssidref,
> -                             struct xen_arch_domainconfig *config)
> +struct domain *domain_create(domid_t domid,
> +                             struct xen_domctl_createdomain *config)
>   {
>       struct domain *d, **pd, *old_hwdom = NULL;
>       enum { INIT_xsm = 1u<<0, INIT_watchdog = 1u<<1, INIT_rangeset = 1u<<2,
> @@ -274,6 +273,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>   
>       d->domain_id = domid;
>   
> +    /* Debug sanity. */
> +    ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
> +
>       TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>   
>       lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
> @@ -304,11 +306,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>       if ( !zalloc_cpumask_var(&d->dirty_cpumask) )
>           goto fail;
>   
> -    if ( domcr_flags & XEN_DOMCTL_CDF_hvm_guest )
> -        d->guest_type = guest_type_hvm;
> -    else
> -        d->guest_type = guest_type_pv;
> -
>       rangeset_domain_initialise(d);
>       init_status |= INIT_rangeset;
>   
> @@ -318,6 +315,11 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>   
>       if ( !is_idle_domain(d) )
>       {
> +        if ( config->flags & XEN_DOMCTL_CDF_hvm_guest )
> +            d->guest_type = guest_type_hvm;
> +        else
> +            d->guest_type = guest_type_pv;
> +
>           watchdog_domain_init(d);
>           init_status |= INIT_watchdog;
>   
> @@ -331,7 +333,7 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>               hardware_domain = d;
>           }
>   
> -        if ( domcr_flags & XEN_DOMCTL_CDF_xs_domain )
> +        if ( config->flags & XEN_DOMCTL_CDF_xs_domain )
>           {
>               d->is_xenstore = 1;
>               d->disable_migrate = 1;
> @@ -342,7 +344,7 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>           if ( !d->iomem_caps || !d->irq_caps )
>               goto fail;
>   
> -        if ( (err = xsm_domain_create(XSM_HOOK, d, ssidref)) != 0 )
> +        if ( (err = xsm_domain_create(XSM_HOOK, d, config->ssidref)) != 0 )
>               goto fail;
>   
>           d->controller_pause_count = 1;
> @@ -373,7 +375,7 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>               goto fail;
>       }
>   
> -    if ( (err = arch_domain_create(d, domcr_flags, config)) != 0 )
> +    if ( (err = arch_domain_create(d, config)) != 0 )
>           goto fail;
>       init_status |= INIT_arch;
>   
> @@ -385,6 +387,11 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>           if ( (err = late_hwdom_init(d)) != 0 )
>               goto fail;
>   
> +        /*
> +         * Must not fail beyond this point, as our caller doesn't know whether
> +         * the domain has been entered into domain_list or not.
> +         */
> +
>           spin_lock(&domlist_update_lock);
>           pd = &domain_list; /* NB. domain_list maintained in order of domid. */
>           for ( pd = &domain_list; *pd != NULL; pd = &(*pd)->next_in_list )
> @@ -395,6 +402,8 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>           rcu_assign_pointer(*pd, d);
>           rcu_assign_pointer(domain_hash[DOMAIN_HASH(domid)], d);
>           spin_unlock(&domlist_update_lock);
> +
> +        memcpy(d->handle, config->handle, sizeof(d->handle));
>       }
>   
>       return d;
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index a73e1a4..9b7bc08 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -532,9 +532,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>               rover = dom;
>           }
>   
> -        d = domain_create(dom, op->u.createdomain.flags,
> -                          op->u.createdomain.ssidref,
> -                          &op->u.createdomain.config);
> +        d = domain_create(dom, &op->u.createdomain);
>           if ( IS_ERR(d) )
>           {
>               ret = PTR_ERR(d);
> @@ -543,10 +541,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>           }
>   
>           ret = 0;
> -
> -        memcpy(d->handle, op->u.createdomain.handle,
> -               sizeof(xen_domain_handle_t));
> -
>           op->domain = d->domain_id;
>           copyback = 1;
>           d = NULL;
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 64524f4..b3c2660 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1734,7 +1734,7 @@ void __init scheduler_init(void)
>           sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
>       }
>   
> -    idle_domain = domain_create(DOMID_IDLE, 0, 0, NULL);
> +    idle_domain = domain_create(DOMID_IDLE, NULL);
>       BUG_ON(IS_ERR(idle_domain));
>       idle_domain->vcpu = idle_vcpu;
>       idle_domain->max_vcpus = nr_cpu_ids;
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index eb62f1d..177cb35 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -55,8 +55,8 @@ void vcpu_destroy(struct vcpu *v);
>   int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset);
>   void unmap_vcpu_info(struct vcpu *v);
>   
> -int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> -                       struct xen_arch_domainconfig *config);
> +int arch_domain_create(struct domain *d,
> +                       struct xen_domctl_createdomain *config);
>   
>   void arch_domain_destroy(struct domain *d);
>   
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 9af78ac..c15c39e 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -539,9 +539,8 @@ void domain_update_node_affinity(struct domain *d);
>    * Create a domain: the configuration is only necessary for real domain
>    * (domid < DOMID_FIRST_RESERVED).
>    */
> -struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
> -                             uint32_t ssidref,
> -                             struct xen_arch_domainconfig *config);
> +struct domain *domain_create(domid_t domid,
> +                             struct xen_domctl_createdomain *config);
>   
>   /*
>    * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
> 

-- 
Julien Grall

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

[PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Andrew Cooper]

The share_xen_page_with_guest() functions are used by common code, and are
implemented the same by each arch.  Move the declarations into the common mm.h
rather than duplicating them in each arch/mm.h

Turn an int readonly into a boolean enum, to retain ro/rw context at the
callsites, but use shorter labels which avoids a large number of split lines.

Implement share_xen_page_with_privileged_guests() as a static inline wrapper
around share_xen_page_with_guest() to avoid having a call into a separate
translation unit whose only purpose is to shuffle function arguments.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/arm/domain.c             |  3 +--
 xen/arch/arm/mm.c                 | 13 ++++---------
 xen/arch/x86/domain.c             |  3 +--
 xen/arch/x86/hvm/dom0_build.c     |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c        |  2 +-
 xen/arch/x86/mm.c                 | 20 +++++++-------------
 xen/arch/x86/pv/shim.c            |  6 ++----
 xen/arch/x86/x86_64/mm.c          | 16 ++++++----------
 xen/common/trace.c                |  9 +++------
 xen/common/xenoprof.c             |  3 +--
 xen/include/asm-arm/grant_table.h |  3 +--
 xen/include/asm-arm/mm.h          |  7 -------
 xen/include/asm-x86/grant_table.h |  6 ++----
 xen/include/asm-x86/mm.h          |  8 --------
 xen/include/xen/mm.h              | 14 ++++++++++++++
 15 files changed, 44 insertions(+), 71 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 4b45fad..23dac5d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -602,8 +602,7 @@ int arch_domain_create(struct domain *d,
         goto fail;
 
     clear_page(d->shared_info);
-    share_xen_page_with_guest(
-        virt_to_page(d->shared_info), d, XENSHARE_writable);
+    share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
 
     switch ( config->config.gic_version )
     {
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a09bea2..baa3b0d 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
     return gfn_x(d->arch.p2m.max_mapped_gfn);
 }
 
-void share_xen_page_with_guest(struct page_info *page,
-                          struct domain *d, int readonly)
+void share_xen_page_with_guest(struct page_info *page, struct domain *d,
+                               enum XENSHARE_flags flags)
 {
     if ( page_get_owner(page) == d )
         return;
@@ -1196,7 +1196,8 @@ void share_xen_page_with_guest(struct page_info *page,
     spin_lock(&d->page_alloc_lock);
 
     /* The incremented type count pins as writable or read-only. */
-    page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page) | 1;
+    page->u.inuse.type_info =
+        (flags == SHARE_ro ? PGT_none : PGT_writable_page) | 1;
 
     page_set_owner(page, d);
     smp_wmb(); /* install valid domain ptr before updating refcnt. */
@@ -1214,12 +1215,6 @@ void share_xen_page_with_guest(struct page_info *page,
     spin_unlock(&d->page_alloc_lock);
 }
 
-void share_xen_page_with_privileged_guests(
-    struct page_info *page, int readonly)
-{
-    share_xen_page_with_guest(page, dom_xen, readonly);
-}
-
 int xenmem_add_to_physmap_one(
     struct domain *d,
     unsigned int space,
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 12d0766..8006bed 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -528,8 +528,7 @@ int arch_domain_create(struct domain *d,
         goto fail;
 
     clear_page(d->shared_info);
-    share_xen_page_with_guest(
-        virt_to_page(d->shared_info), d, XENSHARE_writable);
+    share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
 
     if ( (rc = init_domain_irq_mapping(d)) != 0 )
         goto fail;
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index afebaec..1c70416 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -297,7 +297,7 @@ static void __init pvh_steal_low_ram(struct domain *d, unsigned long start,
             continue;
         }
 
-        share_xen_page_with_guest(pg, d, XENSHARE_writable);
+        share_xen_page_with_guest(pg, d, SHARE_rw);
         rc = guest_physmap_add_entry(d, _gfn(mfn), _mfn(mfn), 0, p2m_ram_rw);
         if ( rc )
             printk("Unable to add mfn %#lx to p2m: %d\n", mfn, rc);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 18d8ce2..ebc6934 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2941,7 +2941,7 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
         return -ENOMEM;
     mfn = page_to_mfn(pg);
     clear_domain_page(_mfn(mfn));
-    share_xen_page_with_guest(pg, d, XENSHARE_writable);
+    share_xen_page_with_guest(pg, d, SHARE_rw);
     d->arch.hvm_domain.vmx.apic_access_mfn = mfn;
     set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), _mfn(mfn),
                        PAGE_ORDER_4K, p2m_get_hostp2m(d)->default_access);
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1d4e396..17558e0 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -301,8 +301,7 @@ void __init arch_init_memory(void)
           i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
                         : 0x100);
           i++ )
-        share_xen_page_with_guest(mfn_to_page(_mfn(i)),
-                                  dom_io, XENSHARE_writable);
+        share_xen_page_with_guest(mfn_to_page(_mfn(i)), dom_io, SHARE_rw);
 
     /* Any areas not specified as RAM by the e820 map are considered I/O. */
     for ( i = 0, pfn = 0; pfn < max_page; i++ )
@@ -342,8 +341,8 @@ void __init arch_init_memory(void)
         {
             if ( !mfn_valid(_mfn(pfn)) )
                 continue;
-            share_xen_page_with_guest(
-                mfn_to_page(_mfn(pfn)), dom_io, XENSHARE_writable);
+
+            share_xen_page_with_guest(mfn_to_page(_mfn(pfn)), dom_io, SHARE_rw);
         }
 
         /* Skip the RAM region. */
@@ -439,8 +438,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
     return (arch_get_max_pfn(d) ?: 1) - 1;
 }
 
-void share_xen_page_with_guest(
-    struct page_info *page, struct domain *d, int readonly)
+void share_xen_page_with_guest(struct page_info *page, struct domain *d,
+                               enum XENSHARE_flags flags)
 {
     if ( page_get_owner(page) == d )
         return;
@@ -450,7 +449,8 @@ void share_xen_page_with_guest(
     spin_lock(&d->page_alloc_lock);
 
     /* The incremented type count pins as writable or read-only. */
-    page->u.inuse.type_info  = (readonly ? PGT_none : PGT_writable_page);
+    page->u.inuse.type_info =
+        (flags == SHARE_ro ? PGT_none : PGT_writable_page);
     page->u.inuse.type_info |= PGT_validated | 1;
 
     page_set_owner(page, d);
@@ -485,12 +485,6 @@ int __init unshare_xen_page_with_guest(struct page_info *page,
     return 0;
 }
 
-void share_xen_page_with_privileged_guests(
-    struct page_info *page, int readonly)
-{
-    share_xen_page_with_guest(page, dom_xen, readonly);
-}
-
 void free_shared_domheap_page(struct page_info *page)
 {
     if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index 534965c..dd76264 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -171,8 +171,7 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
     (si) = param;                                                              \
     if ( va )                                                                  \
     {                                                                          \
-        share_xen_page_with_guest(mfn_to_page(_mfn(param)), d,                 \
-                                  XENSHARE_writable);                          \
+        share_xen_page_with_guest(mfn_to_page(_mfn(param)), d, SHARE_rw);      \
         replace_va_mapping(d, l4start, va, _mfn(param));                       \
         dom0_update_physmap(d, PFN_DOWN((va) - va_start), param, vphysmap);    \
     }                                                                          \
@@ -199,8 +198,7 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
         clear_page(page);
         console_mfn = virt_to_mfn(page);
         si->console.domU.mfn = mfn_x(console_mfn);
-        share_xen_page_with_guest(mfn_to_page(console_mfn), d,
-                                  XENSHARE_writable);
+        share_xen_page_with_guest(mfn_to_page(console_mfn), d, SHARE_rw);
         replace_va_mapping(d, l4start, console_va, console_mfn);
         dom0_update_physmap(d, (console_va - va_start) >> PAGE_SHIFT,
                             mfn_x(console_mfn), vphysmap);
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 9b37da6..8820f5c 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -185,7 +185,7 @@ static int share_hotadd_m2p_table(struct mem_hotadd_info *info)
         {
             struct page_info *page = mfn_to_page(m2p_start_mfn + i);
             if (hotadd_mem_valid(m2p_start_mfn + i, info))
-                share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
+                share_xen_page_with_privileged_guests(page, SHARE_ro);
         }
     }
 
@@ -206,7 +206,7 @@ static int share_hotadd_m2p_table(struct mem_hotadd_info *info)
         {
             struct page_info *page = mfn_to_page(m2p_start_mfn + i);
             if (hotadd_mem_valid(m2p_start_mfn + i, info))
-                share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
+                share_xen_page_with_privileged_guests(page, SHARE_ro);
         }
     }
     return 0;
@@ -860,10 +860,8 @@ void __init subarch_init_memory(void)
         }
 
         for ( i = 0; i < n; i++ )
-        {
-            struct page_info *page = mfn_to_page(m2p_start_mfn + i);
-            share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
-        }
+            share_xen_page_with_privileged_guests(
+                mfn_to_page(m2p_start_mfn + i), SHARE_ro);
     }
 
     for ( v  = RDWR_COMPAT_MPT_VIRT_START;
@@ -880,10 +878,8 @@ void __init subarch_init_memory(void)
         m2p_start_mfn = l2e_get_pfn(l2e);
 
         for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-        {
-            struct page_info *page = mfn_to_page(m2p_start_mfn + i);
-            share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
-        }
+            share_xen_page_with_privileged_guests(
+                mfn_to_page(m2p_start_mfn + i), SHARE_ro);
     }
 
     /* Mark all of direct map NX if hardware supports it. */
diff --git a/xen/common/trace.c b/xen/common/trace.c
index 2e18702..680f6ae 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -227,7 +227,6 @@ static int alloc_trace_bufs(unsigned int pages)
     for_each_online_cpu(cpu)
     {
         struct t_buf *buf;
-        struct page_info *pg;
 
         spin_lock_init(&per_cpu(t_lock, cpu));
 
@@ -242,16 +241,14 @@ static int alloc_trace_bufs(unsigned int pages)
 
         /* Now share the trace pages */
         for ( i = 0; i < pages; i++ )
-        {
-            pg = mfn_to_page(t_info_mfn_list[offset + i]);
-            share_xen_page_with_privileged_guests(pg, XENSHARE_writable);
-        }
+            share_xen_page_with_privileged_guests(
+                mfn_to_page(t_info_mfn_list[offset + i]), SHARE_rw);
     }
 
     /* Finally, share the t_info page */
     for(i = 0; i < t_info_pages; i++)
         share_xen_page_with_privileged_guests(
-            virt_to_page(t_info) + i, XENSHARE_readonly);
+            virt_to_page(t_info) + i, SHARE_ro);
 
     data_size  = (pages * PAGE_SIZE - sizeof(struct t_buf));
     t_buf_highwater = data_size >> 1; /* 50% high water */
diff --git a/xen/common/xenoprof.c b/xen/common/xenoprof.c
index 5acdde5..c1b4303 100644
--- a/xen/common/xenoprof.c
+++ b/xen/common/xenoprof.c
@@ -159,8 +159,7 @@ share_xenoprof_page_with_guest(struct domain *d, mfn_t mfn, int npages)
     }
 
     for ( i = 0; i < npages; i++ )
-        share_xen_page_with_guest(mfn_to_page(mfn_add(mfn, i)),
-                                  d, XENSHARE_writable);
+        share_xen_page_with_guest(mfn_to_page(mfn_add(mfn, i)), d, SHARE_rw);
 
     return 0;
 }
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 5b8994c..be7ed75 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -81,8 +81,7 @@ static inline unsigned int gnttab_dom0_max(void)
 #define gnttab_create_shared_page(d, t, i)                               \
     do {                                                                 \
         share_xen_page_with_guest(                                       \
-            virt_to_page((char *)(t)->shared_raw[i]),                    \
-            (d), XENSHARE_writable);                                     \
+            virt_to_page((char *)(t)->shared_raw[i]), (d), SHARE_rw);    \
     } while ( 0 )
 
 #define gnttab_shared_gmfn(d, t, i)                                      \
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 4d5563b..a0e922f 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -159,13 +159,6 @@ extern vaddr_t xenheap_virt_start;
 
 #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
 
-#define XENSHARE_writable 0
-#define XENSHARE_readonly 1
-extern void share_xen_page_with_guest(
-    struct page_info *page, struct domain *d, int readonly);
-extern void share_xen_page_with_privileged_guests(
-    struct page_info *page, int readonly);
-
 #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
 /* PDX of the first page in the frame table. */
 extern unsigned long frametable_base_pdx;
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 66e9742..8720159 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -57,15 +57,13 @@ static inline unsigned int gnttab_dom0_max(void)
 #define gnttab_create_shared_page(d, t, i)                               \
     do {                                                                 \
         share_xen_page_with_guest(                                       \
-            virt_to_page((char *)(t)->shared_raw[i]),                    \
-            (d), XENSHARE_writable);                                     \
+            virt_to_page((char *)(t)->shared_raw[i]), (d), SHARE_rw);    \
     } while ( 0 )
 
 #define gnttab_create_status_page(d, t, i)                               \
     do {                                                                 \
         share_xen_page_with_guest(                                       \
-           virt_to_page((char *)(t)->status[i]),                         \
-            (d), XENSHARE_writable);                                     \
+            virt_to_page((char *)(t)->status[i]), (d), SHARE_rw);        \
     } while ( 0 )
 
 
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 96f3a34..c115661 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -292,14 +292,6 @@ struct page_info
 
 #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
 
-#define XENSHARE_writable 0
-#define XENSHARE_readonly 1
-extern void share_xen_page_with_guest(
-    struct page_info *page, struct domain *d, int readonly);
-extern int unshare_xen_page_with_guest(struct page_info *page,
-                                       struct domain *d);
-extern void share_xen_page_with_privileged_guests(
-    struct page_info *page, int readonly);
 extern void free_shared_domheap_page(struct page_info *page);
 
 #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 0e0e511..142aa73 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -634,4 +634,18 @@ static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
     }
 }
 
+enum XENSHARE_flags {
+    SHARE_rw,
+    SHARE_ro,
+};
+void share_xen_page_with_guest(struct page_info *page, struct domain *d,
+                               enum XENSHARE_flags flags);
+int unshare_xen_page_with_guest(struct page_info *page, struct domain *d);
+
+static inline void share_xen_page_with_privileged_guests(
+    struct page_info *page, enum XENSHARE_flags flags)
+{
+    share_xen_page_with_guest(page, dom_xen, flags);
+}
+
 #endif /* __XEN_MM_H__ */
-- 
2.1.4


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

Re: [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Wei Liu]

On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
> The share_xen_page_with_guest() functions are used by common code, and are
> implemented the same by each arch.  Move the declarations into the common mm.h
> rather than duplicating them in each arch/mm.h
> 
> Turn an int readonly into a boolean enum, to retain ro/rw context at the
> callsites, but use shorter labels which avoids a large number of split lines.
> 
> Implement share_xen_page_with_privileged_guests() as a static inline wrapper
> around share_xen_page_with_guest() to avoid having a call into a separate
> translation unit whose only purpose is to shuffle function arguments.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Julien Grall]

Hi Andrew,

On 03/09/2018 01:18 PM, Andrew Cooper wrote:
> The share_xen_page_with_guest() functions are used by common code, and are
> implemented the same by each arch.  Move the declarations into the common mm.h
> rather than duplicating them in each arch/mm.h
> 
> Turn an int readonly into a boolean enum, to retain ro/rw context at the
> callsites, but use shorter labels which avoids a large number of split lines.
> 
> Implement share_xen_page_with_privileged_guests() as a static inline wrapper
> around share_xen_page_with_guest() to avoid having a call into a separate
> translation unit whose only purpose is to shuffle function arguments.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   xen/arch/arm/domain.c             |  3 +--
>   xen/arch/arm/mm.c                 | 13 ++++---------
>   xen/arch/x86/domain.c             |  3 +--
>   xen/arch/x86/hvm/dom0_build.c     |  2 +-
>   xen/arch/x86/hvm/vmx/vmx.c        |  2 +-
>   xen/arch/x86/mm.c                 | 20 +++++++-------------
>   xen/arch/x86/pv/shim.c            |  6 ++----
>   xen/arch/x86/x86_64/mm.c          | 16 ++++++----------
>   xen/common/trace.c                |  9 +++------
>   xen/common/xenoprof.c             |  3 +--
>   xen/include/asm-arm/grant_table.h |  3 +--
>   xen/include/asm-arm/mm.h          |  7 -------
>   xen/include/asm-x86/grant_table.h |  6 ++----
>   xen/include/asm-x86/mm.h          |  8 --------
>   xen/include/xen/mm.h              | 14 ++++++++++++++
>   15 files changed, 44 insertions(+), 71 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 4b45fad..23dac5d 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -602,8 +602,7 @@ int arch_domain_create(struct domain *d,
>           goto fail;
>   
>       clear_page(d->shared_info);
> -    share_xen_page_with_guest(
> -        virt_to_page(d->shared_info), d, XENSHARE_writable);
> +    share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
>   
>       switch ( config->config.gic_version )
>       {
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index a09bea2..baa3b0d 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
>       return gfn_x(d->arch.p2m.max_mapped_gfn);
>   }
>   
> -void share_xen_page_with_guest(struct page_info *page,
> -                          struct domain *d, int readonly)
> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
> +                               enum XENSHARE_flags flags)
>   {
>       if ( page_get_owner(page) == d )
>           return;
> @@ -1196,7 +1196,8 @@ void share_xen_page_with_guest(struct page_info *page,
>       spin_lock(&d->page_alloc_lock);
>   
>       /* The incremented type count pins as writable or read-only. */
> -    page->u.inuse.type_info = (readonly ? PGT_none : PGT_writable_page) | 1;
> +    page->u.inuse.type_info =
> +        (flags == SHARE_ro ? PGT_none : PGT_writable_page) | 1;
>   
>       page_set_owner(page, d);
>       smp_wmb(); /* install valid domain ptr before updating refcnt. */
> @@ -1214,12 +1215,6 @@ void share_xen_page_with_guest(struct page_info *page,
>       spin_unlock(&d->page_alloc_lock);
>   }
>   
> -void share_xen_page_with_privileged_guests(
> -    struct page_info *page, int readonly)
> -{
> -    share_xen_page_with_guest(page, dom_xen, readonly);
> -}
> -
>   int xenmem_add_to_physmap_one(
>       struct domain *d,
>       unsigned int space,
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 12d0766..8006bed 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -528,8 +528,7 @@ int arch_domain_create(struct domain *d,
>           goto fail;
>   
>       clear_page(d->shared_info);
> -    share_xen_page_with_guest(
> -        virt_to_page(d->shared_info), d, XENSHARE_writable);
> +    share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
>   
>       if ( (rc = init_domain_irq_mapping(d)) != 0 )
>           goto fail;
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index afebaec..1c70416 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -297,7 +297,7 @@ static void __init pvh_steal_low_ram(struct domain *d, unsigned long start,
>               continue;
>           }
>   
> -        share_xen_page_with_guest(pg, d, XENSHARE_writable);
> +        share_xen_page_with_guest(pg, d, SHARE_rw);
>           rc = guest_physmap_add_entry(d, _gfn(mfn), _mfn(mfn), 0, p2m_ram_rw);
>           if ( rc )
>               printk("Unable to add mfn %#lx to p2m: %d\n", mfn, rc);
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 18d8ce2..ebc6934 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2941,7 +2941,7 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
>           return -ENOMEM;
>       mfn = page_to_mfn(pg);
>       clear_domain_page(_mfn(mfn));
> -    share_xen_page_with_guest(pg, d, XENSHARE_writable);
> +    share_xen_page_with_guest(pg, d, SHARE_rw);
>       d->arch.hvm_domain.vmx.apic_access_mfn = mfn;
>       set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), _mfn(mfn),
>                          PAGE_ORDER_4K, p2m_get_hostp2m(d)->default_access);
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 1d4e396..17558e0 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -301,8 +301,7 @@ void __init arch_init_memory(void)
>             i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
>                           : 0x100);
>             i++ )
> -        share_xen_page_with_guest(mfn_to_page(_mfn(i)),
> -                                  dom_io, XENSHARE_writable);
> +        share_xen_page_with_guest(mfn_to_page(_mfn(i)), dom_io, SHARE_rw);
>   
>       /* Any areas not specified as RAM by the e820 map are considered I/O. */
>       for ( i = 0, pfn = 0; pfn < max_page; i++ )
> @@ -342,8 +341,8 @@ void __init arch_init_memory(void)
>           {
>               if ( !mfn_valid(_mfn(pfn)) )
>                   continue;
> -            share_xen_page_with_guest(
> -                mfn_to_page(_mfn(pfn)), dom_io, XENSHARE_writable);
> +
> +            share_xen_page_with_guest(mfn_to_page(_mfn(pfn)), dom_io, SHARE_rw);
>           }
>   
>           /* Skip the RAM region. */
> @@ -439,8 +438,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
>       return (arch_get_max_pfn(d) ?: 1) - 1;
>   }
>   
> -void share_xen_page_with_guest(
> -    struct page_info *page, struct domain *d, int readonly)
> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
> +                               enum XENSHARE_flags flags)
>   {
>       if ( page_get_owner(page) == d )
>           return;
> @@ -450,7 +449,8 @@ void share_xen_page_with_guest(
>       spin_lock(&d->page_alloc_lock);
>   
>       /* The incremented type count pins as writable or read-only. */
> -    page->u.inuse.type_info  = (readonly ? PGT_none : PGT_writable_page);
> +    page->u.inuse.type_info =
> +        (flags == SHARE_ro ? PGT_none : PGT_writable_page);
>       page->u.inuse.type_info |= PGT_validated | 1;
>   
>       page_set_owner(page, d);
> @@ -485,12 +485,6 @@ int __init unshare_xen_page_with_guest(struct page_info *page,
>       return 0;
>   }
>   
> -void share_xen_page_with_privileged_guests(
> -    struct page_info *page, int readonly)
> -{
> -    share_xen_page_with_guest(page, dom_xen, readonly);
> -}
> -
>   void free_shared_domheap_page(struct page_info *page)
>   {
>       if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
> diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> index 534965c..dd76264 100644
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -171,8 +171,7 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
>       (si) = param;                                                              \
>       if ( va )                                                                  \
>       {                                                                          \
> -        share_xen_page_with_guest(mfn_to_page(_mfn(param)), d,                 \
> -                                  XENSHARE_writable);                          \
> +        share_xen_page_with_guest(mfn_to_page(_mfn(param)), d, SHARE_rw);      \
>           replace_va_mapping(d, l4start, va, _mfn(param));                       \
>           dom0_update_physmap(d, PFN_DOWN((va) - va_start), param, vphysmap);    \
>       }                                                                          \
> @@ -199,8 +198,7 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
>           clear_page(page);
>           console_mfn = virt_to_mfn(page);
>           si->console.domU.mfn = mfn_x(console_mfn);
> -        share_xen_page_with_guest(mfn_to_page(console_mfn), d,
> -                                  XENSHARE_writable);
> +        share_xen_page_with_guest(mfn_to_page(console_mfn), d, SHARE_rw);
>           replace_va_mapping(d, l4start, console_va, console_mfn);
>           dom0_update_physmap(d, (console_va - va_start) >> PAGE_SHIFT,
>                               mfn_x(console_mfn), vphysmap);
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 9b37da6..8820f5c 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -185,7 +185,7 @@ static int share_hotadd_m2p_table(struct mem_hotadd_info *info)
>           {
>               struct page_info *page = mfn_to_page(m2p_start_mfn + i);
>               if (hotadd_mem_valid(m2p_start_mfn + i, info))
> -                share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
> +                share_xen_page_with_privileged_guests(page, SHARE_ro);
>           }
>       }
>   
> @@ -206,7 +206,7 @@ static int share_hotadd_m2p_table(struct mem_hotadd_info *info)
>           {
>               struct page_info *page = mfn_to_page(m2p_start_mfn + i);
>               if (hotadd_mem_valid(m2p_start_mfn + i, info))
> -                share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
> +                share_xen_page_with_privileged_guests(page, SHARE_ro);
>           }
>       }
>       return 0;
> @@ -860,10 +860,8 @@ void __init subarch_init_memory(void)
>           }
>   
>           for ( i = 0; i < n; i++ )
> -        {
> -            struct page_info *page = mfn_to_page(m2p_start_mfn + i);
> -            share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
> -        }
> +            share_xen_page_with_privileged_guests(
> +                mfn_to_page(m2p_start_mfn + i), SHARE_ro);
>       }
>   
>       for ( v  = RDWR_COMPAT_MPT_VIRT_START;
> @@ -880,10 +878,8 @@ void __init subarch_init_memory(void)
>           m2p_start_mfn = l2e_get_pfn(l2e);
>   
>           for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
> -        {
> -            struct page_info *page = mfn_to_page(m2p_start_mfn + i);
> -            share_xen_page_with_privileged_guests(page, XENSHARE_readonly);
> -        }
> +            share_xen_page_with_privileged_guests(
> +                mfn_to_page(m2p_start_mfn + i), SHARE_ro);
>       }
>   
>       /* Mark all of direct map NX if hardware supports it. */
> diff --git a/xen/common/trace.c b/xen/common/trace.c
> index 2e18702..680f6ae 100644
> --- a/xen/common/trace.c
> +++ b/xen/common/trace.c
> @@ -227,7 +227,6 @@ static int alloc_trace_bufs(unsigned int pages)
>       for_each_online_cpu(cpu)
>       {
>           struct t_buf *buf;
> -        struct page_info *pg;
>   
>           spin_lock_init(&per_cpu(t_lock, cpu));
>   
> @@ -242,16 +241,14 @@ static int alloc_trace_bufs(unsigned int pages)
>   
>           /* Now share the trace pages */
>           for ( i = 0; i < pages; i++ )
> -        {
> -            pg = mfn_to_page(t_info_mfn_list[offset + i]);
> -            share_xen_page_with_privileged_guests(pg, XENSHARE_writable);
> -        }
> +            share_xen_page_with_privileged_guests(
> +                mfn_to_page(t_info_mfn_list[offset + i]), SHARE_rw);
>       }
>   
>       /* Finally, share the t_info page */
>       for(i = 0; i < t_info_pages; i++)
>           share_xen_page_with_privileged_guests(
> -            virt_to_page(t_info) + i, XENSHARE_readonly);
> +            virt_to_page(t_info) + i, SHARE_ro);
>   
>       data_size  = (pages * PAGE_SIZE - sizeof(struct t_buf));
>       t_buf_highwater = data_size >> 1; /* 50% high water */
> diff --git a/xen/common/xenoprof.c b/xen/common/xenoprof.c
> index 5acdde5..c1b4303 100644
> --- a/xen/common/xenoprof.c
> +++ b/xen/common/xenoprof.c
> @@ -159,8 +159,7 @@ share_xenoprof_page_with_guest(struct domain *d, mfn_t mfn, int npages)
>       }
>   
>       for ( i = 0; i < npages; i++ )
> -        share_xen_page_with_guest(mfn_to_page(mfn_add(mfn, i)),
> -                                  d, XENSHARE_writable);
> +        share_xen_page_with_guest(mfn_to_page(mfn_add(mfn, i)), d, SHARE_rw);
>   
>       return 0;
>   }
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
> index 5b8994c..be7ed75 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -81,8 +81,7 @@ static inline unsigned int gnttab_dom0_max(void)
>   #define gnttab_create_shared_page(d, t, i)                               \
>       do {                                                                 \
>           share_xen_page_with_guest(                                       \
> -            virt_to_page((char *)(t)->shared_raw[i]),                    \
> -            (d), XENSHARE_writable);                                     \
> +            virt_to_page((char *)(t)->shared_raw[i]), (d), SHARE_rw);    \
>       } while ( 0 )
>   
>   #define gnttab_shared_gmfn(d, t, i)                                      \
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 4d5563b..a0e922f 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -159,13 +159,6 @@ extern vaddr_t xenheap_virt_start;
>   
>   #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
>   
> -#define XENSHARE_writable 0
> -#define XENSHARE_readonly 1
> -extern void share_xen_page_with_guest(
> -    struct page_info *page, struct domain *d, int readonly);
> -extern void share_xen_page_with_privileged_guests(
> -    struct page_info *page, int readonly);
> -
>   #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
>   /* PDX of the first page in the frame table. */
>   extern unsigned long frametable_base_pdx;
> diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
> index 66e9742..8720159 100644
> --- a/xen/include/asm-x86/grant_table.h
> +++ b/xen/include/asm-x86/grant_table.h
> @@ -57,15 +57,13 @@ static inline unsigned int gnttab_dom0_max(void)
>   #define gnttab_create_shared_page(d, t, i)                               \
>       do {                                                                 \
>           share_xen_page_with_guest(                                       \
> -            virt_to_page((char *)(t)->shared_raw[i]),                    \
> -            (d), XENSHARE_writable);                                     \
> +            virt_to_page((char *)(t)->shared_raw[i]), (d), SHARE_rw);    \
>       } while ( 0 )
>   
>   #define gnttab_create_status_page(d, t, i)                               \
>       do {                                                                 \
>           share_xen_page_with_guest(                                       \
> -           virt_to_page((char *)(t)->status[i]),                         \
> -            (d), XENSHARE_writable);                                     \
> +            virt_to_page((char *)(t)->status[i]), (d), SHARE_rw);        \
>       } while ( 0 )
>   
>   
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 96f3a34..c115661 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -292,14 +292,6 @@ struct page_info
>   
>   #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
>   
> -#define XENSHARE_writable 0
> -#define XENSHARE_readonly 1
> -extern void share_xen_page_with_guest(
> -    struct page_info *page, struct domain *d, int readonly);
> -extern int unshare_xen_page_with_guest(struct page_info *page,
> -                                       struct domain *d);
> -extern void share_xen_page_with_privileged_guests(
> -    struct page_info *page, int readonly);
>   extern void free_shared_domheap_page(struct page_info *page);
>   
>   #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 0e0e511..142aa73 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -634,4 +634,18 @@ static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
>       }
>   }
>   
> +enum XENSHARE_flags {
> +    SHARE_rw,
> +    SHARE_ro,
> +};
> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
> +                               enum XENSHARE_flags flags);
> +int unshare_xen_page_with_guest(struct page_info *page, struct domain *d);
> +
> +static inline void share_xen_page_with_privileged_guests(
> +    struct page_info *page, enum XENSHARE_flags flags)
> +{
> +    share_xen_page_with_guest(page, dom_xen, flags);
> +}
> +
>   #endif /* __XEN_MM_H__ */
> 

-- 
Julien Grall

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

Re: [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Roger Pau Monné]

On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
> The share_xen_page_with_guest() functions are used by common code, and are
> implemented the same by each arch.  Move the declarations into the common mm.h
> rather than duplicating them in each arch/mm.h
> 
> Turn an int readonly into a boolean enum, to retain ro/rw context at the
> callsites, but use shorter labels which avoids a large number of split lines.
> 
> Implement share_xen_page_with_privileged_guests() as a static inline wrapper
> around share_xen_page_with_guest() to avoid having a call into a separate
> translation unit whose only purpose is to shuffle function arguments.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/arm/domain.c             |  3 +--
>  xen/arch/arm/mm.c                 | 13 ++++---------
>  xen/arch/x86/domain.c             |  3 +--
>  xen/arch/x86/hvm/dom0_build.c     |  2 +-
>  xen/arch/x86/hvm/vmx/vmx.c        |  2 +-
>  xen/arch/x86/mm.c                 | 20 +++++++-------------
>  xen/arch/x86/pv/shim.c            |  6 ++----
>  xen/arch/x86/x86_64/mm.c          | 16 ++++++----------
>  xen/common/trace.c                |  9 +++------
>  xen/common/xenoprof.c             |  3 +--
>  xen/include/asm-arm/grant_table.h |  3 +--
>  xen/include/asm-arm/mm.h          |  7 -------
>  xen/include/asm-x86/grant_table.h |  6 ++----
>  xen/include/asm-x86/mm.h          |  8 --------
>  xen/include/xen/mm.h              | 14 ++++++++++++++
>  15 files changed, 44 insertions(+), 71 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 4b45fad..23dac5d 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -602,8 +602,7 @@ int arch_domain_create(struct domain *d,
>          goto fail;
>  
>      clear_page(d->shared_info);
> -    share_xen_page_with_guest(
> -        virt_to_page(d->shared_info), d, XENSHARE_writable);
> +    share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
>  
>      switch ( config->config.gic_version )
>      {
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index a09bea2..baa3b0d 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
>      return gfn_x(d->arch.p2m.max_mapped_gfn);
>  }
>  
> -void share_xen_page_with_guest(struct page_info *page,
> -                          struct domain *d, int readonly)
> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
> +                               enum XENSHARE_flags flags)

Naming this _flags feels wrong to me, I would assume flags to be
something which can be used as (SHARE_r | SHARE_w) (ie: stacked) and
so on. I would maybe name this XENSHARE_options rather than flags.

TBH I would be OK with renaming the parameter to "bool ro/readonly"
and let the callers use true and false directly. It seems like
over-engineering to use an enum for this, or maybe you have further
changes in mind that are going to expand the set of options?

Thanks, Roger.

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

Re: [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Jan Beulich]

>>> On 13.03.18 at 13:28, <roger.pau@citrix.com> wrote:
> On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
>>      return gfn_x(d->arch.p2m.max_mapped_gfn);
>>  }
>>  
>> -void share_xen_page_with_guest(struct page_info *page,
>> -                          struct domain *d, int readonly)
>> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>> +                               enum XENSHARE_flags flags)
> 
> Naming this _flags feels wrong to me, I would assume flags to be
> something which can be used as (SHARE_r | SHARE_w) (ie: stacked) and
> so on. I would maybe name this XENSHARE_options rather than flags.
> 
> TBH I would be OK with renaming the parameter to "bool ro/readonly"
> and let the callers use true and false directly. It seems like
> over-engineering to use an enum for this, or maybe you have further
> changes in mind that are going to expand the set of options?

On one hand I agree that an enum like this is somewhat strange
to have, and a boolean would seem like a better fit. Otoh using
plain true/false at the call sites would make it pretty unclear
whether "true" means r/o or r/w. So another option might be
to have multiple inline wrappers around the actual worker, like
share_xen_page_with_guest_ro().

Nevertheless the x86 parts of the patch can also have
Acked-by: Jan Beulich <jbeulich@suse.com>
as they currently are.

Jan


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

Re: [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Andrew Cooper]

On 13/03/18 14:39, Jan Beulich wrote:
>>>> On 13.03.18 at 13:28, <roger.pau@citrix.com> wrote:
>> On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
>>>      return gfn_x(d->arch.p2m.max_mapped_gfn);
>>>  }
>>>  
>>> -void share_xen_page_with_guest(struct page_info *page,
>>> -                          struct domain *d, int readonly)
>>> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>>> +                               enum XENSHARE_flags flags)
>> Naming this _flags feels wrong to me, I would assume flags to be
>> something which can be used as (SHARE_r | SHARE_w) (ie: stacked) and
>> so on. I would maybe name this XENSHARE_options rather than flags.
>>
>> TBH I would be OK with renaming the parameter to "bool ro/readonly"
>> and let the callers use true and false directly. It seems like
>> over-engineering to use an enum for this, or maybe you have further
>> changes in mind that are going to expand the set of options?
> On one hand I agree that an enum like this is somewhat strange
> to have, and a boolean would seem like a better fit. Otoh using
> plain true/false at the call sites would make it pretty unclear
> whether "true" means r/o or r/w. So another option might be
> to have multiple inline wrappers around the actual worker, like
> share_xen_page_with_guest_ro().

Splitting into (SHARE_r | SHARE_w( doesn't make sense because the
underlying implementation take a boolean idea of whether to use PGT_none
or PGT_writable_page.

We've already got share_xen_page_with_privileged_guests() as a wrapper
around share_xen_page_with_guest().  Therefore, we'd end up with a total
of 4 extra wrappers if we wanted _rw and _ro suffixes, which seems over
the top to me.

I agree its not completely great like this, but it is the least bad
option I managed to come up with.

~Andrew

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

Re: [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Jan Beulich]

>>> On 15.03.18 at 21:25, <andrew.cooper3@citrix.com> wrote:
> On 13/03/18 14:39, Jan Beulich wrote:
>>>>> On 13.03.18 at 13:28, <roger.pau@citrix.com> wrote:
>>> On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
>>>> --- a/xen/arch/arm/mm.c
>>>> +++ b/xen/arch/arm/mm.c
>>>> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain 
> *d)
>>>>      return gfn_x(d->arch.p2m.max_mapped_gfn);
>>>>  }
>>>>  
>>>> -void share_xen_page_with_guest(struct page_info *page,
>>>> -                          struct domain *d, int readonly)
>>>> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>>>> +                               enum XENSHARE_flags flags)
>>> Naming this _flags feels wrong to me, I would assume flags to be
>>> something which can be used as (SHARE_r | SHARE_w) (ie: stacked) and
>>> so on. I would maybe name this XENSHARE_options rather than flags.
>>>
>>> TBH I would be OK with renaming the parameter to "bool ro/readonly"
>>> and let the callers use true and false directly. It seems like
>>> over-engineering to use an enum for this, or maybe you have further
>>> changes in mind that are going to expand the set of options?
>> On one hand I agree that an enum like this is somewhat strange
>> to have, and a boolean would seem like a better fit. Otoh using
>> plain true/false at the call sites would make it pretty unclear
>> whether "true" means r/o or r/w. So another option might be
>> to have multiple inline wrappers around the actual worker, like
>> share_xen_page_with_guest_ro().
> 
> Splitting into (SHARE_r | SHARE_w( doesn't make sense because the
> underlying implementation take a boolean idea of whether to use PGT_none
> or PGT_writable_page.
> 
> We've already got share_xen_page_with_privileged_guests() as a wrapper
> around share_xen_page_with_guest().  Therefore, we'd end up with a total
> of 4 extra wrappers if we wanted _rw and _ro suffixes, which seems over
> the top to me.
> 
> I agree its not completely great like this, but it is the least bad
> option I managed to come up with.

Well, without wanting put under question the ack I've already
given, the question of course is whether the code is much
better after the change than it was before. If there's no really
good shape to put this in, leaving things as they are is certainly
also an option.

Jan


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

Re: [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Andrew Cooper]

On 16/03/2018 07:43, Jan Beulich wrote:
>>>> On 15.03.18 at 21:25, <andrew.cooper3@citrix.com> wrote:
>> On 13/03/18 14:39, Jan Beulich wrote:
>>>>>> On 13.03.18 at 13:28, <roger.pau@citrix.com> wrote:
>>>> On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
>>>>> --- a/xen/arch/arm/mm.c
>>>>> +++ b/xen/arch/arm/mm.c
>>>>> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain 
>> *d)
>>>>>      return gfn_x(d->arch.p2m.max_mapped_gfn);
>>>>>  }
>>>>>  
>>>>> -void share_xen_page_with_guest(struct page_info *page,
>>>>> -                          struct domain *d, int readonly)
>>>>> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>>>>> +                               enum XENSHARE_flags flags)
>>>> Naming this _flags feels wrong to me, I would assume flags to be
>>>> something which can be used as (SHARE_r | SHARE_w) (ie: stacked) and
>>>> so on. I would maybe name this XENSHARE_options rather than flags.
>>>>
>>>> TBH I would be OK with renaming the parameter to "bool ro/readonly"
>>>> and let the callers use true and false directly. It seems like
>>>> over-engineering to use an enum for this, or maybe you have further
>>>> changes in mind that are going to expand the set of options?
>>> On one hand I agree that an enum like this is somewhat strange
>>> to have, and a boolean would seem like a better fit. Otoh using
>>> plain true/false at the call sites would make it pretty unclear
>>> whether "true" means r/o or r/w. So another option might be
>>> to have multiple inline wrappers around the actual worker, like
>>> share_xen_page_with_guest_ro().
>> Splitting into (SHARE_r | SHARE_w( doesn't make sense because the
>> underlying implementation take a boolean idea of whether to use PGT_none
>> or PGT_writable_page.
>>
>> We've already got share_xen_page_with_privileged_guests() as a wrapper
>> around share_xen_page_with_guest().  Therefore, we'd end up with a total
>> of 4 extra wrappers if we wanted _rw and _ro suffixes, which seems over
>> the top to me.
>>
>> I agree its not completely great like this, but it is the least bad
>> option I managed to come up with.
> Well, without wanting put under question the ack I've already
> given, the question of course is whether the code is much
> better after the change than it was before. If there's no really
> good shape to put this in, leaving things as they are is certainly
> also an option.

The code is in better shape with the change, than without it (common
prototypes, share_xen_page_with_privileged_guests() being a wrapper,
fewer linebreaks, etc).  I just can't see a way of getting it into a
yet-better state.

~Andrew

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