Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 10/10/2019 16:11, Ian Jackson wrote:
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 69971c97b6..fccb6a6271 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -968,6 +957,50 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
>          goto error_out;
>      }
>  
> +    bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs;
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN) {
> +        c_info->passthrough = need_pt
> +            ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED;
> +    }
> +
> +    bool iommu_enabled = physinfo.cap_hvm_directio;
> +    if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) {
> +        LOGD(ERROR, domid,
> +             "ERROR: passthrough not supported on this platform\n");
> +        ret = ERROR_INVAL;
> +        goto error_out;
> +    }
> +
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
> +        LOGD(ERROR, domid,
> +             "passthrough disabled but devices are specified");

This is the only log message which isn't prefixed with ERROR:

> +        ret = ERROR_INVAL;
> +        goto error_out;
> +    }
> +
> +    const char *whynot_pt_share =
> +        c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
> +        !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" :
> +        NULL;

This is a little more complicated.

For ARM, doesn't libxl treat guests as PV, or has that been fixed now? 
ARM's only passthrough mode is PT_SHARE.

On x86 for PVH, passthrough doesn't work yet.  This may not be an
argument against constructing the guest suitably, but we should check
that things don't explode in new and interesting ways from this change.

For x86 HVM, PT_SHARE is only available for HAP guests, so shadow guests
must use PT_SYNC.


> +
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
> +        assert(iommu_enabled);
> +        c_info->passthrough = whynot_pt_share
> +            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
> +    }
> +
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_SHARE_PT && whynot_pt_share) {
> +        LOGD(ERROR, domid,
> +             "ERROR: passthrough=\"share_pt\" %s\n",
> +             whynot_pt_share);
> +        ret = ERROR_INVAL;
> +        goto error_out;
> +    }
> +
> +    /* An explicit setting should now have been chosen */
> +    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
> +    assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);

This is confusing.  I think it would help if ...

> +
>      /* If target_memkb is smaller than max_memkb, the subsequent call
>       * to libxc when building HVM domain will enable PoD mode.
>       */
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 3ac9494b80..2441c0c233 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -266,8 +266,9 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
>  libxl_passthrough = Enumeration("passthrough", [
>      (0, "unknown"),
>      (1, "disabled"),
> -    (2, "sync_pt"),
> -    (3, "share_pt"),
> +    (2, "enabled"),
> +    (3, "sync_pt"),
> +    (4, "share_pt"),

... this had a comment explaining enabled is just interim value.

(2, "enabled"), # becomes {sync,share}_pt once defaults are evaluated

~Andrew

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