[PATCH] Arm64: make setup_virt_paging()'s pa_range_info[] static

Jan Beulich posted 1 patch 1 year, 4 months ago
Failed in applying to current master (apply log)
[PATCH] Arm64: make setup_virt_paging()'s pa_range_info[] static
Posted by Jan Beulich 1 year, 4 months ago
While not as inefficient as it would be on x86 (due to suitable constant
loading and register pair storing instructions being available to fill
some of the fields), having the compiler construct an array of constants
on the stack still looks odd to me.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Actual space savings could be had if further converting the field types
to e.g. unsigned char (all of the values fit in that type).

--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -2281,12 +2281,12 @@ void __init setup_virt_paging(void)
     val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
     val |= VTCR_SL0(0x1); /* P2M starts at first level */
 #else /* CONFIG_ARM_64 */
-    const struct {
+    static const struct {
         unsigned int pabits; /* Physical Address Size */
         unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
         unsigned int root_order; /* Page order of the root of the p2m */
         unsigned int sl0;    /* Desired SL0, maximum in comment */
-    } pa_range_info[] = {
+    } pa_range_info[] __initconst = {
         /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
         /*      PA size, t0sz(min), root-order, sl0(max) */
         [0] = { 32,      32/*32*/,  0,          1 },
Re: [PATCH] Arm64: make setup_virt_paging()'s pa_range_info[] static
Posted by Julien Grall 1 year, 4 months ago
Hi Jan,

On 29/11/2022 15:39, Jan Beulich wrote:
> While not as inefficient as it would be on x86 (due to suitable constant
> loading and register pair storing instructions being available to fill
> some of the fields), having the compiler construct an array of constants
> on the stack still looks odd to me.

The function is only called once at boot. So this seems more a 
micro-optimization than anything else.

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

Acked-by: Julien Grall <jgrall@amazon.com>

> ---
> Actual space savings could be had if further converting the field types
> to e.g. unsigned char (all of the values fit in that type).

This is a micro-optimization. If you want to send it then I will review it.

> 
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -2281,12 +2281,12 @@ void __init setup_virt_paging(void)
>       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
>       val |= VTCR_SL0(0x1); /* P2M starts at first level */
>   #else /* CONFIG_ARM_64 */
> -    const struct {
> +    static const struct {
>           unsigned int pabits; /* Physical Address Size */
>           unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
>           unsigned int root_order; /* Page order of the root of the p2m */
>           unsigned int sl0;    /* Desired SL0, maximum in comment */
> -    } pa_range_info[] = {
> +    } pa_range_info[] __initconst = {
>           /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
>           /*      PA size, t0sz(min), root-order, sl0(max) */
>           [0] = { 32,      32/*32*/,  0,          1 },

-- 
Julien Grall
Re: [PATCH] Arm64: make setup_virt_paging()'s pa_range_info[] static
Posted by Julien Grall 1 year, 3 months ago

On 29/11/2022 16:01, Julien Grall wrote:
> Hi Jan,
> 
> On 29/11/2022 15:39, Jan Beulich wrote:
>> While not as inefficient as it would be on x86 (due to suitable constant
>> loading and register pair storing instructions being available to fill
>> some of the fields), having the compiler construct an array of constants
>> on the stack still looks odd to me.
> 
> The function is only called once at boot. So this seems more a 
> micro-optimization than anything else.
> 
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

I have now committed the patch.

Cheers,

-- 
Julien Grall
Re: [PATCH] Arm64: make setup_virt_paging()'s pa_range_info[] static
Posted by Jan Beulich 1 year, 4 months ago
On 29.11.2022 17:01, Julien Grall wrote:
> On 29/11/2022 15:39, Jan Beulich wrote:
>> While not as inefficient as it would be on x86 (due to suitable constant
>> loading and register pair storing instructions being available to fill
>> some of the fields), having the compiler construct an array of constants
>> on the stack still looks odd to me.
> 
> The function is only called once at boot. So this seems more a 
> micro-optimization than anything else.

Well, yes - hence the "looks odd" as a justification, not performance or
anything.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks.

>> ---
>> Actual space savings could be had if further converting the field types
>> to e.g. unsigned char (all of the values fit in that type).
> 
> This is a micro-optimization. If you want to send it then I will review it.

I probably won't bother; I've pointed this out largely in case actual
space savings would be made a requirement to accept this change.

Jan