[PATCH 07/17] serial: serial_xen: Add Xen PV serial driver
Anastasiia Lukianenko
Anastasiia_Lukianenko at epam.com
Fri Jul 3 14:59:35 CEST 2020
Hello Simon,
On Thu, 2020-07-02 at 21:50 -0600, Simon Glass wrote:
> Hi Anastasiia,
>
> On Wed, 1 Jul 2020 at 10:30, Anastasiia Lukianenko <
> vicooodin at gmail.com> wrote:
> >
> > From: Peng Fan <peng.fan at 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 at nxp.com>
> > Signed-off-by: Oleksandr Andrushchenko <
> > oleksandr_andrushchenko at epam.com>
> > Signed-off-by: Anastasiia Lukianenko <
> > anastasiia_lukianenko at 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 at 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.
>
This will be removed at all as only DM based drivers are added.
> > +
> > 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
Ok, will add.
>
> > +};
> > +
> > +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.
Sure.
>
> > + 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.
Ok, makes sense.
>
> > +
> > + 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
Ok, will fix.
>
> > + 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
Ok, will fix in the next version.
>
> > + 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?
We used checkpatch before sending.
>
> > +
> > + 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
Regards,
Anastasiia
More information about the U-Boot
mailing list