Re: [Xen-devel] [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.02.18 at 06:37, <blackskygg@xxxxxxxxx> wrote:
> ... Sorry for the incomplete mail. I somehow hit the "send" button before I
> finish composing the previous mail. And now it continues...
> 
> 2018-02-24 10:50 GMT+08:00 Zhongze Liu <blackskygg@xxxxxxxxx>:
>> Hi Jan,
>>
>> (Last week was the Chinese Spring Festival, so I failed to follow up
>> timely. Sorry for that.)
>>
>> 2018-02-15 16:58 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>>>> On 14.02.18 at 18:02, <blackskygg@xxxxxxxxx> wrote:
>>>> 2018-02-14 16:37 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>>>>>> On 14.02.18 at 08:15, <blackskygg@xxxxxxxxx> wrote:
>>>>>> 2018-02-13 23:26 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>>>>>>>> On 13.02.18 at 16:15, <blackskygg@xxxxxxxxx> wrote:
>>>>>>>> I've updated the comments according to your previous suggestions,
>>>>>>>> do they look good to you?
>>>>>>>
>>>>>>> The one in the public header is way too verbose. I specifically don't
>>>>>>> see why you would need to spell out XSM privilege requirements
>>>>>>> there. Please make new comments match existing ones in style and
>>>>>>> verbosity if at all possible, while still conveying all necessary /
>>>>>>> relevant information.
>>>>>>>
>>>>>>
>>>>>> I shortened it a little bit, and now it looks like:
>>>>>>
>>>>>> #define XENMAPSPACE_gmfn_share   6 /* GMFN from another dom. Unlike
>>>>>> gmfn_foreign,
>>>>>>                                       if (c) tries to map pages from (t) 
> into
>>>>>>                                       (d), this doesn't require that (d) 
> itself
>>>>>>                                       has the privilege to map the 
>>>>>> pages, 
> but
>>>>>>                                       instead requires that (c) has the
>>>>>>                                       privilege to do so, as long as (d) 
>>>>>> and 
> (t)
>>>>>>                                       are allowed to share memory pages.
>>>>>>                                       This is 
>>>>>> XENMEM_add_to_physmap_batch 
> only,
>>>>>>                                       and currently ARM only. */
>>>>>
>>>>> Which leaves unclear what (c), (d), and (t) are. How about
>>>>>
>>>>> "GMFN from another dom, XENMEM_add_to_physmap_batch (and
>>>>> currently ARM) only. Other than XENMAPSPACE_gmfn_foreign this
>>>>> <explain here what the difference is with a few simple words>."
>>>>>
>>>>> (You can and should go into further detail in the commit message.)
>>>>> Without this _properly_ explained, I'll continue to ask why you
>>>>> can't simply make XENMAPSPACE_gmfn_foreign do what you want
>>>>> (as it already takes two domid_t-s as input), by suitably adjusting
>>>>> its XSM check(s).
>>>>
>>>> I'm sorry that I failed to see the reason why you say "which leaves
>>>> unclear what (c), (d), and (t) are". I think "if (c) tries to map pages
>>>> from (t) into (d)" has already included the necessary information
>>>> about this: (c) is the caller of the hypercall (current), (d) is the
>>>> dest domain, and (t) the source domain.
>>>> I think I still need more of your explanation here.
>>>
>>> Someone coming across _just_ this comment (while reading the
>>> public header) will not necessarily know what (c), (d), and (t)
>>> stand for, and (s)he shouldn't be forced to dig into git history to
>>> find the patch description. But anyway - all this should go away
>>> from the header anyway, as explained before. All that's needed
>>> here is a terse but understandable explanation of what's different
>>> from XENMAPSPACE_gmfn_foreign.
>>>
>>
>> I think before we can reach a consensus on how the final comment
>> should look like, we need to reach an agreement on what should
>> be included in the <differences> part. And according to our previous
>> discussion, below is what I think is necessary so far:
>>
>> 1. Different privilege requirements
>>
> 
> This is what I've been trying to convey in the comment. But it seems that
> I failed to make it "terse yet understandable". Now my question is: Am I *not*
> supposed to spell out the detail of the exact difference between their
> privilege requirements? If so, do you have a rough picture of how it
> should look like? Are we going to give an example of a use case where
> the old subop is not applicable?
> 
>>
>> 2. Why we can't just modify the original hypercall to meet our needs.
> 
> According to our discussion on the previous versions of this patch, we
> can't fit the checks needed by both subop into one because doing so will
> either regress the original one or permit something that shouldn't be
> allowed in the new use cases. Should we clarify this in the comment, too?

My view on this is: The comment ought to describe the behavior,
nothing else. The commit message ought to explain the difference
to the existing sub-op, to help understand / explain the reason
why the existing one can't be suitably extended.

Jan

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