Re: [PATCH V2] xen/arm: UART addresses are not always aligned to a page.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/10/13 21:18, Jon Fraser wrote:
> When calculating the virtual address of the UART,
> add the page offset of the UART to the mapped address.
>
> Create define EARLY_UART_VIRTUAL_ADDRESS and use instead
> of FIXMAP_ADDR(FIXMAP_CONSOLE)
>
> Signed-off-by: Jon Fraser <jfraser@broadcom.com>
> ---
>  xen/arch/arm/arm32/debug.S         |  5 +++--
>  xen/arch/arm/arm32/head.S          |  3 ++-
>  xen/arch/arm/arm64/debug.S         |  5 +++--
>  xen/arch/arm/arm64/head.S          |  3 ++-
>  xen/include/asm-arm/early_printk.h | 10 ++++++++++
>  5 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/debug.S b/xen/arch/arm/arm32/debug.S
> index ec774cd..a95d9da 100644
> --- a/xen/arch/arm/arm32/debug.S
> +++ b/xen/arch/arm/arm32/debug.S
> @@ -18,6 +18,7 @@
>   */
>  
>  #include <asm/config.h>
> +#include <asm/early_printk.h>
>  
>  #ifdef EARLY_PRINTK_INC
>  #include EARLY_PRINTK_INC
> @@ -26,14 +27,14 @@
>  /* Print a character on the UART - this function is called by C
>   * r0: character to print */
>  GLOBAL(early_putch)
> -        ldr   r1, =FIXMAP_ADDR(FIXMAP_CONSOLE)  /* r1 := VA UART base address */
> +        ldr   r1, =EARLY_UART_VIRTUAL_ADDRESS  /* r1 := VA UART base address */
>          early_uart_ready r1, r2
>          early_uart_transmit r1, r0
>          mov   pc, lr
>  
>  /* Flush the UART - this function is called by C */
>  GLOBAL(early_flush)
> -        ldr   r1, =FIXMAP_ADDR(FIXMAP_CONSOLE)  /* r1 := VA UART base address */
> +        ldr   r1, =EARLY_UART_VIRTUAL_ADDRESS  /* r1 := VA UART base address */
>          early_uart_ready r1, r2
>          mov   pc, lr
>  
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index ce1d21a..96230ac 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -22,6 +22,7 @@
>  #include <asm/processor-ca15.h>
>  #include <asm/processor-ca7.h>
>  #include <asm/asm_defns.h>
> +#include <asm/early_printk.h>
>  
>  #define ZIMAGE_MAGIC_NUMBER 0x016f2818
>  
> @@ -356,7 +357,7 @@ paging:
>          strd  r2, r3, [r1, r4]       /* Map it in the fixmap's slot */
>  
>          /* Use a virtual address to access the UART. */
> -        ldr   r11, =FIXMAP_ADDR(FIXMAP_CONSOLE)
> +        ldr   r11, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
>          /* Map the DTB in the boot misc slot */
>          teq   r12, #0                /* Only on boot CPU */
> diff --git a/xen/arch/arm/arm64/debug.S b/xen/arch/arm/arm64/debug.S
> index 472c157..a855358 100644
> --- a/xen/arch/arm/arm64/debug.S
> +++ b/xen/arch/arm/arm64/debug.S
> @@ -18,6 +18,7 @@
>   */
>  
>  #include <asm/config.h>
> +#include <asm/early_printk.h>
>  
>  #ifdef EARLY_PRINTK_INC
>  #include EARLY_PRINTK_INC
> @@ -26,14 +27,14 @@
>  /* Print a character on the UART - this function is called by C
>   * x0: character to print */
>  GLOBAL(early_putch)
> -        ldr   x15, =FIXMAP_ADDR(FIXMAP_CONSOLE)
> +        ldr   x15, =EARLY_UART_VIRTUAL_ADDRESS
>          early_uart_ready x15, 1
>          early_uart_transmit x15, w0
>          ret
>  
>  /* Flush the UART - this function is called by C */
>  GLOBAL(early_flush)
> -        ldr   x15, =FIXMAP_ADDR(FIXMAP_CONSOLE)  /* x15 := VA UART base address */
> +        ldr   x15, =EARLY_UART_VIRTUAL_ADDRESS  /* x15 := VA UART base address */
>          early_uart_ready x15, 1
>          ret
>  
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index b8b5902..31afdd0 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -23,6 +23,7 @@
>  #include <asm/config.h>
>  #include <asm/page.h>
>  #include <asm/asm_defns.h>
> +#include <asm/early_printk.h>
>  
>  #define PT_PT     0xe7f /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=111 T=1 P=1 */
>  #define PT_MEM    0xe7d /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=111 T=0 P=1 */
> @@ -368,7 +369,7 @@ paging:
>          str   x2, [x4, x1]           /* Map it in the fixmap's slot */
>  
>          /* Use a virtual address to access the UART. */
> -        ldr   x23, =FIXMAP_ADDR(FIXMAP_CONSOLE)
> +        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
>  
>          /* Map the DTB in the boot misc slot */
> diff --git a/xen/include/asm-arm/early_printk.h b/xen/include/asm-arm/early_printk.h
> index 5f7c529..759d59a 100644
> --- a/xen/include/asm-arm/early_printk.h
> +++ b/xen/include/asm-arm/early_printk.h
> @@ -12,6 +12,14 @@
>  
>  #include <xen/config.h>
>  
> +#ifdef __ASSEMBLY__
> +
> +/* need to add the uart address offset in page to the fixmap address */
> +#define EARLY_UART_VIRTUAL_ADDRESS \
> +    (FIXMAP_ADDR(FIXMAP_CONSOLE) +(EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))

Why should this be limited to assembly code?  It would seem valid for
early C code to see and use as well.

With that change, you can drop all the #ifdef'ary

~Andrew

> +
> +#else
> +
>  #ifdef EARLY_PRINTK
>  
>  void early_printk(const char *fmt, ...)
> @@ -31,4 +39,6 @@ __attribute__((format (printf, 1, 2))) early_panic(const char *fmt, ...)
>  
>  #endif
>  
> +#endif  /* __ASSEMBLY__ */
> +
>  #endif