[Xen-devel,v3,38/39] ARM: new VGIC: Allocate two pages for struct vcpu

Message ID 20180321163235.12529-39-andre.przywara@linaro.org
State New
Headers show
Series
  • New VGIC(-v2) implementation
Related show

Commit Message

Andre Przywara March 21, 2018, 4:32 p.m.
At the moment we allocate exactly one page for struct vcpu on ARM, also
have a check in place to prevent it growing beyond 4KB.
As the struct includes the state of all 32 private (per-VCPU) interrupts,
we are at 3840 bytes on arm64 at the moment already. Growing the per-IRQ
VGIC structure even slightly makes the VCPU quickly exceed the 4K limit.
The new VGIC will need more space per virtual IRQ. I spent a few hours
trying to trim this down, but couldn't get it below 4KB, even with the
nasty hacks piling up to save some bytes here and there.
It turns out that beyond efficiency, maybe, there is no real technical
reason this struct has to fit in one page, so lifting the limit to two
pages seems like the most pragmatic solution.
Restrict this to compiling with the new VGIC and for ARM64 only.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
Changelog v2 ... v3:
- rework alloc_vcpu_struct() to avoid nasty #ifdef

Changelog v1 ... v2:
- confine change to new VGIC and ARM64 only

 xen/arch/arm/domain.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Julien Grall March 22, 2018, 8:11 a.m. | #1
Hi Andre,

On 03/21/2018 04:32 PM, Andre Przywara wrote:
> At the moment we allocate exactly one page for struct vcpu on ARM, also
> have a check in place to prevent it growing beyond 4KB.
> As the struct includes the state of all 32 private (per-VCPU) interrupts,
> we are at 3840 bytes on arm64 at the moment already. Growing the per-IRQ
> VGIC structure even slightly makes the VCPU quickly exceed the 4K limit.
> The new VGIC will need more space per virtual IRQ. I spent a few hours
> trying to trim this down, but couldn't get it below 4KB, even with the
> nasty hacks piling up to save some bytes here and there.
> It turns out that beyond efficiency, maybe, there is no real technical
> reason this struct has to fit in one page, so lifting the limit to two
> pages seems like the most pragmatic solution.
> Restrict this to compiling with the new VGIC and for ARM64 only.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
> Changelog v2 ... v3:
> - rework alloc_vcpu_struct() to avoid nasty #ifdef
> 
> Changelog v1 ... v2:
> - confine change to new VGIC and ARM64 only
> 
>   xen/arch/arm/domain.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 9688e62f78..23bda3f7db 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -505,19 +505,36 @@ void dump_pageframe_info(struct domain *d)
>   
>   }
>   
> +/*
> + * The new VGIC has a bigger per-IRQ structure, so we need more than one
> + * page on ARM64. Cowardly increase the limit in this case.
> + */
> +#if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
> +#define PAGES_PER_VCPU  2
> +#else
> +#define PAGES_PER_VCPU  1
> +#endif
> +
>   struct vcpu *alloc_vcpu_struct(void)
>   {
>       struct vcpu *v;
> -    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
> -    v = alloc_xenheap_pages(0, 0);
> +
> +    BUILD_BUG_ON(sizeof(*v) > PAGES_PER_VCPU * PAGE_SIZE);
> +    v = alloc_xenheap_pages(get_order_from_pages(PAGES_PER_VCPU), 0);

I was suggesting to use get_order_from_pages(sizeof (...)) so if we end 
up to be smaller, you don't lose a page for nothing. But I am ok with 
that too and can revisit later. So:

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

>       if ( v != NULL )
> -        clear_page(v);
> +    {
> +        unsigned int i;
> +
> +        for ( i = 0; i < PAGES_PER_VCPU; i++ )
> +            clear_page((void *)v + i * PAGE_SIZE);
> +    }
> +
>       return v;
>   }
>   
>   void free_vcpu_struct(struct vcpu *v)
>   {
> -    free_xenheap_page(v);
> +    free_xenheap_pages(v, get_order_from_pages(PAGES_PER_VCPU));
>   }
>   
>   int vcpu_initialise(struct vcpu *v)
> 

Cheers,
Stefano Stabellini March 27, 2018, 11:07 p.m. | #2
On Thu, 22 Mar 2018, Julien Grall wrote:
> Hi Andre,
> 
> On 03/21/2018 04:32 PM, Andre Przywara wrote:
> > At the moment we allocate exactly one page for struct vcpu on ARM, also
> > have a check in place to prevent it growing beyond 4KB.
> > As the struct includes the state of all 32 private (per-VCPU) interrupts,
> > we are at 3840 bytes on arm64 at the moment already. Growing the per-IRQ
> > VGIC structure even slightly makes the VCPU quickly exceed the 4K limit.
> > The new VGIC will need more space per virtual IRQ. I spent a few hours
> > trying to trim this down, but couldn't get it below 4KB, even with the
> > nasty hacks piling up to save some bytes here and there.
> > It turns out that beyond efficiency, maybe, there is no real technical
> > reason this struct has to fit in one page, so lifting the limit to two
> > pages seems like the most pragmatic solution.
> > Restrict this to compiling with the new VGIC and for ARM64 only.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> > ---
> > Changelog v2 ... v3:
> > - rework alloc_vcpu_struct() to avoid nasty #ifdef
> > 
> > Changelog v1 ... v2:
> > - confine change to new VGIC and ARM64 only
> > 
> >   xen/arch/arm/domain.c | 25 +++++++++++++++++++++----
> >   1 file changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 9688e62f78..23bda3f7db 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -505,19 +505,36 @@ void dump_pageframe_info(struct domain *d)
> >     }
> >   +/*
> > + * The new VGIC has a bigger per-IRQ structure, so we need more than one
> > + * page on ARM64. Cowardly increase the limit in this case.
> > + */
> > +#if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
> > +#define PAGES_PER_VCPU  2
> > +#else
> > +#define PAGES_PER_VCPU  1
> > +#endif
> > +
> >   struct vcpu *alloc_vcpu_struct(void)
> >   {
> >       struct vcpu *v;
> > -    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
> > -    v = alloc_xenheap_pages(0, 0);
> > +
> > +    BUILD_BUG_ON(sizeof(*v) > PAGES_PER_VCPU * PAGE_SIZE);
> > +    v = alloc_xenheap_pages(get_order_from_pages(PAGES_PER_VCPU), 0);
> 
> I was suggesting to use get_order_from_pages(sizeof (...)) so if we end up to
> be smaller, you don't lose a page for nothing. But I am ok with that too and
> can revisit later. So:
> 
> Acked-by: Julien Grall <julien.grall@arm.com>

That is actually a good suggestion, let's not lose track of it.

With that fix:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>



> >       if ( v != NULL )
> > -        clear_page(v);
> > +    {
> > +        unsigned int i;
> > +
> > +        for ( i = 0; i < PAGES_PER_VCPU; i++ )
> > +            clear_page((void *)v + i * PAGE_SIZE);
> > +    }
> > +
> >       return v;
> >   }
> >     void free_vcpu_struct(struct vcpu *v)
> >   {
> > -    free_xenheap_page(v);
> > +    free_xenheap_pages(v, get_order_from_pages(PAGES_PER_VCPU));
> >   }
> >     int vcpu_initialise(struct vcpu *v)
> >

Patch

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 9688e62f78..23bda3f7db 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -505,19 +505,36 @@  void dump_pageframe_info(struct domain *d)
 
 }
 
+/*
+ * The new VGIC has a bigger per-IRQ structure, so we need more than one
+ * page on ARM64. Cowardly increase the limit in this case.
+ */
+#if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
+#define PAGES_PER_VCPU  2
+#else
+#define PAGES_PER_VCPU  1
+#endif
+
 struct vcpu *alloc_vcpu_struct(void)
 {
     struct vcpu *v;
-    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
-    v = alloc_xenheap_pages(0, 0);
+
+    BUILD_BUG_ON(sizeof(*v) > PAGES_PER_VCPU * PAGE_SIZE);
+    v = alloc_xenheap_pages(get_order_from_pages(PAGES_PER_VCPU), 0);
     if ( v != NULL )
-        clear_page(v);
+    {
+        unsigned int i;
+
+        for ( i = 0; i < PAGES_PER_VCPU; i++ )
+            clear_page((void *)v + i * PAGE_SIZE);
+    }
+
     return v;
 }
 
 void free_vcpu_struct(struct vcpu *v)
 {
-    free_xenheap_page(v);
+    free_xenheap_pages(v, get_order_from_pages(PAGES_PER_VCPU));
 }
 
 int vcpu_initialise(struct vcpu *v)