| Message ID | 1489608329-7275-3-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: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Extend the IOMMU code with new APIs and platform callbacks. > These new map_pages/unmap_pages API do almost the same thing > as existing map_page/unmap_page ones except the formers can > handle the number of pages. So do new platform callbacks. > > Currently, this patch requires to modify neither > existing IOMMU drivers nor P2M code. > But, the patch might be rewritten to replace existing > single-page stuff with the multi-page one followed by modifications > of all related parts. Yes please. However, unless you strictly need a page count, please instead use an "order" parameter. Doing that has been on my todo list for quite a while. Jan
Hi Jan On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Extend the IOMMU code with new APIs and platform callbacks. >> These new map_pages/unmap_pages API do almost the same thing >> as existing map_page/unmap_page ones except the formers can >> handle the number of pages. So do new platform callbacks. >> >> Currently, this patch requires to modify neither >> existing IOMMU drivers nor P2M code. >> But, the patch might be rewritten to replace existing >> single-page stuff with the multi-page one followed by modifications >> of all related parts. > > Yes please. However, unless you strictly need a page count, please > instead use an "order" parameter. Doing that has been on my todo > list for quite a while. > > Jan > The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well as new map_pages/unmap_pages platform callbacks. So, we just need to replace single-page APIs with multi-page ones, but leave both "new" (map_pages/unmap_pages) and "old" (map_page/unmap_page) platform callbacks. This way doesn't require to modify existing IOMMU drivers right now, just P2M code. Or we need to replace platform callbacks too? Don't mind to use order instead of page count.
>>> On 22.03.17 at 19:01, <olekstysh@gmail.com> wrote: > Hi Jan > > On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> Extend the IOMMU code with new APIs and platform callbacks. >>> These new map_pages/unmap_pages API do almost the same thing >>> as existing map_page/unmap_page ones except the formers can >>> handle the number of pages. So do new platform callbacks. >>> >>> Currently, this patch requires to modify neither >>> existing IOMMU drivers nor P2M code. >>> But, the patch might be rewritten to replace existing >>> single-page stuff with the multi-page one followed by modifications >>> of all related parts. >> >> Yes please. However, unless you strictly need a page count, please >> instead use an "order" parameter. Doing that has been on my todo >> list for quite a while. > > The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well > as new map_pages/unmap_pages platform callbacks. > > So, we just need to replace single-page APIs with multi-page ones, but > leave both "new" (map_pages/unmap_pages) and "old" > (map_page/unmap_page) platform callbacks. > This way doesn't require to modify existing IOMMU drivers right now, > just P2M code. Or we need to replace platform callbacks too? That was the "yes please" part of my answer: I don't see why we would want two API / callback flavors, with one being a strict superset of the other. Jan
On Thu, Mar 23, 2017 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 22.03.17 at 19:01, <olekstysh@gmail.com> wrote: >> Hi Jan >> >> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> Extend the IOMMU code with new APIs and platform callbacks. >>>> These new map_pages/unmap_pages API do almost the same thing >>>> as existing map_page/unmap_page ones except the formers can >>>> handle the number of pages. So do new platform callbacks. >>>> >>>> Currently, this patch requires to modify neither >>>> existing IOMMU drivers nor P2M code. >>>> But, the patch might be rewritten to replace existing >>>> single-page stuff with the multi-page one followed by modifications >>>> of all related parts. >>> >>> Yes please. However, unless you strictly need a page count, please >>> instead use an "order" parameter. Doing that has been on my todo >>> list for quite a while. >> >> The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well >> as new map_pages/unmap_pages platform callbacks. >> >> So, we just need to replace single-page APIs with multi-page ones, but >> leave both "new" (map_pages/unmap_pages) and "old" >> (map_page/unmap_page) platform callbacks. >> This way doesn't require to modify existing IOMMU drivers right now, >> just P2M code. Or we need to replace platform callbacks too? > > That was the "yes please" part of my answer: I don't see why we > would want two API / callback flavors, with one being a strict > superset of the other. Agree. I'll be back if I have questions that need to be clarified. > > Jan >
Hi Oleksandr, On 15/03/17 20:05, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Extend the IOMMU code with new APIs and platform callbacks. > These new map_pages/unmap_pages API do almost the same thing > as existing map_page/unmap_page ones except the formers can > handle the number of pages. So do new platform callbacks. > > Currently, this patch requires to modify neither > existing IOMMU drivers nor P2M code. > But, the patch might be rewritten to replace existing > single-page stuff with the multi-page one followed by modifications > of all related parts. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > xen/drivers/passthrough/iommu.c | 50 ++++++++++++++++++++++++++++++++--------- > xen/include/xen/iommu.h | 16 ++++++++++--- > 2 files changed, 52 insertions(+), 14 deletions(-) > > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c > index 5e81813..115698f 100644 > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -249,22 +249,37 @@ void iommu_domain_destroy(struct domain *d) > arch_iommu_domain_destroy(d); > } > > -int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, > - unsigned int flags) > +int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long mfn, > + unsigned long page_count, unsigned int flags) It would be nice if you can use mfn_t and gfn_t instead of unsigned long for any new functions. They are typesafe and avoid confusion between gfn and mfn. > { > const struct domain_iommu *hd = dom_iommu(d); > - int rc; > + int rc = 0; > + unsigned long i; > > if ( !iommu_enabled || !hd->platform_ops ) > return 0; > > - rc = hd->platform_ops->map_page(d, gfn, mfn, flags); > + if ( hd->platform_ops->map_pages ) > + rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags); > + else > + { > + for ( i = 0; i < page_count; i++ ) > + { > + rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags); > + if ( unlikely(rc) ) > + { > + /* TODO Do we need to unmap if map failed? */ > + break; > + } > + } > + } > + > if ( unlikely(rc) ) > { > if ( !d->is_shutting_down && printk_ratelimit() ) > printk(XENLOG_ERR > - "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d\n", > - d->domain_id, gfn, mfn, rc); > + "d%d: IOMMU mapping gfn %#lx to mfn %#lx page count %lu failed: %d\n", > + d->domain_id, gfn, mfn, page_count, rc); > > if ( !is_hardware_domain(d) ) > domain_crash(d); > @@ -273,21 +288,34 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, > return rc; > } > > -int iommu_unmap_page(struct domain *d, unsigned long gfn) > +int iommu_unmap_pages(struct domain *d, unsigned long gfn, > + unsigned long page_count) > { > const struct domain_iommu *hd = dom_iommu(d); > - int rc; > + int ret, rc = 0; > + unsigned long i; > > if ( !iommu_enabled || !hd->platform_ops ) > return 0; > > - rc = hd->platform_ops->unmap_page(d, gfn); > + if ( hd->platform_ops->unmap_pages ) > + rc = hd->platform_ops->unmap_pages(d, gfn, page_count); > + else > + { > + for ( i = 0; i < page_count; i++ ) > + { > + ret = hd->platform_ops->unmap_page(d, gfn + i); > + if ( likely(!rc) ) > + rc = ret; > + } > + } > + > if ( unlikely(rc) ) > { > if ( !d->is_shutting_down && printk_ratelimit() ) > printk(XENLOG_ERR > - "d%d: IOMMU unmapping gfn %#lx failed: %d\n", > - d->domain_id, gfn, rc); > + "d%d: IOMMU unmapping gfn %#lx page count %lu failed: %d\n", > + d->domain_id, gfn, page_count, rc); > > if ( !is_hardware_domain(d) ) > domain_crash(d); > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 5803e3f..0446ed3 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d); > #define IOMMUF_readable (1u<<_IOMMUF_readable) > #define _IOMMUF_writable 1 > #define IOMMUF_writable (1u<<_IOMMUF_writable) > -int __must_check iommu_map_page(struct domain *d, unsigned long gfn, > - unsigned long mfn, unsigned int flags); > -int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn); > +int __must_check iommu_map_pages(struct domain *d, unsigned long gfn, > + unsigned long mfn, unsigned long page_count, > + unsigned int flags); > +int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn, > + unsigned long page_count); > + > +#define iommu_map_page(d,gfn,mfn,flags) (iommu_map_pages(d,gfn,mfn,1,flags)) > +#define iommu_unmap_page(d,gfn) (iommu_unmap_pages(d,gfn,1)) > > enum iommu_feature > { > @@ -170,7 +175,12 @@ struct iommu_ops { > void (*teardown)(struct domain *d); > int __must_check (*map_page)(struct domain *d, unsigned long gfn, > unsigned long mfn, unsigned int flags); > + int __must_check (*map_pages)(struct domain *d, unsigned long gfn, > + unsigned long mfn, unsigned long page_count, > + unsigned int flags); > int __must_check (*unmap_page)(struct domain *d, unsigned long gfn); > + int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn, > + unsigned long page_count); > void (*free_page_table)(struct page_info *); > #ifdef CONFIG_X86 > void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value); > Cheers,
On Wed, Apr 19, 2017 at 8:31 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi, Julien > > > On 15/03/17 20:05, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Extend the IOMMU code with new APIs and platform callbacks. >> These new map_pages/unmap_pages API do almost the same thing >> as existing map_page/unmap_page ones except the formers can >> handle the number of pages. So do new platform callbacks. >> >> Currently, this patch requires to modify neither >> existing IOMMU drivers nor P2M code. >> But, the patch might be rewritten to replace existing >> single-page stuff with the multi-page one followed by modifications >> of all related parts. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> xen/drivers/passthrough/iommu.c | 50 >> ++++++++++++++++++++++++++++++++--------- >> xen/include/xen/iommu.h | 16 ++++++++++--- >> 2 files changed, 52 insertions(+), 14 deletions(-) >> >> diff --git a/xen/drivers/passthrough/iommu.c >> b/xen/drivers/passthrough/iommu.c >> index 5e81813..115698f 100644 >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -249,22 +249,37 @@ void iommu_domain_destroy(struct domain *d) >> arch_iommu_domain_destroy(d); >> } >> >> -int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long >> mfn, >> - unsigned int flags) >> +int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long >> mfn, >> + unsigned long page_count, unsigned int flags) > > > It would be nice if you can use mfn_t and gfn_t instead of unsigned long for > any new functions. They are typesafe and avoid confusion between gfn and > mfn. ok > > >> { >> const struct domain_iommu *hd = dom_iommu(d); >> - int rc; >> + int rc = 0; >> + unsigned long i; >> >> if ( !iommu_enabled || !hd->platform_ops ) >> return 0; >> >> - rc = hd->platform_ops->map_page(d, gfn, mfn, flags); >> + if ( hd->platform_ops->map_pages ) >> + rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags); >> + else >> + { >> + for ( i = 0; i < page_count; i++ ) >> + { >> + rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags); >> + if ( unlikely(rc) ) >> + { >> + /* TODO Do we need to unmap if map failed? */ >> + break; >> + } >> + } >> + } >> + >> if ( unlikely(rc) ) >> { >> if ( !d->is_shutting_down && printk_ratelimit() ) >> printk(XENLOG_ERR >> - "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: >> %d\n", >> - d->domain_id, gfn, mfn, rc); >> + "d%d: IOMMU mapping gfn %#lx to mfn %#lx page count >> %lu failed: %d\n", >> + d->domain_id, gfn, mfn, page_count, rc); >> >> if ( !is_hardware_domain(d) ) >> domain_crash(d); >> @@ -273,21 +288,34 @@ int iommu_map_page(struct domain *d, unsigned long >> gfn, unsigned long mfn, >> return rc; >> } >> >> -int iommu_unmap_page(struct domain *d, unsigned long gfn) >> +int iommu_unmap_pages(struct domain *d, unsigned long gfn, >> + unsigned long page_count) >> { >> const struct domain_iommu *hd = dom_iommu(d); >> - int rc; >> + int ret, rc = 0; >> + unsigned long i; >> >> if ( !iommu_enabled || !hd->platform_ops ) >> return 0; >> >> - rc = hd->platform_ops->unmap_page(d, gfn); >> + if ( hd->platform_ops->unmap_pages ) >> + rc = hd->platform_ops->unmap_pages(d, gfn, page_count); >> + else >> + { >> + for ( i = 0; i < page_count; i++ ) >> + { >> + ret = hd->platform_ops->unmap_page(d, gfn + i); >> + if ( likely(!rc) ) >> + rc = ret; >> + } >> + } >> + >> if ( unlikely(rc) ) >> { >> if ( !d->is_shutting_down && printk_ratelimit() ) >> printk(XENLOG_ERR >> - "d%d: IOMMU unmapping gfn %#lx failed: %d\n", >> - d->domain_id, gfn, rc); >> + "d%d: IOMMU unmapping gfn %#lx page count %lu failed: >> %d\n", >> + d->domain_id, gfn, page_count, rc); >> >> if ( !is_hardware_domain(d) ) >> domain_crash(d); >> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >> index 5803e3f..0446ed3 100644 >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d); >> #define IOMMUF_readable (1u<<_IOMMUF_readable) >> #define _IOMMUF_writable 1 >> #define IOMMUF_writable (1u<<_IOMMUF_writable) >> -int __must_check iommu_map_page(struct domain *d, unsigned long gfn, >> - unsigned long mfn, unsigned int flags); >> -int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn); >> +int __must_check iommu_map_pages(struct domain *d, unsigned long gfn, >> + unsigned long mfn, unsigned long >> page_count, >> + unsigned int flags); >> +int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn, >> + unsigned long page_count); >> + >> +#define iommu_map_page(d,gfn,mfn,flags) >> (iommu_map_pages(d,gfn,mfn,1,flags)) >> +#define iommu_unmap_page(d,gfn) (iommu_unmap_pages(d,gfn,1)) >> >> enum iommu_feature >> { >> @@ -170,7 +175,12 @@ struct iommu_ops { >> void (*teardown)(struct domain *d); >> int __must_check (*map_page)(struct domain *d, unsigned long gfn, >> unsigned long mfn, unsigned int flags); >> + int __must_check (*map_pages)(struct domain *d, unsigned long gfn, >> + unsigned long mfn, unsigned long >> page_count, >> + unsigned int flags); >> int __must_check (*unmap_page)(struct domain *d, unsigned long gfn); >> + int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn, >> + unsigned long page_count); >> void (*free_page_table)(struct page_info *); >> #ifdef CONFIG_X86 >> void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, >> unsigned int value); >> > > Cheers, > > -- > Julien Grall
Hi, Jan. There are the questions I would like to clarify. On Thu, Mar 23, 2017 at 2:47 PM, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote: > On Thu, Mar 23, 2017 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 22.03.17 at 19:01, <olekstysh@gmail.com> wrote: >>> Hi Jan >>> >>> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote: >>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>>> >>>>> Extend the IOMMU code with new APIs and platform callbacks. >>>>> These new map_pages/unmap_pages API do almost the same thing >>>>> as existing map_page/unmap_page ones except the formers can >>>>> handle the number of pages. So do new platform callbacks. >>>>> >>>>> Currently, this patch requires to modify neither >>>>> existing IOMMU drivers nor P2M code. >>>>> But, the patch might be rewritten to replace existing >>>>> single-page stuff with the multi-page one followed by modifications >>>>> of all related parts. >>>> >>>> Yes please. However, unless you strictly need a page count, please >>>> instead use an "order" parameter. Doing that has been on my todo >>>> list for quite a while. >>> >>> The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well >>> as new map_pages/unmap_pages platform callbacks. >>> >>> So, we just need to replace single-page APIs with multi-page ones, but >>> leave both "new" (map_pages/unmap_pages) and "old" >>> (map_page/unmap_page) platform callbacks. >>> This way doesn't require to modify existing IOMMU drivers right now, >>> just P2M code. Or we need to replace platform callbacks too? >> >> That was the "yes please" part of my answer: I don't see why we >> would want two API / callback flavors, with one being a strict >> superset of the other. > > Agree. I'll be back if I have questions that need to be clarified. Now I am trying to replace single-page stuff with the multi-page one. Currently, with the single-page API, if map fails we always try to unmap already mapped pages. This is an example of generic code I am speaking about: ... for ( i = 0; i < (1 << order); i++ ) { rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); if ( unlikely(rc) ) { while ( i-- ) /* If statement to satisfy __must_check. */ if ( iommu_unmap_page(p2m->domain, gfn + i) ) continue; break; } } ... After modification the generic code will look like: ... rc = iommu_map_pages(d, gfn, mfn_x(mfn), 1 << order, iommu_flags); ... In case of an error we won't know how many pages have been already mapped and as the result we won't be able to unmap them in order to restore the initial state. Therefore I will move the rollback logic to the IOMMU drivers code. So, the IOMMU drivers code will take care of rollback mapping if something goes wrong. Am I right? If all described above make sense, then there are several places I am trying to optimize, but I am not familiar with) 1. xen/arch/x86/x86_64/mm.c:1443: ... if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) ) { for ( i = spfn; i < epfn; i++ ) if ( iommu_map_page(hardware_domain, i, i, IOMMUF_readable|IOMMUF_writable) ) break; if ( i != epfn ) { while (i-- > old_max) // <--- [1] /* If statement to satisfy __must_check. */ if ( iommu_unmap_page(hardware_domain, i) ) continue; goto destroy_m2p; } } ... [1] Shouldn't we unmap already mapped pages only? I mean to use "while (i-- > spfn)" instead. And if the use of "old_max" here has a special meaning, I think, that this place of code won't be optimized by passing the whole memory block (epfn - spfn) to the IOMMU. Just keep it as is (handle one page at time). 2. xen/drivers/passthrough/vtd/x86/vtd.c:143: ... tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K); for ( j = 0; j < tmp; j++ ) { int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j, IOMMUF_readable|IOMMUF_writable); if ( !rc ) rc = ret; } ... Here we don't try to rollback mapping at all. Was the idea to do so? Or was this place simply missed? P.S. As for using "order" parameter instead of page_count. There are *few* places where "order" doesn't fit. I can introduce something like this: #define __iommu_map_pages(d,gfn,mfn,order,flags) (iommu_map_pages(d,gfn,mfn,1U << (order),flags)) > >> >> Jan >> > > > -- > Regards, > > Oleksandr Tyshchenko
>>> On 27.04.17 at 18:56, <olekstysh@gmail.com> wrote: > Now I am trying to replace single-page stuff with the multi-page one. > Currently, with the single-page API, if map fails we always try to unmap > already mapped pages. > > This is an example of generic code I am speaking about: > ... > for ( i = 0; i < (1 << order); i++ ) > { > rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); > if ( unlikely(rc) ) > { > while ( i-- ) > /* If statement to satisfy __must_check. */ > if ( iommu_unmap_page(p2m->domain, gfn + i) ) > continue; > > break; > } > } > ... > > After modification the generic code will look like: > ... > rc = iommu_map_pages(d, gfn, mfn_x(mfn), 1 << order, iommu_flags); > ... > In case of an error we won't know how many pages have been already mapped > and > as the result we won't be able to unmap them in order to restore the > initial state. > Therefore I will move the rollback logic to the IOMMU drivers code. So, the > IOMMU drivers code > will take care of rollback mapping if something goes wrong. Am I right? Yes, it should be iommu_map_pages() (or its descendants) to clean up after itself upon error. > If all described above make sense, then there are several places I am > trying to optimize, but I am not familiar with) > > 1. xen/arch/x86/x86_64/mm.c:1443: > ... > if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) ) > { > for ( i = spfn; i < epfn; i++ ) > if ( iommu_map_page(hardware_domain, i, i, > IOMMUF_readable|IOMMUF_writable) > ) > break; > if ( i != epfn ) > { > while (i-- > old_max) // <--- [1] > /* If statement to satisfy __must_check. */ > if ( iommu_unmap_page(hardware_domain, i) ) > continue; > > goto destroy_m2p; > } > } > ... > > [1] Shouldn't we unmap already mapped pages only? I mean to use "while > (i-- > spfn)" instead. Both should have the same effect, considering what old_max represents, at least as long as there's no MMIO in between. But yes, generally it would be more logical to unmap using spfn. > And if the use of "old_max" here has a special meaning, I think, that this > place of code won't be optimized > by passing the whole memory block (epfn - spfn) to the IOMMU. Just keep it > as is (handle one page at time). Right, that would have been my general advice: If in doubt, keep the code as is rather than trying to optimize it. > 2. xen/drivers/passthrough/vtd/x86/vtd.c:143: > ... > tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K); > for ( j = 0; j < tmp; j++ ) > { > int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j, > IOMMUF_readable|IOMMUF_writable); > > if ( !rc ) > rc = ret; > } > ... > > Here we don't try to rollback mapping at all. Was the idea to do so? Or was > this place simply missed? Did you consider both the context this is in (establishing hwdom mappings) and the code's history? Both would tell you that yes, this is a best effort mapping attempt deliberately continuing despite errors (but reporting the first one). This behavior will need to be retained. Plus I'm sure you've noticed that this effectively is a single page mapping only anyway (due to PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this "clever" code was used. And as a side note - the way you quote code (by line number and without naming the function it's in) makes it somewhat more complicated to answer your questions. In both cases, had I known the function the code is in, I wouldn't have had a need at all to go look up the context. > P.S. As for using "order" parameter instead of page_count. > There are *few* places where "order" doesn't fit. Well, I'd prefer the "few places" to then break up their requests to fit the "order" parameter. Especially for the purpose of possibly using large pages, an order input is quite a bit more sensible. > I can introduce something like this: > > #define __iommu_map_pages(d,gfn,mfn,order,flags) > (iommu_map_pages(d,gfn,mfn,1U << (order),flags)) I'd prefer if you didn't. In no case should this be an identifier starting with an underscore. Jan
Hi, Jan. Thank you for your reply. On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich <JBeulich@suse.com> wrote: > > >>> On 27.04.17 at 18:56, <olekstysh@gmail.com> wrote: > > Now I am trying to replace single-page stuff with the multi-page one. > > Currently, with the single-page API, if map fails we always try to unmap > > already mapped pages. > > > > This is an example of generic code I am speaking about: > > ... > > for ( i = 0; i < (1 << order); i++ ) > > { > > rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); > > if ( unlikely(rc) ) > > { > > while ( i-- ) > > /* If statement to satisfy __must_check. */ > > if ( iommu_unmap_page(p2m->domain, gfn + i) ) > > continue; > > > > break; > > } > > } > > ... > > > > After modification the generic code will look like: > > ... > > rc = iommu_map_pages(d, gfn, mfn_x(mfn), 1 << order, iommu_flags); > > ... > > In case of an error we won't know how many pages have been already mapped > > and > > as the result we won't be able to unmap them in order to restore the > > initial state. > > Therefore I will move the rollback logic to the IOMMU drivers code. So, the > > IOMMU drivers code > > will take care of rollback mapping if something goes wrong. Am I right? > > Yes, it should be iommu_map_pages() (or its descendants) to clean > up after itself upon error. > > > If all described above make sense, then there are several places I am > > trying to optimize, but I am not familiar with) > > > > 1. xen/arch/x86/x86_64/mm.c:1443: > > ... > > if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) ) > > { > > for ( i = spfn; i < epfn; i++ ) > > if ( iommu_map_page(hardware_domain, i, i, > > IOMMUF_readable|IOMMUF_writable) > > ) > > break; > > if ( i != epfn ) > > { > > while (i-- > old_max) // <--- [1] > > /* If statement to satisfy __must_check. */ > > if ( iommu_unmap_page(hardware_domain, i) ) > > continue; > > > > goto destroy_m2p; > > } > > } > > ... > > > > [1] Shouldn't we unmap already mapped pages only? I mean to use "while > > (i-- > spfn)" instead. > > Both should have the same effect, considering what old_max > represents, at least as long as there's no MMIO in between. But > yes, generally it would be more logical to unmap using spfn. > > > And if the use of "old_max" here has a special meaning, I think, that this > > place of code won't be optimized > > by passing the whole memory block (epfn - spfn) to the IOMMU. Just keep it > > as is (handle one page at time). > > Right, that would have been my general advice: If in doubt, keep > the code as is rather than trying to optimize it. OK > > > > 2. xen/drivers/passthrough/vtd/x86/vtd.c:143: > > ... > > tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K); > > for ( j = 0; j < tmp; j++ ) > > { > > int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j, > > IOMMUF_readable|IOMMUF_writable); > > > > if ( !rc ) > > rc = ret; > > } > > ... > > > > Here we don't try to rollback mapping at all. Was the idea to do so? Or was > > this place simply missed? > > Did you consider both the context this is in (establishing hwdom > mappings) and the code's history? Both would tell you that yes, > this is a best effort mapping attempt deliberately continuing > despite errors (but reporting the first one). This behavior will > need to be retained. Plus I'm sure you've noticed that this > effectively is a single page mapping only anyway (due to > PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this > "clever" code was used. So, if I understand what your meant I don't even need to try to optimize this code. Despite the fact that this code would become much more simple: ... rc = iommu_map_pages(d, pfn, pfn, 1, IOMMUF_readable|IOMMUF_writable); ... Right? > > And as a side note - the way you quote code (by line number and > without naming the function it's in) makes it somewhat more > complicated to answer your questions. In both cases, had I known > the function the code is in, I wouldn't have had a need at all to go > look up the context. Sorry for that. Next time I will provide more detailed snippet. > > > P.S. As for using "order" parameter instead of page_count. > > There are *few* places where "order" doesn't fit. > > Well, I'd prefer the "few places" to then break up their requests to > fit the "order" parameter. Especially for the purpose of possibly > using large pages, an order input is quite a bit more sensible. OK > > > I can introduce something like this: > > > > #define __iommu_map_pages(d,gfn,mfn,order,flags) > > (iommu_map_pages(d,gfn,mfn,1U << (order),flags)) > > I'd prefer if you didn't. In no case should this be an identifier > starting with an underscore. I got it. I proposed because I had seen analogy with __map_domain_page/map_domain_page. > > Jan
>>> On 28.04.17 at 12:16, <olekstysh@gmail.com> wrote: > On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 27.04.17 at 18:56, <olekstysh@gmail.com> wrote: >> > 2. xen/drivers/passthrough/vtd/x86/vtd.c:143: >> > ... >> > tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K); >> > for ( j = 0; j < tmp; j++ ) >> > { >> > int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j, >> > IOMMUF_readable|IOMMUF_writable); >> > >> > if ( !rc ) >> > rc = ret; >> > } >> > ... >> > >> > Here we don't try to rollback mapping at all. Was the idea to do so? Or was >> > this place simply missed? >> >> Did you consider both the context this is in (establishing hwdom >> mappings) and the code's history? Both would tell you that yes, >> this is a best effort mapping attempt deliberately continuing >> despite errors (but reporting the first one). This behavior will >> need to be retained. Plus I'm sure you've noticed that this >> effectively is a single page mapping only anyway (due to >> PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this >> "clever" code was used. > So, if I understand what your meant I don't even need to try to > optimize this code. > Despite the fact that this code would become much more simple: > ... > rc = iommu_map_pages(d, pfn, pfn, 1, > IOMMUF_readable|IOMMUF_writable); > ... > Right? Well, the actual simplification is entirely independent of you patch; what you'd add is just the extra order argument (which ought to be zero, btw). I am, however, of the opinion that the simplification would be good to do, but independent of your patch, and only unless the VT-d maintainers actually think there is a reason for this "cleverness". >> > I can introduce something like this: >> > >> > #define __iommu_map_pages(d,gfn,mfn,order,flags) >> > (iommu_map_pages(d,gfn,mfn,1U << (order),flags)) >> >> I'd prefer if you didn't. In no case should this be an identifier >> starting with an underscore. > I got it. I proposed because I had seen analogy with > __map_domain_page/map_domain_page. Well, there are quite a few things in long standing code which we wouldn't allow in new instances of anymore, one of which being the non-standard-conforming use of leading underscores. Eventually old code would be nice to be cleaned up too, but that's tedious and time consuming, and most if not all of us have better uses for their time. So commonly we do such cleanup only when respective code needs touching anyway. Jan
On Fri, Apr 28, 2017 at 1:29 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 28.04.17 at 12:16, <olekstysh@gmail.com> wrote: >> On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich <JBeulich@suse.com> wrote: >>> >>> On 27.04.17 at 18:56, <olekstysh@gmail.com> wrote: >>> > 2. xen/drivers/passthrough/vtd/x86/vtd.c:143: >>> > ... >>> > tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K); >>> > for ( j = 0; j < tmp; j++ ) >>> > { >>> > int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j, >>> > IOMMUF_readable|IOMMUF_writable); >>> > >>> > if ( !rc ) >>> > rc = ret; >>> > } >>> > ... >>> > >>> > Here we don't try to rollback mapping at all. Was the idea to do so? Or was >>> > this place simply missed? >>> >>> Did you consider both the context this is in (establishing hwdom >>> mappings) and the code's history? Both would tell you that yes, >>> this is a best effort mapping attempt deliberately continuing >>> despite errors (but reporting the first one). This behavior will >>> need to be retained. Plus I'm sure you've noticed that this >>> effectively is a single page mapping only anyway (due to >>> PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this >>> "clever" code was used. >> So, if I understand what your meant I don't even need to try to >> optimize this code. >> Despite the fact that this code would become much more simple: >> ... >> rc = iommu_map_pages(d, pfn, pfn, 1, >> IOMMUF_readable|IOMMUF_writable); >> ... >> Right? > > Well, the actual simplification is entirely independent of you patch; > what you'd add is just the extra order argument (which ought to > be zero, btw). I am, however, of the opinion that the simplification > would be good to do, but independent of your patch, and only > unless the VT-d maintainers actually think there is a reason for this > "cleverness". Clear enough. > >>> > I can introduce something like this: >>> > >>> > #define __iommu_map_pages(d,gfn,mfn,order,flags) >>> > (iommu_map_pages(d,gfn,mfn,1U << (order),flags)) >>> >>> I'd prefer if you didn't. In no case should this be an identifier >>> starting with an underscore. >> I got it. I proposed because I had seen analogy with >> __map_domain_page/map_domain_page. > > Well, there are quite a few things in long standing code which > we wouldn't allow in new instances of anymore, one of which > being the non-standard-conforming use of leading underscores. > Eventually old code would be nice to be cleaned up too, but > that's tedious and time consuming, and most if not all of us > have better uses for their time. So commonly we do such > cleanup only when respective code needs touching anyway. Clear enough. > > Jan >
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 5e81813..115698f 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -249,22 +249,37 @@ void iommu_domain_destroy(struct domain *d) arch_iommu_domain_destroy(d); } -int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, - unsigned int flags) +int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long mfn, + unsigned long page_count, unsigned int flags) { const struct domain_iommu *hd = dom_iommu(d); - int rc; + int rc = 0; + unsigned long i; if ( !iommu_enabled || !hd->platform_ops ) return 0; - rc = hd->platform_ops->map_page(d, gfn, mfn, flags); + if ( hd->platform_ops->map_pages ) + rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags); + else + { + for ( i = 0; i < page_count; i++ ) + { + rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags); + if ( unlikely(rc) ) + { + /* TODO Do we need to unmap if map failed? */ + break; + } + } + } + if ( unlikely(rc) ) { if ( !d->is_shutting_down && printk_ratelimit() ) printk(XENLOG_ERR - "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d\n", - d->domain_id, gfn, mfn, rc); + "d%d: IOMMU mapping gfn %#lx to mfn %#lx page count %lu failed: %d\n", + d->domain_id, gfn, mfn, page_count, rc); if ( !is_hardware_domain(d) ) domain_crash(d); @@ -273,21 +288,34 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, return rc; } -int iommu_unmap_page(struct domain *d, unsigned long gfn) +int iommu_unmap_pages(struct domain *d, unsigned long gfn, + unsigned long page_count) { const struct domain_iommu *hd = dom_iommu(d); - int rc; + int ret, rc = 0; + unsigned long i; if ( !iommu_enabled || !hd->platform_ops ) return 0; - rc = hd->platform_ops->unmap_page(d, gfn); + if ( hd->platform_ops->unmap_pages ) + rc = hd->platform_ops->unmap_pages(d, gfn, page_count); + else + { + for ( i = 0; i < page_count; i++ ) + { + ret = hd->platform_ops->unmap_page(d, gfn + i); + if ( likely(!rc) ) + rc = ret; + } + } + if ( unlikely(rc) ) { if ( !d->is_shutting_down && printk_ratelimit() ) printk(XENLOG_ERR - "d%d: IOMMU unmapping gfn %#lx failed: %d\n", - d->domain_id, gfn, rc); + "d%d: IOMMU unmapping gfn %#lx page count %lu failed: %d\n", + d->domain_id, gfn, page_count, rc); if ( !is_hardware_domain(d) ) domain_crash(d); diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 5803e3f..0446ed3 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d); #define IOMMUF_readable (1u<<_IOMMUF_readable) #define _IOMMUF_writable 1 #define IOMMUF_writable (1u<<_IOMMUF_writable) -int __must_check iommu_map_page(struct domain *d, unsigned long gfn, - unsigned long mfn, unsigned int flags); -int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn); +int __must_check iommu_map_pages(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned long page_count, + unsigned int flags); +int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn, + unsigned long page_count); + +#define iommu_map_page(d,gfn,mfn,flags) (iommu_map_pages(d,gfn,mfn,1,flags)) +#define iommu_unmap_page(d,gfn) (iommu_unmap_pages(d,gfn,1)) enum iommu_feature { @@ -170,7 +175,12 @@ struct iommu_ops { void (*teardown)(struct domain *d); int __must_check (*map_page)(struct domain *d, unsigned long gfn, unsigned long mfn, unsigned int flags); + int __must_check (*map_pages)(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned long page_count, + unsigned int flags); int __must_check (*unmap_page)(struct domain *d, unsigned long gfn); + int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn, + unsigned long page_count); void (*free_page_table)(struct page_info *); #ifdef CONFIG_X86 void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);