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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

Hi Julien,

On Thu, Feb 17, 2022 at 03:20:36PM +0000, Julien Grall wrote:
> 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".

Yes. Thanks.
> 
> > 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/
> 
Shame on me :(. I'll fix that.
> > 
> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> > ---
> >   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?

Ok, I will update the commit message.

> 
> >       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.

Thanks, I will fix that.

> 
> 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.

Do you mean that it's better to set this flag per device, not per
domain? This will require setting this flag for each device which should
require either changing the dtdev format in dom.cfg or setting
xen,force-assign-without-iommu in partial device-tree.

Both of those ways will complicate the configuration. As was mentioned
before, we don't want to make domain configuration more complicated.
What do you think about that?


> 
> >           {
> >               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.

I'll remove this.

> 
> >       if ( !is_iommu_enabled(d) )
> 
> Should not this check be updated to check force_assign?

That's a good point. I'll take a look on it.

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

Because this check will fail if we have CONFIG_HAS_DEVICE_TREE define,
but do not have CONFIG_HAS_PCI and iommu_do_dt_domctl will not be
called.
Same thing if switch/case inside iommu_do_pci_domctl go to default and
return -ENOSYS. This part looked strange for me. But I will definitely
go through this part once again.

Or maybe I've misinterpreted this part? 

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

Also I've posted a task on AT-F Phabricator asking about the feedback
about my SCMI implementation.
Link: https://developer.trustedfirmware.org/T985
Hope I'll be able to start a discussion and get an implementation, which
is approved by AT-F.

Best regards,
Oleksii.