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

[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


2. Why we can't just modify the original hypercall to meet our needs.

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