thr3ads.net - Xen devel - [PATCH v2 0/8] Emulate virtual UART for DOM0 and some UART clean up [Jul 2013]

If this information is useful, please help other people find it:
Share via:

Julien Grall

2013-Jul-31 15:38 UTC

[PATCH v2 0/8] Emulate virtual UART for DOM0 and some UART clean up

Hi,

This second version of this patch series. It follows the thread
http://patches.linaro.org/18358/ on Linux mailing list.

In some configuration, the kernel can use hardcoded code to access to the
UART (for instance early printk). If Xen has stolen the UART for its own use,
DOM0 will abort because Xen has not mapped to DOM0 the UART memory.

With this patch series, Xen will "replace" the real UART by a basic
virtual
UART for DOM0.

For the moment, this patch series is not able to cooperate with early printk
in guest kernel.

This patch series also contains clean up for the pl011 and exynos4210 drivers.

For all the changes see the different patches.

Cheers,

Julien Grall (8):
  pl011: Use ioreadl/iowritel
  pl011: Move registers'' definition in a separate file
  xen/arm: Use define instead of hardcoded value in debug-pl011
  xen/arm: New callback in uart_driver to retrieve serial information
  xen/arm: Implement a virtual UART
  exynos4210: rename UTRSTAT_TX_EMPTY in UTRSTAT_TXFE
  exynos4210: Implement vuart_info callback
  pl011: Implement vuart_info callback

 xen/arch/arm/Makefile                   |    2 +-
 xen/arch/arm/arm32/debug-exynos4210.inc |    2 +-
 xen/arch/arm/arm32/debug-pl011.inc      |   18 +++--
 xen/arch/arm/domain.c                   |   12 ++-
 xen/arch/arm/io.c                       |    2 +-
 xen/arch/arm/io.h                       |    2 +-
 xen/arch/arm/{vpl011.c => vuart.c}      |  128
+++++++++++++++----------------
 xen/arch/arm/{vpl011.h => vuart.h}      |   16 ++--
 xen/drivers/char/exynos4210-uart.c      |   15 ++++
 xen/drivers/char/pl011.c                |  115 +++++++++++----------------
 xen/drivers/char/serial.c               |    8 ++
 xen/include/asm-arm/domain.h            |   14 ++--
 xen/include/asm-arm/exynos4210-uart.h   |    3 +-
 xen/include/asm-arm/pl011-uart.h        |   81 +++++++++++++++++++
 xen/include/xen/serial.h                |   13 ++++
 15 files changed, 264 insertions(+), 167 deletions(-)
 rename xen/arch/arm/{vpl011.c => vuart.c} (43%)
 rename xen/arch/arm/{vpl011.h => vuart.h} (75%)
 create mode 100644 xen/include/asm-arm/pl011-uart.h

-- 
1.7.10.4

Julien Grall

2013-Jul-31 15:38 UTC

head link

[PATCH v2 2/8] pl011: Move registers'' definition in a separate file

---
 xen/drivers/char/pl011.c         |   48 +----------------------
 xen/include/asm-arm/pl011-uart.h |   80 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 47 deletions(-)
 create mode 100644 xen/include/asm-arm/pl011-uart.h

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index 9456f20..fd87e68 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -28,6 +28,7 @@
 #include <asm/device.h>
 #include <xen/mm.h>
 #include <xen/vmap.h>
+#include <asm/pl011-uart.h>
 
 static struct pl011 {
     unsigned int baud, clock_hz, data_bits, parity, stop_bits;
@@ -41,53 +42,6 @@ static struct pl011 {
     /* bool_t probing, intr_works; */
 } pl011_com = {0};
 
-/* PL011 register addresses */
-#define DR     (0x00)
-#define RSR    (0x04)
-#define FR     (0x18)
-#define ILPR   (0x20)
-#define IBRD   (0x24)
-#define FBRD   (0x28)
-#define LCR_H  (0x2c)
-#define CR     (0x30)
-#define IFLS   (0x34)
-#define IMSC   (0x38)
-#define RIS    (0x3c)
-#define MIS    (0x40)
-#define ICR    (0x44)
-#define DMACR  (0x48)
-
-/* CR bits */
-#define RXE    (1<<9) /* Receive enable */
-#define TXE    (1<<8) /* Transmit enable */
-#define UARTEN (1<<0) /* UART enable */
-
-/* FR bits */
-#define TXFE   (1<<7) /* TX FIFO empty */
-#define RXFE   (1<<4) /* RX FIFO empty */
-
-/* LCR_H bits */
-#define SPS    (1<<7) /* Stick parity select */
-#define FEN    (1<<4) /* FIFO enable */
-#define STP2   (1<<3) /* Two stop bits select */
-#define EPS    (1<<2) /* Even parity select */
-#define PEN    (1<<1) /* Parity enable */
-#define BRK    (1<<0) /* Send break */
-
-/* Interrupt bits (IMSC, MIS, ICR) */
-#define OEI   (1<<10) /* Overrun Error interrupt mask */
-#define BEI   (1<<9)  /* Break Error interrupt mask */
-#define PEI   (1<<8)  /* Parity Error interrupt mask */
-#define FEI   (1<<7)  /* Framing Error interrupt mask */
-#define RTI   (1<<6)  /* Receive Timeout interrupt mask */
-#define TXI   (1<<5)  /* Transmit interrupt mask */
-#define RXI   (1<<4)  /* Receive interrupt mask */
-#define DSRMI (1<<3)  /* nUARTDSR Modem interrupt mask */
-#define DCDMI (1<<2)  /* nUARTDCD Modem interrupt mask */
-#define CTSMI (1<<1)  /* nUARTCTS Modem interrupt mask */
-#define RIMI  (1<<0)  /* nUARTRI Modem interrupt mask */
-#define ALLI  OEI|BEI|PEI|FEI|RTI|TXI|RXI|DSRMI|DCDMI|CTSMI|RIMI
-
 /* These parity settings can be ORed directly into the LCR. */
 #define PARITY_NONE  (0)
 #define PARITY_ODD   (PEN)
diff --git a/xen/include/asm-arm/pl011-uart.h b/xen/include/asm-arm/pl011-uart.h
new file mode 100644
index 0000000..8c4edd4
--- /dev/null
+++ b/xen/include/asm-arm/pl011-uart.h
@@ -0,0 +1,80 @@
+/*
+ * xen/include/asm-arm/pl011-uart.h
+ *
+ * Common constant definition between early printk and the UART driver
+ * for the pl011 UART
+ *
+ * Tim Deegan <tim@xen.org>
+ * Copyright (c) 2011 Citrix Systems.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __ASM_ARM_PL011_H
+#define __ASM_ARM_PL011_H
+
+/* PL011 register addresses */
+#define DR     (0x00)
+#define RSR    (0x04)
+#define FR     (0x18)
+#define ILPR   (0x20)
+#define IBRD   (0x24)
+#define FBRD   (0x28)
+#define LCR_H  (0x2c)
+#define CR     (0x30)
+#define IFLS   (0x34)
+#define IMSC   (0x38)
+#define RIS    (0x3c)
+#define MIS    (0x40)
+#define ICR    (0x44)
+#define DMACR  (0x48)
+
+/* CR bits */
+#define RXE    (1<<9) /* Receive enable */
+#define TXE    (1<<8) /* Transmit enable */
+#define UARTEN (1<<0) /* UART enable */
+
+/* FR bits */
+#define TXFE   (1<<7) /* TX FIFO empty */
+#define RXFE   (1<<4) /* RX FIFO empty */
+
+/* LCR_H bits */
+#define SPS    (1<<7) /* Stick parity select */
+#define FEN    (1<<4) /* FIFO enable */
+#define STP2   (1<<3) /* Two stop bits select */
+#define EPS    (1<<2) /* Even parity select */
+#define PEN    (1<<1) /* Parity enable */
+#define BRK    (1<<0) /* Send break */
+
+/* Interrupt bits (IMSC, MIS, ICR) */
+#define OEI   (1<<10) /* Overrun Error interrupt mask */
+#define BEI   (1<<9)  /* Break Error interrupt mask */
+#define PEI   (1<<8)  /* Parity Error interrupt mask */
+#define FEI   (1<<7)  /* Framing Error interrupt mask */
+#define RTI   (1<<6)  /* Receive Timeout interrupt mask */
+#define TXI   (1<<5)  /* Transmit interrupt mask */
+#define RXI   (1<<4)  /* Receive interrupt mask */
+#define DSRMI (1<<3)  /* nUARTDSR Modem interrupt mask */
+#define DCDMI (1<<2)  /* nUARTDCD Modem interrupt mask */
+#define CTSMI (1<<1)  /* nUARTCTS Modem interrupt mask */
+#define RIMI  (1<<0)  /* nUARTRI Modem interrupt mask */
+#define ALLI  OEI|BEI|PEI|FEI|RTI|TXI|RXI|DSRMI|DCDMI|CTSMI|RIMI
+
+#endif /* __ASM_ARM_PL011_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.10.4

Julien Grall

2013-Jul-31 15:38 UTC

head link

[PATCH v2 3/8] xen/arm: Use define instead of hardcoded value in debug-pl011

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/arm32/debug-pl011.inc |   18 ++++++++++--------
 xen/include/asm-arm/pl011-uart.h   |    1 +
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/arm32/debug-pl011.inc
b/xen/arch/arm/arm32/debug-pl011.inc
index 8b085b8..6a64dbf 100644
--- a/xen/arch/arm/arm32/debug-pl011.inc
+++ b/xen/arch/arm/arm32/debug-pl011.inc
@@ -16,19 +16,21 @@
  * GNU General Public License for more details.
  */
 
+#include <asm/pl011-uart.h>
+
 /* PL011 UART initialization
  * rb: register which contains the UART base address
  * rc: scratch register 1
  * rd: scratch register 2 (unused here) */
 .macro early_uart_init rb, rc, rd
         mov   \rc, #(7372800 / EARLY_PRINTK_BAUD % 16)
-        str   \rc, [\rb, #0x28]     /* -> UARTFBRD (Baud divisor fraction)
*/
+        str   \rc, [\rb, #FBRD]     /* -> UARTFBRD (Baud divisor fraction)
*/
         mov   \rc, #(7372800 / EARLY_PRINTK_BAUD / 16)
-        str   \rc, [\rb, #0x24]     /* -> UARTIBRD (Baud divisor integer) */
+        str   \rc, [\rb, #IBRD]     /* -> UARTIBRD (Baud divisor integer) */
         mov   \rc, #0x60            /* 8n1 */
-        str   \rc, [\rb, #0x2C]     /* -> UARTLCR_H (Line control) */
-        ldr   \rc, =0x00000301      /* RXE | TXE | UARTEN */
-        str   \rc, [\rb, #0x30]     /* -> UARTCR (Control Register) */
+        str   \rc, [\rb, #LCR_H]     /* -> UARTLCR_H (Line control) */
+        ldr   \rc, =(RXE | TXE | UARTEN)      /* RXE | TXE | UARTEN */
+        str   \rc, [\rb, #CR]     /* -> UARTCR (Control Register) */
 .endm
 
 /* PL011 UART wait UART to be ready to transmit
@@ -36,8 +38,8 @@
  * rc: scratch register */
 .macro early_uart_ready rb, rc
 1:
-        ldr   \rc, [\rb, #0x18]     /* <- UARTFR (Flag register) */
-        tst   \rc, #0x8             /* Check BUSY bit */
+        ldr   \rc, [\rb, #FR]       /* <- UARTFR (Flag register) */
+        tst   \rc, #BUSY             /* Check BUSY bit */
         bne   1b                    /* Wait for the UART to be ready */
 .endm
 
@@ -45,7 +47,7 @@
  * rb: register which contains the UART base address
  * rt: register which contains the character to transmit */
 .macro early_uart_transmit rb, rt
-        str   \rt, [\rb]            /* -> UARTDR (Data Register) */
+        str   \rt, [\rb, #DR]            /* -> UARTDR (Data Register) */
 .endm
 
 /*
diff --git a/xen/include/asm-arm/pl011-uart.h b/xen/include/asm-arm/pl011-uart.h
index 8c4edd4..3332c51 100644
--- a/xen/include/asm-arm/pl011-uart.h
+++ b/xen/include/asm-arm/pl011-uart.h
@@ -45,6 +45,7 @@
 /* FR bits */
 #define TXFE   (1<<7) /* TX FIFO empty */
 #define RXFE   (1<<4) /* RX FIFO empty */
+#define BUSY   (1<<3) /* Transmit is not complete */
 
 /* LCR_H bits */
 #define SPS    (1<<7) /* Stick parity select */
-- 
1.7.10.4

Julien Grall

2013-Jul-31 15:38 UTC

head link

[PATCH v2 4/8] xen/arm: New callback in uart_driver to retrieve serial information

There is no way to retrieve basic informations (base address, size, ....) for
an UART. This callback will be used later to partially emulate the real UART
for DOM0 on ARM.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
CC: Keir Fraser <keir@xen.org>

---
    Changes in v2:
        - Rename the callback to vuart_info
---
 xen/drivers/char/serial.c |    8 ++++++++
 xen/include/xen/serial.h  |   13 +++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index e1c3f47..cd0b864 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -497,6 +497,14 @@ const struct dt_irq __init *serial_dt_irq(int idx)
     return NULL;
 }
 
+const struct vuart_info *serial_vuart_info(int idx)
+{
+    if ( (idx >= 0) && (idx < ARRAY_SIZE(com)) &&
+         com[idx].driver && com[idx].driver->vuart_info )
+        return com[idx].driver->vuart_info(&com[idx]);
+
+    return NULL;
+}
 
 void serial_suspend(void)
 {
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 9caf776..7d48163 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -32,6 +32,14 @@ enum serial_port_state {
     serial_initialized
 };
 
+struct vuart_info {
+    unsigned long base_addr;    /* Base address of the UART */
+    unsigned long size;         /* Size of the memory region */
+    unsigned long data_off;     /* Data register offset */
+    unsigned long status_off;   /* Status register offset */
+    unsigned long status;       /* Ready status value */
+};
+
 struct serial_port {
     /* Uart-driver parameters. */
     struct uart_driver *driver;
@@ -74,6 +82,8 @@ struct uart_driver {
     int  (*irq)(struct serial_port *);
     /* Get IRQ device node for this port''s serial line: returns NULL
if none. */
     const struct dt_irq *(*dt_irq_get)(struct serial_port *);
+    /* Get serial information */
+    const struct vuart_info *(*vuart_info)(struct serial_port *);
 };
 
 /* ''Serial handles'' are composed from the following fields.
*/
@@ -127,6 +137,9 @@ int serial_irq(int idx);
 /* Return irq device node for specified serial port (identified by index). */
 const struct dt_irq *serial_dt_irq(int idx);
 
+/* Retrieve basic UART information to emulate it (base address, size...) */
+const struct vuart_info* serial_vuart_info(int idx);
+
 /* Serial suspend/resume. */
 void serial_suspend(void);
 void serial_resume(void);
-- 
1.7.10.4

Julien Grall

2013-Jul-31 15:38 UTC

head link

[PATCH v2 5/8] xen/arm: Implement a virtual UART

This code is based on the previous vuart0 implementation. Unlike the latter,
it''s intend to replace UART stolen by XEN to DOM0 via dtuart=... on its
command line.

It''s useful when the kernel is compiled with early printk enabled or
for a
single platform. Most of the time, the hardcoded code to handle the UART
will need 2 registers: status and data, the others registers can be
implemented as RAZ/WI.

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

---
    Changes in v2:
        - Merge and update comments
        - Use the renamed callback
---
 xen/arch/arm/Makefile              |    2 +-
 xen/arch/arm/domain.c              |   12 ++--
 xen/arch/arm/io.c                  |    2 +-
 xen/arch/arm/io.h                  |    2 +-
 xen/arch/arm/{vpl011.c => vuart.c} |  128
++++++++++++++++++------------------
 xen/arch/arm/{vpl011.h => vuart.h} |   16 ++---
 xen/include/asm-arm/domain.h       |   14 ++--
 7 files changed, 90 insertions(+), 86 deletions(-)
 rename xen/arch/arm/{vpl011.c => vuart.c} (43%)
 rename xen/arch/arm/{vpl011.h => vuart.h} (75%)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 5c13a65..003ac84 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -27,7 +27,7 @@ obj-y += shutdown.o
 obj-y += traps.o
 obj-y += vgic.o
 obj-y += vtimer.o
-obj-y += vpl011.o
+obj-y += vuart.o
 obj-y += hvm.o
 obj-y += device.o
 obj-y += decode.o
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 4e9cece..cb0424d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -32,7 +32,7 @@
 
 #include <asm/gic.h>
 #include "vtimer.h"
-#include "vpl011.h"
+#include "vuart.h"
 
 DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
 
@@ -525,8 +525,12 @@ int arch_domain_create(struct domain *d, unsigned int
domcr_flags)
     if ( (rc = vcpu_domain_init(d)) != 0 )
         goto fail;
 
-    /* Domain 0 gets a real UART not an emulated one */
-    if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 )
+    /*
+     * Virtual UART is only used by linux early printk and decompress code.
+     * Only use it for dom0 because the linux kernel may not support
+     * multi-platform.
+     */
+    if ( (d->domain_id == 0) && (rc = domain_vuart_init(d)) )
         goto fail;
 
     return 0;
@@ -542,7 +546,7 @@ void arch_domain_destroy(struct domain *d)
 {
     p2m_teardown(d);
     domain_vgic_free(d);
-    domain_uart0_free(d);
+    domain_vuart_free(d);
     free_xenheap_page(d->shared_info);
 }
 
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index ad28c26..a6db00b 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -25,7 +25,7 @@
 static const struct mmio_handler *const mmio_handlers[]  {
     &vgic_distr_mmio_handler,
-    &uart0_mmio_handler,
+    &vuart_mmio_handler,
 };
 #define MMIO_HANDLER_NR ARRAY_SIZE(mmio_handlers)
 
diff --git a/xen/arch/arm/io.h b/xen/arch/arm/io.h
index 661dce1..8d252c0 100644
--- a/xen/arch/arm/io.h
+++ b/xen/arch/arm/io.h
@@ -41,7 +41,7 @@ struct mmio_handler {
 };
 
 extern const struct mmio_handler vgic_distr_mmio_handler;
-extern const struct mmio_handler uart0_mmio_handler;
+extern const struct mmio_handler vuart_mmio_handler;
 
 extern int handle_mmio(mmio_info_t *info);
 
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vuart.c
similarity index 43%
rename from xen/arch/arm/vpl011.c
rename to xen/arch/arm/vuart.c
index 13ba623..0d672d5 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vuart.c
@@ -1,8 +1,22 @@
 /*
- * xen/arch/arm/vpl011.c
+ * xen/arch/arm/vuart.c
  *
- * ARM PL011 UART Emulator (DEBUG)
+ * Virtual UART Emulator.
  *
+ * This emulator uses the information from dtuart. This is not intended to be
+ * a full emulation of an UART device. Rather it is intended to provide a
+ * sufficient veneer of one that early code (such as Linux''s boot time
+ * decompressor) which hardcodes output directly to such a device are able to
+ * make progress.
+ *
+ * The mininal register set to emulate an UART are:
+ *  - Single byte transmit register
+ *  - Single status register
+ *
+ * /!\ This device is not intended to be enumerable or exposed to the OS
+ * (e.g. via Device Tree).
+ *
+ * Julien Grall <julien.grall@linaro.org>
  * Ian Campbell <ian.campbell@citrix.com>
  * Copyright (c) 2012 Citrix Systems.
  *
@@ -17,53 +31,48 @@
  * GNU General Public License for more details.
  */
 
-/*
- * This is not intended to be a full emulation of a PL011
- * device. Rather it is intended to provide a sufficient veneer of one
- * that early code (such as Linux''s boot time decompressor) which
- * hardcodes output directly to such a device are able to make progress.
- *
- * This device is not intended to be enumerable or exposed to the OS
- * (e.g. via Device Tree).
- */
 
 #include <xen/config.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/errno.h>
 #include <xen/ctype.h>
+#include <xen/serial.h>
 
+#include "vuart.h"
 #include "io.h"
 
-#define UART0_START 0x1c090000
-#define UART0_END   (UART0_START+65536)
-
-#define UARTDR 0x000
-#define UARTFR 0x018
+#define domain_has_vuart(d) ((d)->arch.vuart.info != NULL)
 
-int domain_uart0_init(struct domain *d)
+int domain_vuart_init(struct domain *d)
 {
-    ASSERT( d->domain_id );
+    ASSERT( !d->domain_id );
 
-    spin_lock_init(&d->arch.uart0.lock);
-    d->arch.uart0.idx = 0;
+    d->arch.vuart.info = serial_vuart_info(SERHND_DTUART);
+    if ( !d->arch.vuart.info )
+        return 0;
 
-    d->arch.uart0.buf = xzalloc_array(char, VPL011_BUF_SIZE);
-    if ( !d->arch.uart0.buf )
+    spin_lock_init(&d->arch.vuart.lock);
+    d->arch.vuart.idx = 0;
+
+    d->arch.vuart.buf = xzalloc_array(char, VUART_BUF_SIZE);
+    if ( !d->arch.vuart.buf )
         return -ENOMEM;
 
     return 0;
-
 }
 
-void domain_uart0_free(struct domain *d)
+void domain_vuart_free(struct domain *d)
 {
-    xfree(d->arch.uart0.buf);
+    if ( !domain_has_vuart(d) )
+        return;
+
+    xfree(d->arch.vuart.buf);
 }
 
-static void uart0_print_char(char c)
+static void vuart_print_char(char c)
 {
-    struct vpl011 *uart = &current->domain->arch.uart0;
+    struct vuart *uart = &current->domain->arch.vuart;
 
     /* Accept only printable characters, newline, and horizontal tab. */
     if ( !isprint(c) && (c != ''\n'') && (c !=
''\t'') )
@@ -71,7 +80,7 @@ static void uart0_print_char(char c)
 
     spin_lock(&uart->lock);
     uart->buf[uart->idx++] = c;
-    if ( (uart->idx == (VPL011_BUF_SIZE - 2)) || (c ==
''\n'') )
+    if ( (uart->idx == (VUART_BUF_SIZE - 2)) || (c ==
''\n'') )
     {
         if ( c != ''\n'' )
             uart->buf[uart->idx++] = ''\n'';
@@ -83,62 +92,51 @@ static void uart0_print_char(char c)
     spin_unlock(&uart->lock);
 }
 
-static int uart0_mmio_check(struct vcpu *v, paddr_t addr)
+static int vuart_mmio_check(struct vcpu *v, paddr_t addr)
 {
-    struct domain *d = v->domain;
+    const struct vuart_info *info = v->domain->arch.vuart.info;
 
-    return d->domain_id != 0 && addr >= UART0_START &&
addr < UART0_END;
+    return (domain_has_vuart(v->domain) && addr >=
info->base_addr &&
+            addr <= (info->base_addr + info->size));
 }
 
-static int uart0_mmio_read(struct vcpu *v, mmio_info_t *info)
+static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info)
 {
+    struct domain *d = v->domain;
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     register_t *r = select_user_reg(regs, dabt.reg);
-    int offset = (int)(info->gpa - UART0_START);
+    paddr_t offset = info->gpa - d->arch.vuart.info->base_addr;
 
-    switch ( offset )
-    {
-    case UARTDR:
-        *r = 0;
-        return 1;
-    case UARTFR:
-        *r = 0x87; /* All holding registers empty, ready to send etc */
-        return 1;
-    default:
-        printk("VPL011: unhandled read r%d offset %#08x\n",
-               dabt.reg, offset);
-        domain_crash_synchronous();
-    }
+    /* By default zeroed the register */
+    *r = 0;
+
+    if ( offset == d->arch.vuart.info->status_off )
+        /* All holding registers empty, ready to send etc */
+        *r = d->arch.vuart.info->status;
+
+    return 1;
 }
 
-static int uart0_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info)
 {
+    struct domain *d = v->domain;
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     register_t *r = select_user_reg(regs, dabt.reg);
-    int offset = (int)(info->gpa - UART0_START);
+    paddr_t offset = (int)(info->gpa - d->arch.vuart.info->base_addr);
 
-    switch ( offset )
-    {
-    case UARTDR:
+    if ( offset == d->arch.vuart.info->data_off )
         /* ignore any status bits */
-        uart0_print_char((int)((*r) & 0xFF));
-        return 1;
-    case UARTFR:
-        /* Silently ignore */
-        return 1;
-    default:
-        printk("VPL011: unhandled write r%d=%"PRIregister"
offset %#08x\n",
-               dabt.reg, *r, offset);
-        domain_crash_synchronous();
-    }
+        vuart_print_char((int)((*r) & 0xFF));
+
+    return 1;
 }
 
-const struct mmio_handler uart0_mmio_handler = {
-    .check_handler = uart0_mmio_check,
-    .read_handler  = uart0_mmio_read,
-    .write_handler = uart0_mmio_write,
+const struct mmio_handler vuart_mmio_handler = {
+    .check_handler = vuart_mmio_check,
+    .read_handler  = vuart_mmio_read,
+    .write_handler = vuart_mmio_write,
 };
 
 /*
diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vuart.h
similarity index 75%
rename from xen/arch/arm/vpl011.h
rename to xen/arch/arm/vuart.h
index f0d0a82..9445e50 100644
--- a/xen/arch/arm/vpl011.h
+++ b/xen/arch/arm/vuart.h
@@ -1,7 +1,7 @@
-/*
- * xen/arch/arm/vpl011.h
+/* 
+ * xen/arch/arm/vuart.h
  *
- * ARM PL011 Emulation Support
+ * Virtual UART Emulation Support
  *
  * Ian Campbell <ian.campbell@citrix.com>
  * Copyright (c) 2012 Citrix Systems.
@@ -17,13 +17,13 @@
  * GNU General Public License for more details.
  */
 
-#ifndef __ARCH_ARM_VPL011_H__
-#define __ARCH_ARM_VPL011_H__
+#ifndef __ARCH_ARM_VUART_H__
+#define __ARCH_ARM_VUART_H__
 
-extern int domain_uart0_init(struct domain *d);
-extern void domain_uart0_free(struct domain *d);
+int domain_vuart_init(struct domain *d);
+void domain_vuart_free(struct domain *d);
 
-#endif
+#endif /* __ARCH_ARM_VUART_H__ */
 
 /*
  * Local variables:
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 89f88f6..67bfbbc 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -8,6 +8,7 @@
 #include <asm/p2m.h>
 #include <asm/vfp.h>
 #include <public/hvm/params.h>
+#include <xen/serial.h>
 
 /* Represents state corresponding to a block of 32 interrupts */
 struct vgic_irq_rank {
@@ -103,12 +104,13 @@ struct arch_domain
         paddr_t cbase; /* CPU base address */
     } vgic;
 
-    struct vpl011 {
-#define VPL011_BUF_SIZE 128
-        char                  *buf;
-        int                    idx;
-        spinlock_t             lock;
-    } uart0;
+    struct vuart {
+#define VUART_BUF_SIZE 128
+        char                        *buf;
+        int                         idx;
+        const struct vuart_info     *info;
+        spinlock_t                  lock;
+    } vuart;
 
 }  __cacheline_aligned;
 
-- 
1.7.10.4

Julien Grall

2013-Jul-31 15:38 UTC

head link

[PATCH v2 6/8] exynos4210: rename UTRSTAT_TX_EMPTY in UTRSTAT_TXFE

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

---
    Changes in v2:
        - Missing s-o-b
        - Align comment
---
 xen/arch/arm/arm32/debug-exynos4210.inc |    2 +-
 xen/include/asm-arm/exynos4210-uart.h   |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm32/debug-exynos4210.inc
b/xen/arch/arm/arm32/debug-exynos4210.inc
index d746c35..39f2db3 100644
--- a/xen/arch/arm/arm32/debug-exynos4210.inc
+++ b/xen/arch/arm/arm32/debug-exynos4210.inc
@@ -56,7 +56,7 @@
 .macro early_uart_ready rb rc
 1:
         ldr   \rc, [\rb, #UTRSTAT]   /* <- UTRSTAT (Flag register) */
-        tst   \rc, #UTRSTAT_TX_EMPTY /* Check BUSY bit */
+        tst   \rc, #UTRSTAT_TXFE     /* Check BUSY bit */
         beq   1b                     /* Wait for the UART to be ready */
 .endm
 
diff --git a/xen/include/asm-arm/exynos4210-uart.h
b/xen/include/asm-arm/exynos4210-uart.h
index 330e1c0..bd9a4be 100644
--- a/xen/include/asm-arm/exynos4210-uart.h
+++ b/xen/include/asm-arm/exynos4210-uart.h
@@ -87,7 +87,7 @@
 #define UFSTAT_RX_COUNT_MASK    (0xff << UFSTAT_RX_COUNT_SHIFT)
 
 /* UTRSTAT */
-#define UTRSTAT_TX_EMPTY        (1 << 1)
+#define UTRSTAT_TXFE            (1 << 1)
 
 /* URHX */
 #define URXH_DATA_MASK  (0xff)
-- 
1.7.10.4

Julien Grall

2013-Jul-31 15:38 UTC

head link

[PATCH v2 7/8] exynos4210: Implement vuart_info callback

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

---
    Changes v2:
        - Use the renamed callback
---
 xen/drivers/char/exynos4210-uart.c    |   15 +++++++++++++++
 xen/include/asm-arm/exynos4210-uart.h |    1 +
 2 files changed, 16 insertions(+)

diff --git a/xen/drivers/char/exynos4210-uart.c
b/xen/drivers/char/exynos4210-uart.c
index f7971da..9b34f92 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -33,6 +33,7 @@ static struct exynos4210_uart {
     struct dt_irq irq;
     void *regs;
     struct irqaction irqaction;
+    struct vuart_info vuart;
 } exynos4210_com = {0};
 
 /* These parity settings can be ORed directly into the ULCON. */
@@ -281,6 +282,13 @@ static const struct dt_irq __init
*exynos4210_uart_dt_irq(struct serial_port *po
     return &uart->irq;
 }
 
+static const struct vuart_info *exynos4210_vuart_info(struct serial_port *port)
+{
+    struct exynos4210_uart *uart = port->uart;
+
+    return &uart->vuart;
+}
+
 static struct uart_driver __read_mostly exynos4210_uart_driver = {
     .init_preirq  = exynos4210_uart_init_preirq,
     .init_postirq = exynos4210_uart_init_postirq,
@@ -292,6 +300,7 @@ static struct uart_driver __read_mostly
exynos4210_uart_driver = {
     .getc         = exynos4210_uart_getc,
     .irq          = exynos4210_uart_irq,
     .dt_irq_get   = exynos4210_uart_dt_irq,
+    .vuart_info   = exynos4210_vuart_info,
 };
 
 /* TODO: Parse UART config from the command line */
@@ -337,6 +346,12 @@ static int __init exynos4210_uart_init(struct
dt_device_node *dev,
         return res;
     }
 
+    uart->vuart.base_addr = addr;
+    uart->vuart.size = size;
+    uart->vuart.data_off = UTXH;
+    uart->vuart.status_off = UTRSTAT;
+    uart->vuart.status = UTRSTAT_TXE | UTRSTAT_TXFE;
+
     /* Register with generic serial driver. */
     serial_register_uart(SERHND_DTUART, &exynos4210_uart_driver, uart);
 
diff --git a/xen/include/asm-arm/exynos4210-uart.h
b/xen/include/asm-arm/exynos4210-uart.h
index bd9a4be..e2ab4a4 100644
--- a/xen/include/asm-arm/exynos4210-uart.h
+++ b/xen/include/asm-arm/exynos4210-uart.h
@@ -88,6 +88,7 @@
 
 /* UTRSTAT */
 #define UTRSTAT_TXFE            (1 << 1)
+#define UTRSTAT_TXE             (1 << 2)
 
 /* URHX */
 #define URXH_DATA_MASK  (0xff)
-- 
1.7.10.4

Julien Grall

2013-Jul-31 15:38 UTC

head link

[PATCH v2 8/8] pl011: Implement vuart_info callback

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

---
    Changes in v2:
        - Use the renamed callback
---
 xen/drivers/char/pl011.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index fd87e68..05d034f 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -36,6 +36,7 @@ static struct pl011 {
     void __iomem *regs;
     /* UART with IRQ line: interrupt-driven I/O. */
     struct irqaction irqaction;
+    struct vuart_info vuart;
     /* /\* UART with no IRQ line: periodically-polled I/O. *\/ */
     /* struct timer timer; */
     /* unsigned int timeout_ms; */
@@ -190,6 +191,13 @@ static const struct dt_irq __init *pl011_dt_irq(struct
serial_port *port)
     return &uart->irq;
 }
 
+static const struct vuart_info *pl011_vuart(struct serial_port *port)
+{
+    struct pl011 *uart = port->uart;
+
+    return &uart->vuart;
+}
+
 static struct uart_driver __read_mostly pl011_driver = {
     .init_preirq  = pl011_init_preirq,
     .init_postirq = pl011_init_postirq,
@@ -201,6 +209,7 @@ static struct uart_driver __read_mostly pl011_driver = {
     .getc         = pl011_getc,
     .irq          = pl011_irq,
     .dt_irq_get   = pl011_dt_irq,
+    .vuart_info   = pl011_vuart,
 };
 
 /* TODO: Parse UART config from the command line */
@@ -248,6 +257,12 @@ static int __init pl011_uart_init(struct dt_device_node
*dev,
         return res;
     }
 
+    uart->vuart.base_addr = addr;
+    uart->vuart.size = size;
+    uart->vuart.data_off = DR;
+    uart->vuart.status_off = FR;
+    uart->vuart.status = 0;
+
     /* Register with generic serial driver. */
     serial_register_uart(SERHND_DTUART, &pl011_driver, uart);
 
-- 
1.7.10.4

Tim Deegan

2013-Aug-01 11:01 UTC

head link

Re: [PATCH v2 2/8] pl011: Move registers'' definition in a separate file

At 16:38 +0100 on 31 Jul (1375288702), Julien Grall
wrote:> ---
>  xen/drivers/char/pl011.c         |   48 +----------------------
>  xen/include/asm-arm/pl011-uart.h |   80
++++++++++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+), 47 deletions(-)
>  create mode 100644 xen/include/asm-arm/pl011-uart.h
This is missing a signed-off-by line. 

Cheers,

Tim.

Tim Deegan

2013-Aug-01 11:03 UTC

head link

Re: [PATCH v2 4/8] xen/arm: New callback in uart_driver to retrieve serial information

At 16:38 +0100 on 31 Jul (1375288704), Julien Grall
wrote:> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -32,6 +32,14 @@ enum serial_port_state {
>      serial_initialized
>  };
>  
> +struct vuart_info {
> +    unsigned long base_addr;    /* Base address of the UART */
Should this field be a paddr_t?

Cheers,

Tim.
> +    unsigned long size;         /* Size of the memory region */
> +    unsigned long data_off;     /* Data register offset */
> +    unsigned long status_off;   /* Status register offset */
> +    unsigned long status;       /* Ready status value */
> +};

Tim Deegan

2013-Aug-01 11:22 UTC

head link

Re: [PATCH v2 5/8] xen/arm: Implement a virtual UART

At 16:38 +0100 on 31 Jul (1375288705), Julien Grall
wrote:> This code is based on the previous vuart0 implementation. Unlike the
latter,
> it''s intend to replace UART stolen by XEN to DOM0 via dtuart=...
on its
> command line.
> 
> It''s useful when the kernel is compiled with early printk enabled
or for a
> single platform. Most of the time, the hardcoded code to handle the UART
> will need 2 registers: status and data, the others registers can be
> implemented as RAZ/WI.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
[...]> @@ -525,8 +525,12 @@ int arch_domain_create(struct domain *d, unsigned int
domcr_flags)
>      if ( (rc = vcpu_domain_init(d)) != 0 )
>          goto fail;
>  
> -    /* Domain 0 gets a real UART not an emulated one */
> -    if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 )
> +    /*
> +     * Virtual UART is only used by linux early printk and decompress
code.
> +     * Only use it for dom0 because the linux kernel may not support
> +     * multi-platform.
> +     */
> +    if ( (d->domain_id == 0) && (rc = domain_vuart_init(d)) )
>          goto fail;
So this is changing from:
 - dom0 gets a real UART, domU gets an emulated one; to
 - dom0 gets an emulated UART (and a real on), domU gets nothing.

I think that domU losing its UART should be mentioned in the changeset
description.

And won''t domU kernels need an emulated UART too?  Can an admin not use
the same kernel image for dom0 and domU?

[...]> + * This emulator uses the information from dtuart. This is not intended to
be
> + * a full emulation of an UART device. Rather it is intended to provide a
> + * sufficient veneer of one that early code (such as Linux''s boot
time
> + * decompressor) which hardcodes output directly to such a device are able
to
> + * make progress.
> + *
> + * The mininal register set to emulate an UART are:
s/mininal/minimal/

[...]> -static int uart0_mmio_write(struct vcpu *v, mmio_info_t *info)
> +static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info)
>  {
> +    struct domain *d = v->domain;
>      struct hsr_dabt dabt = info->dabt;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
>      register_t *r = select_user_reg(regs, dabt.reg);
> -    int offset = (int)(info->gpa - UART0_START);
> +    paddr_t offset = (int)(info->gpa -
d->arch.vuart.info->base_addr);
Please drop the cast to int here.  AFAICT it''s just confusing.
>  
> -    switch ( offset )
> -    {
> -    case UARTDR:
> +    if ( offset == d->arch.vuart.info->data_off )
>          /* ignore any status bits */
> -        uart0_print_char((int)((*r) & 0xFF));
> -        return 1;
> -    case UARTFR:
> -        /* Silently ignore */
> -        return 1;
> -    default:
> -        printk("VPL011: unhandled write r%d=%"PRIregister"
offset %#08x\n",
> -               dabt.reg, *r, offset);
> -        domain_crash_synchronous();
> -    }
> +        vuart_print_char((int)((*r) & 0xFF));
What''s this cast to int for?  The argument is implicitly cast to char
in
any case.  It''s also a bity surprising that we''ve got a struct
vcpu *v
in our hands this far down the stack but vuart_print_char() uses current.
(I realise that both these things are issues in the existing code but
since you''re already touching it it might be worth cleaning up.)

Cheers,

Tim.

Tim Deegan

2013-Aug-01 11:28 UTC

head link

Re: [PATCH v2 0/8] Emulate virtual UART for DOM0 and some UART clean up

At 16:38 +0100 on 31 Jul (1375288700), Julien Grall
wrote:>   pl011: Use ioreadl/iowritel
I seem to be missing this one for some reason.
>   pl011: Move registers'' definition in a separate file
Missing s-o-b, but otherwise Reviewed-by: Tim Deegan <tim@xen.org>
>   xen/arm: Use define instead of hardcoded value in debug-pl011
Reviewed-by: Tim Deegan <tim@xen.org>
>   xen/arm: New callback in uart_driver to retrieve serial information
>   xen/arm: Implement a virtual UART
I sent some comments on these two separately. 
>   exynos4210: rename UTRSTAT_TX_EMPTY in UTRSTAT_TXFE
>   exynos4210: Implement vuart_info callback
>   pl011: Implement vuart_info callback
All 3, Reviewed-by: Tim Deegan <tim@xen.org>

Cheers,

Tim.

Julien Grall

2013-Aug-01 12:20 UTC

head link

Re: [PATCH v2 0/8] Emulate virtual UART for DOM0 and some UART clean up

On 08/01/2013 12:28 PM, Tim Deegan wrote:
Hi,
> At 16:38 +0100 on 31 Jul (1375288700), Julien Grall wrote:
>>   pl011: Use ioreadl/iowritel
> 
> I seem to be missing this one for some reason.
Strange. I resent this patch with you in cc.
>>   pl011: Move registers'' definition in a separate file
> 
> Missing s-o-b, but otherwise Reviewed-by: Tim Deegan <tim@xen.org>
> 
>>   xen/arm: Use define instead of hardcoded value in debug-pl011
> 
> Reviewed-by: Tim Deegan <tim@xen.org>
> 
>>   xen/arm: New callback in uart_driver to retrieve serial information
>>   xen/arm: Implement a virtual UART
> 
> I sent some comments on these two separately. 
> 
>>   exynos4210: rename UTRSTAT_TX_EMPTY in UTRSTAT_TXFE
>>   exynos4210: Implement vuart_info callback
>>   pl011: Implement vuart_info callback
> 
> All 3, Reviewed-by: Tim Deegan <tim@xen.org>
Thanks for the review.

Cheers,

-- 
Julien

Julien Grall

2013-Aug-01 12:24 UTC

head link

Re: [PATCH v2 4/8] xen/arm: New callback in uart_driver to retrieve serial information

On 08/01/2013 12:03 PM, Tim Deegan wrote:> At 16:38 +0100 on 31 Jul (1375288704), Julien Grall wrote:
>> --- a/xen/include/xen/serial.h
>> +++ b/xen/include/xen/serial.h
>> @@ -32,6 +32,14 @@ enum serial_port_state {
>>      serial_initialized
>>  };
>>  
>> +struct vuart_info {
>> +    unsigned long base_addr;    /* Base address of the UART */
> 
> Should this field be a paddr_t?
Right. I will fix it on the next patch series.

-- 
Julien

Julien Grall

2013-Aug-01 12:43 UTC

head link

Re: [PATCH v2 5/8] xen/arm: Implement a virtual UART

On 08/01/2013 12:22 PM, Tim Deegan wrote:> At 16:38 +0100 on 31 Jul (1375288705), Julien Grall wrote:
>> This code is based on the previous vuart0 implementation. Unlike the
latter,
>> it''s intend to replace UART stolen by XEN to DOM0 via
dtuart=... on its
>> command line.
>>
>> It''s useful when the kernel is compiled with early printk
enabled or for a
>> single platform. Most of the time, the hardcoded code to handle the
UART
>> will need 2 registers: status and data, the others registers can be
>> implemented as RAZ/WI.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> [...]
>> @@ -525,8 +525,12 @@ int arch_domain_create(struct domain *d, unsigned
int domcr_flags)
>>      if ( (rc = vcpu_domain_init(d)) != 0 )
>>          goto fail;
>>  
>> -    /* Domain 0 gets a real UART not an emulated one */
>> -    if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 )
>> +    /*
>> +     * Virtual UART is only used by linux early printk and decompress
code.
>> +     * Only use it for dom0 because the linux kernel may not support
>> +     * multi-platform.
>> +     */
>> +    if ( (d->domain_id == 0) && (rc = domain_vuart_init(d))
)
>>          goto fail;
> 
> So this is changing from:
>  - dom0 gets a real UART, domU gets an emulated one; to
>  - dom0 gets an emulated UART (and a real on), domU gets nothing.
> 
> I think that domU losing its UART should be mentioned in the changeset
> description.
> 
> And won''t domU kernels need an emulated UART too?  Can an admin
not use
> the same kernel image for dom0 and domU?
In theory, it''s possible to use the same kernel for dom0 and domU only
if the kernel support multi-platform (CONFIG_ARCH_MULTIPLATFORM). In
this case the early printk should be disabled by default, unless for
debugging.

I don''t like the solution to expose the UART to guest because the
memory
layout might be different between the real hardware and the guest.
I would prefer to implement a specific xen early printk in the kernel.
> 
> [...]
>> + * This emulator uses the information from dtuart. This is not
intended to be
>> + * a full emulation of an UART device. Rather it is intended to
provide a
>> + * sufficient veneer of one that early code (such as Linux''s
boot time
>> + * decompressor) which hardcodes output directly to such a device are
able to
>> + * make progress.
>> + *
>> + * The mininal register set to emulate an UART are:
> 
> s/mininal/minimal/
> [...]
>> -static int uart0_mmio_write(struct vcpu *v, mmio_info_t *info)
>> +static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info)
>>  {
>> +    struct domain *d = v->domain;
>>      struct hsr_dabt dabt = info->dabt;
>>      struct cpu_user_regs *regs = guest_cpu_user_regs();
>>      register_t *r = select_user_reg(regs, dabt.reg);
>> -    int offset = (int)(info->gpa - UART0_START);
>> +    paddr_t offset = (int)(info->gpa -
d->arch.vuart.info->base_addr);
> 
> Please drop the cast to int here.  AFAICT it''s just confusing.
> 
>>  
>> -    switch ( offset )
>> -    {
>> -    case UARTDR:
>> +    if ( offset == d->arch.vuart.info->data_off )
>>          /* ignore any status bits */
>> -        uart0_print_char((int)((*r) & 0xFF));
>> -        return 1;
>> -    case UARTFR:
>> -        /* Silently ignore */
>> -        return 1;
>> -    default:
>> -        printk("VPL011: unhandled write
r%d=%"PRIregister" offset %#08x\n",
>> -               dabt.reg, *r, offset);
>> -        domain_crash_synchronous();
>> -    }
>> +        vuart_print_char((int)((*r) & 0xFF));
> 
> What''s this cast to int for?  The argument is implicitly cast to
char in
> any case.  It''s also a bity surprising that we''ve got a
struct vcpu *v
> in our hands this far down the stack but vuart_print_char() uses current.
> (I realise that both these things are issues in the existing code but
> since you''re already touching it it might be worth cleaning up.)
I don''t see any reason for the cast, the code come from Ian Campbell.
Ian, do know why?

I will address all your comments in the next patch series.

-- 
Julien

Ian Campbell

2013-Aug-01 15:42 UTC

head link

Re: [PATCH v2 5/8] xen/arm: Implement a virtual UART

On Thu, 2013-08-01 at 13:43 +0100, Julien Grall wrote:> >>  
> >> -    switch ( offset )
> >> -    {
> >> -    case UARTDR:
> >> +    if ( offset == d->arch.vuart.info->data_off )
> >>          /* ignore any status bits */
> >> -        uart0_print_char((int)((*r) & 0xFF));
> >> -        return 1;
> >> -    case UARTFR:
> >> -        /* Silently ignore */
> >> -        return 1;
> >> -    default:
> >> -        printk("VPL011: unhandled write
r%d=%"PRIregister" offset %#08x\n",
> >> -               dabt.reg, *r, offset);
> >> -        domain_crash_synchronous();
> >> -    }
> >> +        vuart_print_char((int)((*r) & 0xFF));
> > 
> > What''s this cast to int for?  The argument is implicitly cast
to char in
> > any case.  It''s also a bity surprising that we''ve
got a struct vcpu *v
> > in our hands this far down the stack but vuart_print_char() uses
current.
> > (I realise that both these things are issues in the existing code but
> > since you''re already touching it it might be worth cleaning
up.)
> 
> I don''t see any reason for the cast, the code come from Ian
Campbell.
> Ian, do know why?
Nope ;-) If there was a reason I''ve no idea what it was...

Ian.

Xen devel - Jul 2013 - [PATCH v2 0/8] Emulate virtual UART for DOM0 and some UART clean up

[PATCH v2 0/8] Emulate virtual UART for DOM0 and some UART clean up

[PATCH v2 2/8] pl011: Move registers'' definition in a separate file

[PATCH v2 3/8] xen/arm: Use define instead of hardcoded value in debug-pl011

[PATCH v2 4/8] xen/arm: New callback in uart_driver to retrieve serial information

[PATCH v2 5/8] xen/arm: Implement a virtual UART

[PATCH v2 6/8] exynos4210: rename UTRSTAT_TX_EMPTY in UTRSTAT_TXFE

[PATCH v2 7/8] exynos4210: Implement vuart_info callback

[PATCH v2 8/8] pl011: Implement vuart_info callback

Re: [PATCH v2 2/8] pl011: Move registers'' definition in a separate file

Re: [PATCH v2 4/8] xen/arm: New callback in uart_driver to retrieve serial information

Re: [PATCH v2 5/8] xen/arm: Implement a virtual UART

Re: [PATCH v2 0/8] Emulate virtual UART for DOM0 and some UART clean up

Re: [PATCH v2 0/8] Emulate virtual UART for DOM0 and some UART clean up

Re: [PATCH v2 4/8] xen/arm: New callback in uart_driver to retrieve serial information

Re: [PATCH v2 5/8] xen/arm: Implement a virtual UART

Re: [PATCH v2 5/8] xen/arm: Implement a virtual UART