| Message ID | 1489608329-7275-10-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { > /* Is this a xenstore domain? */ > #define _XEN_DOMCTL_CDF_xs_domain 5 > #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) > + /* Should IOMMU page tables be populated at the domain creation time? */ > +#define _XEN_DOMCTL_CDF_use_iommu 6 > +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) > uint32_t flags; The need for this to be done via domain creation flag (rather than as a separate, later step) needs to be well explained. Generally what to add here should only be things which can't be done later in a reasonable way. Jan
On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { >> /* Is this a xenstore domain? */ >> #define _XEN_DOMCTL_CDF_xs_domain 5 >> #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) >> + /* Should IOMMU page tables be populated at the domain creation time? */ >> +#define _XEN_DOMCTL_CDF_use_iommu 6 >> +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) >> uint32_t flags; > > The need for this to be done via domain creation flag (rather than > as a separate, later step) needs to be well explained. Generally > what to add here should only be things which can't be done later > in a reasonable way. Well, the non-shared IOMMU should populate its page table by the time the P2M code will have started update mappings. Theoretically, it might happen right after p2m_init has been completed, that is, even during createdomain domctl execution. For example, I see that domain_vgic_init() inserts mapping to P2M table, because of map_mmio_regions() is being called during VGIC initialization. Not sure that GIC mmio ranges must be present in the IOMMU page table, but anyway, it might be the per-domain initialization of other IPs, co-processors that mapping mustn't be skipped because of IOMMU is not ready. So, as both iommu_domain_init() and p2m_init() are called from arch_domain_create(), i.e. during createdomain domctl execution, we have to know exactly should we use IOMMU for this domain to pass proper value to iommu_domain_init(). If don't take into account everything I wrote above, yes, it is possible to introduce new domctl for this purpose that will be called later, but before any operations with guest_physmap.
>>> On 23.03.17 at 17:36, <olekstysh@gmail.com> wrote: > On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>> --- a/xen/include/public/domctl.h >>> +++ b/xen/include/public/domctl.h >>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { >>> /* Is this a xenstore domain? */ >>> #define _XEN_DOMCTL_CDF_xs_domain 5 >>> #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) >>> + /* Should IOMMU page tables be populated at the domain creation time? */ >>> +#define _XEN_DOMCTL_CDF_use_iommu 6 >>> +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) >>> uint32_t flags; >> >> The need for this to be done via domain creation flag (rather than >> as a separate, later step) needs to be well explained. Generally >> what to add here should only be things which can't be done later >> in a reasonable way. > > Well, the non-shared IOMMU should populate its page table by the time > the P2M code will have started update mappings. Theoretically, it > might happen right after p2m_init has been completed, > that is, even during createdomain domctl execution. For example, I see > that domain_vgic_init() inserts mapping to P2M table, because of > map_mmio_regions() is being called during VGIC initialization. > Not sure that GIC mmio ranges must be present in the IOMMU page table, > but anyway, it might be the per-domain initialization of other IPs, > co-processors that mapping mustn't be skipped because of IOMMU is not > ready. > So, as both iommu_domain_init() and p2m_init() are called from > arch_domain_create(), i.e. during createdomain domctl execution, we > have to know exactly should we use IOMMU for this domain to pass > proper value to iommu_domain_init(). Well, no, iommu_domain_init() is not supposed to do any table setup, so it shouldn't need to know. > If don't take into account everything I wrote above, yes, it is > possible to introduce new domctl for this purpose that will be called > later, but before any operations with guest_physmap. Exactly. Any other things needing syncing, but being done during domain creation may then need syncing over. One might question whether some of those things then actually are being done too early (and quite possibly have been done that way just for simplicity). Jan
Hi Jan On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 23.03.17 at 17:36, <olekstysh@gmail.com> wrote: >> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>> --- a/xen/include/public/domctl.h >>>> +++ b/xen/include/public/domctl.h >>>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { >>>> /* Is this a xenstore domain? */ >>>> #define _XEN_DOMCTL_CDF_xs_domain 5 >>>> #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) >>>> + /* Should IOMMU page tables be populated at the domain creation time? */ >>>> +#define _XEN_DOMCTL_CDF_use_iommu 6 >>>> +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) >>>> uint32_t flags; >>> >>> The need for this to be done via domain creation flag (rather than >>> as a separate, later step) needs to be well explained. Generally >>> what to add here should only be things which can't be done later >>> in a reasonable way. >> >> Well, the non-shared IOMMU should populate its page table by the time >> the P2M code will have started update mappings. Theoretically, it >> might happen right after p2m_init has been completed, >> that is, even during createdomain domctl execution. For example, I see >> that domain_vgic_init() inserts mapping to P2M table, because of >> map_mmio_regions() is being called during VGIC initialization. >> Not sure that GIC mmio ranges must be present in the IOMMU page table, >> but anyway, it might be the per-domain initialization of other IPs, >> co-processors that mapping mustn't be skipped because of IOMMU is not >> ready. >> So, as both iommu_domain_init() and p2m_init() are called from >> arch_domain_create(), i.e. during createdomain domctl execution, we >> have to know exactly should we use IOMMU for this domain to pass >> proper value to iommu_domain_init(). > > Well, no, iommu_domain_init() is not supposed to do any table > setup, so it shouldn't need to know. So, does this mean that you disagree to what this patch does as well as the preceding patch [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to iommu_domain_init(). Am I right? As we need this use_iommu flag on ARM only, the possible solution might be to hide it in struct xen_arch_domainconfig for ARM. In such case we would always call iommu_domain_init() with use_iommu forced to false on x86. > >> If don't take into account everything I wrote above, yes, it is >> possible to introduce new domctl for this purpose that will be called >> later, but before any operations with guest_physmap. > > Exactly. Any other things needing syncing, but being done during > domain creation may then need syncing over. One might question > whether some of those things then actually are being done too > early (and quite possibly have been done that way just for simplicity). Sorry, I'm afraid I don't entirely understand you here.
>>> On 24.03.17 at 12:19, <olekstysh@gmail.com> wrote: > Hi Jan > > On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 23.03.17 at 17:36, <olekstysh@gmail.com> wrote: >>> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>>> --- a/xen/include/public/domctl.h >>>>> +++ b/xen/include/public/domctl.h >>>>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { >>>>> /* Is this a xenstore domain? */ >>>>> #define _XEN_DOMCTL_CDF_xs_domain 5 >>>>> #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) >>>>> + /* Should IOMMU page tables be populated at the domain creation time? */ >>>>> +#define _XEN_DOMCTL_CDF_use_iommu 6 >>>>> +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) >>>>> uint32_t flags; >>>> >>>> The need for this to be done via domain creation flag (rather than >>>> as a separate, later step) needs to be well explained. Generally >>>> what to add here should only be things which can't be done later >>>> in a reasonable way. >>> >>> Well, the non-shared IOMMU should populate its page table by the time >>> the P2M code will have started update mappings. Theoretically, it >>> might happen right after p2m_init has been completed, >>> that is, even during createdomain domctl execution. For example, I see >>> that domain_vgic_init() inserts mapping to P2M table, because of >>> map_mmio_regions() is being called during VGIC initialization. >>> Not sure that GIC mmio ranges must be present in the IOMMU page table, >>> but anyway, it might be the per-domain initialization of other IPs, >>> co-processors that mapping mustn't be skipped because of IOMMU is not >>> ready. >>> So, as both iommu_domain_init() and p2m_init() are called from >>> arch_domain_create(), i.e. during createdomain domctl execution, we >>> have to know exactly should we use IOMMU for this domain to pass >>> proper value to iommu_domain_init(). >> >> Well, no, iommu_domain_init() is not supposed to do any table >> setup, so it shouldn't need to know. > > So, does this mean that you disagree to what this patch does as well > as the preceding patch > [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to > iommu_domain_init(). Yes. > As we need this use_iommu flag on ARM only, the possible solution > might be to hide it in struct xen_arch_domainconfig for ARM. > In such case we would always call iommu_domain_init() with use_iommu > forced to false on x86. If that won't involve a domain creation flag addition anymore, I'd be fine and leave this to the ARM maintainers. >>> If don't take into account everything I wrote above, yes, it is >>> possible to introduce new domctl for this purpose that will be called >>> later, but before any operations with guest_physmap. >> >> Exactly. Any other things needing syncing, but being done during >> domain creation may then need syncing over. One might question >> whether some of those things then actually are being done too >> early (and quite possibly have been done that way just for simplicity). > > Sorry, I'm afraid I don't entirely understand you here. You had mentioned a couple of things where you think you need to know ahead of the time whether IOMMU use will actually be needed. In turn I question whether these things can't be done later, i.e. whether they're being done in their current ordering just for convenience of the original code authors. Jan
On Fri, Mar 24, 2017 at 1:38 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 24.03.17 at 12:19, <olekstysh@gmail.com> wrote: >> Hi Jan >> >> On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 23.03.17 at 17:36, <olekstysh@gmail.com> wrote: >>>> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>>>> --- a/xen/include/public/domctl.h >>>>>> +++ b/xen/include/public/domctl.h >>>>>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { >>>>>> /* Is this a xenstore domain? */ >>>>>> #define _XEN_DOMCTL_CDF_xs_domain 5 >>>>>> #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) >>>>>> + /* Should IOMMU page tables be populated at the domain creation time? */ >>>>>> +#define _XEN_DOMCTL_CDF_use_iommu 6 >>>>>> +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) >>>>>> uint32_t flags; >>>>> >>>>> The need for this to be done via domain creation flag (rather than >>>>> as a separate, later step) needs to be well explained. Generally >>>>> what to add here should only be things which can't be done later >>>>> in a reasonable way. >>>> >>>> Well, the non-shared IOMMU should populate its page table by the time >>>> the P2M code will have started update mappings. Theoretically, it >>>> might happen right after p2m_init has been completed, >>>> that is, even during createdomain domctl execution. For example, I see >>>> that domain_vgic_init() inserts mapping to P2M table, because of >>>> map_mmio_regions() is being called during VGIC initialization. >>>> Not sure that GIC mmio ranges must be present in the IOMMU page table, >>>> but anyway, it might be the per-domain initialization of other IPs, >>>> co-processors that mapping mustn't be skipped because of IOMMU is not >>>> ready. >>>> So, as both iommu_domain_init() and p2m_init() are called from >>>> arch_domain_create(), i.e. during createdomain domctl execution, we >>>> have to know exactly should we use IOMMU for this domain to pass >>>> proper value to iommu_domain_init(). >>> >>> Well, no, iommu_domain_init() is not supposed to do any table >>> setup, so it shouldn't need to know. >> >> So, does this mean that you disagree to what this patch does as well >> as the preceding patch >> [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to >> iommu_domain_init(). > > Yes. > >> As we need this use_iommu flag on ARM only, the possible solution >> might be to hide it in struct xen_arch_domainconfig for ARM. >> In such case we would always call iommu_domain_init() with use_iommu >> forced to false on x86. > > If that won't involve a domain creation flag addition anymore, > I'd be fine and leave this to the ARM maintainers. OK. > >>>> If don't take into account everything I wrote above, yes, it is >>>> possible to introduce new domctl for this purpose that will be called >>>> later, but before any operations with guest_physmap. >>> >>> Exactly. Any other things needing syncing, but being done during >>> domain creation may then need syncing over. One might question >>> whether some of those things then actually are being done too >>> early (and quite possibly have been done that way just for simplicity). >> >> Sorry, I'm afraid I don't entirely understand you here. > > You had mentioned a couple of things where you think you need > to know ahead of the time whether IOMMU use will actually be > needed. In turn I question whether these things can't be done > later, i.e. whether they're being done in their current ordering > just for convenience of the original code authors. Now I got it, but I don't have the right answer at the moment.
Hi Oleksandr, Please CC the appropriate maintainers for all the components you modify. On 15/03/17 20:05, 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. > The primary aim of this knowledge is to help the IOMMUs that don't > share page tables with the CPU be ready before P2M code starts > updating IOMMU mapping. > So, if this flag is set the unshared 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*. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > tools/libxl/libxl_create.c | 5 +++++ > xen/arch/arm/domain.c | 4 +++- > xen/arch/x86/domain.c | 4 +++- > xen/common/domctl.c | 5 ++++- > xen/include/public/domctl.h | 3 +++ > xen/include/xen/sched.h | 3 +++ > 6 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index e741b9a..4393fa2 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > flags |= XEN_DOMCTL_CDF_hap; > } > > + /* 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)) > + flags |= XEN_DOMCTL_CDF_use_iommu; Regardless Jan's comment about the flag, I believe we still want to keep the current behavior for x86 (e.g allocating the page table on-demand). So I think this should be per architecture decision rather than a common change. Maybe in, libxl__arch_domain_prepare_config. This also means we would switch to xen_arch_domainconfig. > + > /* Ultimately, handle is an array of 16 uint8_t, same as uuid */ > libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid); > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index bab62ee..940bb98 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > struct xen_arch_domainconfig *config) > { > int rc, count = 0; > + bool_t use_iommu; s/bool_t/bool/ > > BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS); > d->arch.relmem = RELMEM_not_started; > @@ -550,7 +551,8 @@ 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 ) > + use_iommu = !!(domcr_flags & DOMCRF_use_iommu); > + if ( (rc = iommu_domain_init(d, use_iommu)) != 0 ) > goto fail; > > if ( (rc = p2m_init(d)) != 0 ) > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 8ef4160..7d634ff 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > { > bool paging_initialised = false; > int rc = -ENOMEM; > + bool_t use_iommu; Ditto. > > if ( config == NULL && !is_idle_domain(d) ) > return -EINVAL; > @@ -646,7 +647,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > if ( (rc = init_domain_irq_mapping(d)) != 0 ) > goto fail; > > - if ( (rc = iommu_domain_init(d, false)) != 0 ) > + use_iommu = !!(domcr_flags & DOMCRF_use_iommu); > + if ( (rc = iommu_domain_init(d, use_iommu)) != 0 ) > goto fail; > } > spin_lock_init(&d->arch.e820_lock); > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 93e3029..56c4d38 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > | 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_use_iommu)) ) > break; > > dom = op->domain; > @@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > domcr_flags |= DOMCRF_oos_off; > if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain ) > domcr_flags |= DOMCRF_xs_domain; > + if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu ) > + domcr_flags |= DOMCRF_use_iommu; > > d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref, > &op->u.createdomain.config); > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 85cbb7c..a37a566 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { > /* Is this a xenstore domain? */ > #define _XEN_DOMCTL_CDF_xs_domain 5 > #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) > + /* Should IOMMU page tables be populated at the domain creation time? */ > +#define _XEN_DOMCTL_CDF_use_iommu 6 > +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) > uint32_t flags; > struct xen_arch_domainconfig config; > }; > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index 0929c0b..80e6fdc 100644 > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -561,6 +561,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags, > /* DOMCRF_xs_domain: xenstore domain */ > #define _DOMCRF_xs_domain 6 > #define DOMCRF_xs_domain (1U<<_DOMCRF_xs_domain) > + /* DOMCRF_use_iommu: Populate IOMMU page tables at the domain creation time */ > +#define _DOMCRF_use_iommu 7 > +#define DOMCRF_use_iommu (1U<<_DOMCRF_use_iommu) > > /* > * rcu_lock_domain_by_id() is more efficient than get_domain_by_id(). > Cheers,
On Wed, Apr 19, 2017 at 9:26 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi, Julien > > Please CC the appropriate maintainers for all the components you modify. Sorry, sure. > > > On 15/03/17 20:05, 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. >> The primary aim of this knowledge is to help the IOMMUs that don't >> share page tables with the CPU be ready before P2M code starts >> updating IOMMU mapping. >> So, if this flag is set the unshared 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*. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> tools/libxl/libxl_create.c | 5 +++++ >> xen/arch/arm/domain.c | 4 +++- >> xen/arch/x86/domain.c | 4 +++- >> xen/common/domctl.c | 5 ++++- >> xen/include/public/domctl.h | 3 +++ >> xen/include/xen/sched.h | 3 +++ >> 6 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index e741b9a..4393fa2 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc, >> libxl_domain_config *d_config, >> flags |= XEN_DOMCTL_CDF_hap; >> } >> >> + /* 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)) >> + flags |= XEN_DOMCTL_CDF_use_iommu; > > > Regardless Jan's comment about the flag, I believe we still want to keep the > current behavior for x86 (e.g allocating the page table on-demand). > > So I think this should be per architecture decision rather than a common > change. Maybe in, libxl__arch_domain_prepare_config. This also means we > would switch to xen_arch_domainconfig. Agree. Will do. So, the use_iommu flag will be included to xen_arch_domainconfig and passed to iommu_domain_init() on ARM. On x86 we will always pass the "false" value to iommu_domain_init(). Right? > >> + >> /* Ultimately, handle is an array of 16 uint8_t, same as uuid */ >> libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid); >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index bab62ee..940bb98 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int >> domcr_flags, >> struct xen_arch_domainconfig *config) >> { >> int rc, count = 0; >> + bool_t use_iommu; > > > s/bool_t/bool/ ok > >> >> BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS); >> d->arch.relmem = RELMEM_not_started; >> @@ -550,7 +551,8 @@ 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 ) >> + use_iommu = !!(domcr_flags & DOMCRF_use_iommu); >> + if ( (rc = iommu_domain_init(d, use_iommu)) != 0 ) >> goto fail; >> >> if ( (rc = p2m_init(d)) != 0 ) >> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c >> index 8ef4160..7d634ff 100644 >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int >> domcr_flags, >> { >> bool paging_initialised = false; >> int rc = -ENOMEM; >> + bool_t use_iommu; > > > Ditto. ok > > >> >> if ( config == NULL && !is_idle_domain(d) ) >> return -EINVAL; >> @@ -646,7 +647,8 @@ int arch_domain_create(struct domain *d, unsigned int >> domcr_flags, >> if ( (rc = init_domain_irq_mapping(d)) != 0 ) >> goto fail; >> >> - if ( (rc = iommu_domain_init(d, false)) != 0 ) >> + use_iommu = !!(domcr_flags & DOMCRF_use_iommu); >> + if ( (rc = iommu_domain_init(d, use_iommu)) != 0 ) >> goto fail; >> } >> spin_lock_init(&d->arch.e820_lock); >> diff --git a/xen/common/domctl.c b/xen/common/domctl.c >> index 93e3029..56c4d38 100644 >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) >> u_domctl) >> | 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_use_iommu)) ) >> break; >> >> dom = op->domain; >> @@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) >> u_domctl) >> domcr_flags |= DOMCRF_oos_off; >> if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain ) >> domcr_flags |= DOMCRF_xs_domain; >> + if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu ) >> + domcr_flags |= DOMCRF_use_iommu; >> >> d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref, >> &op->u.createdomain.config); >> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h >> index 85cbb7c..a37a566 100644 >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { >> /* Is this a xenstore domain? */ >> #define _XEN_DOMCTL_CDF_xs_domain 5 >> #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) >> + /* Should IOMMU page tables be populated at the domain creation time? */ >> +#define _XEN_DOMCTL_CDF_use_iommu 6 >> +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) >> uint32_t flags; >> struct xen_arch_domainconfig config; >> }; >> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h >> index 0929c0b..80e6fdc 100644 >> --- a/xen/include/xen/sched.h >> +++ b/xen/include/xen/sched.h >> @@ -561,6 +561,9 @@ struct domain *domain_create(domid_t domid, unsigned >> int domcr_flags, >> /* DOMCRF_xs_domain: xenstore domain */ >> #define _DOMCRF_xs_domain 6 >> #define DOMCRF_xs_domain (1U<<_DOMCRF_xs_domain) >> + /* DOMCRF_use_iommu: Populate IOMMU page tables at the domain creation >> time */ >> +#define _DOMCRF_use_iommu 7 >> +#define DOMCRF_use_iommu (1U<<_DOMCRF_use_iommu) >> >> /* >> * rcu_lock_domain_by_id() is more efficient than get_domain_by_id(). >> > > Cheers, > > -- > Julien Grall
On Wed, Apr 19, 2017 at 07:26:44PM +0100, Julien Grall wrote: > Hi Oleksandr, > > Please CC the appropriate maintainers for all the components you modify. > > On 15/03/17 20:05, 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. > > The primary aim of this knowledge is to help the IOMMUs that don't > > share page tables with the CPU be ready before P2M code starts > > updating IOMMU mapping. > > So, if this flag is set the unshared 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*. > > If it is not able to do so since the very beginning, what will happen? Let me explain where I'm coming from: 1. if not populating the iommu page table causes Xen to malfunction (crash?), it is a bug. It should be fixed without involvement from toolstack. 2. if this is an optimasation, can't we not always populate iommu pt hence no new flag is needed? Overall I'm not too convinced a new flag is needed. Wei.
Hi, Wei On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Wed, Apr 19, 2017 at 07:26:44PM +0100, Julien Grall wrote: >> Hi Oleksandr, >> >> Please CC the appropriate maintainers for all the components you modify. >> >> On 15/03/17 20:05, 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. >> > The primary aim of this knowledge is to help the IOMMUs that don't >> > share page tables with the CPU be ready before P2M code starts >> > updating IOMMU mapping. >> > So, if this flag is set the unshared 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*. >> > > > If it is not able to do so since the very beginning, what will happen? IOMMU page faults will occur if passthroughed/protected devices try to do DMA since the corresponding memory mapping won't be present in IOMMU page table. If we don't populate IOMMU page table (with setting need_iommu flag) during domain creation time we will lose memory mapping (at least domain RAM). Unfortunately, we can't restore this memory mapping on ARM unlike x86. I would like to say that it is true for non-shared IOMMUs only. For shared IOMMU (SMMU) we don't have such problem. > > Let me explain where I'm coming from: > > 1. if not populating the iommu page table causes Xen to malfunction > (crash?), it is a bug. It should be fixed without involvement > from toolstack. > 2. if this is an optimasation, can't we not always populate iommu pt > hence no new flag is needed? > > Overall I'm not too convinced a new flag is needed. We don't want to always populate IOMMU page table since it might be just wasting CPU and memory resources if no devices are assigned to domain. That's why we need this hint. > > Wei.
Oleksandr Tyshchenko writes ("Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl"):
> On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > Let me explain where I'm coming from:
> >
> > 1. if not populating the iommu page table causes Xen to malfunction
> > (crash?), it is a bug. It should be fixed without involvement
> > from toolstack.
> > 2. if this is an optimasation, can't we not always populate iommu pt
> > hence no new flag is needed?
> >
> > Overall I'm not too convinced a new flag is needed.
>
> We don't want to always populate IOMMU page table since it might be
> just wasting CPU and memory resources if no devices are assigned to
> domain. That's why we need this hint.
I thought we already had a way to tell libxl that pci passthrough is
expected for this domain. There are other things that want to know
this in advance. I can't seem to find it now at least in the docs,
though.
Ian.
Hi, Ian On Wed, Apr 26, 2017 at 1:05 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: > Oleksandr Tyshchenko writes ("Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl"): >> On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu <wei.liu2@citrix.com> wrote: >> > Let me explain where I'm coming from: >> > >> > 1. if not populating the iommu page table causes Xen to malfunction >> > (crash?), it is a bug. It should be fixed without involvement >> > from toolstack. >> > 2. if this is an optimasation, can't we not always populate iommu pt >> > hence no new flag is needed? >> > >> > Overall I'm not too convinced a new flag is needed. >> >> We don't want to always populate IOMMU page table since it might be >> just wasting CPU and memory resources if no devices are assigned to >> domain. That's why we need this hint. > > I thought we already had a way to tell libxl that pci passthrough is > expected for this domain. There are other things that want to know > this in advance. I can't seem to find it now at least in the docs, > though. Did mean a way different from the guest configuration file? > > Ian.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index e741b9a..4393fa2 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, flags |= XEN_DOMCTL_CDF_hap; } + /* 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)) + flags |= XEN_DOMCTL_CDF_use_iommu; + /* Ultimately, handle is an array of 16 uint8_t, same as uuid */ libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid); diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index bab62ee..940bb98 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, struct xen_arch_domainconfig *config) { int rc, count = 0; + bool_t use_iommu; BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS); d->arch.relmem = RELMEM_not_started; @@ -550,7 +551,8 @@ 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 ) + use_iommu = !!(domcr_flags & DOMCRF_use_iommu); + if ( (rc = iommu_domain_init(d, use_iommu)) != 0 ) goto fail; if ( (rc = p2m_init(d)) != 0 ) diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 8ef4160..7d634ff 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, { bool paging_initialised = false; int rc = -ENOMEM; + bool_t use_iommu; if ( config == NULL && !is_idle_domain(d) ) return -EINVAL; @@ -646,7 +647,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, if ( (rc = init_domain_irq_mapping(d)) != 0 ) goto fail; - if ( (rc = iommu_domain_init(d, false)) != 0 ) + use_iommu = !!(domcr_flags & DOMCRF_use_iommu); + if ( (rc = iommu_domain_init(d, use_iommu)) != 0 ) goto fail; } spin_lock_init(&d->arch.e820_lock); diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 93e3029..56c4d38 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) | 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_use_iommu)) ) break; dom = op->domain; @@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) domcr_flags |= DOMCRF_oos_off; if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain ) domcr_flags |= DOMCRF_xs_domain; + if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu ) + domcr_flags |= DOMCRF_use_iommu; d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref, &op->u.createdomain.config); diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 85cbb7c..a37a566 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -66,6 +66,9 @@ struct xen_domctl_createdomain { /* Is this a xenstore domain? */ #define _XEN_DOMCTL_CDF_xs_domain 5 #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) + /* Should IOMMU page tables be populated at the domain creation time? */ +#define _XEN_DOMCTL_CDF_use_iommu 6 +#define XEN_DOMCTL_CDF_use_iommu (1U<<_XEN_DOMCTL_CDF_use_iommu) uint32_t flags; struct xen_arch_domainconfig config; }; diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 0929c0b..80e6fdc 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -561,6 +561,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags, /* DOMCRF_xs_domain: xenstore domain */ #define _DOMCRF_xs_domain 6 #define DOMCRF_xs_domain (1U<<_DOMCRF_xs_domain) + /* DOMCRF_use_iommu: Populate IOMMU page tables at the domain creation time */ +#define _DOMCRF_use_iommu 7 +#define DOMCRF_use_iommu (1U<<_DOMCRF_use_iommu) /* * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().