Re: [Xen-devel] [PATCH 04/25 v6] xen/arm: vpl011: Add support for vuart in libxl

[Julien Grall <julien.grall@xxxxxxx>]

Hi Bhupinder,

Sorry I am jumping a bit late in the discussion here.

On 17/07/17 14:06, Bhupinder Thakur wrote:
> An option is provided in libxl to enable/disable sbsa vuart while
s/sbsa/SBSA/

> creating a guest domain.
>
> Libxl now suppots a generic vuart console and sbsa uart is a specific type.
s/suppots/supports/

s/sbsa/SBSA/

> In future support can be added for multiple vuart of different types.
>
> User can enable sbsa vuart by adding the following line in the guest
ditto.

> configuration file:
>
> vuart = "sbsa_uart"
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Acked-by: Wei Liu <wei.liu2@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 "pl011" to "sbsa_uart".
>
> Changes since v3:
> - Added a new config option CONFIG_VUART_CONSOLE to enable/disable vuart console
>   support.
> - Moved libxl_vuart_type to arch-arm part of libxl_domain_build_info
> - Updated xl command help to mention new console type - vuart.
>
> Changes since v2:
> - Defined vuart option as an enum instead of a string.
> - Removed the domain creation flag defined for vuart and the related code
>   to pass on the information while domain creation. Now vpl011 is initialized
>   independent of domain creation through new DOMCTL APIs.
>
>  tools/libxl/libxl.h          | 6 ++++++
>  tools/libxl/libxl_console.c  | 3 +++
>  tools/libxl/libxl_dom.c      | 1 +
>  tools/libxl/libxl_internal.h | 3 +++
>  tools/libxl/libxl_types.idl  | 7 +++++++
>  tools/xl/xl_cmdtable.c       | 2 +-
>  tools/xl/xl_console.c        | 5 ++++-
>  tools/xl/xl_parse.c          | 8 ++++++++
>  8 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 7cf0f31..892ed35 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -306,6 +306,12 @@
>  #define LIBXL_HAVE_BUILDINFO_HVM_ACPI_LAPTOP_SLATE 1
>
>  /*
> + * LIBXL_HAVE_VUART indicates that xenconsole/client supports
> + * virtual uart.
I am not sure why you mention about xenconsole/client supporting VUART. 
It does not really matter here, someone may use another backend for the 
PV console here. What matters is the existence or arm.vuart.
> + */
> +#define LIBXL_HAVE_VUART 1
Here you give the impression the virtual UART is supported for all the 
architectures but ...
[...]

> @@ -581,6 +587,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>
>
>      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> +                               ("vuart", libxl_vuart_type),
... here it is ARM specific. I am not convinced that we should tie vuart 
to ARM only. I cannot see why x86 would not be able to use it in the 
future. Any opinions?
>                                ])),
>      # Alternate p2m is not bound to any architecture or guest type, as it is
>      # supported by x86 HVM and ARM support is planned.
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 30eb93c..9f91651 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -133,7 +133,7 @@ struct cmd_spec cmd_table[] = {
>        &main_console, 0, 0,
>        "Attach to domain's console",
>        "[options] <Domain>\n"
> -      "-t <type>       console type, pv or serial\n"
> +      "-t <type>       console type, pv , serial or vuart\n"
>        "-n <number>     console number"
>      },
>      { "vncviewer",
> diff --git a/tools/xl/xl_console.c b/tools/xl/xl_console.c
> index 0508dda..4e65d73 100644
> --- a/tools/xl/xl_console.c
> +++ b/tools/xl/xl_console.c
> @@ -27,6 +27,7 @@ int main_console(int argc, char **argv)
>      uint32_t domid;
>      int opt = 0, num = 0;
>      libxl_console_type type = 0;
> +    char *console_names = "pv, serial, vuart";
>
>      SWITCH_FOREACH_OPT(opt, "n:t:", NULL, "console", 1) {
>      case 't':
> @@ -34,8 +35,10 @@ int main_console(int argc, char **argv)
>              type = LIBXL_CONSOLE_TYPE_PV;
>          else if (!strcmp(optarg, "serial"))
>              type = LIBXL_CONSOLE_TYPE_SERIAL;
> +        else if (!strcmp(optarg, "vuart"))
> +            type = LIBXL_CONSOLE_TYPE_VUART;
>          else {
> -            fprintf(stderr, "console type supported are: pv, serial\n");
> +            fprintf(stderr, "console type supported are: %s\n", console_names);
>              return EXIT_FAILURE;
>          }
>          break;
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 5c2bf17..71588de 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -918,6 +918,14 @@ void parse_config_data(const char *config_source,
>      if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
>          b_info->max_vcpus = l;
>
> +    if (!xlu_cfg_get_string(config, "vuart", &buf, 0)) {
> +        if (libxl_vuart_type_from_string(buf, &b_info->arch_arm.vuart)) {
> +            fprintf(stderr, "ERROR: invalid value \"%s\" for \"vuart\"\n",
> +                    buf);
> +            exit(1);
> +        }
> +    }
> +
>      parse_vnuma_config(config, b_info);
>
>      /* Set max_memkb to target_memkb and max_vcpus to avail_vcpus if
Cheers,

--
Julien Grall

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