Allow setting up shared memory areas between VMs from xl config files

Quoted 1 line

Quoted 1 line

Quoted 1 line · On 30.01.18 at 18:50, <[email protected]> wrote:

xen/include/xsm/dummy.h static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str · line 524

Quoted 7 lines

xen/include/xsm/dummy.h +1 -0

@@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
     return xsm_default_action(action, d, t);
 }
 
+static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)

Line length.

xen/include/xsm/dummy.h static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str · line 527

Quoted 3 lines

xen/include/xsm/dummy.h +3 -0

+{
+    XSM_ASSERT_ACTION(XSM_TARGET);
+    return xsm_default_action(action, current->domain, t);

How does this represent a proper default equivalent of ...

xen/xsm/flask/hooks.c +4 -0

@@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
     return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }
 
+static int flask_map_gmfn_share(struct domain *d, struct domain *t)
+{
+    return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
+        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
 

... this?

Jan

Quoted 1 line

Quoted 1 line · On 01.02.18 at 19:11, <[email protected]> wrote:

2018-02-01 18:23 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 25 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str · line 527

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 30.01.18 at 18:50, <[email protected]> wrote:

xen/include/xsm/dummy.h +4 -0

@@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
     return xsm_default_action(action, d, t);
 }
 
+static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+{
+    XSM_ASSERT_ACTION(XSM_TARGET);
+    return xsm_default_action(action, current->domain, t);

How does this represent a proper default equivalent of ...

xen/xsm/flask/hooks.c +4 -0

@@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
     return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }
 
+static int flask_map_gmfn_share(struct domain *d, struct domain *t)
+{
+    return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
+        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
 

Quoted 1 line

... this?

The full flask check tries to guarantee that:
1) (c) over (d), which will always be checked somewhere higher in

      the call stack for these kinds of calls;

2) (c) over (t), namely, MMU__MAP_READ/WRITE;
3) (d) over (t), namely, MMU__SHARE_MEM

In my default builtin actions, checks 1) and 2) are done by the xsm_default_action
function, but I can't think of a way to do check 3), because we don't have the proper
equivalence of MMU__SHARE_MEM (xsm_default_action is definitely not a choice).
Want to hear your and other maintainers' suggestions about how to do this properly.

Quoted 27 lines

Quoted 1 line

Quoted 1 line · On 01.02.18 at 19:11, <[email protected]> wrote:

2018-02-01 18:23 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 25 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str · line 527

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 30.01.18 at 18:50, <[email protected]> wrote:

xen/include/xsm/dummy.h +4 -0

@@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
     return xsm_default_action(action, d, t);
 }
 
+static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+{
+    XSM_ASSERT_ACTION(XSM_TARGET);
+    return xsm_default_action(action, current->domain, t);

How does this represent a proper default equivalent of ...

xen/xsm/flask/hooks.c +4 -0

@@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
     return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }
 
+static int flask_map_gmfn_share(struct domain *d, struct domain *t)
+{
+    return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
+        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
 

Quoted 5 lines

Quoted 1 line

... this?

The full flask check tries to guarantee that:
1) (c) over (d), which will always be checked somewhere higher in

      the call stack for these kinds of calls;

This is in no way apparent here. At the very least a comment to that effect should be added, but perhaps even better would be if you had an explicit xsm_default_action(..., current->domain, d) call there. Whether one or the other is preferable I'd leave to Daniel.

Quoted 11 lines

2) (c) over (t), namely, MMU__MAP_READ/WRITE;
3) (d) over (t), namely, MMU__SHARE_MEM

In my default builtin actions, checks 1) and 2) are done by the xsm_default_action
function, but I can't think of a way to do check 3), because we don't have the proper
equivalence of MMU__SHARE_MEM (xsm_default_action is definitely not a choice).
Want to hear your and other maintainers' suggestions about how to do this properly.

"(d) over (t)" would suggest xsm_default_action(action, d, t), which I don't think is appropriate here. In fact aiui d and t are unrelated to one another in terms of mutual privilege. I don't think 3) needs expressing in the dummy version; it's really the apparent lack of 1)k which I've been commenting on. But again, I'll leave it to Daniel to tell you otherwise. What is imperative in any event is that you extend the description to also reason about the dummy logic, at least as long as it isn't a clear equivalent of the flask variant.

xen/arch/x86/mm.c int xenmem_add_to_physmap_one( · line 4132

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 30.01.18 at 18:50, <[email protected]> wrote:

xen/arch/x86/mm.c +4 -0

@@ -4126,6 +4126,10 @@ int xenmem_add_to_physmap_one(
         }
         case XENMAPSPACE_gmfn_foreign:
             return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
+        case XENMAPSPACE_gmfn_share:
+            gdprintk(XENLOG_WARNING,
+                     "XENMAPSPACE_gmfn_share is currently not supported on x86\n");
+            break;

Please don't - a hypervisor log message isn't really useful here. It should be the tool stack to disallow respective options on x86 until that's implemented.

xen/include/public/memory.h DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t); · line 229

Quoted 6 lines

xen/include/public/memory.h +0 -0

@@ -227,6 +227,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
                                       Stage-2 using the Normal Memory
                                       Inner/Outer Write-Back Cacheable
                                       memory attribute. */

xen/include/public/memory.h DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t); · line 233

Quoted 4 lines

xen/include/public/memory.h +4 -0

+#define XENMAPSPACE_gmfn_share   6 /* Same as *_gmfn_foreign, but this is
+                                      for a privileged dom to share pages
+                                      between two doms. */
+

The comment doesn't make clear why XENMAPSPACE_gmfn_foreign then can't be used. In particular it is left open how _both_ domains would be specified.

Also XENMAPSPACE_gmfn_foreign is restricted to XENMEM_add_to_physmap_batch (a comment says so) - how about this new one? According to the actual code changes you do, there's no meaningful difference, in which case the restriction should be named here as well.

Quoted 6 lines

xen/include/xsm/dummy.h +0 -0

@@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
     return xsm_default_action(action, d, t);
 }
 

xen/include/xsm/dummy.h static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str · line 524

Quoted 1 line

xen/include/xsm/dummy.h +1 -0

+static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)

Line length.

Quoted 1 line

xen/include/xsm/dummy.h +1 -0

+{

xen/xsm/flask/hooks.c +2 -0

+    return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
+        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
 

... this?

Quoted 1 line

Quoted 1 line · On 13.02.18 at 16:15, <[email protected]> wrote:

I've updated the comments according to your previous suggestions, do they look good to you?

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 13.02.18 at 16:15, <[email protected]> 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.

Quoted 1 line

Quoted 1 line · On 14.02.18 at 08:15, <[email protected]> wrote:

Hi Jan,

2018-02-13 23:26 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 13.02.18 at 16:15, <[email protected]> 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. */

Quoted 17 lines

Quoted 1 line

Quoted 1 line · On 14.02.18 at 08:15, <[email protected]> wrote:

Hi Jan,

2018-02-13 23:26 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 13.02.18 at 16:15, <[email protected]> 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:

Quoted 10 lines

#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).

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.

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

Quoted 1 line

Quoted 1 line · On 14.02.18 at 18:02, <[email protected]> wrote:

2018-02-14 16:37 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 32 lines

Quoted 15 lines

Quoted 1 line

Quoted 1 line · On 14.02.18 at 08:15, <[email protected]> wrote:

2018-02-13 23:26 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 13.02.18 at 16:15, <[email protected]> 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:

Quoted 10 lines

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

Quoted 5 lines

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

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 14.02.18 at 18:02, <[email protected]> wrote:

2018-02-14 16:37 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 32 lines

Quoted 15 lines

Quoted 1 line

Quoted 1 line · On 14.02.18 at 08:15, <[email protected]> wrote:

2018-02-13 23:26 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 13.02.18 at 16:15, <[email protected]> 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:

Quoted 10 lines

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

Quoted 5 lines

(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).

Quoted 6 lines

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.

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 <[email protected]>:

Quoted 47 lines

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 14.02.18 at 18:02, <[email protected]> wrote:

2018-02-14 16:37 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 32 lines

Quoted 15 lines

Quoted 1 line

Quoted 1 line · On 14.02.18 at 08:15, <[email protected]> wrote:

2018-02-13 23:26 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 13.02.18 at 16:15, <[email protected]> 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:

Quoted 10 lines

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

Quoted 5 lines

(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).

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 1 line

Quoted 1 line · On 24.02.18 at 06:37, <[email protected]> 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 <[email protected]>:

Quoted 26 lines

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 <[email protected]>:

Quoted 20 lines

Quoted 20 lines

Quoted 1 line

Quoted 1 line · On 14.02.18 at 18:02, <[email protected]> wrote:

2018-02-14 16:37 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 18 lines

Quoted 18 lines

Quoted 1 line

Quoted 1 line · On 14.02.18 at 08:15, <[email protected]> wrote:

2018-02-13 23:26 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 13.02.18 at 16:15, <[email protected]> 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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

(d), this doesn't require that (d)

itself

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

has the privilege to map the pages,

but

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

instead requires that (c) has the privilege to do so, as long as (d) and

(t)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

are allowed to share memory pages. This is XENMEM_add_to_physmap_batch

only,

Quoted 30 lines

Quoted 21 lines

Quoted 14 lines

Quoted 8 lines

Quoted 1 line

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

Quoted 5 lines

(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).

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

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?

Quoted 1 line

Quoted 1 line

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?

xen/arch/arm/mm.c +1 -0

index 3c328e2df5..8e385d62a8 100644
@@ -1251,6 +1251,7 @@ int xenmem_add_to_physmap_one(
 
          break;
      case XENMAPSPACE_gmfn_foreign:
+    case XENMAPSPACE_gmfn_share:
      {
          struct domain *od;
          p2m_type_t p2mt;

Add a new structure to the IDL familiy to represent static shared memory regions

as proposed in the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

And deleted some trailing white spaces.

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Reviewed-by Stefano Stabellini <[email protected]>

Acked-by Wei Liu <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
  tools/libxl/libxl.h         |  4 ++++
  tools/libxl/libxl_types.idl | 32 ++++++++++++++++++++++++++++++--
  2 files changed, 34 insertions(+), 2 deletions(-)

tools/libxl/libxl.h +0 -0

index eca0ea2c50..372ad3cd32 100644

tools/libxl/libxl.h +0 -0

  #include <libxl_event.h>
 
  #endif /* LIBXL_H */

tools/libxl/libxl_types.idl +14 -2

index 35038120ca..4e9accf168 100644
@@ -559,10 +559,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                         ("keymap",           string),
                                         ("sdl",              libxl_sdl_info),
                                         ("spice",            libxl_spice_info),
-
+
                                         ("gfx_passthru",     libxl_defbool),
                                         ("gfx_passthru_kind", libxl_gfx_passthru_kind),
-
+
                                         ("serial",           string),
                                         ("boot",             string),
                                         ("usb",              libxl_defbool),

@@ -816,6 +816,33 @@ libxl_device_vdispl = Struct("device_vdispl", [
      ("connectors", Array(libxl_connector_param, "num_connectors"))
      ])
 
+libxl_sshm_cachepolicy = Enumeration("sshm_cachepolicy", [
+    (-1, "UNKNOWN"),
+    (0,  "ARM_NORMAL"),  # ARM policies should be < 32
+    (32,  "X86_NORMAL"), # X86 policies should be >= 32
+    ], init_val = "LIBXL_SSHM_CHCHE_POLICY_UNKNOWN")
+
+libxl_sshm_prot = Enumeration("sshm_prot", [
+    (-1, "UNKNOWN"),
+    (3,  "RW"),
+    ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
+
+libxl_sshm_role = Enumeration("sshm_role", [

tools/libxl/libxl_types.idl +5 -0

+    ("prot", libxl_sshm_prot, {'init_val': 'LIBXL_SSHM_PROT_UNKNOWN'}),
+    ("cache_policy", libxl_sshm_cachepolicy, {'init_val': 'LIBXL_SSHM_CACHEPOLICY_UNKNOWN'}),
+    ("role", libxl_sshm_role, {'init_val': 'LIBXL_SSHM_ROLE_UNKNOWN'}),
+])
+
  libxl_domain_config = Struct("domain_config", [
      ("c_info", libxl_domain_create_info),
      ("b_info", libxl_domain_build_info),

Hi,

Quoted 3 lines · Zhongze Liu

Add a new structure to the IDL familiy to represent static shared memory regions

tools/libxl/libxl_types.idl libxl_device_vdispl = Struct("device_vdispl", [ · line 840

Quoted 5 lines

tools/libxl/libxl_types.idl +5 -0

+libxl_static_shm = Struct("static_shm", [
+    ("id", string),
+    ("offset", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),

We might want to store the size rather than the end. This would allow us to cover region up to the address 2^64-1.

Also, this would make clearer whether end is included in the region or not.

Thanks for reviewing.

2018-02-06 19:27 GMT+08:00 Julien Grall <[email protected]>:

Quoted 6 lines

Hi,

Quoted 3 lines · Zhongze Liu

Add a new structure to the IDL familiy to represent static shared memory regions

[...]

Quoted 10 lines

tools/libxl/libxl_types.idl libxl_device_vdispl = Struct("device_vdispl", [ · line 840

Quoted 5 lines

tools/libxl/libxl_types.idl +5 -0

+libxl_static_shm = Struct("static_shm", [
+    ("id", string),
+    ("offset", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),

We might want to store the size rather than the end. This would allow us to cover region up to the address 2^64-1.

Quoted 2 lines

Also, this would make clearer whether end is included in the region or not.

I think making the range inclusive and documenting it would have the same effect.
But I'm not sure which syntax is more friendly to the users. What do you think?

Hi,

Quoted 32 lines · Zhongze Liu

Thanks for reviewing.

2018-02-06 19:27 GMT+08:00 Julien Grall <[email protected]>:

Quoted 8 lines

Hi,

Quoted 4 lines · Zhongze Liu

Add a new structure to the IDL familiy to represent static shared memory regions

[...]

Quoted 15 lines

tools/libxl/libxl_types.idl libxl_device_vdispl = Struct("device_vdispl", [ · line 840

Quoted 5 lines

tools/libxl/libxl_types.idl +5 -0

+libxl_static_shm = Struct("static_shm", [
+    ("id", string),
+    ("offset", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),

We might want to store the size rather than the end. This would allow us to
cover region up to the address 2^64-1.

Also, this would make clearer whether end is included in the region or not.

Quoted 4 lines

I think making the range inclusive and documenting it would have the same effect.
But I'm not sure which syntax is more friendly to the users. What do you think?

You would still run into some problem. Indeed LIBX_SSHM_RANGE_UNKNOWN is defined as UINT64_MAX. So how would you differentiate them?

Hi,

2018-02-06 23:46 GMT+08:00 Julien Grall <[email protected]>:

Quoted 41 lines

Hi,

Quoted 36 lines · Zhongze Liu

Thanks for reviewing.

2018-02-06 19:27 GMT+08:00 Julien Grall <[email protected]>:

Quoted 8 lines

Hi,

Quoted 4 lines · Zhongze Liu

Add a new structure to the IDL familiy to represent static shared memory regions

[...]

Quoted 8 lines

tools/libxl/libxl_types.idl libxl_device_vdispl = Struct("device_vdispl", [ · line 840

Quoted 5 lines

tools/libxl/libxl_types.idl +5 -0

+libxl_static_shm = Struct("static_shm", [
+    ("id", string),
+    ("offset", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),

Quoted 7 lines

We might want to store the size rather than the end. This would allow us to
cover region up to the address 2^64-1.

Also, this would make clearer whether end is included in the region or not.

I think making the range inclusive and documenting it would have the same effect.
But I'm not sure which syntax is more friendly to the users. What do you think?

Quoted 2 lines

You would still run into some problem. Indeed LIBX_SSHM_RANGE_UNKNOWN is defined as UINT64_MAX. So how would you differentiate them?

But saying inclusive, I was actually trying to say "including the page that begins at @end", so

the only possibility when LIBXL_SSHM_RANGE_UNKNOWN would be a valid value for @end is when the page granularity is 1byte, which, I think, is not very likely to happen.

But soon I find this might lead to more confusion. Now I agree with you that we should use
the begin/size syntax instead of the current one.

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping process involves the follwing steps:

* Set defaults and check for further errors in the static_shm configs:
overlapping areas, invalid ranges, duplicated master domain, not page aligned, no master domain etc.
* Use xc_domain_add_to_physmap_batch to do the page sharing.
* When some of the pages can't be successfully mapped, roll back any successfully mapped pages so that the system stays in a consistent state.
* Write infomation about static shared memory areas into the appropriate

xenstore paths and set the refcount of the shared region accordingly.

Temporarily mark this as unsupported on x86 because calling p2m_add_foreign on two domU's is currently not allowd on x86 (see the comments in x86/mm/p2m.c:p2m_add_foreign for more details).

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
V3:
* Unmap the successfully mapped pages whenever rc != 0.

V4:
* Use XENMAPSPACE_gmfn_share instead of *_gmfn_foreign.

---
  tools/libxl/Makefile         |   2 +-
  tools/libxl/libxl_arch.h     |   6 +
  tools/libxl/libxl_arm.c      |  15 ++
  tools/libxl/libxl_create.c   |  27 +++
  tools/libxl/libxl_internal.h |  14 ++
  tools/libxl/libxl_sshm.c     | 397 +++++++++++++++++++++++++++++++++++++++++++
  tools/libxl/libxl_x86.c      |  19 +++
  7 files changed, 479 insertions(+), 1 deletion(-)

create mode 100644 tools/libxl/libxl_sshm.c

tools/libxl/Makefile +1 -1

index 917ceb0e72..e2834d84dc 100644
@@ -139,7 +139,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
  			libxl_dom_suspend.o libxl_dom_save.o libxl_usb.o \
  			libxl_vtpm.o libxl_nic.o libxl_disk.o libxl_console.o \
  			libxl_cpupool.o libxl_mem.o libxl_sched.o libxl_tmem.o \
-			libxl_9pfs.o libxl_domain.o libxl_vdispl.o \
+			libxl_9pfs.o libxl_domain.o libxl_vdispl.o libxl_sshm.o \
                          $(LIBXL_OBJS-y)
  LIBXL_OBJS += libxl_genid.o
  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o

tools/libxl/libxl_arch.h +6 -0

index 784ec7f609..11433fe466 100644
@@ -78,6 +78,12 @@ int libxl__arch_extra_memory(libxl__gc *gc,
                               const libxl_domain_build_info *info,
                               uint64_t *out);
 
+_hidden
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info);
+
+_hidden
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm);
+
  #if defined(__i386__) || defined(__x86_64__)
 
  #define LAPIC_BASE_ADDRESS  0xfee00000

tools/libxl/libxl_arm.c +15 -0

index 3e46554301..e14879ec08 100644
@@ -1154,6 +1154,21 @@ void libxl__arch_domain_build_info_acpi_setdefault(
      libxl_defbool_setdefault(&b_info->acpi, false);
  }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    return true;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_ARM_NORMAL;
+    if (sshm->cache_policy >= LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;
+}
+
  /*
   * Local variables:
   * mode: C

tools/libxl/libxl_create.c +0 -0

index c498135246..98c16867bf 100644

tools/libxl/libxl_create.c +25 -0

+    ret = libxl__sshm_add(gc, domid, d_config->sshms, d_config->num_sshms);
+    if (ret != 0) {
+        LOG(ERROR, "failed to map static shared memory");
+        goto out;
+    }
+
      ret = libxl__build_post(gc, domid, info, state, vments, localents);
  out:
      return ret;

@@ -957,6 +965,25 @@ static void initiate_domain_create(libxl__egc *egc,
          goto error_out;
      }
 
+    if (d_config->num_sshms != 0 &&
+        !libxl__arch_domain_support_sshm(&d_config->b_info)) {
+        LOGD(ERROR, domid, "static_shm is not supported by this domain type.");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    for (i = 0; i < d_config->num_sshms; ++i) {
+        ret = libxl__sshm_setdefault(gc, domid, &d_config->sshms[i]);
+        if (ret) {
+            LOGD(ERROR, domid, "Unable to set defaults for static shm");
+            goto error_out;
+        }
+    }
+
+    ret = libxl__sshm_check_overlap(gc, domid,
+                                    d_config->sshms, d_config->num_sshms);
+    if (ret) goto error_out;
+
      ret = libxl__domain_make(gc, d_config, &domid, &state->config);
      if (ret) {
          LOGD(ERROR, domid, "cannot make domain: %d", ret);

tools/libxl/libxl_internal.h +14 -0

index 506687fbe9..2cfe4c08a7 100644
@@ -4415,6 +4415,20 @@ static inline bool libxl__string_is_default(char **s)
  }
  #endif
 
+/*
+ * Set up static shared ram pages for HVM domains to communicate
+ *
+ * This function should only be called after the memory map is constructed
+ * and before any further memory access.
+ */
+_hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
+                            libxl_static_shm *sshm, int len);
+
+_hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                      libxl_static_shm *sshms, int len);
+_hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                                   libxl_static_shm *sshm);
+
  /*
   * Local variables:
   * mode: C

tools/libxl/libxl_sshm.c new file +10 -0

new file mode 100644
index 0000000000..562f46f299
@@ -0,0 +1,397 @@
+#include "libxl_osdeps.h"
+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#define SSHM_PATH(id) GCSPRINTF("/libxl/static_shm/%s", id)
+
+#define SSHM_ERROR(domid, sshmid, f, ...)                               \
+    LOGD(ERROR, domid, "static_shm id = %s: " f, sshmid, ##__VA_ARGS__)
+
+

tools/libxl/libxl_sshm.c new file +30 -0

+
+    /* role-specific checks */
+    if (sshm->role == LIBXL_SSHM_ROLE_SLAVE) {
+        if (sshm->offset == LIBXL_SSHM_RANGE_UNKNOWN)
+            sshm->offset = 0;
+        if (sshm->cache_policy != LIBXL_SSHM_CACHEPOLICY_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache_policy is only applicable to master domains");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+    } else {
+        if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,
+                       "offset is only applicable to slave domains");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+
+        rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
+        if (rc) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache policy not supported on this platform");
+            goto out;
+        }
+    }
+
+    rc = 0;
+out:
+    return rc;

tools/libxl/libxl_sshm.c new file +29 -0

+int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                     libxl_static_shm *sshms, int len)
+{
+
+    const libxl_static_shm **slave_sshms = NULL;
+    int num_slaves;
+    int i;
+
+    if (!len) return 0;
+
+    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
+    num_slaves = 0;
+    for (i = 0; i < len; ++i) {
+        if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE)
+            slave_sshms[num_slaves++] = sshms + i;
+    }
+    qsort(slave_sshms, num_slaves, sizeof(slave_sshms[0]), sshm_range_cmp);
+
+    for (i = 0; i < num_slaves - 1; ++i) {
+        if (slave_sshms[i+1]->begin < slave_sshms[i]->end) {
+            SSHM_ERROR(domid, slave_sshms[i+1]->id, "slave ranges overlap.");
+            return ERROR_INVAL;
+        }
+    }
+
+    return 0;
+}
+
+/*   libxl__sshm_do_map -- map pages into slave's physmap

tools/libxl/libxl_sshm.c new file +6 -0

+    int *errs;
+    xen_ulong_t *idxs;
+    xen_pfn_t *gpfns;
+
+    num_mpages = (msshm->end - msshm->begin) >> XC_PAGE_SHIFT;
+    num_spages = (sshm->end - sshm->begin) >> XC_PAGE_SHIFT;

tools/libxl/libxl_sshm.c new file +36 -0

+    errs = libxl__calloc(gc, num_spages, sizeof(int));
+    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
+    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
+    for (i = 0; i < num_spages; i++) {
+        idxs[i] = (msshm->begin >> XC_PAGE_SHIFT) + offset + i;
+        gpfns[i]= (sshm->begin >> XC_PAGE_SHIFT) + i;
+    }
+    rc = xc_domain_add_to_physmap_batch(CTX->xch,
+                                        sid, mid,
+                                        XENMAPSPACE_gmfn_share,
+                                        num_spages,
+                                        idxs, gpfns, errs);
+
+    num_success = 0;
+    for (i = 0; i < num_spages; i++) {
+        if (errs[i]) {
+            SSHM_ERROR(sid, sshm->id,
+                       "can't map at address 0x%"PRIx64".",
+                       gpfns[i] << XC_PAGE_SHIFT);
+            rc = ERROR_FAIL;
+        } else {
+            mapped[num_success++] = gpfns[i];
+        }
+    }
+    *nmapped = num_success;
+    if (rc) goto out;
+
+    rc = 0;
+out:
+    return rc;
+}
+
+static int libxl__sshm_incref(libxl__gc *gc, xs_transaction_t xt,
+                              const char *sshm_path)
+{
+    int rc, count;

tools/libxl/libxl_sshm.c new file +227 -0

+
+    rc = 0;
+out:
+    return rc;
+}
+
+static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
+                                 libxl_static_shm *sshm)
+{
+    int rc, i;
+    const char *sshm_path, *slave_path;
+    const char *dom_path, *dom_sshm_path, *dom_role_path;
+    const char *xs_value;
+    char *ents[9];
+    libxl_static_shm master_sshm;
+    uint32_t master_domid;
+    xen_pfn_t *mapped;
+    unsigned int nmapped = 0;
+    xs_transaction_t xt = XBT_NULL;
+    bool isretry;
+
+    sshm_path = SSHM_PATH(sshm->id);
+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, sshm_path, domid);
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    /* the domain should be in xenstore by now */
+    assert(dom_path);
+    dom_sshm_path = GCSPRINTF("%s/static_shm/%s", dom_path, sshm->id);
+    dom_role_path = GCSPRINTF("%s/role", dom_sshm_path);
+
+    /* prepare the slave xenstore entries */
+    ents[0] = "begin";
+    ents[1] = GCSPRINTF("0x%"PRIx64, sshm->begin);
+    ents[2] = "end";
+    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->end);
+    ents[4] = "offset";
+    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->offset);
+    ents[6] = "prot";
+    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
+    ents[8] = NULL;
+
+    mapped = libxl__calloc(gc, (sshm->end - sshm->begin) >> XC_PAGE_SHIFT,
+                           sizeof(xen_pfn_t));
+
+    isretry = false;
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (!libxl__xs_read(gc, xt, sshm_path)) {
+            SSHM_ERROR(domid, sshm->id, "no master found.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        /* every ID can appear in each domain at most once */
+        if (libxl__xs_read(gc, xt, dom_sshm_path)) {
+            SSHM_ERROR(domid, sshm->id,
+                       "domain tried to map the same ID twice.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        /* look at the master info and see if we could do the mapping */
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/prot", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        libxl_sshm_prot_from_string(xs_value, &master_sshm.prot);
+
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/begin", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        master_sshm.begin = strtoull(xs_value, NULL, 16);
+
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/end", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        master_sshm.end = strtoull(xs_value, NULL, 16);
+
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/master", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        master_domid = strtoull(xs_value, NULL, 16);
+
+        if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN)
+            sshm->prot = master_sshm.prot;
+
+        /* check if the slave is asking too much permission */
+        if (master_sshm.prot < sshm->prot) {
+            SSHM_ERROR(domid, sshm->id, "slave is asking too much permission.");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+
+        /* all checks passed, do the job */
+        if (!isretry) {
+            rc = libxl__sshm_do_map(gc, master_domid, domid,
+                                    sshm, &master_sshm,
+                                    mapped, &nmapped);
+            if (rc) goto out;
+        }
+
+        /* write the result to xenstore and commit */
+        rc = libxl__xs_write_checked(gc, xt, dom_role_path, "slave");
+        if (rc) goto out;
+        rc = libxl__xs_writev(gc, xt, slave_path, ents);
+        if (rc) goto out;
+        rc = libxl__sshm_incref(gc, xt, sshm_path);
+        if (rc) goto out;
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+        isretry = true;
+    }
+
+    rc = 0;
+out:
+    if (rc) {
+        /* roll back successfully mapped pages */
+        SSHM_ERROR(domid, sshm->id, "failed to map some pages, cancelling.");
+        for (i = 0; i < nmapped; i++) {
+            xc_domain_remove_from_physmap(CTX->xch, domid, mapped[i]);
+        }
+    }
+
+    libxl__xs_transaction_abort(gc, &xt);
+
+    return rc;
+}
+
+static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)
+{
+    int rc;
+    const char *sshm_path, *dom_path, *dom_role_path;
+    char *ents[13];
+    struct xs_permissions noperm;
+    xs_transaction_t xt = XBT_NULL;
+
+    sshm_path = SSHM_PATH(sshm->id);
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    /* the domain should be in xenstore by now */
+    assert(dom_path);
+    dom_role_path = GCSPRINTF("%s/static_shm/%s/role", dom_path, sshm->id);
+
+    /* prepare the xenstore entries */
+    ents[0] = "master";
+    ents[1] = GCSPRINTF("%"PRIu32, domid);
+    ents[2] = "begin";
+    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->begin);
+    ents[4] = "end";
+    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->end);
+    ents[6] = "prot";
+    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
+    ents[8] = "cache_policy";
+    ents[9] = libxl__strdup(gc,
+                            libxl_sshm_cachepolicy_to_string(sshm->cache_policy));
+    ents[10] = "usercnt";
+    ents[11] = "1";
+    ents[12] = NULL;
+
+    /* could only be accessed by Dom0 */
+    noperm.id = 0;
+    noperm.perms = XS_PERM_NONE;
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (!libxl__xs_read(gc, xt, sshm_path)) {
+            /* every ID can appear in each domain at most once */
+            if (libxl__xs_read(gc, xt, dom_role_path)) {
+                SSHM_ERROR(domid, sshm->id,
+                           "domain tried to map the same ID twice.");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+            rc = libxl__xs_write_checked(gc, xt, dom_role_path, "master");
+            if (rc) goto out;;
+
+            libxl__xs_mknod(gc, xt, sshm_path, &noperm, 1);
+            libxl__xs_writev(gc, xt, sshm_path, ents);
+        } else {
+            SSHM_ERROR(domid, sshm->id, "can only have one master.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
+}
+
+int libxl__sshm_add(libxl__gc *gc,  uint32_t domid,
+                    libxl_static_shm *sshms, int len)
+{
+    int rc, i;
+
+    for (i = 0; i < len; ++i) {
+        if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE) {
+            rc = libxl__sshm_add_slave(gc, domid, sshms+i);
+        } else {
+            rc = libxl__sshm_add_master(gc, domid, sshms+i);
+        }
+        if (rc)  return rc;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

tools/libxl/libxl_x86.c +12 -0

index 5f91fe4f92..8ec5044eb2 100644
@@ -596,6 +596,25 @@ void libxl__arch_domain_build_info_acpi_setdefault(
      libxl_defbool_setdefault(&b_info->acpi, true);
  }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    /* FIXME: Mark this as unsupported for calling p2m_add_foreign on two
+     * DomU's is currently not allowd on x86, see the comments in
+     * x86/mm/p2m.c: p2m_add_foreign.
+     */
+     return false;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)

Hi,

Quoted 4 lines · Zhongze Liu

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping
process involves the follwing steps:

Quoted 7 lines

tools/libxl/libxl_sshm.c new file +7 -0

+
+/* Set default values for libxl_static_shm */
+int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                           libxl_static_shm *sshm)
+{
+    int rc;
+

tools/libxl/libxl_sshm.c line 20

Quoted 4 lines

tools/libxl/libxl_sshm.c new file +4 -0

+    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN)
+        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
+    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN)
+        sshm->prot = LIBXL_SSHM_PROT_RW;

What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?

Quoted 6 lines

tools/libxl/libxl_sshm.c new file +5 -0

+
+/*   libxl__sshm_do_map -- map pages into slave's physmap
+ *
+ *   This functions maps
+ *     master gfn: [@msshm->begin + @sshm->offset, @msshm->end +

@sshm->offset)

Quoted 8 lines

+ * into
+ * slave gfn: [@sshm->begin, @sshm->end)
+ *
+ * The gfns of the pages that are successfully mapped will be stored
+ * in @mapped, and the number of the gfns will be stored in @nmapped.
+ *
+ * The caller have to guarentee that sshm->begin < sshm->end and all
the

s/have to/has to/ I think.
s/guarentee/guarantee/

Quoted 1 line

+ * values are page-aligned.

Hmmm, I don't see the alignement check in libxl. So do you rely on the toolstack to do it?

Hi Julien,

2018-02-06 21:07 GMT+08:00 Julien Grall <[email protected]>:

Quoted 7 lines

Hi,

Quoted 4 lines · Zhongze Liu

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping
process involves the follwing steps:

[...]

Quoted 7 lines

Quoted 7 lines

tools/libxl/libxl_sshm.c new file +7 -0

+
+/* Set default values for libxl_static_shm */
+int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                           libxl_static_shm *sshm)
+{
+    int rc;
+

Quoted 8 lines

tools/libxl/libxl_sshm.c line 20

Quoted 4 lines

tools/libxl/libxl_sshm.c new file +4 -0

+    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN)
+        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
+    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN)
+        sshm->prot = LIBXL_SSHM_PROT_RW;

What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?

The *_UNKNOWN values are used by the libxlu code to check whether a specific option was set more than once.

Without the default *_UNKNOWN value, I will not
be able to judge if, say, role is set to 'slave' by the user or not, and therefore, if I
see the user setting role to 'master', I won't be able to tell if role is specified twice
or not.

I think treating re-specification of options as errors will be good for the users.

[...]

Quoted 19 lines

Quoted 6 lines

tools/libxl/libxl_sshm.c new file +5 -0

+
+/*   libxl__sshm_do_map -- map pages into slave's physmap
+ *
+ *   This functions maps
+ *     master gfn: [@msshm->begin + @sshm->offset, @msshm->end +

@sshm->offset)

Quoted 8 lines

+ * into
+ * slave gfn: [@sshm->begin, @sshm->end)
+ *
+ * The gfns of the pages that are successfully mapped will be stored
+ * in @mapped, and the number of the gfns will be stored in @nmapped.
+ *
+ * The caller have to guarentee that sshm->begin < sshm->end and all
the

s/have to/has to/ I think.
s/guarentee/guarantee/

Quoted 5 lines

Quoted 1 line

+ * values are page-aligned.

Hmmm, I don't see the alignement check in libxl. So do you rely on the toolstack to do it?

Yes, This was done in libxlu_sshm.c.

Quoted 1 line · Zhongze Liu

Hi Julien,

Hi,

Quoted 27 lines

2018-02-06 21:07 GMT+08:00 Julien Grall <[email protected]>:

Quoted 7 lines

Hi,

Quoted 4 lines · Zhongze Liu

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping
process involves the follwing steps:

[...]

Quoted 15 lines

tools/libxl/libxl_sshm.c line 20

Quoted 11 lines

tools/libxl/libxl_sshm.c new file +11 -0

+
+/* Set default values for libxl_static_shm */
+int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                           libxl_static_shm *sshm)
+{
+    int rc;
+
+    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN)
+        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
+    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN)
+        sshm->prot = LIBXL_SSHM_PROT_RW;

What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?

Quoted 2 lines

The *_UNKNOWN values are used by the libxlu code to check whether a specific option was set more than once.

AFAIK, a toolstack is free to not use libxlu. Someone may implement their own toolstack on top of libxl and may use ROLE_UNKNOWN by mistake.

Quoted 10 lines

Without the default *_UNKNOWN value, I will not
be able to judge if, say, role is set to 'slave' by the user or not, and therefore, if I
see the user setting role to 'master', I won't be able to tell if role is specified twice
or not.

I think treating re-specification of options as errors will be good for the users.

In that case, you should treat that as an error for everyone and not only xl. This would avoid confusion on other toolstack.

Quoted 25 lines

[...]

Quoted 22 lines

Quoted 6 lines

tools/libxl/libxl_sshm.c new file +5 -0

+
+/*   libxl__sshm_do_map -- map pages into slave's physmap
+ *
+ *   This functions maps
+ *     master gfn: [@msshm->begin + @sshm->offset, @msshm->end +

@sshm->offset)

Quoted 8 lines

+ * into
+ * slave gfn: [@sshm->begin, @sshm->end)
+ *
+ * The gfns of the pages that are successfully mapped will be stored
+ * in @mapped, and the number of the gfns will be stored in @nmapped.
+ *
+ * The caller have to guarentee that sshm->begin < sshm->end and all
the

s/have to/has to/ I think.
s/guarentee/guarantee/

Quoted 1 line

+ * values are page-aligned.

Quoted 4 lines

Quoted 2 lines

Hmmm, I don't see the alignement check in libxl. So do you rely on the toolstack to do it?

Yes, This was done in libxlu_sshm.c.

Same remark as above regarding libxlu. Note that I am maintaining the tools. Ian and Wei may have a different opinion here.

Quoted 35 lines · Zhongze Liu

Quoted 1 line

Hi Julien,

Hi,

Quoted 27 lines

2018-02-06 21:07 GMT+08:00 Julien Grall <[email protected]>:

Quoted 7 lines

Hi,

Quoted 4 lines · Zhongze Liu

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping
process involves the follwing steps:

[...]

Quoted 15 lines

tools/libxl/libxl_sshm.c line 20

Quoted 11 lines

tools/libxl/libxl_sshm.c new file +11 -0

+
+/* Set default values for libxl_static_shm */
+int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                           libxl_static_shm *sshm)
+{
+    int rc;
+
+    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN)
+        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
+    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN)
+        sshm->prot = LIBXL_SSHM_PROT_RW;

What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?

Quoted 5 lines

Quoted 2 lines

The *_UNKNOWN values are used by the libxlu code to check whether a specific option was set more than once.

AFAIK, a toolstack is free to not use libxlu. Someone may implement their own toolstack on top of libxl and may use ROLE_UNKNOWN by mistake.

Yes.

Quoted 42 lines

Quoted 10 lines

Without the default *_UNKNOWN value, I will not
be able to judge if, say, role is set to 'slave' by the user or not, and therefore, if I
see the user setting role to 'master', I won't be able to tell if role is specified twice
or not.

I think treating re-specification of options as errors will be good for the users.

In that case, you should treat that as an error for everyone and not only xl. This would avoid confusion on other toolstack.

Quoted 28 lines

[...]

Quoted 24 lines

Quoted 6 lines

tools/libxl/libxl_sshm.c new file +5 -0

+
+/*   libxl__sshm_do_map -- map pages into slave's physmap
+ *
+ *   This functions maps
+ *     master gfn: [@msshm->begin + @sshm->offset, @msshm->end +

@sshm->offset)

Quoted 8 lines

+ * into
+ * slave gfn: [@sshm->begin, @sshm->end)
+ *
+ * The gfns of the pages that are successfully mapped will be stored
+ * in @mapped, and the number of the gfns will be stored in @nmapped.
+ *
+ * The caller have to guarentee that sshm->begin < sshm->end and all
the

s/have to/has to/ I think.
s/guarentee/guarantee/

Quoted 1 line

+ * values are page-aligned.

Hmmm, I don't see the alignement check in libxl. So do you rely on the toolstack to do it?

Quoted 5 lines

Quoted 1 line

Yes, This was done in libxlu_sshm.c.

Same remark as above regarding libxlu. Note that I am maintaining the tools. Ian and Wei may have a different opinion here.

Please move the check to libxl.

Quoted 27 lines

Quoted 27 lines

Quoted 27 lines

[...]

Quoted 24 lines

Quoted 6 lines

tools/libxl/libxl_sshm.c new file +5 -0

+
+/*   libxl__sshm_do_map -- map pages into slave's physmap
+ *
+ *   This functions maps
+ *     master gfn: [@msshm->begin + @sshm->offset, @msshm->end +

@sshm->offset)

Quoted 8 lines

+ * into
+ * slave gfn: [@sshm->begin, @sshm->end)
+ *
+ * The gfns of the pages that are successfully mapped will be stored
+ * in @mapped, and the number of the gfns will be stored in @nmapped.
+ *
+ * The caller have to guarentee that sshm->begin < sshm->end and all
the

s/have to/has to/ I think.
s/guarentee/guarantee/

Quoted 1 line

+ * values are page-aligned.

Hmmm, I don't see the alignement check in libxl. So do you rely on the toolstack to do it?

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Yes, This was done in libxlu_sshm.c.

Same remark as above regarding libxlu. Note that I am maintaining the tools. Ian and Wei may have a different opinion here.

Please move the check to libxl.

I agree that we should move the alignment check to libxl.

But I still think that re-specification checks could only be done in libxlu, because
this could only be spotted in the parsing phase -- it shouldn't be libxl's job. For
libxl, an *_UNKNOWN values just indicates that the user of libxl didn't explicitly
assign a value to the option upon calling the constructor of the sshm struct, and the code in libxl will set the option to its default value. However, I do think
I failed to handle the possibilities where an option value is not not *_UNKNOWN and
not any valid values, which might occur if the user of libxl didn't use the constructor
to initialize the sshm struct.

Add libxl__sshm_del to unmap static shared memory areas mapped by libxl__sshm_add during domain creation. The unmapping process is:

* For a master: decrease the refcount of the sshm region, if the refcount reaches 0, cleanup the whole sshm path.
* For a slave: 1) unmap the shared pages, and cleanup related xs entries. If the system works normally, all the shared pages will be unmapped, so there won't be page leaks. In case of errors, the unmapping process will go on and unmap all the other pages that can be unmapped, so the other pages won't be leaked, either. 2) Decrease the refcount of the sshm region, if the refcount reaches 0, cleanup the whole sshm path.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Wei Liu <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
  tools/libxl/libxl_domain.c   |   5 ++
  tools/libxl/libxl_internal.h |   2 +
  tools/libxl/libxl_sshm.c     | 106 +++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 113 insertions(+)

tools/libxl/libxl_domain.c +5 -0

index 13b1c73d40..37f001554b 100644
@@ -1026,6 +1026,11 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
          goto out;
      }
 
+    rc = libxl__sshm_del(gc, domid);
+    if (rc) {
+        LOGD(ERROR, domid, "Deleting static shm failed.");
+    }
+
      if (libxl__device_pci_destroy_all(gc, domid) < 0)
          LOGD(ERROR, domid, "Pci shutdown failed");
      rc = xc_domain_pause(ctx->xch, domid);

tools/libxl/libxl_internal.h +2 -0

index 2cfe4c08a7..c398b6a6b8 100644
@@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
  _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
                              libxl_static_shm *sshm, int len);
 
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
  _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
                                        libxl_static_shm *sshms, int len);
  _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,

tools/libxl/libxl_sshm.c +0 -0

index 562f46f299..1bf4d4c2dc 100644

tools/libxl/libxl_sshm.c +10 -0

+ * clean up the whole sshm path.
+ */
+static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
+                               const char *sshm_path)
+{
+    int count;
+    const char *count_path, *count_string;
+
+    count_path = GCSPRINTF("%s/usercnt", sshm_path);
+    if (libxl__xs_read_checked(gc, xt, count_path, &count_string))

tools/libxl/libxl_sshm.c +24 -0

+
+    return;
+}
+
+static void libxl__sshm_do_unmap(libxl__gc *gc, uint32_t domid, const char *id,
+                                 uint64_t begin, uint64_t end)
+{
+    begin >>= XC_PAGE_SHIFT;
+    end >>= XC_PAGE_SHIFT;
+    for (; begin < end; ++begin) {
+        if (xc_domain_remove_from_physmap(CTX->xch, domid, begin)) {
+            SSHM_ERROR(domid, id,
+                       "unable to unmap shared page at 0x%"PRIx64".",
+                       begin);
+        }
+    }
+}
+
+static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
+                                  uint32_t domid, const char *id, bool isretry)
+{
+    const char *slave_path, *begin_str, *end_str;
+    uint64_t begin, end;
+

tools/libxl/libxl_sshm.c +17 -0

+
+    libxl__xs_path_cleanup(gc, xt, slave_path);
+}
+
+/* Delete static_shm entries in the xensotre. */
+int libxl__sshm_del(libxl__gc *gc,  uint32_t domid)
+{
+    int rc, i;
+    bool isretry;
+    xs_transaction_t xt = XBT_NULL;
+    const char *dom_path, *dom_sshm_path, *role;
+    char **sshm_ents;
+    unsigned int sshm_num;
+
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    dom_sshm_path = GCSPRINTF("%s/static_shm", dom_path);
+

tools/libxl/libxl_sshm.c +17 -0

+                role = libxl__xs_read(gc, xt,
+                                      GCSPRINTF("%s/%s/role",
+                                                dom_sshm_path,
+                                                sshm_ents[i]));
+                assert(role);
+                if (!strncmp(role, "slave", 5))
+                    libxl__sshm_del_slave(gc, xt, domid, sshm_ents[i], isretry);
+
+                libxl__sshm_decref(gc, xt, SSHM_PATH(sshm_ents[i]));
+            }
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+        isretry = true;
+    }

Quoted 18 lines

      if (libxl__device_pci_destroy_all(gc, domid) < 0)
          LOGD(ERROR, domid, "Pci shutdown failed");
      rc = xc_domain_pause(ctx->xch, domid);

tools/libxl/libxl_internal.h +2 -0

index 2cfe4c08a7..c398b6a6b8 100644
@@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
  _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
                              libxl_static_shm *sshm, int len);
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
  _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
                                        libxl_static_shm *sshms, int len);
  _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,

tools/libxl/libxl_sshm.c +0 -1

index 562f46f299..1bf4d4c2dc 100644
--- a/tools/libxl/libxl_sshm.c

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 88

Quoted 5 lines

+++ b/tools/libxl/libxl_sshm.c

tools/libxl/libxl_sshm.c +1 -0

@@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
      return 0;
  }
+/* Decrease the refcount of an sshm. When refcount reaches 0,

NIT: Libxl coding style regarding the comment seems to be uncleared (Ian, Wei?). But I feel keep /* and */ in separate line is nicer.

Quoted 11 lines

tools/libxl/libxl_sshm.c +10 -0

+ * clean up the whole sshm path.
+ */
+static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
+                               const char *sshm_path)
+static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
+                                  uint32_t domid, const char *id, bool isretry)
+{
+    const char *slave_path, *begin_str, *end_str;
+    uint64_t begin, end;
+

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 143

Quoted 10 lines

tools/libxl/libxl_sshm.c +10 -0

+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), domid);
+
+    begin_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/begin", slave_path));
+    end_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/end", slave_path));
+    begin = strtoull(begin_str, NULL, 16);
+    end = strtoull(end_str, NULL, 16);
+
+    /* Avoid calling do_unmap many times in case of xs transaction retry */
+    if (!isretry)
+        libxl__sshm_do_unmap(gc, domid, id, begin, end);

IHMO, by unmapping the regions in middle of the transaction, you increase the potential failure of it. I would move that out of the transaction path.

I would be interested to hear the opinion of the tools maintainers here.

Quoted 18 lines

Quoted 18 lines

      if (libxl__device_pci_destroy_all(gc, domid) < 0)
          LOGD(ERROR, domid, "Pci shutdown failed");
      rc = xc_domain_pause(ctx->xch, domid);

tools/libxl/libxl_internal.h +2 -0

index 2cfe4c08a7..c398b6a6b8 100644
@@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
  _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
                              libxl_static_shm *sshm, int len);
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
  _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
                                        libxl_static_shm *sshms, int len);
  _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,

tools/libxl/libxl_sshm.c +0 -1

index 562f46f299..1bf4d4c2dc 100644
--- a/tools/libxl/libxl_sshm.c

Quoted 8 lines

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 88

Quoted 5 lines

+++ b/tools/libxl/libxl_sshm.c

tools/libxl/libxl_sshm.c +1 -0

@@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
      return 0;
  }
+/* Decrease the refcount of an sshm. When refcount reaches 0,

NIT: Libxl coding style regarding the comment seems to be uncleared (Ian, Wei?). But I feel keep /* and */ in separate line is nicer.

I don't have an opinion here.

Quoted 25 lines

Quoted 11 lines

tools/libxl/libxl_sshm.c +10 -0

+ * clean up the whole sshm path.
+ */
+static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
+                               const char *sshm_path)
+static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
+                                  uint32_t domid, const char *id, bool isretry)
+{
+    const char *slave_path, *begin_str, *end_str;
+    uint64_t begin, end;
+

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 143

Quoted 10 lines

tools/libxl/libxl_sshm.c +10 -0

+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), domid);
+
+    begin_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/begin", slave_path));
+    end_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/end", slave_path));
+    begin = strtoull(begin_str, NULL, 16);
+    end = strtoull(end_str, NULL, 16);
+
+    /* Avoid calling do_unmap many times in case of xs transaction retry */
+    if (!isretry)
+        libxl__sshm_do_unmap(gc, domid, id, begin, end);

IHMO, by unmapping the regions in middle of the transaction, you increase the potential failure of it. I would move that out of the transaction path.

Quoted 2 lines

I would be interested to hear the opinion of the tools maintainers here.

If you move the unmap after the loop you create a window in which the pages are still mapped but the toolstack thinks they are unmapped.

While the code as-is now makes sure (assuming no error in unmap) the pages are unmapped no later than the transaction is committed. I think this can be done by moving unmap before the transaction.

Zhongze, do you think the unmap must be done inside the loop? What kind of invariants do you have in mind?

Then there is the question of "what do we do if unmap fails". Honestly I don't have an answer. It seems rather screwed up in that case and I doubt there is much libxl can do to rectify things.

Hi Wei and Julien,

2018-02-07 2:06 GMT+08:00 Wei Liu <[email protected]>:

Quoted 65 lines · Julien Grall

Quoted 18 lines

Quoted 18 lines

       if (libxl__device_pci_destroy_all(gc, domid) < 0)
           LOGD(ERROR, domid, "Pci shutdown failed");
       rc = xc_domain_pause(ctx->xch, domid);

tools/libxl/libxl_internal.h +2 -0

index 2cfe4c08a7..c398b6a6b8 100644
@@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
   _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
                               libxl_static_shm *sshm, int len);
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
   _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
                                         libxl_static_shm *sshms, int len);
   _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,

tools/libxl/libxl_sshm.c +0 -1

index 562f46f299..1bf4d4c2dc 100644
--- a/tools/libxl/libxl_sshm.c

Quoted 8 lines

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 88

Quoted 5 lines

+++ b/tools/libxl/libxl_sshm.c

tools/libxl/libxl_sshm.c +1 -0

@@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
       return 0;
   }
+/* Decrease the refcount of an sshm. When refcount reaches 0,

NIT: Libxl coding style regarding the comment seems to be uncleared (Ian, Wei?). But I feel keep /* and */ in separate line is nicer.

I don't have an opinion here.

Quoted 25 lines

Quoted 11 lines

tools/libxl/libxl_sshm.c +10 -0

+ * clean up the whole sshm path.
+ */
+static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
+                               const char *sshm_path)
+static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
+                                  uint32_t domid, const char *id, bool isretry)
+{
+    const char *slave_path, *begin_str, *end_str;
+    uint64_t begin, end;
+

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 143

Quoted 10 lines

tools/libxl/libxl_sshm.c +10 -0

+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), domid);
+
+    begin_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/begin", slave_path));
+    end_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/end", slave_path));
+    begin = strtoull(begin_str, NULL, 16);
+    end = strtoull(end_str, NULL, 16);
+
+    /* Avoid calling do_unmap many times in case of xs transaction retry */
+    if (!isretry)
+        libxl__sshm_do_unmap(gc, domid, id, begin, end);

IHMO, by unmapping the regions in middle of the transaction, you increase the potential failure of it. I would move that out of the transaction path.

Quoted 2 lines

I would be interested to hear the opinion of the tools maintainers here.

If you move the unmap after the loop you create a window in which the pages are still mapped but the toolstack thinks they are unmapped.

While the code as-is now makes sure (assuming no error in unmap) the pages are unmapped no later than the transaction is committed. I think this can be done by moving unmap before the transaction.

Quoted 7 lines

Zhongze, do you think the unmap must be done inside the loop? What kind of invariants do you have in mind?

Then there is the question of "what do we do if unmap fails". Honestly I don't have an answer. It seems rather screwed up in that case and I doubt there is much libxl can do to rectify things.

I put the unmap inside the transaction because I want to make the whole read_mapping_info->unmap->update_mapping_info process atomic. If I put unmap outside the transaction: after I read out the information that I need to do the unmap, and before I do the unmap and decrease the refcnt, there could be another instance of this code trying to do the same thing, which might lead to race condition.

Quoted 2 lines

Hi Wei and Julien,

Hi,

Quoted 89 lines

2018-02-07 2:06 GMT+08:00 Wei Liu <[email protected]>:

Quoted 80 lines · Julien Grall

Quoted 25 lines

Quoted 25 lines

       if (libxl__device_pci_destroy_all(gc, domid) < 0)
           LOGD(ERROR, domid, "Pci shutdown failed");
       rc = xc_domain_pause(ctx->xch, domid);

tools/libxl/libxl_internal.h +2 -0

index 2cfe4c08a7..c398b6a6b8 100644
@@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char
 **s)
   _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
                               libxl_static_shm *sshm, int len);
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
   _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
                                         libxl_static_shm *sshms, int
 len);

_hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,

tools/libxl/libxl_sshm.c +0 -0

index 562f46f299..1bf4d4c2dc 100644
@@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc,
 uint32_t domid,

Quoted 8 lines

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 89

Quoted 3 lines

tools/libxl/libxl_sshm.c +1 -0

       return 0;
   }
+/* Decrease the refcount of an sshm. When refcount reaches 0,

NIT: Libxl coding style regarding the comment seems to be uncleared (Ian,
Wei?). But I feel keep /* and */ in separate line is nicer.

I don't have an opinion here.

Quoted 31 lines

Quoted 22 lines

tools/libxl/libxl_sshm.c +18 -0

+ * clean up the whole sshm path.
+ */
+static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
+                               const char *sshm_path)
+static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
+                                  uint32_t domid, const char *id, bool
 isretry)
+{
+    const char *slave_path, *begin_str, *end_str;
+    uint64_t begin, end;
+
+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), domid);
+
+    begin_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/begin",
 slave_path));
+    end_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/end", slave_path));
+    begin = strtoull(begin_str, NULL, 16);
+    end = strtoull(end_str, NULL, 16);
+
+    /* Avoid calling do_unmap many times in case of xs transaction

retry */

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 143

Quoted 2 lines

tools/libxl/libxl_sshm.c +2 -0

+    if (!isretry)
+        libxl__sshm_do_unmap(gc, domid, id, begin, end);

IHMO, by unmapping the regions in middle of the transaction, you increase
the potential failure of it. I would move that out of the transaction path.

Quoted 2 lines

I would be interested to hear the opinion of the tools maintainers here.

If you move the unmap after the loop you create a window in which the pages are still mapped but the toolstack thinks they are unmapped.

While the code as-is now makes sure (assuming no error in unmap) the pages are unmapped no later than the transaction is committed. I think this can be done by moving unmap before the transaction.

Quoted 7 lines

Zhongze, do you think the unmap must be done inside the loop? What kind of invariants do you have in mind?

Then there is the question of "what do we do if unmap fails". Honestly I don't have an answer. It seems rather screwed up in that case and I doubt there is much libxl can do to rectify things.

Quoted 6 lines

I put the unmap inside the transaction because I want to make the whole read_mapping_info->unmap->update_mapping_info process atomic. If I put unmap outside the transaction: after I read out the information that I need to do the unmap, and before I do the unmap and decrease the refcnt, there could be another instance of this code trying to do the same thing, which might lead to race condition.

AFAIU, the transaction is not a "global" lock. You will just not see the the change from the others during the transactions. Your changes will be only be visible at the end. So two transaction can be happily started concurrently, and try to do the unmap together. Not even your code will protect against that.

So can you give a bit more details here?

2018-02-08 0:54 GMT+08:00 Julien Grall <[email protected]>:

Quoted 1 line · Zhongze Liu

It seems that I mistakenly use transaction as a global lock. Now I don't have any reasons not putting the unmap out of the transaction, but this will break the original transaction into two, and I do think that we need some explicit locking here.

Hi,

Quoted 5 lines · Zhongze Liu

2018-02-08 0:54 GMT+08:00 Julien Grall <[email protected]>:

Quoted 2 lines · Zhongze Liu

Quoted 7 lines

It seems that I mistakenly use transaction as a global lock. Now I don't have
any reasons not putting the unmap out of the transaction, but this will break
the original transaction into two, and I do think that we need some explicit
locking here.

Can you explain why you need specific locking here? What are you trying to protect? Are you trying to protect against two process doing the unmap?

Hi Julien,

2018-02-12 23:09 GMT+08:00 Julien Grall <[email protected]>:

Quoted 17 lines

Hi,

Quoted 12 lines · Zhongze Liu

2018-02-08 0:54 GMT+08:00 Julien Grall <[email protected]>:

Quoted 2 lines · Zhongze Liu

It seems that I mistakenly use transaction as a global lock. Now I don't have
any reasons not putting the unmap out of the transaction, but this will break
the original transaction into two, and I do think that we need some explicit
locking here.

Quoted 3 lines

Can you explain why you need specific locking here? What are you trying to protect? Are you trying to protect against two process doing the unmap?

Yes.

Quoted 1 line · Zhongze Liu

Hi Julien,

Hi,

Quoted 19 lines

2018-02-12 23:09 GMT+08:00 Julien Grall <[email protected]>:

Quoted 17 lines

Hi,

Quoted 12 lines · Zhongze Liu

2018-02-08 0:54 GMT+08:00 Julien Grall <[email protected]>:

Quoted 2 lines · Zhongze Liu

It seems that I mistakenly use transaction as a global lock. Now I don't have
any reasons not putting the unmap out of the transaction, but this will break
the original transaction into two, and I do think that we need some explicit
locking here.

Quoted 5 lines

Quoted 3 lines

Can you explain why you need specific locking here? What are you trying to protect? Are you trying to protect against two process doing the unmap?

Yes.

I don't think you have to worry about the locking here. With the current interface, the regions cannot be modified after the guest has booted. So the addresses will always stay valid.

This code path should never be called concurrently, as a lot of code in libxl, so I think someone else will take care about that for you (I will let Wei confirm here).

Quoted 1 line · Zhongze Liu

Hi Julien,

Hi,

Quoted 19 lines

2018-02-12 23:09 GMT+08:00 Julien Grall <[email protected]>:

Quoted 17 lines

Hi,

Quoted 12 lines · Zhongze Liu

2018-02-08 0:54 GMT+08:00 Julien Grall <[email protected]>:

Quoted 2 lines · Zhongze Liu

It seems that I mistakenly use transaction as a global lock. Now I don't have
any reasons not putting the unmap out of the transaction, but this will break
the original transaction into two, and I do think that we need some explicit
locking here.

Quoted 5 lines

Quoted 3 lines

Can you explain why you need specific locking here? What are you trying to protect? Are you trying to protect against two process doing the unmap?

Yes.

I don't think you have to worry about the locking here. With the current interface, the regions cannot be modified after the guest has booted. So the addresses will always stay valid.

This code path should never be called concurrently, as a lot of code in libxl, so I think someone else will take care about that for you (I will let Wei confirm here).

In any case, the worst that could happen is the unmap is called twice on the same region. So you would get spurious error message. Not that bad.

Quoted 2 lines

In any case, the worst that could happen is the unmap is called twice on the same region. So you would get spurious error message. Not that bad.

Yeah, not that bad. Not going to be a security issue, not going to leak resources in the end.

To avoid spurious unmap, can we maybe unmap the pages after the xenstore transaction is committed? In that case, only the successful one gets to unmap, the ones that aren't committed will bail.

(Just tossing around ideas)