[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/2] xen/arm: introduce XENFEAT_grant_map_identity
On Thu, 24 Jul 2014, Jan Beulich wrote: > >>> On 23.07.14 at 19:19, <stefano.stabellini@xxxxxxxxxxxxx> wrote: > > Changes in v2: > > - rename XENFEAT_grant_map_11 to XENFEAT_grant_map_identity; > > - remove superfluous ifdef CONFIG_ARM in xen/common/kernel.c; > > Looks like you forgot to add an is_domain_direct_mapped() stub for > x86, breaking the build there? Sorry, I assumed there was already one. I'll build test on x86 too next time. > > @@ -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) || is_domain_direct_mapped(ld) ) > > This is bogus: On x86 the right part is always false, while on > ARM the right part is already part of gnttab_need_iommu_mapping(). > IOW no need to change anything here afaict. On ARM gnttab_need_iommu_mapping expands to #define gnttab_need_iommu_mapping(d) \ (is_domain_direct_mapped(d) && need_iommu(d)) however we would need to do the second mapping even if need_iommu(d) returns false. Changing gnttab_need_iommu_mapping(d) on ARM the way I did in the previous version of the patch series seems wrong to me now, because it is not an iommu mapping that gnttab needs, but a second regular p2m mapping. This is way I decided to add the "|| is_domain_direct_mapped(ld)". > > @@ -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_iommu_mapping(ld) ) > > + err = iommu_map_page(ld, frame, frame, > > + IOMMUF_readable|IOMMUF_writable); > > + else > > + err = arch_grant_map_page_identity(ld, frame, true); > > Irrespective of the above this is inefficient too: If you used > !is_domain_direct_mapped() as the conditional here, the compiler > would be able to eliminate the arch_grant_map_page_identity() > path on x86, making it unnecessary to have the stub as an inline > function (a simple declaration without definition would then > suffice as long as we don't ever build with -Od, which we already > depend upon elsewhere). It would be nicer the way you suggested, but on ARM a direct_mapped domain could still need the iommu mapping. Conceptually we need to check on gnttab_need_iommu_mapping. Alternatively we could introduce a new macro: gnttab_need_identity_mapping that is always (0) on x86. That is probably the best option. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |