Re: [Xen-devel] [PATCH v12 6/6] introduce a 'passthrough' configuration option to xl.cfg...

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Sep 16, 2019 at 10:27:08AM +0100, Paul Durrant wrote:
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 59dbcb50a0..7afae81432 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -30,6 +30,12 @@
>  int libxl__domain_create_info_setdefault(libxl__gc *gc,
>                                           libxl_domain_create_info *c_info)
>  {
> +    libxl_physinfo info;
> +    int rc = libxl_get_physinfo(CTX, &info);
> +
> +    if (rc)
> +            return rc;
> +

I think this hunk would be more readable if it was written:
    int rc;

    rc = libxl_get_physinfo(CTX, &info);
    if (rc)
        return rc;

Otherwise, the check for error is alone which might mean "we are in a
callback function and check for rc passed by parameter" or that it's a
stray check.

> @@ -62,6 +62,12 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
>      if (!c_info->ssidref)
>          c_info->ssidref = SECINITSID_DOMU;
>  
> +    if (info.cap_hvm_directio) {
> +        c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
> +                               !info.cap_iommu_hap_pt_share) ?
> +            LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
> +    }

I don't think you can do that. This will overwrite the value that was in
c_info before (that a user as set). The _setdefault() function is only
called after c_info have been filled by users of the libxl, it only has
to change the value if it was the default. c_info->passthrough has no
default value, so it's not possible to know what a user wants.

What about adding "default"==0 in libxl_passthrough enum?

>      return 0;
>  }
>  
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index d52c63b6b0..22f05711e3 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -263,6 +263,12 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
>      (2, "LINUX")
>      ])
>  
> +libxl_passthrough = Enumeration("passthrough", [
> +    (0, "disabled"),
> +    (1, "sync_pt"),
> +    (2, "share_pt"),
> +    ])
> +
>  #
>  # Complex libxl types
>  #
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 293f5f730e..4b2baa0403 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1461,6 +1461,107 @@ void parse_config_data(const char *config_source,
> +    if (xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
> +        buf = (d_config->num_pcidevs || d_config->num_dtdevs)
> +            ? "enabled" : "disabled";
> +    }
> +
> +    if (!strncmp(buf, "enabled", strlen(buf))) {

Do you intend to have "passthrough=''" been the equivalent of
"passthrough='enabled'" in the config file ?
(same with "e", "en", "ena", ...)

> +        /* Choose a suitable default */
> +        c_info->passthrough =
> +            (c_info->type == LIBXL_DOMAIN_TYPE_PV) || !iommu_hap_pt_share
> +            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel