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

["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.

Jan


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