Re: [PATCH 04/10 v2] xen/arm: vpl011: Add support for vuart in libxl

[Julien Grall <julien.grall@arm.com>]

Hi Bhupinder,

On 28/04/17 17:01, Bhupinder Thakur wrote:
> An option is provided in libxl to enable/disable pl011 vuart while
> creating a guest domain.
>
> Libxl now suppots a generic vuart console and pl011 is a specific type.

s/supports/supports/

> In future support can be added for multiple vuart of different types.
>
> User can enable pl011 vuart by adding the following line in the guest
> configuration file:
>
> vuart = "pl011"
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>
> Changes since v1:
> - Modified the syntax for taking the pl011 as a console type in the
>   configuration file. Now the syntax is vuart = "pl011".
> - Replaced the console type VCON with VUART, as it is more
>   intuitive.
>
>  tools/libxl/libxl.h          |  6 ++++++
>  tools/libxl/libxl_create.c   | 10 ++++++++++
>  tools/libxl/libxl_internal.h |  4 ++++
>  tools/libxl/libxl_types.idl  |  2 ++
>  tools/xl/xl_console.c        |  4 +++-
>  tools/xl/xl_parse.c          |  3 +++
>  6 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index cf8687a..bcfbb6c 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.
> + */
> +#define LIBXL_HAVE_VUART 1
> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index bffbc45..5d70bc2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -536,6 +536,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>          flags |= libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
>      }
>
> +    if (!strcmp(d_config->b_info.vuart, "pl011"))
> +        flags |= XEN_DOMCTL_VUART_enable;

Likely, this would have to be arch specific and not in the common code. 
Have a look to libxl__arch_domain_prepare_config

> +
>      /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
>      libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid);
>
> @@ -900,6 +903,11 @@ static void initiate_domain_create(libxl__egc *egc,
>          goto error_out;
>      }
>
> +    if (!strcmp(d_config->b_info.vuart, "pl011"))
> +        state->vuart_enabled = true;
> +    else
> +        state->vuart_enabled = false;
> +

Same here, this should be in arch code. Also you don't need if/else to 
set a boolean.

You could simply do:

state->vuart_enable = !strcmp(d_config->b_info.vuart, "pl011");

>      if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>          (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
>          (libxl_defbool_val(d_config->b_info.u.hvm.altp2m) ||
> @@ -918,6 +926,8 @@ static void initiate_domain_create(libxl__egc *egc,
>          goto error_out;
>      }
>
> +    state->config.console_domid = state->console_domid;
> +
>      ret = libxl__domain_make(gc, d_config, &domid, &state->config);
>      if (ret) {
>          LOGD(ERROR, domid, "cannot make domain: %d", ret);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 5d082c5..9dba8e7 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1135,6 +1135,10 @@ typedef struct {
>      uint32_t num_vmemranges;
>
>      xc_domain_configuration_t config;
> +
> +    unsigned long vuart_mfn;

s/unsigned long/xen_pfn_t/ and s/vuart_mfn/vuart_gfn/

Also, what is this frame number for? The MMIO region or the console 
frame number?


> +    uint32_t    vuart_port;

What is port?

> +    bool        vuart_enabled;
>  } libxl__domain_build_state;
>
>  _hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 2204425..5d53f2c 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -105,6 +105,7 @@ libxl_console_type = Enumeration("console_type", [
>      (0, "UNKNOWN"),
>      (1, "SERIAL"),
>      (2, "PV"),
> +    (3, "VUART"),
>      ])
>
>  libxl_disk_format = Enumeration("disk_format", [
> @@ -470,6 +471,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("disable_migrate", libxl_defbool),
>      ("cpuid",           libxl_cpuid_policy_list),
>      ("blkdev_start",    string),
> +    ("vuart",           string),
>
>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>
> diff --git a/tools/xl/xl_console.c b/tools/xl/xl_console.c
> index 0508dda..6f3cd7f 100644
> --- a/tools/xl/xl_console.c
> +++ b/tools/xl/xl_console.c
> @@ -34,8 +34,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: pv, serial, vuart\n");
>              return EXIT_FAILURE;
>          }
>          break;
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 856a304..80fd184 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -916,6 +916,9 @@ 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_replace_string(config, "vuart", &b_info->vuart, 0))
> +        b_info->vuart = strdup("unknown");
> +
>      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@lists.xen.org
https://lists.xen.org/xen-devel