Re: [Xen-devel] [PATCH 22/25 v6] xen/arm: vpl011: Add support for vuart console in xenconsole

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 17 Jul 2017, Bhupinder Thakur wrote:
> This patch finally adds the support for vuart console. It adds
> two new fields in the console initialization:
> 
> - optional
> - prefer_gnttab
> 
> optional flag tells whether the console is optional.
> 
> prefer_gnttab tells whether the ring buffer should be allocated using
> grant table.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> 
> Changes since v4:
> - Renamed VUART_CFLAGS- to CFLAGS_vuart- in the Makefile as per the 
> convention.
> 
>  config/arm32.mk           |  1 +
>  config/arm64.mk           |  1 +
>  tools/console/Makefile    |  3 ++-
>  tools/console/daemon/io.c | 29 ++++++++++++++++++++++++++++-
>  4 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/config/arm32.mk b/config/arm32.mk
> index f95228e..b9f23fe 100644
> --- a/config/arm32.mk
> +++ b/config/arm32.mk
> @@ -1,5 +1,6 @@
>  CONFIG_ARM := y
>  CONFIG_ARM_32 := y
> +CONFIG_VUART_CONSOLE := y
>  CONFIG_ARM_$(XEN_OS) := y
>  
>  CONFIG_XEN_INSTALL_SUFFIX :=

What about leaving this off for ARM32 by default?


> diff --git a/config/arm64.mk b/config/arm64.mk
> index aa45772..861d0a4 100644
> --- a/config/arm64.mk
> +++ b/config/arm64.mk
> @@ -1,5 +1,6 @@
>  CONFIG_ARM := y
>  CONFIG_ARM_64 := y
> +CONFIG_VUART_CONSOLE := y
>  CONFIG_ARM_$(XEN_OS) := y
>  
>  CONFIG_XEN_INSTALL_SUFFIX :=
> diff --git a/tools/console/Makefile b/tools/console/Makefile
> index c8b0300..1cddb6e 100644
> --- a/tools/console/Makefile
> +++ b/tools/console/Makefile
> @@ -11,6 +11,7 @@ LDLIBS += $(SOCKET_LIBS)
>  
>  LDLIBS_xenconsoled += $(UTIL_LIBS)
>  LDLIBS_xenconsoled += -lrt
> +CFLAGS_vuart-$(CONFIG_VUART_CONSOLE) = -DCONFIG_VUART_CONSOLE
>  
>  BIN      = xenconsoled xenconsole
>  
> @@ -28,7 +29,7 @@ clean:
>  distclean: clean
>  
>  daemon/main.o: daemon/_paths.h
> -daemon/io.o: CFLAGS += $(CFLAGS_libxenevtchn) $(CFLAGS_libxengnttab)
> +daemon/io.o: CFLAGS += $(CFLAGS_libxenevtchn) $(CFLAGS_libxengnttab) 
> $(CFLAGS_vuart-y)
>  xenconsoled: $(patsubst %.c,%.o,$(wildcard daemon/*.c))
>       $(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_libxenevtchn) 
> $(LDLIBS_libxengnttab) $(LDLIBS_xenconsoled) $(APPEND_LDFLAGS)
>  
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 49f085c..c6d4cae 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -107,12 +107,16 @@ struct console {
>       xenevtchn_port_or_error_t remote_port;
>       struct xencons_interface *interface;
>       struct domain *d;
> +     bool optional;
> +     bool prefer_gnttab;
>  };
>  
>  struct console_data {
>       const char *const xsname;
>       const char *const ttyname;
>       const char *const log_suffix;
> +     bool optional;
> +     bool prefer_gnttab;
>  };
>  
>  static struct console_data console_data[] = {
> @@ -120,7 +124,18 @@ static struct console_data console_data[] = {
>               .xsname = "/console",
>               .ttyname = "tty",
>               .log_suffix = "",
> +             .optional = false,
> +             .prefer_gnttab = true,
>       },
> +#if defined(CONFIG_VUART_CONSOLE)
> +     {
> +             .xsname = "/vuart/0",
> +             .ttyname = "tty",
> +             .log_suffix = "-vuart0",
> +             .optional = true,
> +             .prefer_gnttab = false,
> +     },
> +#endif
>  };
>  
>  #define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
> @@ -655,8 +670,18 @@ static int console_create_ring(struct console *con)
>                       "ring-ref", "%u", &ring_ref,
>                       "port", "%i", &remote_port,
>                       NULL);
> +
>       if (err)
> +     {

wrong code style for tools/console


> +             /*
> +              * This is a normal condition for optional consoles: they might 
> not be
> +              * present on xenstore at all. In that case, just return 
> without error.
> +             */
> +             if (con->optional)
> +                     err = 0;
> +
>               goto out;
> +     }
>  
>       snprintf(path, sizeof(path), "%s/type", con->xspath);
>       type = xs_read(xs, XBT_NULL, path, NULL);
> @@ -670,7 +695,9 @@ static int console_create_ring(struct console *con)
>       if (ring_ref != con->ring_ref && con->ring_ref != -1)
>               console_unmap_interface(con);
>  
> -     if (!con->interface && xgt_handle) {
> +     if (!con->interface &&
> +         xgt_handle &&
> +         con->prefer_gnttab) {
>               /* Prefer using grant table */
>               con->interface = xengnttab_map_grant_ref(xgt_handle,
>                       dom->domid, GNTTAB_RESERVED_CONSOLE,

I noticed that you removed the initialization of optional and
prefer_gnttab from console_init from this patch:

@@ -790,6 +817,8 @@ static int console_init(struct console *con, struct domain 
*dom, void **data)
      con->d = dom;
      con->ttyname = (*con_data)->ttyname;
      con->log_suffix = (*con_data)->log_suffix;
+     con->optional = (*con_data)->optional;
+     con->prefer_gnttab = (*con_data)->prefer_gnttab;
      xsname = (*con_data)->xsname;
      con->xspath = xs_get_domain_path(xs, dom->domid);
      s = realloc(con->xspath, strlen(con->xspath) +

Why? It that intended?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel