Re: [PATCH v2] xen/arm{32, 64}: fix section shift when mapping 2MB block in boot page table

[Julien Grall <julien.grall@linaro.org>]

On 01/15/2014 01:32 PM, Ian Campbell wrote:
> On Tue, 2014-01-14 at 06:19 +0800, Chen Baozi wrote:
>> Section shift for level-2 page table should be #21 rather than #20. Besides,
>> since there are {FIRST,SECOND,THIRD}_SHIFT macros defined in asm/page.h, use
>> these macros instead of hard-coded shift value.
>>
>> Signed-off-by: Chen Baozi <baozich@gmail.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> WRT a 4.4 freeze exception the main bit is the use of #21 instead of #20
> as the shift for the L2 entry, which can result in an UNK/SBZP bit being
> set. ARM ARM says:
> 
>         Hardware must implement the bit as Read-As-Zero, and must ignore
>         writes to the field.
>         
>         Software must not rely on the field reading as all 0s, and
>         except for writing back to the register must treat the value
>         as if it is UNKNOWN. Software must use an SBZP policy to write
>         to the field.
> 
> The danger is that some future version of the architecture assigns
> meaning to that bit. All in all this seems like a pretty benign issue,
> but on the flip side the fix is reasonable low risk, the only real
> danger is that one of the replacements is wrong and most of them are
> pretty trivial, although s/#18/#(SECOND_SHIFT - 3)/ is a bit less so.
> 
> I was initially leaning towards putting this into the queue for 4.5, but
> on reflection I'm now starting to lean the other way.
> 
> Does anyone feel strongly that this shouldn't go into 4.4?

This sounds a good fix for Xen 4.4.

Acked-by: Julien Grall <julien.grall@linaro.org>

>> ---
>>  xen/arch/arm/arm32/head.S | 20 ++++++++++----------
>>  xen/arch/arm/arm64/head.S | 26 +++++++++++++-------------
>>  2 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 96230ac..f3eab89 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -291,14 +291,14 @@ cpu_init_done:
>>          ldr   r4, =boot_second
>>          add   r4, r4, r10            /* r1 := paddr (boot_second) */
>>  
>> -        lsr   r2, r9, #20            /* Base address for 2MB mapping */
>> -        lsl   r2, r2, #20
>> +        lsr   r2, r9, #SECOND_SHIFT  /* Base address for 2MB mapping */
>> +        lsl   r2, r2, #SECOND_SHIFT
>>          orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
>>          orr   r2, r2, #PT_LOWER(MEM)
>>  
>>          /* ... map of vaddr(start) in boot_second */
>>          ldr   r1, =start
>> -        lsr   r1, #18                /* Slot for vaddr(start) */
>> +        lsr   r1, #(SECOND_SHIFT - 3)   /* Slot for vaddr(start) */
>>          strd  r2, r3, [r4, r1]       /* Map vaddr(start) */
>>  
>>          /* ... map of paddr(start) in boot_second */
>> @@ -307,7 +307,7 @@ cpu_init_done:
>>                                        * then the mapping was done in
>>                                        * boot_pgtable above */
>>  
>> -        mov   r1, r9, lsr #18        /* Slot for paddr(start) */
>> +        mov   r1, r9, lsr #(SECOND_SHIFT - 3)   /* Slot for paddr(start) */
>>          strd  r2, r3, [r4, r1]       /* Map Xen there */
>>  1:
>>  
>> @@ -339,8 +339,8 @@ paging:
>>          /* Add UART to the fixmap table */
>>          ldr   r1, =xen_fixmap        /* r1 := vaddr (xen_fixmap) */
>>          mov   r3, #0
>> -        lsr   r2, r11, #12
>> -        lsl   r2, r2, #12            /* 4K aligned paddr of UART */
>> +        lsr   r2, r11, #THIRD_SHIFT
>> +        lsl   r2, r2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
>>          orr   r2, r2, #PT_UPPER(DEV_L3)
>>          orr   r2, r2, #PT_LOWER(DEV_L3) /* r2:r3 := 4K dev map including UART */
>>          strd  r2, r3, [r1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
>> @@ -353,7 +353,7 @@ paging:
>>          orr   r2, r2, #PT_UPPER(PT)
>>          orr   r2, r2, #PT_LOWER(PT)  /* r2:r3 := table map of xen_fixmap */
>>          ldr   r4, =FIXMAP_ADDR(0)
>> -        mov   r4, r4, lsr #18        /* r4 := Slot for FIXMAP(0) */
>> +        mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* r4 := Slot for FIXMAP(0) */
>>          strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
>>  
>>          /* Use a virtual address to access the UART. */
>> @@ -365,12 +365,12 @@ paging:
>>  
>>          ldr   r1, =boot_second
>>          mov   r3, #0x0
>> -        lsr   r2, r8, #21
>> -        lsl   r2, r2, #21            /* r2: 2MB-aligned paddr of DTB */
>> +        lsr   r2, r8, #SECOND_SHIFT
>> +        lsl   r2, r2, #SECOND_SHIFT  /* r2: 2MB-aligned paddr of DTB */
>>          orr   r2, r2, #PT_UPPER(MEM)
>>          orr   r2, r2, #PT_LOWER(MEM) /* r2:r3 := 2MB RAM incl. DTB */
>>          ldr   r4, =BOOT_FDT_VIRT_START
>> -        mov   r4, r4, lsr #18        /* Slot for BOOT_FDT_VIRT_START */
>> +        mov   r4, r4, lsr #(SECOND_SHIFT)   /* Slot for BOOT_FDT_VIRT_START */
>>          strd  r2, r3, [r1, r4]       /* Map it in the early fdt slot */
>>          dsb
>>  1:
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index bebddf0..5b164e9 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -278,11 +278,11 @@ skip_bss:
>>          str   x2, [x4, #0]           /* Map it in slot 0 */
>>  
>>          /* ... map of paddr(start) in boot_first */
>> -        lsr   x2, x19, #30           /* x2 := Offset of base paddr in boot_first */
>> +        lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in boot_first */
>>          and   x1, x2, 0x1ff          /* x1 := Slot to use */
>>          cbz   x1, 1f                 /* It's in slot 0, map in boot_second */
>>  
>> -        lsl   x2, x2, #30            /* Base address for 1GB mapping */
>> +        lsl   x2, x2, #FIRST_SHIFT   /* Base address for 1GB mapping */
>>          mov   x3, #PT_MEM            /* x2 := Section map */
>>          orr   x2, x2, x3
>>          lsl   x1, x1, #3             /* x1 := Slot offset */
>> @@ -292,23 +292,23 @@ skip_bss:
>>          ldr   x4, =boot_second
>>          add   x4, x4, x20            /* x4 := paddr (boot_second) */
>>  
>> -        lsr   x2, x19, #20           /* Base address for 2MB mapping */
>> -        lsl   x2, x2, #20
>> +        lsr   x2, x19, #SECOND_SHIFT /* Base address for 2MB mapping */
>> +        lsl   x2, x2, #SECOND_SHIFT
>>          mov   x3, #PT_MEM            /* x2 := Section map */
>>          orr   x2, x2, x3
>>  
>>          /* ... map of vaddr(start) in boot_second */
>>          ldr   x1, =start
>> -        lsr   x1, x1, #18            /* Slot for vaddr(start) */
>> +        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* Slot for vaddr(start) */
>>          str   x2, [x4, x1]           /* Map vaddr(start) */
>>  
>>          /* ... map of paddr(start) in boot_second */
>> -        lsr   x1, x19, #30           /* Base paddr */
>> +        lsr   x1, x19, #FIRST_SHIFT  /* Base paddr */
>>          cbnz  x1, 1f                 /* If paddr(start) is not in slot 0
>>                                        * then the mapping was done in
>>                                        * boot_pgtable or boot_first above */
>>  
>> -        lsr   x1, x19, #18           /* Slot for paddr(start) */
>> +        lsr   x1, x19, #(SECOND_SHIFT - 3)  /* Slot for paddr(start) */
>>          str   x2, [x4, x1]           /* Map Xen there */
>>  1:
>>  
>> @@ -340,8 +340,8 @@ paging:
>>          /* Add UART to the fixmap table */
>>          ldr   x1, =xen_fixmap
>>          add   x1, x1, x20            /* x1 := paddr (xen_fixmap) */
>> -        lsr   x2, x23, #12
>> -        lsl   x2, x2, #12            /* 4K aligned paddr of UART */
>> +        lsr   x2, x23, #THIRD_SHIFT
>> +        lsl   x2, x2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
>>          mov   x3, #PT_DEV_L3
>>          orr   x2, x2, x3             /* x2 := 4K dev map including UART */
>>          str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
>> @@ -354,7 +354,7 @@ paging:
>>          mov   x3, #PT_PT
>>          orr   x2, x2, x3             /* x2 := table map of xen_fixmap */
>>          ldr   x1, =FIXMAP_ADDR(0)
>> -        lsr   x1, x1, #18            /* x1 := Slot for FIXMAP(0) */
>> +        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
>>          str   x2, [x4, x1]           /* Map it in the fixmap's slot */
>>  
>>          /* Use a virtual address to access the UART. */
>> @@ -364,12 +364,12 @@ paging:
>>          /* Map the DTB in the boot misc slot */
>>          cbnz  x22, 1f                /* Only on boot CPU */
>>  
>> -        lsr   x2, x21, #21
>> -        lsl   x2, x2, #21            /* x2 := 2MB-aligned paddr of DTB */
>> +        lsr   x2, x21, #SECOND_SHIFT
>> +        lsl   x2, x2, #SECOND_SHIFT  /* x2 := 2MB-aligned paddr of DTB */
>>          mov   x3, #PT_MEM            /* x2 := 2MB RAM incl. DTB */
>>          orr   x2, x2, x3
>>          ldr   x1, =BOOT_FDT_VIRT_START
>> -        lsr   x1, x1, #18            /* x4 := Slot for BOOT_FDT_VIRT_START */
>> +        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x4 := Slot for BOOT_FDT_VIRT_START */
>>          str   x2, [x4, x1]           /* Map it in the early fdt slot */
>>          dsb   sy
>>  1:
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 


-- 
Julien Grall