[PATCH 07/17] serial: serial_xen: Add Xen PV serial driver

[Simon Glass <sjg@chromium.org>]

Hi Anastasiia,

On Wed, 1 Jul 2020 at 10:30, Anastasiia Lukianenko <vicooodin@gmail.com> wrote:
>
> From: Peng Fan <peng.fan@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.fan@nxp.com>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Anastasiia Lukianenko <anastasiia_lukianenko@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 <sjg@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