Hi Anastasiia,
On Wed, 1 Jul 2020 at 10:30, Anastasiia Lukianenko <vicooo...@gmail.com> wrote:
>
> From: Peng Fan <peng....@nxp.com>
>
> Add support for Xen para-virtualized serial driver. This
> driver fully supports serial console for the virtual machine.
>
> Please note that as the driver is initialized late, so no banner
> nor memory size is visible.
>
> Signed-off-by: Peng Fan <peng....@nxp.com>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukiane...@epam.com>
> ---
> arch/arm/Kconfig | 1 +
> board/xen/xenguest_arm64/xenguest_arm64.c | 31 +++-
> configs/xenguest_arm64_defconfig | 4 +-
> drivers/serial/Kconfig | 7 +
> drivers/serial/Makefile | 1 +
> drivers/serial/serial_xen.c | 175 ++++++++++++++++++++++
> drivers/xen/events.c | 4 +
> 7 files changed, 214 insertions(+), 9 deletions(-)
> create mode 100644 drivers/serial/serial_xen.c
Reviewed-by: Simon Glass <s...@chromium.org>
nits below
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index c469863967..d4de1139aa 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1723,6 +1723,7 @@ config TARGET_XENGUEST_ARM64
> select XEN
> select OF_CONTROL
> select LINUX_KERNEL_IMAGE_HEADER
> + select XEN_SERIAL
> endchoice
>
> config ARCH_SUPPORT_TFABOOT
> diff --git a/board/xen/xenguest_arm64/xenguest_arm64.c
> b/board/xen/xenguest_arm64/xenguest_arm64.c
> index 9e099f388f..fd10a002e9 100644
> --- a/board/xen/xenguest_arm64/xenguest_arm64.c
> +++ b/board/xen/xenguest_arm64/xenguest_arm64.c
> @@ -18,9 +18,12 @@
> #include <asm/armv8/mmu.h>
> #include <asm/xen.h>
> #include <asm/xen/hypercall.h>
> +#include <asm/xen/system.h>
>
> #include <linux/compiler.h>
>
> +#include <xen/hvm.h>
> +
> DECLARE_GLOBAL_DATA_PTR;
>
> int board_init(void)
> @@ -57,9 +60,28 @@ static int get_next_memory_node(const void *blob, int mem)
>
> static int setup_mem_map(void)
> {
> - int i, ret, mem, reg = 0;
> + int i = 0, ret, mem, reg = 0;
> struct fdt_resource res;
> const void *blob = gd->fdt_blob;
> + u64 gfn;
> +
> + /*
> + * Add "magic" region which is used by Xen to provide some essentials
> + * for the guest: we need console.
> + */
> + ret = hvm_get_parameter_maintain_dcache(HVM_PARAM_CONSOLE_PFN, &gfn);
> + if (ret < 0) {
> + printf("%s: Can't get HVM_PARAM_CONSOLE_PFN, ret %d\n",
> + __func__, ret);
> + return -EINVAL;
> + }
> +
> + xen_mem_map[i].virt = PFN_PHYS(gfn);
> + xen_mem_map[i].phys = PFN_PHYS(gfn);
> + xen_mem_map[i].size = PAGE_SIZE;
> + xen_mem_map[i].attrs = (PTE_BLOCK_MEMTYPE(MT_NORMAL) |
> + PTE_BLOCK_INNER_SHARE);
> + i++;
>
> mem = get_next_memory_node(blob, -1);
> if (mem < 0) {
> @@ -67,7 +89,7 @@ static int setup_mem_map(void)
> return -EINVAL;
> }
>
> - for (i = 0; i < MAX_MEM_MAP_REGIONS; i++) {
> + for (; i < MAX_MEM_MAP_REGIONS; i++) {
> ret = fdt_get_resource(blob, mem, "reg", reg++, &res);
> if (ret == -FDT_ERR_NOTFOUND) {
> reg = 0;
> @@ -146,8 +168,3 @@ int print_cpuinfo(void)
> return 0;
> }
>
> -__weak struct serial_device *default_serial_console(void)
> -{
> - return NULL;
> -}
> -
> diff --git a/configs/xenguest_arm64_defconfig
> b/configs/xenguest_arm64_defconfig
> index 2a8caf8647..45559a161b 100644
> --- a/configs/xenguest_arm64_defconfig
> +++ b/configs/xenguest_arm64_defconfig
> @@ -47,9 +47,9 @@ CONFIG_CMD_UMS=n
> #CONFIG_EFI_PARTITION=y
> # CONFIG_EFI_LOADER is not set
>
> -# CONFIG_DM is not set
> +CONFIG_DM=y
> # CONFIG_MMC is not set
> -# CONFIG_DM_SERIAL is not set
> +CONFIG_DM_SERIAL=y
> # CONFIG_REQUIRE_SERIAL_CONSOLE is not set
>
> CONFIG_OF_BOARD=y
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 17d0e73623..33c989a66d 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -821,6 +821,13 @@ config MPC8XX_CONS
> depends on MPC8xx
> default y
>
> +config XEN_SERIAL
> + bool "XEN serial support"
> + depends on XEN
> + help
> + If built without DM support, then requires Xen
> + to be built with CONFIG_VERBOSE_DEBUG.
Yes but what does it do? Also should probably not support non-DM at this point.
> +
> choice
> prompt "Console port"
> default 8xx_CONS_SMC1
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index e4a92bbbb7..25f7f8d342 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_OWL_SERIAL) += serial_owl.o
> obj-$(CONFIG_OMAP_SERIAL) += serial_omap.o
> obj-$(CONFIG_MTK_SERIAL) += serial_mtk.o
> obj-$(CONFIG_SIFIVE_SERIAL) += serial_sifive.o
> +obj-$(CONFIG_XEN_SERIAL) += serial_xen.o
>
> ifndef CONFIG_SPL_BUILD
> obj-$(CONFIG_USB_TTY) += usbtty.o
> diff --git a/drivers/serial/serial_xen.c b/drivers/serial/serial_xen.c
> new file mode 100644
> index 0000000000..dcd4b2df79
> --- /dev/null
> +++ b/drivers/serial/serial_xen.c
> @@ -0,0 +1,175 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0+
> + *
> + * (C) 2018 NXP
> + * (C) 2020 EPAM Systems Inc.
> + */
> +#include <common.h>
> +#include <cpu_func.h>
> +#include <dm.h>
> +#include <serial.h>
> +#include <watchdog.h>
> +
> +#include <linux/bug.h>
> +
> +#include <xen/hvm.h>
> +#include <xen/events.h>
> +
> +#include <xen/interface/sched.h>
> +#include <xen/interface/hvm/hvm_op.h>
> +#include <xen/interface/hvm/params.h>
> +#include <xen/interface/io/console.h>
> +#include <xen/interface/io/ring.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +u32 console_evtchn;
> +
> +struct xen_uart_priv {
> + struct xencons_interface *intf;
> + u32 evtchn;
> + int vtermno;
> + struct hvc_struct *hvc;
comment for struct
> +};
> +
> +int xen_serial_setbrg(struct udevice *dev, int baudrate)
> +{
> + return 0;
> +}
> +
> +static int xen_serial_probe(struct udevice *dev)
> +{
> + struct xen_uart_priv *priv = dev_get_priv(dev);
> + u64 v = 0;
> + unsigned long gfn;
> + int r;
> +
> + r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
Can you use ret and val instead of single-char var names? It is OK for
loops, but not here.
> + if (r < 0 || v == 0)
> + return r;
> +
> + priv->evtchn = v;
> + console_evtchn = v;
> +
> + r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> + if (r < 0 || v == 0)
> + return -ENODEV;
return r if non-zero
return -EINVAL perhaps or -ENXIO if !v
-ENODEV means there is no device and is reserved for driver model.
Clearly in this case there is a device.
> +
> + gfn = v;
> +
> + priv->intf = (struct xencons_interface *)(gfn << XEN_PAGE_SHIFT);
> + if (!priv->intf)
Don't you already check for !v above?
> + return -EINVAL;
Blank line
> + return 0;
> +}
> +
> +static int xen_serial_pending(struct udevice *dev, bool input)
> +{
> + struct xen_uart_priv *priv = dev_get_priv(dev);
> + struct xencons_interface *intf = priv->intf;
> +
> + if (!input || intf->in_cons == intf->in_prod)
> + return 0;
blank line before final return. Please fix globally
> + return 1;
> +}
> +
> +static int xen_serial_getc(struct udevice *dev)
> +{
> + struct xen_uart_priv *priv = dev_get_priv(dev);
> + struct xencons_interface *intf = priv->intf;
> + XENCONS_RING_IDX cons;
> + char c;
> +
> + while (intf->in_cons == intf->in_prod) {
> + mb(); /* wait */
> + }
Drop {}. Has this been through patman?
> +
> + cons = intf->in_cons;
> + mb(); /* get pointers before reading ring */
> +
> + c = intf->in[MASK_XENCONS_IDX(cons++, intf->in)];
> +
> + mb(); /* read ring before consuming */
> + intf->in_cons = cons;
> +
> + notify_remote_via_evtchn(priv->evtchn);
> + return c;
> +}
> +
> +static int __write_console(struct udevice *dev, const char *data, int len)
> +{
> + struct xen_uart_priv *priv = dev_get_priv(dev);
> + struct xencons_interface *intf = priv->intf;
> + XENCONS_RING_IDX cons, prod;
> + int sent = 0;
> +
> + cons = intf->out_cons;
> + prod = intf->out_prod;
> + mb(); /* Update pointer */
> +
> + WARN_ON((prod - cons) > sizeof(intf->out));
> +
> + while ((sent < len) && ((prod - cons) < sizeof(intf->out)))
> + intf->out[MASK_XENCONS_IDX(prod++, intf->out)] = data[sent++];
> +
> + mb(); /* Update data before pointer */
> + intf->out_prod = prod;
> +
> + if (sent)
> + notify_remote_via_evtchn(priv->evtchn);
> + return sent;
> +}
> +
> +static int write_console(struct udevice *dev, const char *data, int len)
> +{
> + /*
> + * Make sure the whole buffer is emitted, polling if
> + * necessary. We don't ever want to rely on the hvc daemon
> + * because the most interesting console output is when the
> + * kernel is crippled.
> + */
> + while (len) {
> + int sent = __write_console(dev, data, len);
> +
> + data += sent;
> + len -= sent;
> +
> + if (unlikely(len))
> + HYPERVISOR_sched_op(SCHEDOP_yield, NULL);
> + }
> + return 0;
> +}
> +
> +static int xen_serial_putc(struct udevice *dev, const char ch)
> +{
> + write_console(dev, &ch, 1);
> + return 0;
> +}
> +
> +static const struct dm_serial_ops xen_serial_ops = {
> + .putc = xen_serial_putc,
> + .getc = xen_serial_getc,
> + .pending = xen_serial_pending,
> +};
> +
> +#if CONFIG_IS_ENABLED(OF_CONTROL)
> +static const struct udevice_id xen_serial_ids[] = {
> + { .compatible = "xen,xen" },
> + { }
> +};
> +#endif
> +
> +U_BOOT_DRIVER(serial_xen) = {
> + .name = "serial_xen",
> + .id = UCLASS_SERIAL,
> +#if CONFIG_IS_ENABLED(OF_CONTROL)
of_patch_ptr() - but I think you can drop this
> + .of_match = xen_serial_ids,
> +#endif
> + .priv_auto_alloc_size = sizeof(struct xen_uart_priv),
> + .probe = xen_serial_probe,
> + .ops = &xen_serial_ops,
> +#if !CONFIG_IS_ENABLED(OF_CONTROL)
and this?
> + .flags = DM_FLAG_PRE_RELOC,
> +#endif
> +};
> +
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index eddc6b6e29..a1b36a2196 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -25,6 +25,8 @@
> #include <xen/events.h>
> #include <xen/hvm.h>
>
> +extern u32 console_evtchn;
> +
> #define NR_EVS 1024
>
> /* this represents a event handler. Chaining or sharing is not allowed */
> @@ -47,6 +49,8 @@ void unbind_all_ports(void)
> struct vcpu_info *vcpu_info = &s->vcpu_info[cpu];
>
> for (i = 0; i < NR_EVS; i++) {
> + if (i == console_evtchn)
> + continue;
> if (test_and_clear_bit(i, bound_ports)) {
> printf("port %d still bound!\n", i);
> unbind_evtchn(i);
> --
> 2.17.1
>
Regards,
Simon
