| Message ID | 1494424994-26232-10-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Oleksandr, On 10/05/17 15:03, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > This flag is intended to let Xen know that the guest has devices > which will most likely be used for passthrough and as the result > the use of IOMMU is expected for this domain. > The primary aim of this knowledge is to help the IOMMUs that don't > share page tables with the CPU on ARM be ready before P2M code starts > updating IOMMU mapping. > So, if this flag is set the non-shared IOMMUs will populate > their page tables at the domain creation time and thereby will be able > to handle IOMMU mapping updates from *the very beginning*. > > In order to retain the current behavior for x86 still call > iommu_domain_init() with use_iommu flag being forced to false. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Julien Grall <julien.grall@arm.com> > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > > --- > Changes in V1: > - Treat use_iommu flag as the ARM decision only. Don't use > common domain creation flag for it, use ARM config instead. > - Clarify patch subject/description. > --- > tools/libxl/libxl_arm.c | 10 ++++++++++ > xen/arch/arm/domain.c | 2 +- > xen/include/public/arch-arm.h | 5 +++++ > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index d842d88..9c4705e 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > return ERROR_FAIL; > } > > + /* TODO Are these assumptions enough to make decision about using IOMMU? */ > + if ((d_config->num_dtdevs && d_config->dtdevs) || > + (d_config->num_pcidevs && d_config->pcidevs)) Checking num_dtdevs and num_pcidevs is enough. It would be a bug if dtdevs and pcidevs are not null. > + xc_config->use_iommu = 1; > + else > + xc_config->use_iommu = 0; > + > + LOG(DEBUG, "The use of IOMMU %s expected for this domain", > + xc_config->use_iommu ? "is" : "isn't"); > + > return 0; > } > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index ec19310..81c4b90 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > ASSERT(config != NULL); > > /* p2m_init relies on some value initialized by the IOMMU subsystem */ > - if ( (rc = iommu_domain_init(d, false)) != 0 ) > + if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != 0 ) !!config->use_iommu is enough. > goto fail; > > if ( (rc = p2m_init(d)) != 0 ) > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index bd974fb..cb33f75 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -322,6 +322,11 @@ struct xen_arch_domainconfig { > * > */ > uint32_t clock_frequency; > + /* > + * IN > + * Inform the hypervisor that the use of IOMMU is expected for this domain. I would simplify to : "IOMMU is expected to be used for this domain". > + */ > + uint8_t use_iommu; > }; > #endif /* __XEN__ || __XEN_TOOLS__ */ > > Cheers,
On Thu, May 11, 2017 at 2:42 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi Julien > > > On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> This flag is intended to let Xen know that the guest has devices >> which will most likely be used for passthrough and as the result >> the use of IOMMU is expected for this domain. >> The primary aim of this knowledge is to help the IOMMUs that don't >> share page tables with the CPU on ARM be ready before P2M code starts >> updating IOMMU mapping. >> So, if this flag is set the non-shared IOMMUs will populate >> their page tables at the domain creation time and thereby will be able >> to handle IOMMU mapping updates from *the very beginning*. >> >> In order to retain the current behavior for x86 still call >> iommu_domain_init() with use_iommu flag being forced to false. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Julien Grall <julien.grall@arm.com> >> CC: Ian Jackson <ian.jackson@eu.citrix.com> >> CC: Wei Liu <wei.liu2@citrix.com> >> >> --- >> Changes in V1: >> - Treat use_iommu flag as the ARM decision only. Don't use >> common domain creation flag for it, use ARM config instead. >> - Clarify patch subject/description. >> --- >> tools/libxl/libxl_arm.c | 10 ++++++++++ >> xen/arch/arm/domain.c | 2 +- >> xen/include/public/arch-arm.h | 5 +++++ >> 3 files changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >> index d842d88..9c4705e 100644 >> --- a/tools/libxl/libxl_arm.c >> +++ b/tools/libxl/libxl_arm.c >> @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >> return ERROR_FAIL; >> } >> >> + /* TODO Are these assumptions enough to make decision about using >> IOMMU? */ >> + if ((d_config->num_dtdevs && d_config->dtdevs) || >> + (d_config->num_pcidevs && d_config->pcidevs)) > > > Checking num_dtdevs and num_pcidevs is enough. It would be a bug if dtdevs > and pcidevs are not null. ok > >> + xc_config->use_iommu = 1; >> + else >> + xc_config->use_iommu = 0; >> + >> + LOG(DEBUG, "The use of IOMMU %s expected for this domain", >> + xc_config->use_iommu ? "is" : "isn't"); >> + >> return 0; >> } >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index ec19310..81c4b90 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int >> domcr_flags, >> ASSERT(config != NULL); >> >> /* p2m_init relies on some value initialized by the IOMMU subsystem >> */ >> - if ( (rc = iommu_domain_init(d, false)) != 0 ) >> + if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != >> 0 ) > > > !!config->use_iommu is enough. ok > >> goto fail; >> >> if ( (rc = p2m_init(d)) != 0 ) >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index bd974fb..cb33f75 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -322,6 +322,11 @@ struct xen_arch_domainconfig { >> * >> */ >> uint32_t clock_frequency; >> + /* >> + * IN >> + * Inform the hypervisor that the use of IOMMU is expected for this >> domain. > > > I would simplify to : "IOMMU is expected to be used for this domain". ok > >> + */ >> + uint8_t use_iommu; >> }; >> #endif /* __XEN__ || __XEN_TOOLS__ */ >> >> > > Cheers, > > -- > Julien Grall
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index d842d88..9c4705e 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, return ERROR_FAIL; } + /* TODO Are these assumptions enough to make decision about using IOMMU? */ + if ((d_config->num_dtdevs && d_config->dtdevs) || + (d_config->num_pcidevs && d_config->pcidevs)) + xc_config->use_iommu = 1; + else + xc_config->use_iommu = 0; + + LOG(DEBUG, "The use of IOMMU %s expected for this domain", + xc_config->use_iommu ? "is" : "isn't"); + return 0; } diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index ec19310..81c4b90 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, ASSERT(config != NULL); /* p2m_init relies on some value initialized by the IOMMU subsystem */ - if ( (rc = iommu_domain_init(d, false)) != 0 ) + if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != 0 ) goto fail; if ( (rc = p2m_init(d)) != 0 ) diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index bd974fb..cb33f75 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -322,6 +322,11 @@ struct xen_arch_domainconfig { * */ uint32_t clock_frequency; + /* + * IN + * Inform the hypervisor that the use of IOMMU is expected for this domain. + */ + uint8_t use_iommu; }; #endif /* __XEN__ || __XEN_TOOLS__ */