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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Aug 16, 2019 at 06:20:01PM +0100, Paul Durrant wrote:
> ...and hence the ability to disable IOMMU mappings, and control EPT
> sharing.
> 
> This patch introduces a new 'libxl_passthrough' enumeration into
> libxl_domain_create_info. The value will be set by xl either when it parses
> a new 'passthrough' option in xl.cfg, or implicitly if there is passthrough
> hardware specified for the domain.
> 
> If the value of the passthrough configuration option is 'disabled' then
> the XEN_DOMCTL_CDF_iommu flag will be clear in the xen_domctl_createdomain
> flags, thus allowing the toolstack to control whether the domain gets
> IOMMU mappings or not (where previously they were globally set).
> 
> If the value of the passthrough configuration option is 'sync_pt' then
> a new 'iommu_opts' field in xen_domctl_createdomain will be set with the
> value XEN_DOMCTL_IOMMU_no_sharept. This will override the global default
> set in iommu_hap_pt_share, thus allowing the toolstack to control whether
> EPT sharing is used for the domain.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> 
> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
> 
> v6:
>  - Remove the libxl_physinfo() call since it's usefulness is limited to x86
> 
> v5:
>  - Expand xen_domctl_createdomain flags field and hence bump interface
>    version
>  - Fix spelling mistakes in context line
> ---
>  docs/man/xl.cfg.5.pod.in        | 52 +++++++++++++++++++++++++++++++++
>  tools/libxl/libxl.h             |  5 ++++
>  tools/libxl/libxl_create.c      |  9 ++++++
>  tools/libxl/libxl_types.idl     |  7 +++++
>  tools/xl/xl_parse.c             | 38 ++++++++++++++++++++++++
>  xen/arch/arm/domain.c           | 10 ++++++-
>  xen/arch/x86/domain.c           |  2 +-
>  xen/common/domain.c             |  7 +++++
>  xen/drivers/passthrough/iommu.c | 13 ++++++++-
>  xen/include/public/domctl.h     |  6 +++-
>  xen/include/xen/iommu.h         | 19 ++++++++----
>  11 files changed, 158 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index c99d40307e..fd35685e9e 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -605,6 +605,58 @@ option should only be used with a trusted device tree.
>  Note that the partial device tree should avoid using the phandle 65000
>  which is reserved by the toolstack.
>  
> +=item B<passthrough="STRING">
> +
> +Specify whether IOMMU mappings are enabled for the domain and hence whether
> +it will be enabled for passthrough hardware. Valid values for this option
> +are:
> +
> +=over 4
> +
> +=item B<disabled>
> +
> +IOMMU mappings are disabled for the domain and so hardware may not be
> +passed through.
> +
> +This option is the default if no passthrough hardware is specified
> +in the domain's configuration.
> +
> +=item B<sync_pt>

I would maybe name this exclusive_pt, but historically it's been named
sync_pt.

> +
> +This option means that IOMMU mappings will be synchronized with the
> +domain's P2M table as follows:
> +
> +For a PV domain, all writable pages assigned to the domain are identity
> +mapped by MFN in the IOMMU page table. Thus a device driver running in the
> +domain may program passthrough hardware for DMA using MFN values
> +(i.e. host/machine frame numbers) looked up in its P2M.
> +
> +For an HVM domain, all non-foreign RAM pages present in its P2M will be
> +mapped by GFN in the IOMMU page table. Thus a device driver running in the
> +domain may program passthrough hardware using GFN values (i.e. guest
> +physical frame numbers) without any further translation.
> +
> +This option is the default if the domain is PV and passthrough hardware
> +is specified in the configuration.
> +
> +This option is not available on Arm.
> +
> +=item B<share_pt>
> +
> +This option is unavailable for a PV domain. For an HVM domain, this option
> +means that the IOMMU will be programmed to directly reference the domain's
> +P2M table as its page table. From the point of view of a device driver
> +running in the domain this is functionally equivalent to B<sync_pt> but
> +places less load on the hypervisor and so should generally be selected in
> +preference. However, the availability of this option is hardware specific
> +and thus, if it is specified for a domain running on hardware that does
> +not allow it (e.g. AMD), B<sync_pt> will be used instead.
> +
> +This option is the default if the domain is HVM and passthrough hardware
> +is specified in the configuration.
> +
> +=back
> +
>  =back
>  
>  =head2 Devices
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 9bacfb97f0..5de7c07a41 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -394,6 +394,11 @@
>   */
>  #define LIBXL_HAVE_EXTENDED_VKB 1
>  
> +/*
> + * libxl_domain_create_info has libxl_passthrough enumeration.
> + */
> +#define LIBXL_HAVE_CREATEINFO_PASSTHROUGH 1
> +
>  /*
>   * libxl ABI compatibility
>   *
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 03ce166f4f..f288e13dc1 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -564,6 +564,15 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>                  libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
>          }
>  
> +        LOG(DETAIL, "passthrough: %s",
> +            libxl_passthrough_to_string(info->passthrough));
> +
> +        if (info->passthrough != LIBXL_PASSTHROUGH_DISABLED)
> +            create.flags |= XEN_DOMCTL_CDF_iommu;
> +
> +        if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
> +            create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
> +
>          /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
>          libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
>  
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index b61399ce36..7e37de8646 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
>  #
> @@ -408,6 +414,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
>      ("pool_name",    string),
>      ("run_hotplug_scripts",libxl_defbool),
>      ("driver_domain",libxl_defbool),
> +    ("passthrough",  libxl_passthrough),
>      ], dir=DIR_IN)
>  
>  libxl_domain_restore_params = Struct("domain_restore_params", [
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index e105bda2bb..c904604008 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2326,6 +2326,44 @@ skip_vfb:
>          }
>      }
>  
> +    if (!xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
> +        libxl_passthrough o;
> +
> +        e = libxl_passthrough_from_string(buf, &o);
> +        if (e) {
> +            fprintf(stderr,
> +                    "ERROR: unknown passthrough option '%s'\n",
> +                    buf);
> +            exit(-ERROR_FAIL);
> +        }
> +
> +        switch (o) {
> +        case LIBXL_PASSTHROUGH_DISABLED:
> +            if (d_config->num_pcidevs || d_config->num_dtdevs) {
> +                fprintf(stderr,
> +                        "ERROR: passthrough disabled but devices are specified\n");
> +                exit(-ERROR_FAIL);
> +            }

Don't you need a break here?

> +        case LIBXL_PASSTHROUGH_SHARE_PT:
> +            if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
> +                fprintf(stderr,
> +                        "ERROR: passthrough=\"share_pt\" not valid for PV domain\n");
> +                exit(-ERROR_FAIL);
> +            }

And here likely (or a /* fallthrough */ comment)

> +        default:
> +            break;
> +        }
> +
> +        c_info->passthrough = o;
> +    } else if (d_config->num_pcidevs || d_config->num_dtdevs) {
> +        /*
> +         * Passthrough devices are specified so set an appropriate
> +         * default value.
> +         */
> +        c_info->passthrough = (c_info->type == LIBXL_DOMAIN_TYPE_PV) ?
> +            LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
> +    }
> +
>      if (!xlu_cfg_get_list(config, "usbctrl", &usbctrls, 0, 0)) {
>          d_config->num_usbctrls = 0;
>          d_config->usbctrls = NULL;
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 941bbff4fe..b12de6ff3d 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -614,6 +614,14 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    /* Always share P2M Table between the CPU and the IOMMU */
> +    if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "Unsupported iommu option: XEN_DOMCTL_IOMMU_no_sharept\n");
> +        return -EINVAL;
> +    }
> +
>      /* Fill in the native GIC version, passed back to the toolstack. */
>      if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
>      {
> @@ -674,7 +682,7 @@ int arch_domain_create(struct domain *d,
>      ASSERT(config != NULL);
>  
>      /* p2m_init relies on some value initialized by the IOMMU subsystem */
> -    if ( (rc = iommu_domain_init(d)) != 0 )
> +    if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
>          goto fail;
>  
>      if ( (rc = p2m_init(d)) != 0 )
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index f144d8fe9a..4f7dad49c5 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -604,7 +604,7 @@ int arch_domain_create(struct domain *d,
>      if ( (rc = init_domain_irq_mapping(d)) != 0 )
>          goto fail;
>  
> -    if ( (rc = iommu_domain_init(d)) != 0 )
> +    if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
>          goto fail;
>  
>      psr_domain_init(d);
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index e832a5c4aa..142b08174b 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -308,6 +308,13 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    if ( !(config->flags & XEN_DOMCTL_CDF_iommu) && config->iommu_opts )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "IOMMU options specified but IOMMU not enabled\n");
> +        return -EINVAL;
> +    }
> +
>      if ( config->max_vcpus < 1 )
>      {
>          dprintk(XENLOG_INFO, "No vCPUS\n");
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index b24be5ffa6..a526aa6c09 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -159,7 +159,7 @@ static void __hwdom_init check_hwdom_reqs(struct domain *d)
>      iommu_hwdom_strict = true;
>  }
>  
> -int iommu_domain_init(struct domain *d)
> +int iommu_domain_init(struct domain *d, unsigned int opts)
>  {
>      struct domain_iommu *hd = dom_iommu(d);
>      int ret = 0;
> @@ -176,6 +176,15 @@ int iommu_domain_init(struct domain *d)
>      if ( ret )
>          return ret;
>  
> +    /*
> +     * Use shared page tables for HAP and IOMMU if the global option
> +     * is enabled (from which we can infer the h/w is capable) and
> +     * the domain options do not disallow it. HAP must, of course, also
> +     * be enabled.
> +     */
> +    hd->hap_pt_share = hap_enabled(d) && iommu_hap_pt_share &&
> +        !(opts & XEN_DOMCTL_IOMMU_no_sharept);
> +
>      /*
>       * NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept
>       *     in-sync with their assigned pages because all host RAM will be
> @@ -187,6 +196,8 @@ int iommu_domain_init(struct domain *d)
>      if ( !is_hardware_domain(d) || iommu_hwdom_strict )
>          hd->need_sync = !iommu_use_hap_pt(d);
>  
> +    ASSERT(!(hd->need_sync && hd->hap_pt_share));
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 3f82c78870..922ed50a11 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000011
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000012
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> @@ -70,6 +70,10 @@ struct xen_domctl_createdomain {
>  
>      uint32_t flags;
>  
> +#define _XEN_DOMCTL_IOMMU_no_sharept  0
> +#define XEN_DOMCTL_IOMMU_no_sharept   (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
> +    uint32_t iommu_opts;
> +
>      /*
>       * Various domain limits, which impact the quantity of resources (global
>       * mapping space, xenheap, etc) a guest may consume.
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 5e7ca98170..01025e372b 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -67,7 +67,7 @@ extern unsigned int iommu_dev_iotlb_timeout;
>  int iommu_setup(void);
>  int iommu_hardware_setup(void);
>  
> -int iommu_domain_init(struct domain *d);
> +int iommu_domain_init(struct domain *d, unsigned int opts);
>  void iommu_hwdom_init(struct domain *d);
>  void iommu_domain_destroy(struct domain *d);
>  
> @@ -257,9 +257,17 @@ struct domain_iommu {
>      DECLARE_BITMAP(features, IOMMU_FEAT_count);
>  
>      /*
> -     * Does the guest reqire mappings to be synchonized, to maintain
> -     * the default dfn == pfn map. (See comment on dfn at the top of
> -     * include/xen/mm.h).
> +     * Does the guest share HAP mapping with the IOMMU? This is always
> +     * true for ARM systems and may be true for x86 systems where the
> +     * the hardware is capable.
> +     */
> +    bool hap_pt_share;
> +
> +    /*
> +     * Does the guest require mappings to be synchronized, to maintain
> +     * the default dfn == pfn map? (See comment on dfn at the top of
> +     * include/xen/mm.h). Note that hap_pt_share == false does not
> +     * necessarily imply this is true.
>       */
>      bool need_sync;

I'm lost here, doesn't hap_pt_share = !need_sync?

ie: sync is required because the page-tables are not shared?

Thanks, Roger.

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