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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: 23 August 2019 15:17
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei 
> Liu <wl@xxxxxxx>; Andrew
> Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; 
> Jan Beulich
> <jbeulich@xxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Konrad Rzeszutek 
> Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> Anthony Perard <anthony.perard@xxxxxxxxxx>; Volodymyr Babchuk 
> <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v6 10/10] introduce a 'passthrough' configuration option 
> to xl.cfg...
> 
> 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@xxxxxxxxxx>
> > ---
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >
> > 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.
> 

Yes, I prefer to stick with the incumbent name.

[snip]
> > 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)
> 

Nope. Missing breaks in both cases. Good spot.

> > +        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?
> 

I thought the comment was clear, but maybe not... Consider the 'inclusive' PV 
h/w domain mappings. No sharing (because it's a PV domain) and no sync, because 
all RAM is mapped anyway. Can I phrase it better?

  Paul

> Thanks, Roger.

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