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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.07.14 at 12:47, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Thu, 24 Jul 2014, Jan Beulich wrote:
>> >>> On 23.07.14 at 19:19, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>> > @@ -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)".

I guessed that you did it for reasons along those lines. But unless
we're certain the compiler will always fold the redundant expressions
and unless you're certain the redundancy won't cause future
similar questions by readers of the code, I'd suggest to drop the
addition.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel