Re: [Xen-devel] [PATCH v2 1/2] introduce GFN notification for translated domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2019 11:07, Julien Grall wrote:
> 
> 
> On 21/11/2019 09:04, Jan Beulich wrote:
>> On 20.11.2019 21:22, Julien Grall wrote:
>>> On 14/11/2019 16:43, Jan Beulich wrote:
>>>> TBD: Does Arm actually have anything to check against in its
>>>>        arch_notify_gfn()?
>>>
>>> I understand that we want to keep the code mostly generic, but I am a
>>> bit concerned of the extra cost to use notify_gfn() (and indirectly
>>> iommu_notify_gfn()) for doing nothing.
>>>
>>> I can't see any direct use of this for the foreseable future on Arm. So
>>> could we gate this under a config option?
>>
>> This is an option, sure. Alternatively I could see about making this
>> an inline function, but iirc there were header dependency issues.
>> Then again - is a call to a function doing almost nothing really so
>> much extra overhead on Arm.
> 
> AFAICT, this is a workaround for AMD driver. So any impact (no matter 
> the size) feels not right for Arm.
> 
> In this particular case, the only thing I request is to protect the 
> notify_gfn & callback with !CONFIG_IOMMU_FORCE_SHARE.

Oh, there already is a suitable config option. Will do (and
cover share_p2m_table() at the same time).

>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -946,6 +946,16 @@ map_grant_ref(
>>>>            return;
>>>>        }
>>>>    
>>>> +    if ( paging_mode_translate(ld) /* && (op->flags & GNTMAP_host_map) */ 
>>>> &&
>>>
>>> I think this wants an explanation in the code why the check is commented.
>>
>> Hmm, in such a case I'd rather omit the commented condition. It
>> being commented has the purpose of documenting itself.
> 
> I fail to understand why GNTMAP_host_map would always be true in the case.
> 
> AFAIU the code, this is only correct for paging_mode_external(ld) == 1. 
> Does it mean that paging_mode_translate(ld) and paging_mode_external(ld) 
> are always equal? If so, what's the point of having two macro (and two 
> flags)?

Historical reasons. Nowadays translate == external == refcounts on
x86. But since this is common code, perhaps I better un-comment that
part of the conditional.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel