| Message ID | d333126d12f2281f8df92e66cfba1c9eb2425dca.1644341635.git.oleksii_moisieiev@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce SCI-mediator feature | expand |
On Tue, Feb 08, 2022 at 06:00:12PM +0000, Oleksii Moisieiev wrote: > If set, Xen is allowed to assign the devices even if they are not under > IOMMU. > Can be confugired from dom.cfg in the following format: > force_assign_without_iommu = 1 > > This parameter has the same purpose as xen,force-assign-without-iommu > property in dom0less archtecture. > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > --- > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index 2a42da2f7d..1080966c33 100644 > --- a/tools/libs/light/libxl_types.idl > +++ b/tools/libs/light/libxl_types.idl > @@ -564,6 +564,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("apic", libxl_defbool), > ("dm_restrict", libxl_defbool), > ("tee", libxl_tee_type), > + ("force_assign_without_iommu", libxl_defbool), As you are making changes to libxl's API, could you add a LIBXL_HAVE_* macro in "tools/include/libxl.h", they are plenty of example there about adding new fields in "libxl_domain_build_info". Thanks,
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,
Hi Anthony, On Thu, Feb 17, 2022 at 02:52:44PM +0000, Anthony PERARD wrote: > On Tue, Feb 08, 2022 at 06:00:12PM +0000, Oleksii Moisieiev wrote: > > If set, Xen is allowed to assign the devices even if they are not under > > IOMMU. > > Can be confugired from dom.cfg in the following format: > > force_assign_without_iommu = 1 > > > > This parameter has the same purpose as xen,force-assign-without-iommu > > property in dom0less archtecture. > > > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> > > --- > > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > > index 2a42da2f7d..1080966c33 100644 > > --- a/tools/libs/light/libxl_types.idl > > +++ b/tools/libs/light/libxl_types.idl > > @@ -564,6 +564,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > > ("apic", libxl_defbool), > > ("dm_restrict", libxl_defbool), > > ("tee", libxl_tee_type), > > + ("force_assign_without_iommu", libxl_defbool), > > As you are making changes to libxl's API, could you add a LIBXL_HAVE_* > macro in "tools/include/libxl.h", they are plenty of example there about > adding new fields in "libxl_domain_build_info". Sure, I will add it in v3. -- Oleksii.
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@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? 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.
On 18/02/2022 09:16, Oleksii Moisieiev wrote: > Hi Julien, Hi Oleksii, > On Thu, Feb 17, 2022 at 03:20:36PM +0000, Julien Grall wrote: >>> 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? We have two interfaces here: 1) User -> tools 2) tools -> Xen We can chose different policy for each interface. For the tools -> Xen interface, I think this should be per device (similar to XEN_DOMCTL_DEV_RDM_RELAXED). For the User -> tools, I am open to discussion. One advantage with per device is the user explicitely vet each device. So it is harder to passthrough a device wrongly. But I agree this also complicates the interface. What do other thinks? >> >>> 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. Below the implementation of iommu_do_domctl() on staging: int iommu_do_domctl( struct xen_domctl *domctl, struct domain *d, XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) { int ret = -ENODEV; if ( !is_iommu_enabled(d) ) return -EOPNOTSUPP; #ifdef CONFIG_HAS_PCI ret = iommu_do_pci_domctl(domctl, d, u_domctl); #endif #ifdef CONFIG_HAS_DEVICE_TREE if ( ret == -ENODEV ) ret = iommu_do_dt_domctl(domctl, d, u_domctl); #endif return ret; } 'ret' is initialized to -ENODEV. So for !CONFIG_HAS_PCI, then ret will not be changed. Therefore the current check is correct. AFAICT, your patch is setting 'ret' so I don't expect any change here. > 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. We use the same sub-op to assign/deassign a PCI and "DT" device. So we are not interested in -ENOSYS but -ENODEV that would be returned by the checks: if ( domct->u.assign_device.dev != XEN_DOMCTL_DEV_PCI ) At the moment, there are no sub-op specific to "DT" device. So it is not necessary for us to check -ENOSYS yet. I haven't looked at the rest of the series to see if we need it. But if we do, then I think the check should be extended in the patch that requires it. Cheers,
Hi Julien, On Fri, Feb 18, 2022 at 10:17:33AM +0000, Julien Grall wrote: > > > On 18/02/2022 09:16, Oleksii Moisieiev wrote: > > Hi Julien, > > Hi Oleksii, > > > On Thu, Feb 17, 2022 at 03:20:36PM +0000, Julien Grall wrote: > > > > 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? > > We have two interfaces here: > 1) User -> tools > 2) tools -> Xen > > We can chose different policy for each interface. > > For the tools -> Xen interface, I think this should be per device (similar > to XEN_DOMCTL_DEV_RDM_RELAXED). > > For the User -> tools, I am open to discussion. One advantage with per > device is the user explicitely vet each device. So it is harder to > passthrough a device wrongly. > > But I agree this also complicates the interface. What do other thinks? > I see the following ways of User -> tools format: a) Set force_assign_without_iommu = 1 in dom.cfg b) Update dtdev format add force_iommu parameter, so dtdev will look like this: dtdev = [ "/soc/dma-controller@e6700000", "/soc/gpio@e6055000,force_iommu", ... ] c)... Tools -> Xen possible ways: d) Set force_assign_without_iommu to domain globally e) Pass force_assign_without_iommu via device-assign domctl. a) + d) is what we have in the patch series. I think a) + e) can work for now so we will have an interface to make force_assign_without_iommu per device in future. What do you think about 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. > > Below the implementation of iommu_do_domctl() on staging: > > int iommu_do_domctl( > struct xen_domctl *domctl, struct domain *d, > XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > { > int ret = -ENODEV; > > if ( !is_iommu_enabled(d) ) > return -EOPNOTSUPP; > > #ifdef CONFIG_HAS_PCI > ret = iommu_do_pci_domctl(domctl, d, u_domctl); > #endif > > #ifdef CONFIG_HAS_DEVICE_TREE > if ( ret == -ENODEV ) > ret = iommu_do_dt_domctl(domctl, d, u_domctl); > #endif > > return ret; > } > > 'ret' is initialized to -ENODEV. So for !CONFIG_HAS_PCI, then ret will not > be changed. Therefore the current check is correct. > > AFAICT, your patch is setting 'ret' so I don't expect any change here. > > > 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. > We use the same sub-op to assign/deassign a PCI and "DT" device. So we are > not interested in -ENOSYS but -ENODEV that would be returned by the checks: > > if ( domct->u.assign_device.dev != XEN_DOMCTL_DEV_PCI ) > > At the moment, there are no sub-op specific to "DT" device. So it is not > necessary for us to check -ENOSYS yet. > > I haven't looked at the rest of the series to see if we need it. But if we > do, then I think the check should be extended in the patch that requires it. > Thank you for the comment. I will refactor this code. Also I wanted to share with you some thoughts about using SMC client_id field to pass agent_id to the SCMI. Posted question regarding this approach to trustedfirmware phabricator [0]. I've found that ATF already has multiagent approach implemented for stm32mp1 platform, see plat/st/stm32mp1/include/stm32mp1_smc.h [1]. It uses 2 funcids hardcoded for AGENT0 and AGENT1: STM32_SIP_SMC_SCMI_AGENT0 0x82002000 STM32_SIP_SMC_SCMI_AGENT1 0x82002001 I think this approach will be very promising for SCI mediator. Firmware defines a range of func_ids, let's say from 0x82000010 to 0x82000020, where 0x82000010 is the base func_id for trusted agent. This func_id is set in arm,scmi-smc node in Xen device-tree. During startup Xen requests agent configuration and calculate func_id for each channel the following way: <Base Func_ID> + <channel_id> Calculated func_id should be assigned to the Domain by setting it as arm,scmi-id in arm,scmi-smc node. So for the Domain Xen will generate the following nodes: scmi { compatible = "arm,scmi-smc"; arm,smc-id = <calculated func_id>; ... shmem = <&shmem_node> }; shmem_node { compatible = "arm,scmi-shmem"; ... }; In this case each domain will get unique func_id to send SCMI commands. I see the following advantages of this approach: 1) There is no need for Xen to intercept SMC requests. All requests from agents will go directly to the Firmware, which can calculate agent_id from func_id. This mean that there is no need for scmi_handle_call function. 2) This approach already implemented for stm32mp1 board so it's more likely to be accepted. Another thing I want to discuss is how Xen should handle scmi related nodes from xen device-tree. Currently Xen device-tree includes arm,scmi-smc node and a list of scmi-shmem nodes for the channels: scmi { compatible = "arm,scmi-smc"; ... }; sram@0x53ff0000 { compatible = "mmio-sram"; ... cpu_scp_shm: scp-shmem@0x0 { compatible = "arm,scmi-shmem"; ... }; scp-shmem@0x1000 { ... }; ... scp-shmem@0xF000 { ... }; }; We do not want all of this nodes to be present in Dom0. I suggest to set xen,passthrough for all this nodes to ensure that Dom0 will not get information about other channels and generate nodes arm,scmi-shmem and arm,scmi-smc for Dom0. I think this approach will be more secure. What do you think about both suggested approaches? [0] https://developer.trustedfirmware.org/T985 [1] https://review.trustedfirmware.org/TF-A/trusted-firmware-a
On 08.02.2022 19:00, Oleksii Moisieiev wrote: > --- 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 ) > { > dprintk(XENLOG_INFO, "Unknown IOMMU options %#x\n", > config->iommu_opts); While in common code this is perhaps okay, the new bit wants rejecting (or also implementing) for x86. > @@ -534,6 +536,7 @@ int iommu_do_domctl( > { > int ret = -ENODEV; > > + > if ( !is_iommu_enabled(d) ) > return -EOPNOTSUPP; Please don't. > @@ -542,7 +545,7 @@ int iommu_do_domctl( > #endif > > #ifdef CONFIG_HAS_DEVICE_TREE > - if ( ret == -ENODEV ) > + if ( ret == -ENOSYS ) > ret = iommu_do_dt_domctl(domctl, d, u_domctl); > #endif Why? Jan
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. + +=back + =back =head2 Paravirtualised (PV) Guest Specific Options diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index b746ff1081..664933bbb8 100644 --- a/tools/golang/xenlight/helpers.gen.go +++ b/tools/golang/xenlight/helpers.gen.go @@ -1091,6 +1091,11 @@ if err := x.DmRestrict.fromC(&xc.dm_restrict);err != nil { return fmt.Errorf("converting field DmRestrict: %v", err) } x.Tee = TeeType(xc.tee) + +if err := x.ForceAssignWithoutIommu.fromC(&xc.force_assign_without_iommu);err != nil { +return fmt.Errorf("converting field ForceAssignWithoutIommu: %v", err) +} + x.Type = DomainType(xc._type) switch x.Type{ case DomainTypeHvm: diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go index b1e84d5258..2f7a088c3b 100644 --- a/tools/golang/xenlight/types.gen.go +++ b/tools/golang/xenlight/types.gen.go @@ -512,6 +512,7 @@ NestedHvm Defbool Apic Defbool DmRestrict Defbool Tee TeeType +ForceAssignWithoutIommu Defbool Type DomainType TypeUnion DomainBuildInfoTypeUnion ArchArm struct { diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index eef1de0939..c5090e2b32 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, return ERROR_FAIL; } + if (libxl_defbool_val(d_config->b_info.force_assign_without_iommu)) + config->iommu_opts |= XEN_DOMCTL_IOMMU_force_iommu; + return 0; } diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index 2a42da2f7d..1080966c33 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -564,6 +564,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("apic", libxl_defbool), ("dm_restrict", libxl_defbool), ("tee", libxl_tee_type), + ("force_assign_without_iommu", libxl_defbool), ("u", KeyedUnion(None, libxl_domain_type, "type", [("hvm", Struct(None, [("firmware", string), ("bios", libxl_bios_type), diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 117fcdcb2b..67fa96d949 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -2747,6 +2747,9 @@ skip_usbdev: } } + xlu_cfg_get_defbool(config, "force_assign_without_iommu", + &b_info->force_assign_without_iommu, 0); + parse_vkb_list(config, d_config); 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 ) { 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; + if ( !is_iommu_enabled(d) ) return -EOPNOTSUPP; @@ -542,7 +545,7 @@ int iommu_do_domctl( #endif #ifdef CONFIG_HAS_DEVICE_TREE - if ( ret == -ENODEV ) + if ( ret == -ENOSYS ) 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)
If set, Xen is allowed to assign the devices even if they are not under IOMMU. Can be confugired from dom.cfg in the following format: force_assign_without_iommu = 1 This parameter has the same purpose as xen,force-assign-without-iommu property in dom0less archtecture. 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(-)