> -----Original Message-----
> From: Roger Pau Monne <[email protected]>
> Sent: 23 August 2019 15:17
> To: Paul Durrant <[email protected]>
> Cc: [email protected]; Ian Jackson <[email protected]>; Wei
> Liu <[email protected]>; Andrew
> Cooper <[email protected]>; George Dunlap <[email protected]>;
> Jan Beulich
> <[email protected]>; Julien Grall <[email protected]>; Konrad Rzeszutek
> Wilk
> <[email protected]>; Stefano Stabellini <[email protected]>; Tim
> (Xen.org) <[email protected]>;
> Anthony Perard <[email protected]>; Volodymyr Babchuk
> <[email protected]>
> 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 <[email protected]>
> > ---
> > Cc: Ian Jackson <[email protected]>
> > Cc: Wei Liu <[email protected]>
> > Cc: Andrew Cooper <[email protected]>
> > Cc: George Dunlap <[email protected]>
> > Cc: Jan Beulich <[email protected]>
> > Cc: Julien Grall <[email protected]>
> > Cc: Konrad Rzeszutek Wilk <[email protected]>
> > Cc: Stefano Stabellini <[email protected]>
> > Cc: Tim Deegan <[email protected]>
> > Cc: Anthony PERARD <[email protected]>
> > Cc: Volodymyr Babchuk <[email protected]>
> > Cc: "Roger Pau Monné" <[email protected]>
> >
> > 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
