diff mbox series

[Xen-devel,v2,44/45] ARM: new VGIC: Allocate two pages for struct vcpu

Message ID 20180315203050.19791-45-andre.przywara@linaro.org
State New
Headers show
Series New VGIC(-v2) implementation | expand

Commit Message

Andre Przywara March 15, 2018, 8:30 p.m. UTC 
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 v1 ... v2:
- confine change to new VGIC and ARM64 only

 xen/arch/arm/domain.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Julien Grall March 19, 2018, 10 a.m. UTC | #1 
Hi Andre,

Title: Please update it.

On 03/15/2018 08:30 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 v1 ... v2:
> - confine change to new VGIC and ARM64 only
> 
>   xen/arch/arm/domain.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 18b915d2e9..3fba05bda5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -508,10 +508,25 @@ void dump_pageframe_info(struct domain *d)
>   struct vcpu *alloc_vcpu_struct(void)
>   {
>       struct vcpu *v;
> +
> +    /*
> +     * 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)
> +    BUILD_BUG_ON(sizeof(*v) > 2 * PAGE_SIZE);
> +    v = alloc_xenheap_pages(1, 0);
> +    if ( v != NULL ) {
> +        clear_page(v);
> +        clear_page((void *)v + PAGE_SIZE);
> +    }
> +#else
>       BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
>       v = alloc_xenheap_pages(0, 0);
>       if ( v != NULL )
>           clear_page(v);
> +#endif.
All this logic can just be simplified using get_order_* helpers. This 
will avoid #ifdef like that in alloc_vcpu_struct which is really really 
ugly.


Cheers,
Julien Grall March 19, 2018, 10:04 a.m. UTC | #2 
Hi Andre,

On 03/19/2018 10:00 AM, Julien Grall wrote:
> Hi Andre,
> 
> Title: Please update it.
> 
> On 03/15/2018 08:30 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 v1 ... v2:
>> - confine change to new VGIC and ARM64 only
>>
>>   xen/arch/arm/domain.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 18b915d2e9..3fba05bda5 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -508,10 +508,25 @@ void dump_pageframe_info(struct domain *d)
>>   struct vcpu *alloc_vcpu_struct(void)
>>   {
>>       struct vcpu *v;
>> +
>> +    /*
>> +     * 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)
>> +    BUILD_BUG_ON(sizeof(*v) > 2 * PAGE_SIZE);
>> +    v = alloc_xenheap_pages(1, 0);
>> +    if ( v != NULL ) {
>> +        clear_page(v);
>> +        clear_page((void *)v + PAGE_SIZE);
>> +    }
>> +#else
>>       BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
>>       v = alloc_xenheap_pages(0, 0);
>>       if ( v != NULL )
>>           clear_page(v);
>> +#endif.
> All this logic can just be simplified using get_order_* helpers. This 
> will avoid #ifdef like that in alloc_vcpu_struct which is really really 
> ugly.

To clarify, what I suggest is:

unsigned int order = get_order(...);
BUILD_BUG_ON(sizeof(*v) > 2 * PAGE_SIZE);

v = alloc_xenheap_pages(order, 0);
if ( v != NULL )
{
    for ( i = 0; i < (1U << order) )
       clear_page(v + (i * PAGE_SIZE));
}

Cheers,

> 
> 
> Cheers,
>
diff mbox series

Patch

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 18b915d2e9..3fba05bda5 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -508,10 +508,25 @@  void dump_pageframe_info(struct domain *d)
 struct vcpu *alloc_vcpu_struct(void)
 {
     struct vcpu *v;
+
+    /*
+     * 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)
+    BUILD_BUG_ON(sizeof(*v) > 2 * PAGE_SIZE);
+    v = alloc_xenheap_pages(1, 0);
+    if ( v != NULL ) {
+        clear_page(v);
+        clear_page((void *)v + PAGE_SIZE);
+    }
+#else
     BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
     v = alloc_xenheap_pages(0, 0);
     if ( v != NULL )
         clear_page(v);
+#endif
+
     return v;
 }