Re: [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Aug 16, 2019 at 06:19:57PM +0100, Paul Durrant wrote:
> This patch introduces a common domain creation flag to determine whether
> the domain is permitted to make use of the IOMMU. Currently the flag is
> always set (for both dom0 and domU) if the IOMMU is globally enabled
> (i.e. iommu_enabled == 1). sanitise_domain_config() is modified to reject
> the flag if !iommu_enabled.
> 
> A new helper function, is_iommu_enabled(), is added to test the flag and
> iommu_domain_init() will return immediately if !is_iommu_enabled(). This is
> slightly different to the previous behaviour based on !iommu_enabled where
> the call to arch_iommu_domain_init() was made regardless, however it appears
> that this call was only necessary to initialize the dt_devices list for ARM
> such that iommu_release_dt_devices() can be called unconditionally by
> domain_relinquish_resources(). Adding a simple check of is_iommu_enabled()
> into iommu_release_dt_devices() keeps this unconditional call working.
> 
> No functional change should be observed with this patch applied.
> 
> Subsequent patches will allow the toolstack to control whether use of the
> IOMMU is enabled for a domain.
> 
> NOTE: The introduction of the is_iommu_enabled() helper function might
>       seem excessive but its use is expected to increase with subsequent
>       patches. Also, having iommu_domain_init() bail before calling
>       arch_iommu_domain_init() is not strictly necessary, but I think the
>       consequent addition of the call to is_iommu_enabled() in
>       iommu_release_dt_devices() makes the code clearer.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

I have one ARM-related question and one 'nice to have', but the code
LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 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: 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 toolstack parts as there's no nice method of testing whether
>    the IOMMU is enabled in an architecture-neutral way
> 
> v5:
>  - Move is_iommu_enabled() check into iommu_domain_init()
>  - Reject XEN_DOMCTL_CDF_iommu in sanitise_domain_config() if !iommu_enabled
>  - Use evaluate_nospec() in defintion of is_iommu_enabled()
> ---
>  xen/arch/arm/setup.c                  | 3 +++
>  xen/arch/x86/setup.c                  | 3 +++
>  xen/common/domain.c                   | 9 ++++++++-
>  xen/common/domctl.c                   | 8 ++++++++
>  xen/drivers/passthrough/device_tree.c | 3 +++
>  xen/drivers/passthrough/iommu.c       | 6 +++---
>  xen/include/public/domctl.h           | 4 ++++
>  xen/include/xen/sched.h               | 5 +++++
>  8 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2c5d1372c0..20021ee0ca 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -914,6 +914,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>      dom0_cfg.arch.tee_type = tee_get_type();
>      dom0_cfg.max_vcpus = dom0_max_vcpus();
>  
> +    if ( iommu_enabled )
> +        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> +
>      dom0 = domain_create(0, &dom0_cfg, true);
>      if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>          panic("Error creating domain 0\n");
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index d0b35b0ce2..fa226a2bab 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1733,6 +1733,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      }
>      dom0_cfg.max_vcpus = dom0_max_vcpus();
>  
> +    if ( iommu_enabled )
> +        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> +
>      /* Create initial domain 0. */
>      dom0 = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
>      if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 76e6976617..e832a5c4aa 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -301,7 +301,8 @@ static int sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>                             XEN_DOMCTL_CDF_hap |
>                             XEN_DOMCTL_CDF_s3_integrity |
>                             XEN_DOMCTL_CDF_oos_off |
> -                           XEN_DOMCTL_CDF_xs_domain) )
> +                           XEN_DOMCTL_CDF_xs_domain |
> +                           XEN_DOMCTL_CDF_iommu) )
>      {
>          dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
>          return -EINVAL;
> @@ -328,6 +329,12 @@ static int sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          config->flags |= XEN_DOMCTL_CDF_oos_off;
>      }
>  
> +    if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled )
> +    {
> +        dprintk(XENLOG_INFO, "IOMMU is not enabled\n");
> +        return -EINVAL;
> +    }
> +
>      return arch_sanitise_domain_config(config);
>  }
>  
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 6e6e9b9866..fddf20f1b5 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -515,6 +515,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>              rover = dom;
>          }
>  
> +        /*
> +         * For now, make sure the createdomain IOMMU flag is set if the
> +         * IOMMU is enabled. When the flag comes under toolstack control
> +         * this can go away.
> +         */
> +        if ( iommu_enabled )
> +            op->u.createdomain.flags |= XEN_DOMCTL_CDF_iommu;

Can you add some kind of safety check here to make sure this bodge is
removed when the toolstack takes control of the flag, ie:

BUG_ON(op->u.createdomain.flags & XEN_DOMCTL_CDF_iommu);

Or maybe an ASSERT_UNREACHABLE() followed by returning EINVAL?

> +
>          d = domain_create(dom, &op->u.createdomain, false);
>          if ( IS_ERR(d) )
>          {
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index b6eaae7283..d32b172664 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -119,6 +119,9 @@ int iommu_release_dt_devices(struct domain *d)
>      struct dt_device_node *dev, *_dev;
>      int rc;
>  
> +    if ( !is_iommu_enabled(d) )
> +        return 0;

How could you get here? If the domain doesn't have an iommu how did it
got the devices assigned in the first place?

The hardware domain on ARM would be an exception here, since it uses
an identity second stage translation, but I don't think
iommu_release_dt_devices is ever used against the hardware domain.

Thanks, Roger.

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