| Message ID | 20190814133852.20491-6-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | use stashed domain create flags... | expand |
Hi Paul, On 14/08/2019 14:38, 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@citrix.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: Anthony PERARD <anthony.perard@citrix.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Tim Deegan <tim@xen.org> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > > Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html > > 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() > --- > tools/libxl/libxl_create.c | 8 ++++++++ > xen/arch/arm/domain.c | 1 - > xen/arch/arm/setup.c | 3 +++ > xen/arch/x86/setup.c | 3 +++ > xen/common/domain.c | 9 ++++++++- > 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 +++++ > 9 files changed, 37 insertions(+), 5 deletions(-) > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 03ce166f4f..050ef042cd 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -555,6 +555,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > .max_grant_frames = b_info->max_grant_frames, > .max_maptrack_frames = b_info->max_maptrack_frames, > }; > + libxl_physinfo physinfo; > > if (info->type != LIBXL_DOMAIN_TYPE_PV) { > create.flags |= XEN_DOMCTL_CDF_hvm_guest; > @@ -564,6 +565,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off; > } > > + rc = libxl_get_physinfo(ctx, &physinfo); > + if (rc < 0) > + goto out; > + > + if (physinfo.cap_hvm_directio) > + create.flags |= XEN_DOMCTL_CDF_iommu; This is not going to work well on Arm as XEN_SYSCTL_PHYSCAP_directio is never set. > + > /* Ultimately, handle is an array of 16 uint8_t, same as uuid */ > libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid); > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 941bbff4fe..3ff19bc1ca 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -673,7 +673,6 @@ int arch_domain_create(struct domain *d, > > ASSERT(config != NULL); > > - /* p2m_init relies on some value initialized by the IOMMU subsystem */ Even with this patch, I still think iommu_domain_init() is required before calling p2m_init(). For instance, this function will set the features flag. > if ( (rc = iommu_domain_init(d)) != 0 ) > goto fail; Cheers,
> -----Original Message----- > From: Julien Grall <julien.grall@arm.com> > Sent: 14 August 2019 15:00 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wl@xen.org>; Anthony Perard > <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com> > Subject: Re: [PATCH v5 05/10] domain: introduce XEN_DOMCTL_CDF_iommu flag > > Hi Paul, > > On 14/08/2019 14:38, 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@citrix.com> > > --- > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Wei Liu <wl@xen.org> > > Cc: Anthony PERARD <anthony.perard@citrix.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Julien Grall <julien.grall@arm.com> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: Tim Deegan <tim@xen.org> > > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > > > > Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html > > > > 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() > > --- > > tools/libxl/libxl_create.c | 8 ++++++++ > > xen/arch/arm/domain.c | 1 - > > xen/arch/arm/setup.c | 3 +++ > > xen/arch/x86/setup.c | 3 +++ > > xen/common/domain.c | 9 ++++++++- > > 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 +++++ > > 9 files changed, 37 insertions(+), 5 deletions(-) > > > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > > index 03ce166f4f..050ef042cd 100644 > > --- a/tools/libxl/libxl_create.c > > +++ b/tools/libxl/libxl_create.c > > @@ -555,6 +555,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > > .max_grant_frames = b_info->max_grant_frames, > > .max_maptrack_frames = b_info->max_maptrack_frames, > > }; > > + libxl_physinfo physinfo; > > > > if (info->type != LIBXL_DOMAIN_TYPE_PV) { > > create.flags |= XEN_DOMCTL_CDF_hvm_guest; > > @@ -564,6 +565,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > > libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off; > > } > > > > + rc = libxl_get_physinfo(ctx, &physinfo); > > + if (rc < 0) > > + goto out; > > + > > + if (physinfo.cap_hvm_directio) > > + create.flags |= XEN_DOMCTL_CDF_iommu; > > This is not going to work well on Arm as XEN_SYSCTL_PHYSCAP_directio is never set. Oh, that's true. I need to pull that into the common sysctl handler. It also looks like XEN_SYSCTL_PHYSCAP_hvm should always be set for too, but ARMs arch_do_physinfo() is completely empty at the moment. > > > + > > /* Ultimately, handle is an array of 16 uint8_t, same as uuid */ > > libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid); > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > > index 941bbff4fe..3ff19bc1ca 100644 > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -673,7 +673,6 @@ int arch_domain_create(struct domain *d, > > > > ASSERT(config != NULL); > > > > - /* p2m_init relies on some value initialized by the IOMMU subsystem */ > > Even with this patch, I still think iommu_domain_init() is required before > calling p2m_init(). For instance, this function will set the features flag. Oh, yes, I see that's in the SMMU code. I'll leave the comment in place... I thought it was only there because of arch_iommu_domain_init() being called unconditionally before (regardless of iommu_enabled). Paul > > > if ( (rc = iommu_domain_init(d)) != 0 ) > > goto fail; > > Cheers, > > -- > Julien Grall
Hi Paul, On 14/08/2019 15:26, Paul Durrant wrote: >> -----Original Message----- >> From: Julien Grall <julien.grall@arm.com> >> Sent: 14 August 2019 15:00 >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org >> Cc: Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wl@xen.org>; Anthony Perard >> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap >> <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad Rzeszutek Wilk >> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com> >> Subject: Re: [PATCH v5 05/10] domain: introduce XEN_DOMCTL_CDF_iommu flag >> >> Hi Paul, >> >> On 14/08/2019 14:38, 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@citrix.com> >>> --- >>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>> Cc: Wei Liu <wl@xen.org> >>> Cc: Anthony PERARD <anthony.perard@citrix.com> >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>> Cc: George Dunlap <George.Dunlap@eu.citrix.com> >>> Cc: Jan Beulich <jbeulich@suse.com> >>> Cc: Julien Grall <julien.grall@arm.com> >>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Tim Deegan <tim@xen.org> >>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>> Cc: "Roger Pau Monné" <roger.pau@citrix.com> >>> >>> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html >>> >>> 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() >>> --- >>> tools/libxl/libxl_create.c | 8 ++++++++ >>> xen/arch/arm/domain.c | 1 - >>> xen/arch/arm/setup.c | 3 +++ >>> xen/arch/x86/setup.c | 3 +++ >>> xen/common/domain.c | 9 ++++++++- >>> 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 +++++ >>> 9 files changed, 37 insertions(+), 5 deletions(-) >>> >>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >>> index 03ce166f4f..050ef042cd 100644 >>> --- a/tools/libxl/libxl_create.c >>> +++ b/tools/libxl/libxl_create.c >>> @@ -555,6 +555,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, >>> .max_grant_frames = b_info->max_grant_frames, >>> .max_maptrack_frames = b_info->max_maptrack_frames, >>> }; >>> + libxl_physinfo physinfo; >>> >>> if (info->type != LIBXL_DOMAIN_TYPE_PV) { >>> create.flags |= XEN_DOMCTL_CDF_hvm_guest; >>> @@ -564,6 +565,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, >>> libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off; >>> } >>> >>> + rc = libxl_get_physinfo(ctx, &physinfo); >>> + if (rc < 0) >>> + goto out; >>> + >>> + if (physinfo.cap_hvm_directio) >>> + create.flags |= XEN_DOMCTL_CDF_iommu; >> >> This is not going to work well on Arm as XEN_SYSCTL_PHYSCAP_directio is never set. > > Oh, that's true. I need to pull that into the common sysctl handler. It also looks like XEN_SYSCTL_PHYSCAP_hvm should always be set for too, but ARMs arch_do_physinfo() is completely empty at the moment. Looking at the header (include/public/sysctl.h) it is pretty clear that the two PHYSCAP are x86 specific. Even if Arm guests are based on the same concept as HVM/PVH guest. They are just called "Arm guest". We have always taken action so far to say they are HVM/PVH guest when presenting to the user. For instance, we strongly recommend to have the guest type in your configuration. So I am not entirely sure we want to expose the capability for Arm. Otherwise a user may think it is fine to use "hvm" guest type. Cheers,
> -----Original Message----- > From: Julien Grall <julien.grall@arm.com> > Sent: 14 August 2019 18:15 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wl@xen.org>; Anthony Perard > <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com> > Subject: Re: [PATCH v5 05/10] domain: introduce XEN_DOMCTL_CDF_iommu flag > > Hi Paul, > > On 14/08/2019 15:26, Paul Durrant wrote: > >> -----Original Message----- > >> From: Julien Grall <julien.grall@arm.com> > >> Sent: 14 August 2019 15:00 > >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > >> Cc: Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wl@xen.org>; Anthony Perard > >> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap > >> <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad Rzeszutek Wilk > >> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; > >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com> > >> Subject: Re: [PATCH v5 05/10] domain: introduce XEN_DOMCTL_CDF_iommu flag > >> > >> Hi Paul, > >> > >> On 14/08/2019 14:38, 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@citrix.com> > >>> --- > >>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >>> Cc: Wei Liu <wl@xen.org> > >>> Cc: Anthony PERARD <anthony.perard@citrix.com> > >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >>> Cc: George Dunlap <George.Dunlap@eu.citrix.com> > >>> Cc: Jan Beulich <jbeulich@suse.com> > >>> Cc: Julien Grall <julien.grall@arm.com> > >>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >>> Cc: Stefano Stabellini <sstabellini@kernel.org> > >>> Cc: Tim Deegan <tim@xen.org> > >>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > >>> Cc: "Roger Pau Monné" <roger.pau@citrix.com> > >>> > >>> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019- > 07/msg02267.html > >>> > >>> 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() > >>> --- > >>> tools/libxl/libxl_create.c | 8 ++++++++ > >>> xen/arch/arm/domain.c | 1 - > >>> xen/arch/arm/setup.c | 3 +++ > >>> xen/arch/x86/setup.c | 3 +++ > >>> xen/common/domain.c | 9 ++++++++- > >>> 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 +++++ > >>> 9 files changed, 37 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > >>> index 03ce166f4f..050ef042cd 100644 > >>> --- a/tools/libxl/libxl_create.c > >>> +++ b/tools/libxl/libxl_create.c > >>> @@ -555,6 +555,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > >>> .max_grant_frames = b_info->max_grant_frames, > >>> .max_maptrack_frames = b_info->max_maptrack_frames, > >>> }; > >>> + libxl_physinfo physinfo; > >>> > >>> if (info->type != LIBXL_DOMAIN_TYPE_PV) { > >>> create.flags |= XEN_DOMCTL_CDF_hvm_guest; > >>> @@ -564,6 +565,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > >>> libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off; > >>> } > >>> > >>> + rc = libxl_get_physinfo(ctx, &physinfo); > >>> + if (rc < 0) > >>> + goto out; > >>> + > >>> + if (physinfo.cap_hvm_directio) > >>> + create.flags |= XEN_DOMCTL_CDF_iommu; > >> > >> This is not going to work well on Arm as XEN_SYSCTL_PHYSCAP_directio is never set. > > > > Oh, that's true. I need to pull that into the common sysctl handler. It also looks like > XEN_SYSCTL_PHYSCAP_hvm should always be set for too, but ARMs arch_do_physinfo() is completely empty > at the moment. > > Looking at the header (include/public/sysctl.h) it is pretty clear that the two > PHYSCAP are x86 specific. > > Even if Arm guests are based on the same concept as HVM/PVH guest. They are just > called "Arm guest". We have always taken action so far to say they are HVM/PVH > guest when presenting to the user. For instance, we strongly recommend to have > the guest type in your configuration. > > So I am not entirely sure we want to expose the capability for Arm. Otherwise a > user may think it is fine to use "hvm" guest type. Ok, yes I see the directio flag is listed as x86 too... I guess I'll have to forget this mechanism and hardcode the flag to the global iommu_enabled flag inside the hypervisor. Paul > > Cheers, > > -- > Julien Grall
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 03ce166f4f..050ef042cd 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -555,6 +555,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, .max_grant_frames = b_info->max_grant_frames, .max_maptrack_frames = b_info->max_maptrack_frames, }; + libxl_physinfo physinfo; if (info->type != LIBXL_DOMAIN_TYPE_PV) { create.flags |= XEN_DOMCTL_CDF_hvm_guest; @@ -564,6 +565,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off; } + rc = libxl_get_physinfo(ctx, &physinfo); + if (rc < 0) + goto out; + + if (physinfo.cap_hvm_directio) + create.flags |= XEN_DOMCTL_CDF_iommu; + /* Ultimately, handle is an array of 16 uint8_t, same as uuid */ libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid); diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 941bbff4fe..3ff19bc1ca 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -673,7 +673,6 @@ 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 ) goto fail; diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 215746a5c3..fca1e62901 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/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; + list_for_each_entry_safe(dev, _dev, &hd->dt_devices, domain_list) { rc = iommu_deassign_dt_device(d, dev); diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 37eb0f7d01..e61d3d1368 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -151,13 +151,13 @@ int iommu_domain_init(struct domain *d) struct domain_iommu *hd = dom_iommu(d); int ret = 0; + if ( !is_iommu_enabled(d) ) + return 0; + ret = arch_iommu_domain_init(d); if ( ret ) return ret; - if ( !iommu_enabled ) - return 0; - hd->platform_ops = iommu_get_ops(); return hd->platform_ops->init(d); } diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 19486d5e32..3f82c78870 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -64,6 +64,10 @@ struct xen_domctl_createdomain { /* Is this a xenstore domain? */ #define _XEN_DOMCTL_CDF_xs_domain 4 #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) + /* Should this domain be permitted to use the IOMMU? */ +#define _XEN_DOMCTL_CDF_iommu 5 +#define XEN_DOMCTL_CDF_iommu (1U<<_XEN_DOMCTL_CDF_iommu) + uint32_t flags; /* diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index a62161cc54..07ed66913a 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -981,6 +981,11 @@ static inline bool is_xenstore_domain(const struct domain *d) return d->options & XEN_DOMCTL_CDF_xs_domain; } +static inline bool is_iommu_enabled(const struct domain *d) +{ + return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu); +} + extern bool sched_smt_power_savings; extern enum cpufreq_controller {
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@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wl@xen.org> Cc: Anthony PERARD <anthony.perard@citrix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> Cc: "Roger Pau Monné" <roger.pau@citrix.com> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html 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() --- tools/libxl/libxl_create.c | 8 ++++++++ xen/arch/arm/domain.c | 1 - xen/arch/arm/setup.c | 3 +++ xen/arch/x86/setup.c | 3 +++ xen/common/domain.c | 9 ++++++++- 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 +++++ 9 files changed, 37 insertions(+), 5 deletions(-)