[PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Andrew Cooper]

This essentially reverts c/s 2037f2adb "x86: introduce
alloc_vcpu_guest_context()", including the newer arm bits, but achieves
the same end goal by using the newer vmalloc() infrastructure.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>

Build tested on all architectures, functionally tested on x86.
---
 xen/arch/arm/domain.c        |   11 ----------
 xen/arch/arm/vpsci.c         |    5 +++--
 xen/arch/x86/domain.c        |   46 ------------------------------------------
 xen/common/domain.c          |    7 ++++---
 xen/common/domctl.c          |    5 +++--
 xen/include/asm-x86/fixmap.h |    3 ---
 xen/include/xen/domain.h     |    6 ------
 7 files changed, 10 insertions(+), 73 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index b2bfc7d..e0ef0d8 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -466,17 +466,6 @@ void free_vcpu_struct(struct vcpu *v)
     free_xenheap_page(v);
 }
 
-struct vcpu_guest_context *alloc_vcpu_guest_context(void)
-{
-    return xmalloc(struct vcpu_guest_context);
-
-}
-
-void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
-{
-    xfree(vgc);
-}
-
 int vcpu_initialise(struct vcpu *v)
 {
     int rc = 0;
diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index aebe1e2..ba3414d 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -13,6 +13,7 @@
 #include <xen/errno.h>
 #include <xen/sched.h>
 #include <xen/types.h>
+#include <xen/vmap.h>
 
 #include <asm/current.h>
 #include <asm/gic.h>
@@ -45,7 +46,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
             ( !test_bit(_VPF_down, &v->pause_flags) ) )
         return PSCI_ALREADY_ON;
 
-    if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
+    if ( (ctxt = vmalloc(sizeof(struct vcpu_guest_context))) == NULL )
         return PSCI_DENIED;
 
     vgic_clear_pending_irqs(v);
@@ -78,7 +79,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
 
     domain_lock(d);
     rc = arch_set_info_guest(v, ctxt);
-    free_vcpu_guest_context(ctxt);
+    vfree(ctxt);
 
     if ( rc < 0 )
     {
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 045f6ff..12ccdb8 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -48,7 +48,6 @@
 #include <asm/cpuidle.h>
 #include <asm/mpspec.h>
 #include <asm/ldt.h>
-#include <asm/fixmap.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/viridian.h>
@@ -272,51 +271,6 @@ void free_vcpu_struct(struct vcpu *v)
     free_xenheap_page(v);
 }
 
-static DEFINE_PER_CPU(struct page_info *[
-    PFN_UP(sizeof(struct vcpu_guest_context))], vgc_pages);
-
-struct vcpu_guest_context *alloc_vcpu_guest_context(void)
-{
-    unsigned int i, cpu = smp_processor_id();
-    enum fixed_addresses idx = FIX_VGC_BEGIN -
-        cpu * PFN_UP(sizeof(struct vcpu_guest_context));
-
-    BUG_ON(per_cpu(vgc_pages[0], cpu) != NULL);
-
-    for ( i = 0; i < PFN_UP(sizeof(struct vcpu_guest_context)); ++i )
-    {
-        struct page_info *pg = alloc_domheap_page(current->domain,
-                                                  MEMF_no_owner);
-
-        if ( unlikely(pg == NULL) )
-        {
-            free_vcpu_guest_context(NULL);
-            return NULL;
-        }
-        __set_fixmap(idx - i, page_to_mfn(pg), __PAGE_HYPERVISOR_RW);
-        per_cpu(vgc_pages[i], cpu) = pg;
-    }
-    return (void *)fix_to_virt(idx);
-}
-
-void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
-{
-    unsigned int i, cpu = smp_processor_id();
-    enum fixed_addresses idx = FIX_VGC_BEGIN -
-        cpu * PFN_UP(sizeof(struct vcpu_guest_context));
-
-    BUG_ON(vgc && vgc != (void *)fix_to_virt(idx));
-
-    for ( i = 0; i < PFN_UP(sizeof(struct vcpu_guest_context)); ++i )
-    {
-        if ( !per_cpu(vgc_pages[i], cpu) )
-            continue;
-        clear_fixmap(idx - i);
-        free_domheap_page(per_cpu(vgc_pages[i], cpu));
-        per_cpu(vgc_pages[i], cpu) = NULL;
-    }
-}
-
 static int setup_compat_l4(struct vcpu *v)
 {
     struct page_info *pg;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 1b9fcfc..31d9e56 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -43,6 +43,7 @@
 #include <xen/trace.h>
 #include <xen/tmem.h>
 #include <asm/setup.h>
+#include <xen/vmap.h>
 
 /* Linux config option: propageted to domain0 */
 /* xen_processor_pmbits: xen control Cx, Px, ... */
@@ -1185,12 +1186,12 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( v->vcpu_info == &dummy_vcpu_info )
             return -EINVAL;
 
-        if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
+        if ( (ctxt = vmalloc(sizeof(struct vcpu_guest_context))) == NULL )
             return -ENOMEM;
 
         if ( copy_from_guest(ctxt, arg, 1) )
         {
-            free_vcpu_guest_context(ctxt);
+            vfree(ctxt);
             return -EFAULT;
         }
 
@@ -1198,7 +1199,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
         rc = v->is_initialised ? -EEXIST : arch_set_info_guest(v, ctxt);
         domain_unlock(d);
 
-        free_vcpu_guest_context(ctxt);
+        vfree(ctxt);
 
         if ( rc == -ERESTART )
             rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 7f959f3..90cc13c 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -32,6 +32,7 @@
 #include <asm/monitor.h>
 #include <public/domctl.h>
 #include <xsm/xsm.h>
+#include <xen/vmap.h>
 
 static DEFINE_SPINLOCK(domctl_lock);
 DEFINE_SPINLOCK(vcpu_alloc_lock);
@@ -492,7 +493,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                      < sizeof(struct compat_vcpu_guest_context));
 #endif
         ret = -ENOMEM;
-        if ( (c.nat = alloc_vcpu_guest_context()) == NULL )
+        if ( (c.nat = vmalloc(sizeof(struct vcpu_guest_context))) == NULL )
             break;
 
 #ifdef CONFIG_COMPAT
@@ -518,7 +519,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                           __HYPERVISOR_domctl, "h", u_domctl);
         }
 
-        free_vcpu_guest_context(c.nat);
+        vfree(c.nat);
         break;
     }
 
diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
index 70eadff..1e24b11 100644
--- a/xen/include/asm-x86/fixmap.h
+++ b/xen/include/asm-x86/fixmap.h
@@ -47,9 +47,6 @@ enum fixed_addresses {
     FIX_COM_END,
     FIX_EHCI_DBGP,
     /* Everything else should go further down. */
-    FIX_VGC_END,
-    FIX_VGC_BEGIN = FIX_VGC_END
-      + PFN_UP(sizeof(struct vcpu_guest_context)) * NR_CPUS - 1,
     FIX_APIC_BASE,
     FIX_IO_APIC_BASE_0,
     FIX_IO_APIC_BASE_END = FIX_IO_APIC_BASE_0 + MAX_IO_APICS-1,
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 848db8a..db191d3 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -31,12 +31,6 @@ struct vcpu *alloc_vcpu(
 struct vcpu *alloc_vcpu_struct(void);
 void free_vcpu_struct(struct vcpu *v);
 
-/* Allocate/free a vcpu_guest_context structure. */
-#ifndef alloc_vcpu_guest_context
-struct vcpu_guest_context *alloc_vcpu_guest_context(void);
-void free_vcpu_guest_context(struct vcpu_guest_context *);
-#endif
-
 /* Allocate/free a PIRQ structure. */
 #ifndef alloc_pirq_struct
 struct pirq *alloc_pirq_struct(struct domain *);
-- 
1.7.10.4

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Konrad Rzeszutek Wilk]

On Fri, Aug 21, 2015 at 06:51:46PM +0100, Andrew Cooper wrote:
> This essentially reverts c/s 2037f2adb "x86: introduce
> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
> the same end goal by using the newer vmalloc() infrastructure.

Could you explain what this fixes? Or perhaps with an explanation
of why this will make Xen [ ]better; [ ] faster [ ] magical.

:-)


Thanks.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> 
> Build tested on all architectures, functionally tested on x86.
> ---
>  xen/arch/arm/domain.c        |   11 ----------
>  xen/arch/arm/vpsci.c         |    5 +++--
>  xen/arch/x86/domain.c        |   46 ------------------------------------------
>  xen/common/domain.c          |    7 ++++---
>  xen/common/domctl.c          |    5 +++--
>  xen/include/asm-x86/fixmap.h |    3 ---
>  xen/include/xen/domain.h     |    6 ------
>  7 files changed, 10 insertions(+), 73 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index b2bfc7d..e0ef0d8 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -466,17 +466,6 @@ void free_vcpu_struct(struct vcpu *v)
>      free_xenheap_page(v);
>  }
>  
> -struct vcpu_guest_context *alloc_vcpu_guest_context(void)
> -{
> -    return xmalloc(struct vcpu_guest_context);
> -
> -}
> -
> -void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
> -{
> -    xfree(vgc);
> -}
> -
>  int vcpu_initialise(struct vcpu *v)
>  {
>      int rc = 0;
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index aebe1e2..ba3414d 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -13,6 +13,7 @@
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/types.h>
> +#include <xen/vmap.h>
>  
>  #include <asm/current.h>
>  #include <asm/gic.h>
> @@ -45,7 +46,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
>              ( !test_bit(_VPF_down, &v->pause_flags) ) )
>          return PSCI_ALREADY_ON;
>  
> -    if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
> +    if ( (ctxt = vmalloc(sizeof(struct vcpu_guest_context))) == NULL )
>          return PSCI_DENIED;
>  
>      vgic_clear_pending_irqs(v);
> @@ -78,7 +79,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
>  
>      domain_lock(d);
>      rc = arch_set_info_guest(v, ctxt);
> -    free_vcpu_guest_context(ctxt);
> +    vfree(ctxt);
>  
>      if ( rc < 0 )
>      {
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 045f6ff..12ccdb8 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -48,7 +48,6 @@
>  #include <asm/cpuidle.h>
>  #include <asm/mpspec.h>
>  #include <asm/ldt.h>
> -#include <asm/fixmap.h>
>  #include <asm/hvm/hvm.h>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/viridian.h>
> @@ -272,51 +271,6 @@ void free_vcpu_struct(struct vcpu *v)
>      free_xenheap_page(v);
>  }
>  
> -static DEFINE_PER_CPU(struct page_info *[
> -    PFN_UP(sizeof(struct vcpu_guest_context))], vgc_pages);
> -
> -struct vcpu_guest_context *alloc_vcpu_guest_context(void)
> -{
> -    unsigned int i, cpu = smp_processor_id();
> -    enum fixed_addresses idx = FIX_VGC_BEGIN -
> -        cpu * PFN_UP(sizeof(struct vcpu_guest_context));
> -
> -    BUG_ON(per_cpu(vgc_pages[0], cpu) != NULL);
> -
> -    for ( i = 0; i < PFN_UP(sizeof(struct vcpu_guest_context)); ++i )
> -    {
> -        struct page_info *pg = alloc_domheap_page(current->domain,
> -                                                  MEMF_no_owner);
> -
> -        if ( unlikely(pg == NULL) )
> -        {
> -            free_vcpu_guest_context(NULL);
> -            return NULL;
> -        }
> -        __set_fixmap(idx - i, page_to_mfn(pg), __PAGE_HYPERVISOR_RW);
> -        per_cpu(vgc_pages[i], cpu) = pg;
> -    }
> -    return (void *)fix_to_virt(idx);
> -}
> -
> -void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
> -{
> -    unsigned int i, cpu = smp_processor_id();
> -    enum fixed_addresses idx = FIX_VGC_BEGIN -
> -        cpu * PFN_UP(sizeof(struct vcpu_guest_context));
> -
> -    BUG_ON(vgc && vgc != (void *)fix_to_virt(idx));
> -
> -    for ( i = 0; i < PFN_UP(sizeof(struct vcpu_guest_context)); ++i )
> -    {
> -        if ( !per_cpu(vgc_pages[i], cpu) )
> -            continue;
> -        clear_fixmap(idx - i);
> -        free_domheap_page(per_cpu(vgc_pages[i], cpu));
> -        per_cpu(vgc_pages[i], cpu) = NULL;
> -    }
> -}
> -
>  static int setup_compat_l4(struct vcpu *v)
>  {
>      struct page_info *pg;
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 1b9fcfc..31d9e56 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -43,6 +43,7 @@
>  #include <xen/trace.h>
>  #include <xen/tmem.h>
>  #include <asm/setup.h>
> +#include <xen/vmap.h>
>  
>  /* Linux config option: propageted to domain0 */
>  /* xen_processor_pmbits: xen control Cx, Px, ... */
> @@ -1185,12 +1186,12 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( v->vcpu_info == &dummy_vcpu_info )
>              return -EINVAL;
>  
> -        if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
> +        if ( (ctxt = vmalloc(sizeof(struct vcpu_guest_context))) == NULL )
>              return -ENOMEM;
>  
>          if ( copy_from_guest(ctxt, arg, 1) )
>          {
> -            free_vcpu_guest_context(ctxt);
> +            vfree(ctxt);
>              return -EFAULT;
>          }
>  
> @@ -1198,7 +1199,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>          rc = v->is_initialised ? -EEXIST : arch_set_info_guest(v, ctxt);
>          domain_unlock(d);
>  
> -        free_vcpu_guest_context(ctxt);
> +        vfree(ctxt);
>  
>          if ( rc == -ERESTART )
>              rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 7f959f3..90cc13c 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -32,6 +32,7 @@
>  #include <asm/monitor.h>
>  #include <public/domctl.h>
>  #include <xsm/xsm.h>
> +#include <xen/vmap.h>
>  
>  static DEFINE_SPINLOCK(domctl_lock);
>  DEFINE_SPINLOCK(vcpu_alloc_lock);
> @@ -492,7 +493,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>                       < sizeof(struct compat_vcpu_guest_context));
>  #endif
>          ret = -ENOMEM;
> -        if ( (c.nat = alloc_vcpu_guest_context()) == NULL )
> +        if ( (c.nat = vmalloc(sizeof(struct vcpu_guest_context))) == NULL )
>              break;
>  
>  #ifdef CONFIG_COMPAT
> @@ -518,7 +519,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>                            __HYPERVISOR_domctl, "h", u_domctl);
>          }
>  
> -        free_vcpu_guest_context(c.nat);
> +        vfree(c.nat);
>          break;
>      }
>  
> diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
> index 70eadff..1e24b11 100644
> --- a/xen/include/asm-x86/fixmap.h
> +++ b/xen/include/asm-x86/fixmap.h
> @@ -47,9 +47,6 @@ enum fixed_addresses {
>      FIX_COM_END,
>      FIX_EHCI_DBGP,
>      /* Everything else should go further down. */
> -    FIX_VGC_END,
> -    FIX_VGC_BEGIN = FIX_VGC_END
> -      + PFN_UP(sizeof(struct vcpu_guest_context)) * NR_CPUS - 1,
>      FIX_APIC_BASE,
>      FIX_IO_APIC_BASE_0,
>      FIX_IO_APIC_BASE_END = FIX_IO_APIC_BASE_0 + MAX_IO_APICS-1,
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 848db8a..db191d3 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -31,12 +31,6 @@ struct vcpu *alloc_vcpu(
>  struct vcpu *alloc_vcpu_struct(void);
>  void free_vcpu_struct(struct vcpu *v);
>  
> -/* Allocate/free a vcpu_guest_context structure. */
> -#ifndef alloc_vcpu_guest_context
> -struct vcpu_guest_context *alloc_vcpu_guest_context(void);
> -void free_vcpu_guest_context(struct vcpu_guest_context *);
> -#endif
> -
>  /* Allocate/free a PIRQ structure. */
>  #ifndef alloc_pirq_struct
>  struct pirq *alloc_pirq_struct(struct domain *);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Andrew Cooper]

On 21/08/15 18:55, Konrad Rzeszutek Wilk wrote:
> On Fri, Aug 21, 2015 at 06:51:46PM +0100, Andrew Cooper wrote:
>> This essentially reverts c/s 2037f2adb "x86: introduce
>> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
>> the same end goal by using the newer vmalloc() infrastructure.
> Could you explain what this fixes? Or perhaps with an explanation
> of why this will make Xen [ ]better; [ ] faster [ ] magical.
>
> :-)

Ain't the diffstat enough to qualify for [x]better ;) ?

>
>
> Thanks.

It is relevant to a patch of Rogers which I am reviewing from the no-DM
series.  I was writing in reply to that, but this can also do.

alloc_vcpu_guest_context() was introduced long before vmalloc(), and
attempts to make the same end result using per-pcpu fixmap entries,
which scale by the compile-time NR_CPUS.

This patch causes a net reduction in compiled size for each arch (small
for ARM, larger for x86) and removes a scalability limit for compiling
with large numbers of cpus.

I think I can guess what you are going to ask me to do, given this
explanation.

~Andrew

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Jan Beulich]

>>> On 21.08.15 at 20:10, <andrew.cooper3@citrix.com> wrote:
> On 21/08/15 18:55, Konrad Rzeszutek Wilk wrote:
>> On Fri, Aug 21, 2015 at 06:51:46PM +0100, Andrew Cooper wrote:
>>> This essentially reverts c/s 2037f2adb "x86: introduce
>>> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
>>> the same end goal by using the newer vmalloc() infrastructure.
>> Could you explain what this fixes? Or perhaps with an explanation
>> of why this will make Xen [ ]better; [ ] faster [ ] magical.
>>
>> :-)
> 
> Ain't the diffstat enough to qualify for [x]better ;) ?
> 
>>
>>
>> Thanks.
> 
> It is relevant to a patch of Rogers which I am reviewing from the no-DM
> series.  I was writing in reply to that, but this can also do.
> 
> alloc_vcpu_guest_context() was introduced long before vmalloc(), and
> attempts to make the same end result using per-pcpu fixmap entries,
> which scale by the compile-time NR_CPUS.
> 
> This patch causes a net reduction in compiled size for each arch (small
> for ARM, larger for x86) and removes a scalability limit for compiling
> with large numbers of cpus.
> 
> I think I can guess what you are going to ask me to do, given this
> explanation.

To be honest the patch (and its description) looks fine to me as is.

Jan

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Jan Beulich]

>>> On 21.08.15 at 19:51, <andrew.cooper3@citrix.com> wrote:
> This essentially reverts c/s 2037f2adb "x86: introduce
> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
> the same end goal by using the newer vmalloc() infrastructure.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Julien Grall]

Hi Andrew,

On 21/08/15 18:51, Andrew Cooper wrote:
> This essentially reverts c/s 2037f2adb "x86: introduce
> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
> the same end goal by using the newer vmalloc() infrastructure.

I would keep alloc_vcpu_guest_context and replace the content by
vmalloc(...). It would avoid to open-coding the allocation on the vCPU
on different places.

Regards,

-- 
Julien Grall

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Andrew Cooper]

On 28/08/15 13:41, Julien Grall wrote:
> Hi Andrew,
>
> On 21/08/15 18:51, Andrew Cooper wrote:
>> This essentially reverts c/s 2037f2adb "x86: introduce
>> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
>> the same end goal by using the newer vmalloc() infrastructure.
> I would keep alloc_vcpu_guest_context and replace the content by
> vmalloc(...). It would avoid to open-coding the allocation on the vCPU
> on different places.

alloc_vcpu_guest_context() only existed because x86 used to need to do
something quite cumbersome.  This is no longer the case, given vmalloc()
as a more general solution.

Retaining alloc_vcpu_guest_context() as just think wrapper, identical on
all architectures, is a bad idea as it call into a separate translation
unit which cannot be optimised.

~Andrew

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Julien Grall]

On 28/08/15 13:56, Andrew Cooper wrote:
> On 28/08/15 13:41, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 21/08/15 18:51, Andrew Cooper wrote:
>>> This essentially reverts c/s 2037f2adb "x86: introduce
>>> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
>>> the same end goal by using the newer vmalloc() infrastructure.
>> I would keep alloc_vcpu_guest_context and replace the content by
>> vmalloc(...). It would avoid to open-coding the allocation on the vCPU
>> on different places.
> 
> alloc_vcpu_guest_context() only existed because x86 used to need to do
> something quite cumbersome.  This is no longer the case, given vmalloc()
> as a more general solution.
> 
> Retaining alloc_vcpu_guest_context() as just think wrapper, identical on
> all architectures, is a bad idea as it call into a separate translation
> unit which cannot be optimised.

Unless if you introduce a static inline helper in the header. It would
avoid open coding vmalloc and make easier future usage of it.

Regards,

-- 
Julien Grall

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Andrew Cooper]

On 28/08/15 13:57, Julien Grall wrote:
> On 28/08/15 13:56, Andrew Cooper wrote:
>> On 28/08/15 13:41, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 21/08/15 18:51, Andrew Cooper wrote:
>>>> This essentially reverts c/s 2037f2adb "x86: introduce
>>>> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
>>>> the same end goal by using the newer vmalloc() infrastructure.
>>> I would keep alloc_vcpu_guest_context and replace the content by
>>> vmalloc(...). It would avoid to open-coding the allocation on the vCPU
>>> on different places.
>> alloc_vcpu_guest_context() only existed because x86 used to need to do
>> something quite cumbersome.  This is no longer the case, given vmalloc()
>> as a more general solution.
>>
>> Retaining alloc_vcpu_guest_context() as just think wrapper, identical on
>> all architectures, is a bad idea as it call into a separate translation
>> unit which cannot be optimised.
> Unless if you introduce a static inline helper in the header. It would
> avoid open coding vmalloc and make easier future usage of it.

Hiding the type allocated makes the code harder to read, not easier.

We don't special case other plain allocations like this, so I still
don't see a compelling reason to break the norm here.

~Andrew

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Julien Grall]

On 28/08/15 14:07, Andrew Cooper wrote:
> On 28/08/15 13:57, Julien Grall wrote:
>> On 28/08/15 13:56, Andrew Cooper wrote:
>>> On 28/08/15 13:41, Julien Grall wrote:
>>>> Hi Andrew,
>>>>
>>>> On 21/08/15 18:51, Andrew Cooper wrote:
>>>>> This essentially reverts c/s 2037f2adb "x86: introduce
>>>>> alloc_vcpu_guest_context()", including the newer arm bits, but achieves
>>>>> the same end goal by using the newer vmalloc() infrastructure.
>>>> I would keep alloc_vcpu_guest_context and replace the content by
>>>> vmalloc(...). It would avoid to open-coding the allocation on the vCPU
>>>> on different places.
>>> alloc_vcpu_guest_context() only existed because x86 used to need to do
>>> something quite cumbersome.  This is no longer the case, given vmalloc()
>>> as a more general solution.
>>>
>>> Retaining alloc_vcpu_guest_context() as just think wrapper, identical on
>>> all architectures, is a bad idea as it call into a separate translation
>>> unit which cannot be optimised.
>> Unless if you introduce a static inline helper in the header. It would
>> avoid open coding vmalloc and make easier future usage of it.
> 
> Hiding the type allocated makes the code harder to read, not easier.
> 
> We don't special case other plain allocations like this, so I still
> don't see a compelling reason to break the norm here.

Let me explain it in a different way: allocation is usually done with
xmalloc, but here you are using vmalloc. Why did you use vmalloc rather
than xalloc? AFAICT there is no improvement on ARM.

If we open code the allocation, one could decide to use xmalloc which is
the common allocation. So what would be the drawback to use xmalloc vs
vmalloc?

Regards,

-- 
Julien Grall

Re: [PATCH for 4.7] xen: Replace alloc_vcpu_guest_context() with vmalloc()

[Jan Beulich]

>>> On 28.08.15 at 15:13, <julien.grall@citrix.com> wrote:
> Let me explain it in a different way: allocation is usually done with
> xmalloc, but here you are using vmalloc. Why did you use vmalloc rather
> than xalloc? AFAICT there is no improvement on ARM.
> 
> If we open code the allocation, one could decide to use xmalloc which is
> the common allocation. So what would be the drawback to use xmalloc vs
> vmalloc?

Actually I think Julien's point is valid: If ARM can get away with
xmalloc(), why should we force it to use the heavier vmalloc()?

Jan