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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

... 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?

Cheers,
and Best Wishes.

Zhongze Liu

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