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

["Jon Fraser" <jfraser@broadcom.com>]

On Wed, 2013-10-30 at 21:25 +0000, Andrew Cooper wrote:
> 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
> 

The problem is that the C code in the include can't be included in the
assembler files.  I suppose that EARLY_UART_VIRTUAL_ADDRESS could be
used in C code.  I'd still have to ifdef out the C code in the include
file for the assembler files.
Jon

> > +
> > +#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
> 
>