Re: [Xen-devel] [PATCH v2 2/2] xen/arm: introduce XENFEAT_grant_map_identity

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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