Re: [PATCH v9] xen/grant-table: Avoid m2p_override during mapping

[Zoltan Kiss <zoltan.kiss@citrix.com>]

On 20/02/14 17:26, Stefano Stabellini wrote:
> On Mon, 17 Feb 2014, Zoltan Kiss wrote:
>> On 16/02/14 18:36, Stefano Stabellini wrote:
>>> On Wed, 12 Feb 2014, Zoltan Kiss wrote:
>>>> diff --git a/arch/arm/include/asm/xen/page.h
>>>> b/arch/arm/include/asm/xen/page.h
>>>> index e0965ab..4eaeb3f 100644
>>>> --- a/arch/arm/include/asm/xen/page.h
>>>> +++ b/arch/arm/include/asm/xen/page.h
>>>> @@ -97,16 +97,15 @@ static inline pte_t *lookup_address(unsigned long
>>>> address, unsigned int *level)
>>>>    	return NULL;
>>>>    }
>>>>
>>>> -static inline int m2p_add_override(unsigned long mfn, struct page *page,
>>>> -		struct gnttab_map_grant_ref *kmap_op)
>>>> -{
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static inline int m2p_remove_override(struct page *page, bool clear_pte)
>>>> -{
>>>> -	return 0;
>>>> -}
>>>> +extern int set_foreign_p2m_mapping(struct gnttab_map_grant_ref *map_ops,
>>>> +				   struct gnttab_map_grant_ref *kmap_ops,
>>>> +				   struct page **pages, unsigned int count,
>>>> +				   bool m2p_override);
>>>> +
>>>> +extern int clear_foreign_p2m_mapping(struct gnttab_unmap_grant_ref
>>>> *unmap_ops,
>>>> +				     struct gnttab_map_grant_ref *kmap_ops,
>>>> +				     struct page **pages, unsigned int count,
>>>> +				     bool m2p_override);
>>>
>>> Much much better.
>>> The only comment I have is about this m2p_override boolean parameter.
>>> m2p_override is now meaningless in this context, what we really want to
>>> let the arch specific implementation know is whether the mapping is a
>>> kernel only mapping or a userspace mapping.
>>> Testing for kmap_ops != NULL might even be enough, but it would not
>>> improve the interface.
>> gntdev is the only user of this, the kmap_ops parameter there is:
>> use_ptemod ? map->kmap_ops + offset : NULL
>> where:
>> use_ptemod = !xen_feature(XENFEAT_auto_translated_physmap);
>> So I think we can't rely on kmap_ops to decide whether we should m2p_override
>> or not.
>>
>>> Is it possible to realize if the mapping is a userspace mapping by
>>> checking for GNTMAP_application_map in map_ops?
>>> Otherwise I would keep the boolean and rename it to user_mapping.
>> Sounds better, but as far as I see gntdev set that flag in find_grant_ptes,
>> which is called only
>>
>> if (use_ptemod) {
>> 	err = apply_to_page_range(vma->vm_mm, vma->vm_start,
>> 				  vma->vm_end - vma->vm_start,
>> 				  find_grant_ptes, map);
>>
>> So if xen_feature(XENFEAT_auto_translated_physmap), we don't have kmap_ops,
>> and GNTMAP_application_map is not set as well, but I guess we still need
>> m2p_override. Or not? I'm a bit confused, maybe because of Monday ...
>
> If xen_feature(XENFEAT_auto_translated_physmap) we shouldn't need the
> m2p_override.
>

So it's safe to assume that we need m2p_override only if kmap_ops != 
NULL, and we can avoid the extra bool parameter, is that correct? At 
least with the current users of grant mapping it seems to be true.
In which case we don't need the wrappers for gnttab_[un]map_refs as well.
Actually the most of m2p_add/remove_override takes effect only if there 
is a kmap_op parameter, but what about the rest of the code there?

Zoli