Re: [RFC v2 6/8] tools/arm: Introduce force_assign_without_iommu option to xl.cfg

[Julien Grall <julien@xen.org>]

Hi,

On 08/02/2022 18:00, Oleksii Moisieiev wrote:
> If set, Xen is allowed to assign the devices even if they are not under
> IOMMU.

I think you mean "not protected by an IOMMU".

> Can be confugired from dom.cfg in the following format:

s/confugired/configured/

> force_assign_without_iommu = 1
> 
> This parameter has the same purpose as xen,force-assign-without-iommu
> property in dom0less archtecture.

s/archtecture/architecture/

> 
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> ---
>   docs/man/xl.cfg.5.pod.in              |  9 +++++++++
>   tools/golang/xenlight/helpers.gen.go  |  5 +++++
>   tools/golang/xenlight/types.gen.go    |  1 +
>   tools/libs/light/libxl_arm.c          |  3 +++
>   tools/libs/light/libxl_types.idl      |  1 +
>   tools/xl/xl_parse.c                   |  3 +++
>   xen/common/domain.c                   |  2 +-
>   xen/drivers/passthrough/device_tree.c | 19 +++++++++++++++++--
>   xen/drivers/passthrough/iommu.c       |  5 ++++-
>   xen/include/public/domctl.h           |  5 ++++-
>   xen/include/xen/iommu.h               |  3 +++
>   11 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index b98d161398..ddf82cb3bc 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1614,6 +1614,15 @@ This feature is a B<technology preview>.
>   
>   =back
>   
> +=over 4
> +
> +=item B<force_assign_without_iommu=BOOLEAN>
> +
> +If set, Xen allows to assign a devices even if it is not behind an IOMMU.
> +This renders your platform *unsafe* if the device is DMA-capable.

I agree this is going to be unsafe. But the more important bit here is 
this is not going to work because the guest has no way to translate a 
GFN to an MFN.

Your guest will need to be direct map to make it usable. So I would add 
that this will *not* work with DMA-capable devices.

Also, can you explain in the commit message why you want to allow this 
setup?

>       xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat",
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 093bb4403f..f1f19bf711 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -512,7 +512,7 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>   
>       if ( iommu )
>       {
> -        if ( config->iommu_opts & ~XEN_DOMCTL_IOMMU_no_sharept )
> +        if ( config->iommu_opts >> XEN_DOMCTL_IOMMU_MAX )

XEN_DOMCTL_IOMMU_MAX will be defined as:

(1U << _XEN_DOMCTL_IOMMU_force_iommu)

This means the shift will do the wrong thing. However, AFAICT, this new 
option will only be supported by Arm and likely only for platform device 
for the time being.

That said, I am not convinced this flag should be per-domain in Xen. 
Instead, I think it would be better to pass the flag via the device 
assign domctl.

>           {
>               dprintk(XENLOG_INFO, "Unknown IOMMU options %#x\n",
>                       config->iommu_opts);
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 98f2aa0dad..103608dec1 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -198,6 +198,7 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>   {
>       int ret;
>       struct dt_device_node *dev;
> +    struct domain_iommu *hd = dom_iommu(d);
>   
>       switch ( domctl->cmd )
>       {
> @@ -238,6 +239,16 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>               return -EINVAL;
>   
>           ret = iommu_add_dt_device(dev);
> +
> +        /*
> +         * iommu_add_dt_device returns 1 if iommu is disabled or device don't
> +         * have iommus property
> +         */
> +        if ( (ret == 1) && (hd->force_assign_iommu) ) {
> +            ret = -ENOSYS;
> +            break;
> +        }
> +
>           if ( ret < 0 )
>           {
>               printk(XENLOG_G_ERR "Failed to add %s to the IOMMU\n",
> @@ -275,10 +286,14 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>   
>           ret = iommu_deassign_dt_device(d, dev);
>   
> -        if ( ret )
> -            printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign \"%s\""
> +        if ( ret ) {
> +            if ( hd->force_assign_iommu )
> +                ret = -ENOSYS;
> +            else
> +                printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign \"%s\""
>                      " to dom%u failed (%d)\n",
>                      dt_node_full_name(dev), d->domain_id, ret);
> +        }
>           break;
>   
>       default:
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 6334370109..216a9058c0 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -193,6 +193,8 @@ int iommu_domain_init(struct domain *d, unsigned int opts)
>       hd->node = NUMA_NO_NODE;
>   #endif
>   
> +    hd->force_assign_iommu = opts & XEN_DOMCTL_IOMMU_force_iommu;
> +
>       ret = arch_iommu_domain_init(d);
>       if ( ret )
>           return ret;
> @@ -534,6 +536,7 @@ int iommu_do_domctl(
>   {
>       int ret = -ENODEV;
>   
> +

Spurious change.

>       if ( !is_iommu_enabled(d) )

Should not this check be updated to check force_assign?

>           return -EOPNOTSUPP;
>   
> @@ -542,7 +545,7 @@ int iommu_do_domctl(
>   #endif
>   
>   #ifdef CONFIG_HAS_DEVICE_TREE
> -    if ( ret == -ENODEV )
> +    if ( ret == -ENOSYS )

AFAICT, none of the code (including callee) before ret have been 
modified. So why are you modifying the check here?

>           ret = iommu_do_dt_domctl(domctl, d, u_domctl);
>   #endif
>   
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index b85e6170b0..bf5f8c5b6b 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -81,8 +81,11 @@ struct xen_domctl_createdomain {
>   #define _XEN_DOMCTL_IOMMU_no_sharept  0
>   #define XEN_DOMCTL_IOMMU_no_sharept   (1U << _XEN_DOMCTL_IOMMU_no_sharept)
>   
> +#define _XEN_DOMCTL_IOMMU_force_iommu 1
> +#define XEN_DOMCTL_IOMMU_force_iommu  (1U << _XEN_DOMCTL_IOMMU_force_iommu)
> +
>   /* Max XEN_DOMCTL_IOMMU_* constant.  Used for ABI checking. */
> -#define XEN_DOMCTL_IOMMU_MAX XEN_DOMCTL_IOMMU_no_sharept
> +#define XEN_DOMCTL_IOMMU_MAX XEN_DOMCTL_IOMMU_force_iommu
>   
>       uint32_t iommu_opts;
>   
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 6b2cdffa4a..a9cf2334af 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -330,6 +330,9 @@ struct domain_iommu {
>        * necessarily imply this is true.
>        */
>       bool need_sync;
> +
> +    /* Do not return error if the device without iommu is assigned */
> +    bool force_assign_iommu;
>   };
>   
>   #define dom_iommu(d)              (&(d)->iommu)

Cheers,

-- 
Julien Grall