| Message ID | 1474991845-27962-10-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > The current code used by Intel VTd will only map RMRR regions for the > hardware domain, but will fail to map RMRR regions for unprivileged domains > unless the page tables are shared between EPT and IOMMU. Okay, if that's the case it surely should get fixed. > Fix this and > simplify the code, removing the {set/clear}_identity_p2m_entry helpers and > just using the normal MMIO mapping functions. This simplification, however, goes too far. Namely ... > -int set_identity_p2m_entry(struct domain *d, unsigned long gfn, > - p2m_access_t p2ma, unsigned int flag) > -{ > - p2m_type_t p2mt; > - p2m_access_t a; > - mfn_t mfn; > - struct p2m_domain *p2m = p2m_get_hostp2m(d); > - int ret; > - > - if ( !paging_mode_translate(p2m->domain) ) > - { > - if ( !need_iommu(d) ) > - return 0; > - return iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable); > - } > - > - gfn_lock(p2m, gfn, 0); > - > - mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL); > - > - if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm ) > - ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K, > - p2m_mmio_direct, p2ma); > - else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma ) > - { > - ret = 0; > - /* > - * PVH fixme: during Dom0 PVH construction, p2m entries are being set > - * but iomem regions are not mapped with IOMMU. This makes sure that > - * RMRRs are correctly mapped with IOMMU. > - */ > - if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) ) > - ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable); > - } > - else > - { > - if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED ) > - ret = 0; > - else > - ret = -EBUSY; > - printk(XENLOG_G_WARNING > - "Cannot setup identity map d%d:%lx," > - " gfn already mapped to %lx.\n", > - d->domain_id, gfn, mfn_x(mfn)); ... this logic (and its clear side counterpart) should not be removed without replacement. Note in this context how you render "flag" an unused parameter of rmrr_identity_mapping(). > --- a/xen/include/xen/p2m-common.h > +++ b/xen/include/xen/p2m-common.h > @@ -2,6 +2,7 @@ > #define _XEN_P2M_COMMON_H > > #include <public/vm_event.h> > +#include <xen/softirq.h> > > /* > * Additional access types, which are used to further restrict > @@ -46,6 +47,35 @@ int unmap_mmio_regions(struct domain *d, > mfn_t mfn); > > /* > + * Preemptive Helper for mapping/unmapping MMIO regions. > + */ Single line comment. > +static inline int modify_mmio_11(struct domain *d, unsigned long pfn, > + unsigned long nr_pages, bool map) Why do you make this an inline function? And I have to admit that I dislike this strange use of number 11 - what's wrong with continuing to use the term "direct map" in one way or another? > +{ > + int rc; > + > + while ( nr_pages > 0 ) > + { > + rc = (map ? map_mmio_regions : unmap_mmio_regions) > + (d, _gfn(pfn), nr_pages, _mfn(pfn)); > + if ( rc == 0 ) > + break; > + if ( rc < 0 ) > + { > + printk(XENLOG_ERR > + "Failed to %smap %#lx - %#lx into domain %d memory map: %d\n", "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n" And I think XENLOG_WARNING would do - whether this actually is a problem depends on further factors. > + map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); > + return rc; > + } > + nr_pages -= rc; > + pfn += rc; > + process_pending_softirqs(); Is this what you call "preemptive"? > + } > + > + return rc; The way this is coded it appears to possibly return non-zero even in success case. I think this would therefore better be a for ( ; ; ) loop. Jan
On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote: > >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > > The current code used by Intel VTd will only map RMRR regions for the > > hardware domain, but will fail to map RMRR regions for unprivileged domains > > unless the page tables are shared between EPT and IOMMU. > > Okay, if that's the case it surely should get fixed. > > > Fix this and > > simplify the code, removing the {set/clear}_identity_p2m_entry helpers and > > just using the normal MMIO mapping functions. > > This simplification, however, goes too far. Namely ... > > > -int set_identity_p2m_entry(struct domain *d, unsigned long gfn, > > - p2m_access_t p2ma, unsigned int flag) > > -{ > > - p2m_type_t p2mt; > > - p2m_access_t a; > > - mfn_t mfn; > > - struct p2m_domain *p2m = p2m_get_hostp2m(d); > > - int ret; > > - > > - if ( !paging_mode_translate(p2m->domain) ) > > - { > > - if ( !need_iommu(d) ) > > - return 0; > > - return iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable); > > - } > > - > > - gfn_lock(p2m, gfn, 0); > > - > > - mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL); > > - > > - if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm ) > > - ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K, > > - p2m_mmio_direct, p2ma); > > - else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma ) > > - { > > - ret = 0; > > - /* > > - * PVH fixme: during Dom0 PVH construction, p2m entries are being set > > - * but iomem regions are not mapped with IOMMU. This makes sure that > > - * RMRRs are correctly mapped with IOMMU. > > - */ > > - if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) ) > > - ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable); > > - } > > - else > > - { > > - if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED ) > > - ret = 0; > > - else > > - ret = -EBUSY; > > - printk(XENLOG_G_WARNING > > - "Cannot setup identity map d%d:%lx," > > - " gfn already mapped to %lx.\n", > > - d->domain_id, gfn, mfn_x(mfn)); > > ... this logic (and its clear side counterpart) should not be removed > without replacement. Note in this context how you render "flag" an > unused parameter of rmrr_identity_mapping(). OK, so I'm just going to fix {set/clear}_identity_p2m_entry, because leaving the current logic while using something like modify_mmio_11 (or whatever we end up calling it) it's too complex IMHO. > > --- a/xen/include/xen/p2m-common.h > > +++ b/xen/include/xen/p2m-common.h > > @@ -2,6 +2,7 @@ > > #define _XEN_P2M_COMMON_H > > > > #include <public/vm_event.h> > > +#include <xen/softirq.h> > > > > /* > > * Additional access types, which are used to further restrict > > @@ -46,6 +47,35 @@ int unmap_mmio_regions(struct domain *d, > > mfn_t mfn); > > > > /* > > + * Preemptive Helper for mapping/unmapping MMIO regions. > > + */ > > Single line comment. > > > +static inline int modify_mmio_11(struct domain *d, unsigned long pfn, > > + unsigned long nr_pages, bool map) > > Why do you make this an inline function? And I have to admit that I > dislike this strange use of number 11 - what's wrong with continuing > to use the term "direct map" in one way or another? I've renamed it to modify_mmio_direct and moved it to common/memory.c, since none of the files in passthrough/ seemed like a good place to put it. > > +{ > > + int rc; > > + > > + while ( nr_pages > 0 ) > > + { > > + rc = (map ? map_mmio_regions : unmap_mmio_regions) > > + (d, _gfn(pfn), nr_pages, _mfn(pfn)); > > + if ( rc == 0 ) > > + break; > > + if ( rc < 0 ) > > + { > > + printk(XENLOG_ERR > > + "Failed to %smap %#lx - %#lx into domain %d memory map: %d\n", > > "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n" > > And I think XENLOG_WARNING would do - whether this actually is > a problem depends on further factors. Done. > > + map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); > > + return rc; > > + } > > + nr_pages -= rc; > > + pfn += rc; > > + process_pending_softirqs(); > > Is this what you call "preemptive"? Right, I've removed preemptive from the comment since it makes no sense. > > + } > > + > > + return rc; > > The way this is coded it appears to possibly return non-zero even in > success case. I think this would therefore better be a for ( ; ; ) loop. I don't think this is possible, {un}map_mmio_regions will return < 0 on error, > 0 if there are pending pages to map, and 0 when all the requested pages have been mapped successfully. Thanks, Roger.
>>> On 30.09.16 at 13:27, <roger.pau@citrix.com> wrote: > On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote: >> >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: >> > +{ >> > + int rc; >> > + >> > + while ( nr_pages > 0 ) >> > + { >> > + rc = (map ? map_mmio_regions : unmap_mmio_regions) >> > + (d, _gfn(pfn), nr_pages, _mfn(pfn)); >> > + if ( rc == 0 ) >> > + break; >> > + if ( rc < 0 ) >> > + { >> > + printk(XENLOG_ERR >> > + "Failed to %smap %#lx - %#lx into domain %d memory map: %d\n", >> > + map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); >> > + return rc; >> > + } >> > + nr_pages -= rc; >> > + pfn += rc; >> > + process_pending_softirqs(); >> > + } >> > + >> > + return rc; >> >> The way this is coded it appears to possibly return non-zero even in >> success case. I think this would therefore better be a for ( ; ; ) loop. > > I don't think this is possible, {un}map_mmio_regions will return < 0 on > error, > 0 if there are pending pages to map, and 0 when all the requested > pages have been mapped successfully. Right - hence the "appears to" in my reply; it took me a while to figure it's not actually possible, and hence my desire to make this more obvious to the reader. Jan
On Fri, Sep 30, 2016 at 07:21:41AM -0600, Jan Beulich wrote: > >>> On 30.09.16 at 13:27, <roger.pau@citrix.com> wrote: > > On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote: > >> >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > >> > +{ > >> > + int rc; > >> > + > >> > + while ( nr_pages > 0 ) > >> > + { > >> > + rc = (map ? map_mmio_regions : unmap_mmio_regions) > >> > + (d, _gfn(pfn), nr_pages, _mfn(pfn)); > >> > + if ( rc == 0 ) > >> > + break; > >> > + if ( rc < 0 ) > >> > + { > >> > + printk(XENLOG_ERR > >> > + "Failed to %smap %#lx - %#lx into domain %d memory map: %d\n", > >> > + map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); > >> > + return rc; > >> > + } > >> > + nr_pages -= rc; > >> > + pfn += rc; > >> > + process_pending_softirqs(); > >> > + } > >> > + > >> > + return rc; > >> > >> The way this is coded it appears to possibly return non-zero even in > >> success case. I think this would therefore better be a for ( ; ; ) loop. > > > > I don't think this is possible, {un}map_mmio_regions will return < 0 on > > error, > 0 if there are pending pages to map, and 0 when all the requested > > pages have been mapped successfully. > > Right - hence the "appears to" in my reply; it took me a while to > figure it's not actually possible, and hence my desire to make this > more obvious to the reader. Ah, OK, I misunderstood you then. What about changing the last return rc to return 0? This would make it more obvious, because I'm not really sure a for loop would change much (IMHO the problem is the return semantics used by {un}map_mmio_regions). Roger.
>>> On 30.09.16 at 17:02, <roger.pau@citrix.com> wrote: > On Fri, Sep 30, 2016 at 07:21:41AM -0600, Jan Beulich wrote: >> >>> On 30.09.16 at 13:27, <roger.pau@citrix.com> wrote: >> > On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote: >> >> >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: >> >> > +{ >> >> > + int rc; >> >> > + >> >> > + while ( nr_pages > 0 ) >> >> > + { >> >> > + rc = (map ? map_mmio_regions : unmap_mmio_regions) >> >> > + (d, _gfn(pfn), nr_pages, _mfn(pfn)); >> >> > + if ( rc == 0 ) >> >> > + break; >> >> > + if ( rc < 0 ) >> >> > + { >> >> > + printk(XENLOG_ERR >> >> > + "Failed to %smap %#lx - %#lx into domain %d memory map: > %d\n", >> >> > + map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); >> >> > + return rc; >> >> > + } >> >> > + nr_pages -= rc; >> >> > + pfn += rc; >> >> > + process_pending_softirqs(); >> >> > + } >> >> > + >> >> > + return rc; >> >> >> >> The way this is coded it appears to possibly return non-zero even in >> >> success case. I think this would therefore better be a for ( ; ; ) loop. >> > >> > I don't think this is possible, {un}map_mmio_regions will return < 0 on >> > error, > 0 if there are pending pages to map, and 0 when all the requested >> > pages have been mapped successfully. >> >> Right - hence the "appears to" in my reply; it took me a while to >> figure it's not actually possible, and hence my desire to make this >> more obvious to the reader. > > Ah, OK, I misunderstood you then. What about changing the last return rc > to return 0? This would make it more obvious, because I'm not really sure a > for loop would change much (IMHO the problem is the return semantics used by > {un}map_mmio_regions). Well, my suggestion was not to use "a for loop" but specifically "for ( ; ; )" to clarify the only loop exit condition is the single break statement. That break statement could of course also become a "return 0". I'd rather not see the return at the end of the function become "return 0"; if anything (i.e. if you follow my suggestion) it could be deleted altogether. Jan
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 9526fff..44492ae 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1029,56 +1029,6 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, return set_typed_p2m_entry(d, gfn, mfn, order, p2m_mmio_direct, access); } -int set_identity_p2m_entry(struct domain *d, unsigned long gfn, - p2m_access_t p2ma, unsigned int flag) -{ - p2m_type_t p2mt; - p2m_access_t a; - mfn_t mfn; - struct p2m_domain *p2m = p2m_get_hostp2m(d); - int ret; - - if ( !paging_mode_translate(p2m->domain) ) - { - if ( !need_iommu(d) ) - return 0; - return iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable); - } - - gfn_lock(p2m, gfn, 0); - - mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL); - - if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm ) - ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K, - p2m_mmio_direct, p2ma); - else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma ) - { - ret = 0; - /* - * PVH fixme: during Dom0 PVH construction, p2m entries are being set - * but iomem regions are not mapped with IOMMU. This makes sure that - * RMRRs are correctly mapped with IOMMU. - */ - if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) ) - ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable); - } - else - { - if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED ) - ret = 0; - else - ret = -EBUSY; - printk(XENLOG_G_WARNING - "Cannot setup identity map d%d:%lx," - " gfn already mapped to %lx.\n", - d->domain_id, gfn, mfn_x(mfn)); - } - - gfn_unlock(p2m, gfn, 0); - return ret; -} - /* * Returns: * 0 for success @@ -1127,42 +1077,6 @@ int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, return rc; } -int clear_identity_p2m_entry(struct domain *d, unsigned long gfn) -{ - p2m_type_t p2mt; - p2m_access_t a; - mfn_t mfn; - struct p2m_domain *p2m = p2m_get_hostp2m(d); - int ret; - - if ( !paging_mode_translate(d) ) - { - if ( !need_iommu(d) ) - return 0; - return iommu_unmap_page(d, gfn); - } - - gfn_lock(p2m, gfn, 0); - - mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL); - if ( p2mt == p2m_mmio_direct && mfn_x(mfn) == gfn ) - { - ret = p2m_set_entry(p2m, gfn, INVALID_MFN, PAGE_ORDER_4K, - p2m_invalid, p2m->default_access); - gfn_unlock(p2m, gfn, 0); - } - else - { - gfn_unlock(p2m, gfn, 0); - printk(XENLOG_G_WARNING - "non-identity map d%d:%lx not cleared (mapped to %lx)\n", - d->domain_id, gfn, mfn_x(mfn)); - ret = 0; - } - - return ret; -} - /* Returns: 0 for success, -errno for failure */ int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) { diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 919993e..714a19e 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1896,6 +1896,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, unsigned long end_pfn = PAGE_ALIGN_4K(rmrr->end_address) >> PAGE_SHIFT_4K; struct mapped_rmrr *mrmrr; struct domain_iommu *hd = dom_iommu(d); + int ret = 0; ASSERT(pcidevs_locked()); ASSERT(rmrr->base_address < rmrr->end_address); @@ -1909,8 +1910,6 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, if ( mrmrr->base == rmrr->base_address && mrmrr->end == rmrr->end_address ) { - int ret = 0; - if ( map ) { ++mrmrr->count; @@ -1920,9 +1919,10 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, if ( --mrmrr->count ) return 0; - while ( base_pfn < end_pfn ) + ret = modify_mmio_11(d, base_pfn, end_pfn - base_pfn, false); + while ( !iommu_use_hap_pt(d) && base_pfn < end_pfn ) { - if ( clear_identity_p2m_entry(d, base_pfn) ) + if ( iommu_unmap_page(d, base_pfn) ) ret = -ENXIO; base_pfn++; } @@ -1936,12 +1936,15 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, if ( !map ) return -ENOENT; - while ( base_pfn < end_pfn ) + ret = modify_mmio_11(d, base_pfn, end_pfn - base_pfn, true); + if ( ret ) + return ret; + while ( !iommu_use_hap_pt(d) && base_pfn < end_pfn ) { - int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag); - - if ( err ) - return err; + ret = iommu_map_page(d, base_pfn, base_pfn, + IOMMUF_readable|IOMMUF_writable); + if ( ret ) + return ret; base_pfn++; } diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 7035860..ccf19e5 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -602,11 +602,6 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, unsigned int order); -/* Set identity addresses in the p2m table (for pass-through) */ -int set_identity_p2m_entry(struct domain *d, unsigned long gfn, - p2m_access_t p2ma, unsigned int flag); -int clear_identity_p2m_entry(struct domain *d, unsigned long gfn); - /* Add foreign mapping to the guest's p2m table. */ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, unsigned long gpfn, domid_t foreign_domid); diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h index 3be1e91..5f6b4ef 100644 --- a/xen/include/xen/p2m-common.h +++ b/xen/include/xen/p2m-common.h @@ -2,6 +2,7 @@ #define _XEN_P2M_COMMON_H #include <public/vm_event.h> +#include <xen/softirq.h> /* * Additional access types, which are used to further restrict @@ -46,6 +47,35 @@ int unmap_mmio_regions(struct domain *d, mfn_t mfn); /* + * Preemptive Helper for mapping/unmapping MMIO regions. + */ +static inline int modify_mmio_11(struct domain *d, unsigned long pfn, + unsigned long nr_pages, bool map) +{ + int rc; + + while ( nr_pages > 0 ) + { + rc = (map ? map_mmio_regions : unmap_mmio_regions) + (d, _gfn(pfn), nr_pages, _mfn(pfn)); + if ( rc == 0 ) + break; + if ( rc < 0 ) + { + printk(XENLOG_ERR + "Failed to %smap %#lx - %#lx into domain %d memory map: %d\n", + map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc); + return rc; + } + nr_pages -= rc; + pfn += rc; + process_pending_softirqs(); + } + + return rc; +} + +/* * Set access type for a region of gfns. * If gfn == INVALID_GFN, sets the default access type. */
The current code used by Intel VTd will only map RMRR regions for the hardware domain, but will fail to map RMRR regions for unprivileged domains unless the page tables are shared between EPT and IOMMU. Fix this and simplify the code, removing the {set/clear}_identity_p2m_entry helpers and just using the normal MMIO mapping functions. Introduce a new MMIO mapping/unmapping helper, that takes care of checking for pending IRQs if the mapped region is big enough that it cannot be done in one shot. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: Feng Wu <feng.wu@intel.com> --- xen/arch/x86/mm/p2m.c | 86 ------------------------------------- xen/drivers/passthrough/vtd/iommu.c | 21 +++++---- xen/include/asm-x86/p2m.h | 5 --- xen/include/xen/p2m-common.h | 30 +++++++++++++ 4 files changed, 42 insertions(+), 100 deletions(-)