| Message ID | 1406208666-23547-3-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 07/24/2014 02:31 PM, Stefano Stabellini wrote: > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > index 7e83353..dacbe38 100644 > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -15,6 +15,7 @@ > #include <xen/guest_access.h> > #include <xen/hypercall.h> > #include <asm/current.h> > +#include <asm/grant_table.h> > #include <public/nmi.h> > #include <public/version.h> > > @@ -325,6 +326,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > break; > } > #endif > + if ( gnttab_need_identity_mapping(d) ) Actually even platform the IOMMU support needs to have this flags on. With this solution you break platform where not every DMA-capable device are behind an SMMU. > + fi.submap |= 1U << XENFEAT_grant_map_identity; > break; > default: > return -EINVAL; > diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h > index eac8a70..6f7ccd9 100644 > --- a/xen/include/asm-arm/grant_table.h > +++ b/xen/include/asm-arm/grant_table.h > @@ -36,6 +36,9 @@ static inline int replace_grant_supported(void) > #define gnttab_need_iommu_mapping(d) \ > (is_domain_direct_mapped(d) && need_iommu(d)) > > +#define gnttab_need_identity_mapping(d) \ > + (is_domain_direct_mapped(d) && !need_iommu(d)) > + Why didn't you drop the need_iommu(d) in is_domain_direct_mapped? Hence the name is confusing. You may think that we don't need identity mapping when IOMMU is used while it's actually the case, but we use a different function.
On Thu, 24 Jul 2014, Julien Grall wrote: > Hi Stefano, > > On 07/24/2014 02:31 PM, Stefano Stabellini wrote: > > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > > index 7e83353..dacbe38 100644 > > --- a/xen/common/kernel.c > > +++ b/xen/common/kernel.c > > @@ -15,6 +15,7 @@ > > #include <xen/guest_access.h> > > #include <xen/hypercall.h> > > #include <asm/current.h> > > +#include <asm/grant_table.h> > > #include <public/nmi.h> > > #include <public/version.h> > > > > @@ -325,6 +326,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > break; > > } > > #endif > > + if ( gnttab_need_identity_mapping(d) ) > > Actually even platform the IOMMU support needs to have this flags on. > With this solution you break platform where not every DMA-capable device > are behind an SMMU. I see. > > + fi.submap |= 1U << XENFEAT_grant_map_identity; > > break; > > default: > > return -EINVAL; > > diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h > > index eac8a70..6f7ccd9 100644 > > --- a/xen/include/asm-arm/grant_table.h > > +++ b/xen/include/asm-arm/grant_table.h > > @@ -36,6 +36,9 @@ static inline int replace_grant_supported(void) > > #define gnttab_need_iommu_mapping(d) \ > > (is_domain_direct_mapped(d) && need_iommu(d)) > > > > +#define gnttab_need_identity_mapping(d) \ > > + (is_domain_direct_mapped(d) && !need_iommu(d)) > > + > > Why didn't you drop the need_iommu(d) in is_domain_direct_mapped? I guess you mean why didn't you drop the need_iommu(d) in gnttab_need_iommu_mapping? Because how can you need an iommu mapping if an iommu is not present? > Hence the name is confusing. You may think that we don't need identity > mapping when IOMMU is used while it's actually the case, but we use a > different function. Yes, you are right. The whole thing is a bit confusing, I am trying to come up with a naming scheme that is as clear as possible. What if we simply define: #define gnttab_need_identity_mapping(d) (is_domain_direct_mapped(d))
On 07/24/2014 03:14 PM, Stefano Stabellini wrote: > On Thu, 24 Jul 2014, Julien Grall wrote: >> Hi Stefano, >> >> On 07/24/2014 02:31 PM, Stefano Stabellini wrote: >>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c >>> index 7e83353..dacbe38 100644 >>> --- a/xen/common/kernel.c >>> +++ b/xen/common/kernel.c >>> @@ -15,6 +15,7 @@ >>> #include <xen/guest_access.h> >>> #include <xen/hypercall.h> >>> #include <asm/current.h> >>> +#include <asm/grant_table.h> >>> #include <public/nmi.h> >>> #include <public/version.h> >>> >>> @@ -325,6 +326,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> break; >>> } >>> #endif >>> + if ( gnttab_need_identity_mapping(d) ) >> >> Actually even platform the IOMMU support needs to have this flags on. >> With this solution you break platform where not every DMA-capable device >> are behind an SMMU. > > I see. > > >>> + fi.submap |= 1U << XENFEAT_grant_map_identity; >>> break; >>> default: >>> return -EINVAL; >>> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h >>> index eac8a70..6f7ccd9 100644 >>> --- a/xen/include/asm-arm/grant_table.h >>> +++ b/xen/include/asm-arm/grant_table.h >>> @@ -36,6 +36,9 @@ static inline int replace_grant_supported(void) >>> #define gnttab_need_iommu_mapping(d) \ >>> (is_domain_direct_mapped(d) && need_iommu(d)) >>> >>> +#define gnttab_need_identity_mapping(d) \ >>> + (is_domain_direct_mapped(d) && !need_iommu(d)) >>> + >> >> Why didn't you drop the need_iommu(d) in is_domain_direct_mapped? > > I guess you mean why didn't you drop the need_iommu(d) in > gnttab_need_iommu_mapping? > > Because how can you need an iommu mapping if an iommu is not present? I would invert the name of the 2 macros. And drop !need_iommu(d) in the second. The code would look like: if (gnttab_need_identity_mapping(d)) { if (gnttab_need_iommu_mapping(d)) iommu_map_page else arch_ } On x86, the gnttab_need_iommu_mapping would be equal to 1. Regards,
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 464007e..67cbed9 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -727,7 +727,7 @@ __gnttab_map_grant_ref( double_gt_lock(lgt, rgt); - if ( gnttab_need_iommu_mapping(ld) ) + if ( gnttab_need_iommu_mapping(ld) || gnttab_need_identity_mapping(ld) ) { unsigned int wrc, rdc; int err = 0; @@ -738,13 +738,23 @@ __gnttab_map_grant_ref( !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) ) { if ( wrc == 0 ) - err = iommu_map_page(ld, frame, frame, - IOMMUF_readable|IOMMUF_writable); + { + if ( gnttab_need_identity_mapping(ld) ) + err = arch_grant_map_page_identity(ld, frame, 1); + else + err = iommu_map_page(ld, frame, frame, + IOMMUF_readable|IOMMUF_writable); + } } else if ( act_pin && !old_pin ) { if ( (wrc + rdc) == 0 ) - err = iommu_map_page(ld, frame, frame, IOMMUF_readable); + { + if ( gnttab_need_identity_mapping(ld) ) + err = arch_grant_map_page_identity(ld, frame, 0); + else + err = iommu_map_page(ld, frame, frame, IOMMUF_readable); + } } if ( err ) { @@ -935,15 +945,24 @@ __gnttab_unmap_common( act->pin -= GNTPIN_hstw_inc; } - if ( gnttab_need_iommu_mapping(ld) ) + if ( gnttab_need_iommu_mapping(ld) || gnttab_need_identity_mapping(ld) ) { unsigned int wrc, rdc; int err = 0; mapcount(lgt, rd, op->frame, &wrc, &rdc); if ( (wrc + rdc) == 0 ) - err = iommu_unmap_page(ld, op->frame); - else if ( wrc == 0 ) - err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable); + { + if ( gnttab_need_identity_mapping(ld) ) + err = arch_grant_unmap_page_identity(ld, op->frame); + else + err = iommu_unmap_page(ld, op->frame); + } else if ( wrc == 0 ) + { + if ( gnttab_need_identity_mapping(ld) ) + err = arch_grant_map_page_identity(ld, op->frame, 0); + else + err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable); + } if ( err ) { rc = GNTST_general_error; diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 7e83353..dacbe38 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -15,6 +15,7 @@ #include <xen/guest_access.h> #include <xen/hypercall.h> #include <asm/current.h> +#include <asm/grant_table.h> #include <public/nmi.h> #include <public/version.h> @@ -325,6 +326,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) break; } #endif + if ( gnttab_need_identity_mapping(d) ) + fi.submap |= 1U << XENFEAT_grant_map_identity; break; default: return -EINVAL; diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h index eac8a70..6f7ccd9 100644 --- a/xen/include/asm-arm/grant_table.h +++ b/xen/include/asm-arm/grant_table.h @@ -36,6 +36,9 @@ static inline int replace_grant_supported(void) #define gnttab_need_iommu_mapping(d) \ (is_domain_direct_mapped(d) && need_iommu(d)) +#define gnttab_need_identity_mapping(d) \ + (is_domain_direct_mapped(d) && !need_iommu(d)) + #endif /* __ASM_GRANT_TABLE_H__ */ /* * Local variables: diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h index 8c9bbcf..56861a7 100644 --- a/xen/include/asm-x86/grant_table.h +++ b/xen/include/asm-x86/grant_table.h @@ -68,6 +68,8 @@ static inline void gnttab_clear_flag(unsigned int nr, uint16_t *st) #define gnttab_need_iommu_mapping(d) \ (!paging_mode_translate(d) && need_iommu(d)) +#define gnttab_need_identity_mapping(d) (0) + static inline int replace_grant_supported(void) { return 1; diff --git a/xen/include/public/features.h b/xen/include/public/features.h index a149aa6..ba753a0 100644 --- a/xen/include/public/features.h +++ b/xen/include/public/features.h @@ -94,6 +94,9 @@ /* operation as Dom0 is supported */ #define XENFEAT_dom0 11 +/* Xen also maps grant references at pfn = mfn */ +#define XENFEAT_grant_map_identity 12 + #define XENFEAT_NR_SUBMAPS 1 #endif /* __XEN_PUBLIC_FEATURES_H__ */
The flag specifies that the hypervisor maps a grant page to guest physical address == machine address of the page in addition to the normal grant mapping address. Frontends are allowed to map the same page multiple times using multiple grant references. On the backend side it can be difficult to find out the physical address corresponding to a particular machine address, especially at the completation of a dma operation. To simplify address translations, we introduce a second mapping of the grant at physical address == machine address so that dom0 can issue cache maintenance operations without having to find the pfn. Call arch_grant_map_page_identity and arch_grant_unmap_page_identity from __gnttab_map_grant_ref and __gnttab_unmap_common to introduce the second mapping if the domain is directly mapped. Introduce gnttab_need_identity_mapping, to check if a domain needs the identity mapping. On x86 it just returns always 0. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v3: - introduce gnttab_need_identity_mapping; - check gnttab_need_identity_mapping in __gnttab_map_grant_ref and __gnttab_unmap_common. Changes in v2: - rename XENFEAT_grant_map_11 to XENFEAT_grant_map_identity; - remove superfluous ifdef CONFIG_ARM in xen/common/kernel.c; - don't modify gnttab_need_iommu_mapping; - call arch_grant_map_page_identity and arch_grant_unmap_page_identity from grant_table functions. --- xen/common/grant_table.c | 35 +++++++++++++++++++++++++++-------- xen/common/kernel.c | 3 +++ xen/include/asm-arm/grant_table.h | 3 +++ xen/include/asm-x86/grant_table.h | 2 ++ xen/include/public/features.h | 3 +++ 5 files changed, 38 insertions(+), 8 deletions(-)