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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Jan,

2018-02-14 16:37 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>> On 14.02.18 at 08:15, <blackskygg@xxxxxxxxx> wrote:
>> Hi Jan,
>>
>> 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.

>
> You'd also need to adjust the comment on the foreign_domid
> structure field, as it saying "gmfn_foreign" would otherwise become
> stale with your change.

Thanks for pointing out that. I've already updated it.

>
> I don't, btw, like the ARM only part here - there's nothing
> inherently wrong with the same operation being sensible on x86.
>

I agree that we should make this also available to x86 guests, but
we have to fix the FIXME in x86/mm/p2m.c: p2m_add_foreign() first.
And that, I think, should be done in another patch set. And "currently"
here also means that it's planned to be fixed. I just don't want to
disappoint the users who are eager to try this new subop out but end
up getting weired errors.

Cheers,

Zhongze Liu

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