* [PATCH v3 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap
2017-10-19 2:36 [PATCH v3 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
@ 2017-10-19 2:36 ` Zhongze Liu
2017-10-31 12:40 ` Wei Liu
2017-10-19 2:36 ` [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin Zhongze Liu
` (5 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-10-19 2:36 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
This is for the proposal "Allow setting up shared memory areas between VMs
from xl config file". See:
https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
Then plan is to use XENMEM_add_to_physmap_batch to map the shared pages from
one domU to another and use XENMEM_remove_from_physmap to cancel the sharing.
A wrapper to XENMEM_add_to_physmap_batch was added in the following commit:
commit 20e725e9364cff4a29945f66986ecd88cca8743d
Now add the wrapper to XENMEM_remove_from_physmap.
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
tools/libxc/include/xenctrl.h | 4 ++++
tools/libxc/xc_domain.c | 11 +++++++++++
2 files changed, 15 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 666db0b919..0d8364ea4b 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1415,6 +1415,10 @@ int xc_domain_add_to_physmap_batch(xc_interface *xch,
xen_pfn_t *gfpns,
int *errs);
+int xc_domain_remove_from_physmap(xc_interface *xch,
+ domid_t domid,
+ xen_pfn_t gpfn);
+
int xc_domain_populate_physmap(xc_interface *xch,
uint32_t domid,
unsigned long nr_extents,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 3ccd27f101..b8a9ff87e9 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1104,6 +1104,17 @@ out:
return rc;
}
+int xc_domain_remove_from_physmap(xc_interface *xch,
+ domid_t domid,
+ xen_pfn_t gpfn)
+{
+ struct xen_remove_from_physmap xrfp = {
+ .domid = domid,
+ .gpfn = gpfn,
+ };
+ return do_memory_op(xch, XENMEM_remove_from_physmap, &xrfp, sizeof(xrfp));
+}
+
int xc_domain_claim_pages(xc_interface *xch,
uint32_t domid,
unsigned long nr_pages)
--
2.14.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap
2017-10-19 2:36 ` [PATCH v3 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
@ 2017-10-31 12:40 ` Wei Liu
0 siblings, 0 replies; 29+ messages in thread
From: Wei Liu @ 2017-10-31 12:40 UTC (permalink / raw)
To: Zhongze Liu
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On Thu, Oct 19, 2017 at 10:36:29AM +0800, Zhongze Liu wrote:
> This is for the proposal "Allow setting up shared memory areas between VMs
> from xl config file". See:
>
> https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
>
> Then plan is to use XENMEM_add_to_physmap_batch to map the shared pages from
> one domU to another and use XENMEM_remove_from_physmap to cancel the sharing.
> A wrapper to XENMEM_add_to_physmap_batch was added in the following commit:
>
> commit 20e725e9364cff4a29945f66986ecd88cca8743d
>
> Now add the wrapper to XENMEM_remove_from_physmap.
>
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: xen-devel@lists.xen.org
> ---
> tools/libxc/include/xenctrl.h | 4 ++++
> tools/libxc/xc_domain.c | 11 +++++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 666db0b919..0d8364ea4b 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1415,6 +1415,10 @@ int xc_domain_add_to_physmap_batch(xc_interface *xch,
> xen_pfn_t *gfpns,
> int *errs);
>
> +int xc_domain_remove_from_physmap(xc_interface *xch,
> + domid_t domid,
We recently made changes to use uint32_t for domid.
You can keep my ack after the change.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-19 2:36 [PATCH v3 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
2017-10-19 2:36 ` [PATCH v3 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
@ 2017-10-19 2:36 ` Zhongze Liu
2017-10-19 11:58 ` Jan Beulich
2017-10-19 17:36 ` Daniel De Graaf
2017-10-19 2:36 ` [PATCH v3 3/7] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
` (4 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Zhongze Liu @ 2017-10-19 2:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Zhongze Liu, Ian Jackson,
Julien Grall, Daniel De Graaf
The original dummy xsm_map_gmfn_foregin checks if source domain has the proper
privileges over the target domain. Under this policy, it's not allowed if a Dom0
wants to map pages from one DomU to another, which restricts some useful yet not
dangerous use cases of the API, such as sharing pages among DomU's by calling
XENMEM_add_to_physmap from Dom0.
For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current domain
has the proper privileges on (d) and (t), grant the access.
For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to denote if
two domains can share memory through map_gmfn_foregin. 2) Change to hook to
grant the access IFF the current domain has proper MMU privileges on (d) and (t),
and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default xen.te
to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event
channels.
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 <blackskygg@gmail.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
V3:
* Change several if statements to the GCC '... = a ?: b' extention.
* lookup the current domain in the hooks instead of passing it as an arg
---
tools/flask/policy/modules/xen.if | 2 ++
xen/include/xsm/dummy.h | 3 ++-
xen/xsm/flask/hooks.c | 4 +++-
xen/xsm/flask/policy/access_vectors | 4 ++++
4 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 55437496f6..3ffd1c6239 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -127,6 +127,8 @@ define(`domain_comms', `
domain_event_comms($1, $2)
allow $1 $2:grant { map_read map_write copy unmap };
allow $2 $1:grant { map_read map_write copy unmap };
+ allow $1 $2:mmu share_mem;
+ allow $2 $1:mmu share_mem;
')
# domain_self_comms(domain)
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index b2cd56cdc5..65e7060ad5 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -516,7 +516,8 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
{
XSM_ASSERT_ACTION(XSM_TARGET);
- return xsm_default_action(action, d, t);
+ return xsm_default_action(action, current->domain, d) ?:
+ xsm_default_action(action, current->domain, t);
}
static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index f01b4cfaaa..16103bafc9 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
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);
+ return domain_has_perm(current->domain, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
+ domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
+ domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
}
static int flask_hvm_param(struct domain *d, unsigned long op)
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 3a2d863b8f..a0330f914a 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -387,6 +387,10 @@ class mmu
# Allow a privileged domain to install a map of a page it does not own. Used
# for stub domain device models with the PV framebuffer.
target_hack
+# Checked when using map_gmfn_foreign to share memory:
+# source = domain whose memory is being shared
+# target = client domain
+ share_mem
}
# control of the paging_domctl split by subop
--
2.14.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-19 2:36 ` [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin Zhongze Liu
@ 2017-10-19 11:58 ` Jan Beulich
2017-10-19 17:36 ` Daniel De Graaf
2017-10-19 17:36 ` Daniel De Graaf
1 sibling, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2017-10-19 11:58 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 19.10.17 at 04:36, <blackskygg@gmail.com> wrote:
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -516,7 +516,8 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
> {
> XSM_ASSERT_ACTION(XSM_TARGET);
> - return xsm_default_action(action, d, t);
> + return xsm_default_action(action, current->domain, d) ?:
> + xsm_default_action(action, current->domain, t);
> }
When all three domains are different, how does the changed
policy reflect the original "d has privilege over t" requirement?
I understand you want to relax the current condition, but this
shouldn't come at the price of granting access when access
should be denied. Nor the inverse - the current domain not
having privilege over both does also not mean d doesn't have
the necessary privilege over t.
I continue to think that you can't validly retrofit the new
intended functionality onto the existing hypercall, even if
nothing except the permission check needs to be different.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-19 11:58 ` Jan Beulich
@ 2017-10-19 17:36 ` Daniel De Graaf
2017-10-20 6:14 ` Jan Beulich
0 siblings, 1 reply; 29+ messages in thread
From: Daniel De Graaf @ 2017-10-19 17:36 UTC (permalink / raw)
To: Jan Beulich, Zhongze Liu
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 10/19/2017 07:58 AM, Jan Beulich wrote:
>>>> On 19.10.17 at 04:36, <blackskygg@gmail.com> wrote:
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -516,7 +516,8 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
>> {
>> XSM_ASSERT_ACTION(XSM_TARGET);
>> - return xsm_default_action(action, d, t);
>> + return xsm_default_action(action, current->domain, d) ?:
>> + xsm_default_action(action, current->domain, t);
>> }
>
> When all three domains are different, how does the changed
> policy reflect the original "d has privilege over t" requirement?
> I understand you want to relax the current condition, but this
> shouldn't come at the price of granting access when access
> should be denied. Nor the inverse - the current domain not
> having privilege over both does also not mean d doesn't have
> the necessary privilege over t.
>
> I continue to think that you can't validly retrofit the new
> intended functionality onto the existing hypercall, even if
> nothing except the permission check needs to be different.
>
> Jan
If this operation is going to be allowed at all (and I agree it has
valid use cases), then there won't be a privilege relationship between
(d) and (t) to check - they'll both be (somewhat related) domUs as far
as Xen can tell. If this hypercall isn't used, adding a new hypercall
(subop) is the only way I'd see to do it - and that seems very redundant
as it'd need to do all the same checks except for the one about the
relationship between (d) and (t). I don't see the reason why the
existing hypercall should deny being used for that purpose once it's
possible using other means.
The only possible problem that springs to mind is a restricted kernel
interface (such as the one used by QEMU in dom0 that restricts to a
single target domain) that now doesn't realize it's relaying an
operation that also requires permission over (t) after only checking
that the origin is allowed to modify (d).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-19 17:36 ` Daniel De Graaf
@ 2017-10-20 6:14 ` Jan Beulich
2017-10-20 13:34 ` Daniel De Graaf
0 siblings, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2017-10-20 6:14 UTC (permalink / raw)
To: Zhongze Liu, Daniel De Graaf
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
>>> On 19.10.17 at 19:36, <dgdegra@tycho.nsa.gov> wrote:
> On 10/19/2017 07:58 AM, Jan Beulich wrote:
>>>>> On 19.10.17 at 04:36, <blackskygg@gmail.com> wrote:
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -516,7 +516,8 @@ static XSM_INLINE int
> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
> *d, struct domain *t)
>>> {
>>> XSM_ASSERT_ACTION(XSM_TARGET);
>>> - return xsm_default_action(action, d, t);
>>> + return xsm_default_action(action, current->domain, d) ?:
>>> + xsm_default_action(action, current->domain, t);
>>> }
>>
>> When all three domains are different, how does the changed
>> policy reflect the original "d has privilege over t" requirement?
>> I understand you want to relax the current condition, but this
>> shouldn't come at the price of granting access when access
>> should be denied. Nor the inverse - the current domain not
>> having privilege over both does also not mean d doesn't have
>> the necessary privilege over t.
>>
>> I continue to think that you can't validly retrofit the new
>> intended functionality onto the existing hypercall, even if
>> nothing except the permission check needs to be different.
>>
>> Jan
>
> If this operation is going to be allowed at all (and I agree it has
> valid use cases), then there won't be a privilege relationship between
> (d) and (t) to check - they'll both be (somewhat related) domUs as far
> as Xen can tell. If this hypercall isn't used, adding a new hypercall
> (subop) is the only way I'd see to do it - and that seems very redundant
> as it'd need to do all the same checks except for the one about the
> relationship between (d) and (t). I don't see the reason why the
> existing hypercall should deny being used for that purpose once it's
> possible using other means.
One problem is, as you mention here, ...
> The only possible problem that springs to mind is a restricted kernel
> interface (such as the one used by QEMU in dom0 that restricts to a
> single target domain) that now doesn't realize it's relaying an
> operation that also requires permission over (t) after only checking
> that the origin is allowed to modify (d).
... the delegation of privilege checking responsibility to a
possibly untrusted environment. Plus, as explained before,
current callers expect privilege of d over t to be validated,
which isn't happening anymore with the proposed change. If
the existing sub-op was to be modified, I think we'd need
(with c representing the current domain)
- (d over t) || ((c over d) && (c over t)) for not regressing
the pre-existing use case,
- only (c over d) && (c over t) for not permitting something
that isn't intended to be permitted in the new use case.
Unless the sub-op has room for adding a flag to indicate
which of the two is meant (I didn't check), I don't see a way
around adding another sub-op, no matter how similar this
would end up being.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-20 6:14 ` Jan Beulich
@ 2017-10-20 13:34 ` Daniel De Graaf
2017-10-22 11:21 ` Zhongze Liu
0 siblings, 1 reply; 29+ messages in thread
From: Daniel De Graaf @ 2017-10-20 13:34 UTC (permalink / raw)
To: Jan Beulich, Zhongze Liu
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 10/20/2017 02:14 AM, Jan Beulich wrote:
>>>> On 19.10.17 at 19:36, <dgdegra@tycho.nsa.gov> wrote:
>> On 10/19/2017 07:58 AM, Jan Beulich wrote:
>>>>>> On 19.10.17 at 04:36, <blackskygg@gmail.com> wrote:
>>>> --- a/xen/include/xsm/dummy.h
>>>> +++ b/xen/include/xsm/dummy.h
>>>> @@ -516,7 +516,8 @@ static XSM_INLINE int
>> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>>> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
>> *d, struct domain *t)
>>>> {
>>>> XSM_ASSERT_ACTION(XSM_TARGET);
>>>> - return xsm_default_action(action, d, t);
>>>> + return xsm_default_action(action, current->domain, d) ?:
>>>> + xsm_default_action(action, current->domain, t);
>>>> }
>>>
>>> When all three domains are different, how does the changed
>>> policy reflect the original "d has privilege over t" requirement?
>>> I understand you want to relax the current condition, but this
>>> shouldn't come at the price of granting access when access
>>> should be denied. Nor the inverse - the current domain not
>>> having privilege over both does also not mean d doesn't have
>>> the necessary privilege over t.
>>>
>>> I continue to think that you can't validly retrofit the new
>>> intended functionality onto the existing hypercall, even if
>>> nothing except the permission check needs to be different.
>>>
>>> Jan
>>
>> If this operation is going to be allowed at all (and I agree it has
>> valid use cases), then there won't be a privilege relationship between
>> (d) and (t) to check - they'll both be (somewhat related) domUs as far
>> as Xen can tell. If this hypercall isn't used, adding a new hypercall
>> (subop) is the only way I'd see to do it - and that seems very redundant
>> as it'd need to do all the same checks except for the one about the
>> relationship between (d) and (t). I don't see the reason why the
>> existing hypercall should deny being used for that purpose once it's
>> possible using other means.
>
> One problem is, as you mention here, ...
>
>> The only possible problem that springs to mind is a restricted kernel
>> interface (such as the one used by QEMU in dom0 that restricts to a
>> single target domain) that now doesn't realize it's relaying an
>> operation that also requires permission over (t) after only checking
>> that the origin is allowed to modify (d).
>
> ... the delegation of privilege checking responsibility to a
> possibly untrusted environment. Plus, as explained before,
> current callers expect privilege of d over t to be validated,
> which isn't happening anymore with the proposed change. If
> the existing sub-op was to be modified, I think we'd need
> (with c representing the current domain)
> - (d over t) || ((c over d) && (c over t)) for not regressing
> the pre-existing use case,
> - only (c over d) && (c over t) for not permitting something
> that isn't intended to be permitted in the new use case.
> Unless the sub-op has room for adding a flag to indicate
> which of the two is meant (I didn't check), I don't see a way
> around adding another sub-op, no matter how similar this
> would end up being.
>
> Jan
I would say the current lack of a check for (c over t) is an oversight,
which mostly doesn't matter because the ability to modify arbitrary
memory in your target is transitive in almost any security model (c can
modify d's code to modify t, so a malicious c can compromise t anyway).
If the three domains are all different, the only way this can happen in
non-XSM is for (c) to be dom0 or for your device model to have a device
model (which I don't think is forbidden, but doubt anyone uses).
I now agree that this deserves a new subop, since this code is reached
via the stable memory_op and not just a domctl.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-20 13:34 ` Daniel De Graaf
@ 2017-10-22 11:21 ` Zhongze Liu
2017-10-23 7:26 ` Jan Beulich
0 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-10-22 11:21 UTC (permalink / raw)
To: Daniel De Graaf, Jan Beulich
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
Hi Daniel and Jan,
2017-10-20 21:34 GMT+08:00 Daniel De Graaf <dgdegra@tycho.nsa.gov>:
> On 10/20/2017 02:14 AM, Jan Beulich wrote:
>>>>>
>>>>> On 19.10.17 at 19:36, <dgdegra@tycho.nsa.gov> wrote:
>>>
>>> On 10/19/2017 07:58 AM, Jan Beulich wrote:
>>>>>>>
>>>>>>> On 19.10.17 at 04:36, <blackskygg@gmail.com> wrote:
>>>>>
>>>>> --- a/xen/include/xsm/dummy.h
>>>>> +++ b/xen/include/xsm/dummy.h
>>>>> @@ -516,7 +516,8 @@ static XSM_INLINE int
>>>
>>> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>>>>
>>>>> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct
>>>>> domain
>>>
>>> *d, struct domain *t)
>>>>>
>>>>> {
>>>>> XSM_ASSERT_ACTION(XSM_TARGET);
>>>>> - return xsm_default_action(action, d, t);
>>>>> + return xsm_default_action(action, current->domain, d) ?:
>>>>> + xsm_default_action(action, current->domain, t);
>>>>> }
>>>>
>>>>
>>>> When all three domains are different, how does the changed
>>>> policy reflect the original "d has privilege over t" requirement?
>>>> I understand you want to relax the current condition, but this
>>>> shouldn't come at the price of granting access when access
>>>> should be denied. Nor the inverse - the current domain not
>>>> having privilege over both does also not mean d doesn't have
>>>> the necessary privilege over t.
>>>>
>>>> I continue to think that you can't validly retrofit the new
>>>> intended functionality onto the existing hypercall, even if
>>>> nothing except the permission check needs to be different.
>>>>
>>>> Jan
>>>
>>>
>>> If this operation is going to be allowed at all (and I agree it has
>>> valid use cases), then there won't be a privilege relationship between
>>> (d) and (t) to check - they'll both be (somewhat related) domUs as far
>>> as Xen can tell. If this hypercall isn't used, adding a new hypercall
>>> (subop) is the only way I'd see to do it - and that seems very redundant
>>> as it'd need to do all the same checks except for the one about the
>>> relationship between (d) and (t). I don't see the reason why the
>>> existing hypercall should deny being used for that purpose once it's
>>> possible using other means.
>>
>>
>> One problem is, as you mention here, ...
>>
>>> The only possible problem that springs to mind is a restricted kernel
>>> interface (such as the one used by QEMU in dom0 that restricts to a
>>> single target domain) that now doesn't realize it's relaying an
>>> operation that also requires permission over (t) after only checking
>>> that the origin is allowed to modify (d).
>>
>>
>> ... the delegation of privilege checking responsibility to a
>> possibly untrusted environment. Plus, as explained before,
>> current callers expect privilege of d over t to be validated,
>> which isn't happening anymore with the proposed change. If
>> the existing sub-op was to be modified, I think we'd need
>> (with c representing the current domain)
>> - (d over t) || ((c over d) && (c over t)) for not regressing
>> the pre-existing use case,
>> - only (c over d) && (c over t) for not permitting something
>> that isn't intended to be permitted in the new use case.
>> Unless the sub-op has room for adding a flag to indicate
>> which of the two is meant (I didn't check), I don't see a way
>> around adding another sub-op, no matter how similar this
>> would end up being.
>>
>> Jan
>
>
> I would say the current lack of a check for (c over t) is an oversight,
> which mostly doesn't matter because the ability to modify arbitrary
> memory in your target is transitive in almost any security model (c can
> modify d's code to modify t, so a malicious c can compromise t anyway).
> If the three domains are all different, the only way this can happen in
> non-XSM is for (c) to be dom0 or for your device model to have a device
> model (which I don't think is forbidden, but doubt anyone uses).
>
> I now agree that this deserves a new subop, since this code is reached
> via the stable memory_op and not just a domctl.
How about changing the policy to (c over d) && ((d over t) || (c over t))?
Given that (c over d) is a must, which is always checked somewhere higher
in the call stack as Daniel pointed out, permitting (d over t) or (c
over t) actually infers
permitting the other.
- if you permit (d over t) but not (c over t):
Given (c over t),
(c) can first map the src page from (t) into its own memory space and then map
this page from its own memory space to (d)'s memory space.
- if you permit (c over t) but not (d over t):
Given (d over t),
(c) can first map (d)'s pages into its own memory space and modify (d)'s code
to issues a hypercall that maps (t)'s memory pages into (d)'s memory space.
I'm not very familiar with Xen's security model. So I might be totally
wrong here.
If so, please correct me.
And if you still think adding a new subop is necessary, do you have
any suggestions
on this?
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-22 11:21 ` Zhongze Liu
@ 2017-10-23 7:26 ` Jan Beulich
2017-10-23 9:54 ` Zhongze Liu
0 siblings, 1 reply; 29+ messages in thread
From: Jan Beulich @ 2017-10-23 7:26 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 22.10.17 at 13:21, <blackskygg@gmail.com> wrote:
> How about changing the policy to (c over d) && ((d over t) || (c over t))?
> Given that (c over d) is a must, which is always checked somewhere higher
> in the call stack as Daniel pointed out, permitting (d over t) or (c
> over t) actually infers
> permitting the other.
>
> - if you permit (d over t) but not (c over t):
> Given (c over t),
> (c) can first map the src page from (t) into its own memory space and then map
> this page from its own memory space to (d)'s memory space.
Would that work? The page, when in (c)'s space, is still owned by (t),
so I don't see how mapping into (d)'s space could become possible
just because it's mapped into (c)'s.
> - if you permit (c over t) but not (d over t):
> Given (d over t),
> (c) can first map (d)'s pages into its own memory space and modify (d)'s code
> to issues a hypercall that maps (t)'s memory pages into (d)'s memory space.
I can buy this one (after having thought about it a little only for
now), albeit (c) modifying code in (d) is certainly something I'd call
abuse rather than use of permissions.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-23 7:26 ` Jan Beulich
@ 2017-10-23 9:54 ` Zhongze Liu
2017-10-25 9:37 ` Zhongze Liu
0 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-10-23 9:54 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
2017-10-23 15:26 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 22.10.17 at 13:21, <blackskygg@gmail.com> wrote:
>> How about changing the policy to (c over d) && ((d over t) || (c over t))?
>> Given that (c over d) is a must, which is always checked somewhere higher
>> in the call stack as Daniel pointed out, permitting (d over t) or (c
>> over t) actually infers
>> permitting the other.
>>
>> - if you permit (d over t) but not (c over t):
>> Given (c over t),
>> (c) can first map the src page from (t) into its own memory space and then map
>> this page from its own memory space to (d)'s memory space.
>
> Would that work? The page, when in (c)'s space, is still owned by (t),
> so I don't see how mapping into (d)'s space could become possible
> just because it's mapped into (c)'s.
Yes, indeed. This won't work. Sorry for giving a wrong example here.
I think I now agree to add a new subop, too.
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-23 9:54 ` Zhongze Liu
@ 2017-10-25 9:37 ` Zhongze Liu
2017-10-25 15:36 ` Zhongze Liu
2017-10-26 6:41 ` Jan Beulich
0 siblings, 2 replies; 29+ messages in thread
From: Zhongze Liu @ 2017-10-25 9:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
Hi,
My current plan is to add the following new MAPSPACE to public/memory.h:
+#define XENMEMSPACE_gmfn_foreign_share 6 /* Same as *_gmfn_foreign, but this is
+ for a privileged dom to
+ shared pages between two doms. */
and create a corresponding entry xsm_map_gmfn_foreign_share to the
xsm structure, which will be filled with
the proposed policy.
Does this look good to you?
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-25 9:37 ` Zhongze Liu
@ 2017-10-25 15:36 ` Zhongze Liu
2017-10-26 6:41 ` Jan Beulich
1 sibling, 0 replies; 29+ messages in thread
From: Zhongze Liu @ 2017-10-25 15:36 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
2017-10-25 17:37 GMT+08:00 Zhongze Liu <blackskygg@gmail.com>:
> Hi,
>
> My current plan is to add the following new MAPSPACE to public/memory.h:
>
> +#define XENMEMSPACE_gmfn_foreign_share 6 /* Same as *_gmfn_foreign, but this is
> + for a privileged dom to
> + shared pages between two doms. */
s/shared/share
>
> and create a corresponding entry xsm_map_gmfn_foreign_share to the
> xsm structure, which will be filled with
> the proposed policy.
>
> Does this look good to you?
>
> Cheers,
>
> Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-25 9:37 ` Zhongze Liu
2017-10-25 15:36 ` Zhongze Liu
@ 2017-10-26 6:41 ` Jan Beulich
1 sibling, 0 replies; 29+ messages in thread
From: Jan Beulich @ 2017-10-26 6:41 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 25.10.17 at 11:37, <blackskygg@gmail.com> wrote:
> My current plan is to add the following new MAPSPACE to public/memory.h:
>
> +#define XENMEMSPACE_gmfn_foreign_share 6 /* Same as *_gmfn_foreign, but this is
> + for a privileged dom to
> + shared pages between two doms. */
I think XENMEMSPACE_gmfn_share would suffice as a name.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-19 2:36 ` [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin Zhongze Liu
2017-10-19 11:58 ` Jan Beulich
@ 2017-10-19 17:36 ` Daniel De Graaf
2017-10-20 0:34 ` Zhongze Liu
1 sibling, 1 reply; 29+ messages in thread
From: Daniel De Graaf @ 2017-10-19 17:36 UTC (permalink / raw)
To: Zhongze Liu, xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu
On 10/18/2017 10:36 PM, Zhongze Liu wrote:
> The original dummy xsm_map_gmfn_foregin checks if source domain has the proper
> privileges over the target domain. Under this policy, it's not allowed if a Dom0
> wants to map pages from one DomU to another, which restricts some useful yet not
> dangerous use cases of the API, such as sharing pages among DomU's by calling
> XENMEM_add_to_physmap from Dom0.
>
> For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current domain
> has the proper privileges on (d) and (t), grant the access.
>
> For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to denote if
> two domains can share memory through map_gmfn_foregin. 2) Change to hook to
> grant the access IFF the current domain has proper MMU privileges on (d) and (t),
> and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default xen.te
> to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event
> channels.
>
> 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 <blackskygg@gmail.com>
>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: xen-devel@lists.xen.org
> ---
> V3:
> * Change several if statements to the GCC '... = a ?: b' extention.
> * lookup the current domain in the hooks instead of passing it as an arg
> ---
> tools/flask/policy/modules/xen.if | 2 ++
> xen/include/xsm/dummy.h | 3 ++-
> xen/xsm/flask/hooks.c | 4 +++-
> xen/xsm/flask/policy/access_vectors | 4 ++++
> 4 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
> index 55437496f6..3ffd1c6239 100644
> --- a/tools/flask/policy/modules/xen.if
> +++ b/tools/flask/policy/modules/xen.if
> @@ -127,6 +127,8 @@ define(`domain_comms', `
> domain_event_comms($1, $2)
> allow $1 $2:grant { map_read map_write copy unmap };
> allow $2 $1:grant { map_read map_write copy unmap };
> + allow $1 $2:mmu share_mem;
> + allow $2 $1:mmu share_mem;
> ')
>
> # domain_self_comms(domain)
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index b2cd56cdc5..65e7060ad5 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -516,7 +516,8 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
> {
> XSM_ASSERT_ACTION(XSM_TARGET);
> - return xsm_default_action(action, d, t);
> + return xsm_default_action(action, current->domain, d) ?:
> + xsm_default_action(action, current->domain, t);
> }
Same comment as below, the check between (current->domain) and (d) should
be redundant with one higher up in the call stack. The check between
(current->domain) and (t) should remain, although this *does* result in a
relaxing of the existing permission checks on the call as Jan noted.
> static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index f01b4cfaaa..16103bafc9 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
>
> 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);
> + return domain_has_perm(current->domain, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
> + domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
> + domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
> }
This is at least partially redundant with the higher-level permission checks
needed to get to the xenmem_add_* functions (xatp_permission_check call in
xen/common/memory.c, for example). That check already verifies the permission
for (current->domain) to modify (d)'s page tables.
The other two checks here look correct.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-19 17:36 ` Daniel De Graaf
@ 2017-10-20 0:34 ` Zhongze Liu
2017-10-20 0:55 ` Zhongze Liu
0 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-10-20 0:34 UTC (permalink / raw)
To: Daniel De Graaf
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
Hi Daniel,
2017-10-20 1:36 GMT+08:00 Daniel De Graaf <dgdegra@tycho.nsa.gov>:
> On 10/18/2017 10:36 PM, Zhongze Liu wrote:
>>
>> The original dummy xsm_map_gmfn_foregin checks if source domain has the
>> proper
>> privileges over the target domain. Under this policy, it's not allowed if
>> a Dom0
>> wants to map pages from one DomU to another, which restricts some useful
>> yet not
>> dangerous use cases of the API, such as sharing pages among DomU's by
>> calling
>> XENMEM_add_to_physmap from Dom0.
>>
>> For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current
>> domain
>> has the proper privileges on (d) and (t), grant the access.
>>
>> For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to
>> denote if
>> two domains can share memory through map_gmfn_foregin. 2) Change to hook
>> to
>> grant the access IFF the current domain has proper MMU privileges on (d)
>> and (t),
>> and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default
>> xen.te
>> to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event
>> channels.
>>
>> 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 <blackskygg@gmail.com>
>>
>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: xen-devel@lists.xen.org
>> ---
>> V3:
>> * Change several if statements to the GCC '... = a ?: b' extention.
>> * lookup the current domain in the hooks instead of passing it as an
>> arg
>> ---
>> tools/flask/policy/modules/xen.if | 2 ++
>> xen/include/xsm/dummy.h | 3 ++-
>> xen/xsm/flask/hooks.c | 4 +++-
>> xen/xsm/flask/policy/access_vectors | 4 ++++
>> 4 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/flask/policy/modules/xen.if
>> b/tools/flask/policy/modules/xen.if
>> index 55437496f6..3ffd1c6239 100644
>> --- a/tools/flask/policy/modules/xen.if
>> +++ b/tools/flask/policy/modules/xen.if
>> @@ -127,6 +127,8 @@ define(`domain_comms', `
>> domain_event_comms($1, $2)
>> allow $1 $2:grant { map_read map_write copy unmap };
>> allow $2 $1:grant { map_read map_write copy unmap };
>> + allow $1 $2:mmu share_mem;
>> + allow $2 $1:mmu share_mem;
>> ')
>> # domain_self_comms(domain)
>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>> index b2cd56cdc5..65e7060ad5 100644
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -516,7 +516,8 @@ static XSM_INLINE int
>> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
>> *d, struct domain *t)
>> {
>> XSM_ASSERT_ACTION(XSM_TARGET);
>> - return xsm_default_action(action, d, t);
>> + return xsm_default_action(action, current->domain, d) ?:
>> + xsm_default_action(action, current->domain, t);
>> }
>
>
> Same comment as below, the check between (current->domain) and (d) should
> be redundant with one higher up in the call stack. The check between
> (current->domain) and (t) should remain, although this *does* result in a
> relaxing of the existing permission checks on the call as Jan noted.
>
>> static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d,
>> unsigned long op)
>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>> index f01b4cfaaa..16103bafc9 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain
>> *d1, struct domain *d2)
>> 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);
>> + return domain_has_perm(current->domain, d, SECCLASS_MMU,
>> MMU__MAP_READ | MMU__MAP_WRITE) ?:
>> + domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ |
>> MMU__MAP_WRITE) ?:
>> + domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>> }
>
>
> This is at least partially redundant with the higher-level permission checks
> needed to get to the xenmem_add_* functions (xatp_permission_check call in
> xen/common/memory.c, for example). That check already verifies the
> permission
> for (current->domain) to modify (d)'s page tables.
>
> The other two checks here look correct.
Do you mean that the checks that verify the permission for (current->domain) to
modify (d)'s page tables have already been done somewhere higher up in the
call stack so that I can eliminate them in both hooks?
Cheers,
Zhongze Liu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-20 0:34 ` Zhongze Liu
@ 2017-10-20 0:55 ` Zhongze Liu
2017-10-20 13:02 ` Daniel De Graaf
0 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-10-20 0:55 UTC (permalink / raw)
To: Daniel De Graaf
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
2017-10-20 8:34 GMT+08:00 Zhongze Liu <blackskygg@gmail.com>:
> Hi Daniel,
>
> 2017-10-20 1:36 GMT+08:00 Daniel De Graaf <dgdegra@tycho.nsa.gov>:
>> On 10/18/2017 10:36 PM, Zhongze Liu wrote:
>>>
>>> The original dummy xsm_map_gmfn_foregin checks if source domain has the
>>> proper
>>> privileges over the target domain. Under this policy, it's not allowed if
>>> a Dom0
>>> wants to map pages from one DomU to another, which restricts some useful
>>> yet not
>>> dangerous use cases of the API, such as sharing pages among DomU's by
>>> calling
>>> XENMEM_add_to_physmap from Dom0.
>>>
>>> For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current
>>> domain
>>> has the proper privileges on (d) and (t), grant the access.
>>>
>>> For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to
>>> denote if
>>> two domains can share memory through map_gmfn_foregin. 2) Change to hook
>>> to
>>> grant the access IFF the current domain has proper MMU privileges on (d)
>>> and (t),
>>> and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default
>>> xen.te
>>> to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event
>>> channels.
>>>
>>> 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 <blackskygg@gmail.com>
>>>
>>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> Cc: xen-devel@lists.xen.org
>>> ---
>>> V3:
>>> * Change several if statements to the GCC '... = a ?: b' extention.
>>> * lookup the current domain in the hooks instead of passing it as an
>>> arg
>>> ---
>>> tools/flask/policy/modules/xen.if | 2 ++
>>> xen/include/xsm/dummy.h | 3 ++-
>>> xen/xsm/flask/hooks.c | 4 +++-
>>> xen/xsm/flask/policy/access_vectors | 4 ++++
>>> 4 files changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/flask/policy/modules/xen.if
>>> b/tools/flask/policy/modules/xen.if
>>> index 55437496f6..3ffd1c6239 100644
>>> --- a/tools/flask/policy/modules/xen.if
>>> +++ b/tools/flask/policy/modules/xen.if
>>> @@ -127,6 +127,8 @@ define(`domain_comms', `
>>> domain_event_comms($1, $2)
>>> allow $1 $2:grant { map_read map_write copy unmap };
>>> allow $2 $1:grant { map_read map_write copy unmap };
>>> + allow $1 $2:mmu share_mem;
>>> + allow $2 $1:mmu share_mem;
>>> ')
>>> # domain_self_comms(domain)
>>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>>> index b2cd56cdc5..65e7060ad5 100644
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -516,7 +516,8 @@ static XSM_INLINE int
>>> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
>>> *d, struct domain *t)
>>> {
>>> XSM_ASSERT_ACTION(XSM_TARGET);
>>> - return xsm_default_action(action, d, t);
>>> + return xsm_default_action(action, current->domain, d) ?:
>>> + xsm_default_action(action, current->domain, t);
>>> }
>>
>>
>> Same comment as below, the check between (current->domain) and (d) should
>> be redundant with one higher up in the call stack. The check between
>> (current->domain) and (t) should remain, although this *does* result in a
>> relaxing of the existing permission checks on the call as Jan noted.
>>
>>> static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d,
>>> unsigned long op)
>>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>>> index f01b4cfaaa..16103bafc9 100644
>>> --- a/xen/xsm/flask/hooks.c
>>> +++ b/xen/xsm/flask/hooks.c
>>> @@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain
>>> *d1, struct domain *d2)
>>> 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);
>>> + return domain_has_perm(current->domain, d, SECCLASS_MMU,
>>> MMU__MAP_READ | MMU__MAP_WRITE) ?:
>>> + domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ |
>>> MMU__MAP_WRITE) ?:
>>> + domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>>> }
>>
>>
>> This is at least partially redundant with the higher-level permission checks
>> needed to get to the xenmem_add_* functions (xatp_permission_check call in
>> xen/common/memory.c, for example). That check already verifies the
>> permission
>> for (current->domain) to modify (d)'s page tables.
>>
>> The other two checks here look correct.
>
> Do you mean that the checks that verify the permission for (current->domain) to
> modify (d)'s page tables have already been done somewhere higher up in the
> call stack so that I can eliminate them in both hooks?
Although xatp_permission_chec() does check (current->domain)'s permission over
(d), I'm not sure if this is the case for all the call paths that
would finally lead to map_gmfn_foregin().
If the answer is yes, I would happily remove the redundant checks.
Cheers,
Zhongze Liu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin
2017-10-20 0:55 ` Zhongze Liu
@ 2017-10-20 13:02 ` Daniel De Graaf
0 siblings, 0 replies; 29+ messages in thread
From: Daniel De Graaf @ 2017-10-20 13:02 UTC (permalink / raw)
To: Zhongze Liu
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 10/19/2017 08:55 PM, Zhongze Liu wrote:
> 2017-10-20 8:34 GMT+08:00 Zhongze Liu <blackskygg@gmail.com>:
>> Hi Daniel,
>>
>> 2017-10-20 1:36 GMT+08:00 Daniel De Graaf <dgdegra@tycho.nsa.gov>:
>>> On 10/18/2017 10:36 PM, Zhongze Liu wrote:
>>>>
>>>> The original dummy xsm_map_gmfn_foregin checks if source domain has the
>>>> proper
>>>> privileges over the target domain. Under this policy, it's not allowed if
>>>> a Dom0
>>>> wants to map pages from one DomU to another, which restricts some useful
>>>> yet not
>>>> dangerous use cases of the API, such as sharing pages among DomU's by
>>>> calling
>>>> XENMEM_add_to_physmap from Dom0.
>>>>
>>>> For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current
>>>> domain
>>>> has the proper privileges on (d) and (t), grant the access.
>>>>
>>>> For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to
>>>> denote if
>>>> two domains can share memory through map_gmfn_foregin. 2) Change to hook
>>>> to
>>>> grant the access IFF the current domain has proper MMU privileges on (d)
>>>> and (t),
>>>> and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default
>>>> xen.te
>>>> to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event
>>>> channels.
>>>>
>>>> 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 <blackskygg@gmail.com>
>>>>
>>>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>> Cc: xen-devel@lists.xen.org
>>>> ---
>>>> V3:
>>>> * Change several if statements to the GCC '... = a ?: b' extention.
>>>> * lookup the current domain in the hooks instead of passing it as an
>>>> arg
>>>> ---
>>>> tools/flask/policy/modules/xen.if | 2 ++
>>>> xen/include/xsm/dummy.h | 3 ++-
>>>> xen/xsm/flask/hooks.c | 4 +++-
>>>> xen/xsm/flask/policy/access_vectors | 4 ++++
>>>> 4 files changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/flask/policy/modules/xen.if
>>>> b/tools/flask/policy/modules/xen.if
>>>> index 55437496f6..3ffd1c6239 100644
>>>> --- a/tools/flask/policy/modules/xen.if
>>>> +++ b/tools/flask/policy/modules/xen.if
>>>> @@ -127,6 +127,8 @@ define(`domain_comms', `
>>>> domain_event_comms($1, $2)
>>>> allow $1 $2:grant { map_read map_write copy unmap };
>>>> allow $2 $1:grant { map_read map_write copy unmap };
>>>> + allow $1 $2:mmu share_mem;
>>>> + allow $2 $1:mmu share_mem;
>>>> ')
>>>> # domain_self_comms(domain)
>>>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>>>> index b2cd56cdc5..65e7060ad5 100644
>>>> --- a/xen/include/xsm/dummy.h
>>>> +++ b/xen/include/xsm/dummy.h
>>>> @@ -516,7 +516,8 @@ static XSM_INLINE int
>>>> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>>> static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
>>>> *d, struct domain *t)
>>>> {
>>>> XSM_ASSERT_ACTION(XSM_TARGET);
>>>> - return xsm_default_action(action, d, t);
>>>> + return xsm_default_action(action, current->domain, d) ?:
>>>> + xsm_default_action(action, current->domain, t);
>>>> }
>>>
>>>
>>> Same comment as below, the check between (current->domain) and (d) should
>>> be redundant with one higher up in the call stack. The check between
>>> (current->domain) and (t) should remain, although this *does* result in a
>>> relaxing of the existing permission checks on the call as Jan noted.
>>>
>>>> static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d,
>>>> unsigned long op)
>>>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>>>> index f01b4cfaaa..16103bafc9 100644
>>>> --- a/xen/xsm/flask/hooks.c
>>>> +++ b/xen/xsm/flask/hooks.c
>>>> @@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain
>>>> *d1, struct domain *d2)
>>>> 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);
>>>> + return domain_has_perm(current->domain, d, SECCLASS_MMU,
>>>> MMU__MAP_READ | MMU__MAP_WRITE) ?:
>>>> + domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ |
>>>> MMU__MAP_WRITE) ?:
>>>> + domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>>>> }
>>>
>>>
>>> This is at least partially redundant with the higher-level permission checks
>>> needed to get to the xenmem_add_* functions (xatp_permission_check call in
>>> xen/common/memory.c, for example). That check already verifies the
>>> permission
>>> for (current->domain) to modify (d)'s page tables.
>>>
>>> The other two checks here look correct.
>>
>> Do you mean that the checks that verify the permission for (current->domain) to
>> modify (d)'s page tables have already been done somewhere higher up in the
>> call stack so that I can eliminate them in both hooks?
>
> Although xatp_permission_chec() does check (current->domain)'s permission over
> (d), I'm not sure if this is the case for all the call paths that
> would finally lead to map_gmfn_foregin().
> If the answer is yes, I would happily remove the redundant checks.
>
> Cheers,
>
> Zhongze Liu.
If this were not the case, there would be other security bugs for the other
XENMAPSPACE_* flags that don't have their own XSM check at this level. Last
time I checked, all paths that led here have already gone through such a
check.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 3/7] libxl: introduce a new structure to represent static shared memory regions
2017-10-19 2:36 [PATCH v3 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
2017-10-19 2:36 ` [PATCH v3 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
2017-10-19 2:36 ` [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin Zhongze Liu
@ 2017-10-19 2:36 ` Zhongze Liu
2017-10-31 12:48 ` Wei Liu
2017-10-19 2:36 ` [PATCH v3 4/7] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
` (3 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-10-19 2:36 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
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]).
[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
tools/libxl/libxl.h | 4 ++++
tools/libxl/libxl_types.idl | 32 ++++++++++++++++++++++++++++++--
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5e9aed739d..5fbbe266e5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2320,6 +2320,10 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
const char *command_line, char **output);
+/* Constants for libxl_static_shm */
+#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
+#define LIBXL_SSHM_ID_MAXLEN 128
+
#include <libxl_event.h>
#endif /* LIBXL_H */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a239324341..78cd2696b0 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -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),
@@ -812,6 +812,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", [
+ (-1, "UNKNOWN"),
+ (0, "MASTER"),
+ (1, "SLAVE"),
+ ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
+
+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'}),
+ ("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),
@@ -831,6 +858,7 @@ libxl_domain_config = Struct("domain_config", [
("channels", Array(libxl_device_channel, "num_channels")),
("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
+ ("sshms", Array(libxl_static_shm, "num_sshms")),
("on_poweroff", libxl_action_on_shutdown),
("on_reboot", libxl_action_on_shutdown),
--
2.14.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 4/7] libxl: support mapping static shared memory areas during domain creation
2017-10-19 2:36 [PATCH v3 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (2 preceding siblings ...)
2017-10-19 2:36 ` [PATCH v3 3/7] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
@ 2017-10-19 2:36 ` Zhongze Liu
2017-11-01 15:55 ` Wei Liu
2017-10-19 2:36 ` [PATCH v3 5/7] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
` (2 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-10-19 2:36 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
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,
no master domain etc.
* Write infomation of static shared memory areas into the appropriate
xenstore paths.
* Use xc_domain_add_to_physmap_batch to do the page sharing.
* Set the refcount of the shared region accordingly
Temporarily mark this as unsupported on x86 because calling p2m_add_foregin on
two domU's is currently not allowd on x86 (see the comments in
x86/mm/p2m.c:p2m_add_foregin 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 <blackskygg@gmail.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
V3:
* unmap the successfully mapped pages whenever rc != 0
---
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 | 13 ++
tools/libxl/libxl_sshm.c | 395 +++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_x86.c | 18 ++
7 files changed, 475 insertions(+), 1 deletion(-)
create mode 100644 tools/libxl/libxl_sshm.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 5a861f72cb..91bc70cda2 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -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
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index 784ec7f609..11433fe466 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -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
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index de1840bece..d687dad0bd 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -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
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f15fb215c2..fecc873b7a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -514,6 +514,14 @@ int libxl__domain_build(libxl__gc *gc,
ret = ERROR_INVAL;
goto out;
}
+
+ /* the p2m has been setup, we could map the static shared memory now. */
+ 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;
@@ -939,6 +947,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);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 45e6df6c82..fc28191625 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4350,6 +4350,19 @@ 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
diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
new file mode 100644
index 0000000000..b20c37dab8
--- /dev/null
+++ b/tools/libxl/libxl_sshm.c
@@ -0,0 +1,395 @@
+#include "libxl_osdeps.h"
+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#define SSHM_PATH(id) GCSPRINTF("/local/static_shm/%s", id)
+
+#define SSHM_ERROR(domid, sshmid, f, ...) \
+ LOGD(ERROR, domid, "static_shm id = %s: " f, sshmid, ##__VA_ARGS__)
+
+
+/* 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;
+
+ /* 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;
+}
+
+/* Compare function for sorting sshm ranges by sshm->begin */
+static int sshm_range_cmp(const void *a, const void *b)
+{
+ libxl_static_shm *const *sshma = a, *const *sshmb = b;
+ return (*sshma)->begin > (*sshmb)->begin ? 1 : -1;
+}
+
+/* check if the sshm slave configs in @sshm overlap */
+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
+ *
+ * This functions maps
+ * mater gfn: [@msshm->begin + @sshm->offset, @msshm->end + @sshm->offset)
+ * 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 */
+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+ libxl_static_shm *sshm, libxl_static_shm *msshm,
+ xen_pfn_t *mapped, unsigned int *nmapped)
+{
+ int rc;
+ int i;
+ unsigned int num_mpages, num_spages, num_success, offset;
+ 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;
+ offset = sshm->offset >> XC_PAGE_SHIFT;
+
+ /* Check range. Test offset < mpages first to avoid overflow */
+ if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
+ SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ /* fill out the pfn's and do the mapping */
+ 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_foreign,
+ 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;
+ const char *count_path, *count_string;
+
+ count_path = GCSPRINTF("%s/users", sshm_path);
+ rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
+ if (rc) goto out;
+ count = atoi(count_string);
+
+ count_string = GCSPRINTF("%d", count+1);
+ rc = libxl__xs_write_checked(gc, xt, count_path, count_string);
+ if (rc) goto out;
+
+ 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) {
+ /* role 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] = "users";
+ 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:
+ */
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 5f91fe4f92..450fb3d354 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -596,6 +596,24 @@ 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)
+ sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
+ if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+ return ERROR_INVAL;
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
--
2.14.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 4/7] libxl: support mapping static shared memory areas during domain creation
2017-10-19 2:36 ` [PATCH v3 4/7] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
@ 2017-11-01 15:55 ` Wei Liu
2017-11-09 0:48 ` Zhongze Liu
0 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2017-11-01 15:55 UTC (permalink / raw)
To: Zhongze Liu
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On Thu, Oct 19, 2017 at 10:36:32AM +0800, Zhongze Liu wrote:
> 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,
> no master domain etc.
> * Write infomation of static shared memory areas into the appropriate
> xenstore paths.
> * Use xc_domain_add_to_physmap_batch to do the page sharing.
> * Set the refcount of the shared region accordingly
>
> Temporarily mark this as unsupported on x86 because calling p2m_add_foregin on
> two domU's is currently not allowd on x86 (see the comments in
> x86/mm/p2m.c:p2m_add_foregin 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 <blackskygg@gmail.com>
>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: xen-devel@lists.xen.org
> ---
> V3:
> * unmap the successfully mapped pages whenever rc != 0
A general note: please properly capitalise the comments.
> ---
> 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 | 13 ++
> tools/libxl/libxl_sshm.c | 395 +++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_x86.c | 18 ++
> 7 files changed, 475 insertions(+), 1 deletion(-)
> create mode 100644 tools/libxl/libxl_sshm.c
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 5a861f72cb..91bc70cda2 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
[...]
> +
> +/* check if the sshm slave configs in @sshm overlap */
> +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
> + *
> + * This functions maps
> + * mater gfn: [@msshm->begin + @sshm->offset, @msshm->end + @sshm->offset)
"master"
This is confusing. What if offset is not page aligned?
> + * 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 */
> +static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
> + libxl_static_shm *sshm, libxl_static_shm *msshm,
> + xen_pfn_t *mapped, unsigned int *nmapped)
> +{
> + int rc;
> + int i;
> + unsigned int num_mpages, num_spages, num_success, offset;
> + 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;
> + offset = sshm->offset >> XC_PAGE_SHIFT;
> +
> + /* Check range. Test offset < mpages first to avoid overflow */
> + if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
> + SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + /* fill out the pfn's and do the mapping */
> + 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_foreign,
> + 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;
> + const char *count_path, *count_string;
> +
> + count_path = GCSPRINTF("%s/users", sshm_path);
> + rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
> + if (rc) goto out;
> + count = atoi(count_string);
> +
> + count_string = GCSPRINTF("%d", count+1);
> + rc = libxl__xs_write_checked(gc, xt, count_path, count_string);
> + if (rc) goto out;
> +
> + rc = 0;
> +out:
> + return rc;
> +}
> +
[...]
> +
> + /* 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) {
> + /* role 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]);
> + }
It is possible to get here when the first attempt of mapping has failed.
The do_map function can return early before it modifies nmapped. At
first glance this code snippet is buggy.
I would suggest you change isretry to mapped and adjust the predicate
for rolling back.
> + }
> +
> + 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] = "users";
> + ents[11] = "1";
> + ents[12] = NULL;
> +
> + /* could only be accessed by Dom0 */
> + noperm.id = 0;
> + noperm.perms = XS_PERM_NONE;
> +
The owner of the path is going to be Dom0 and all other domains have no
permission, which matches the comment.
Do you not want domid to be able to read? If you don't want it to read
from the path, why not put in under some other paths like /libxl/?
> + 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;
> +}
> +
[...]
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 5f91fe4f92..450fb3d354 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -596,6 +596,24 @@ 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 */
Move the */ to a new line please. And add "." after "p2m_add_foreign".
> + return false;
> +}
> +
> +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_X86_NORMAL;
> + if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
> + return ERROR_INVAL;
> +
> + return 0;
> +}
> +
> /*
> * Local variables:
> * mode: C
> --
> 2.14.2
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 4/7] libxl: support mapping static shared memory areas during domain creation
2017-11-01 15:55 ` Wei Liu
@ 2017-11-09 0:48 ` Zhongze Liu
0 siblings, 0 replies; 29+ messages in thread
From: Zhongze Liu @ 2017-11-09 0:48 UTC (permalink / raw)
To: Wei Liu; +Cc: Julien Grall, Stefano Stabellini, Ian Jackson, xen-devel
Hi Wei,
2017-11-01 23:55 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
> On Thu, Oct 19, 2017 at 10:36:32AM +0800, Zhongze Liu wrote:
>> 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,
>> no master domain etc.
>> * Write infomation of static shared memory areas into the appropriate
>> xenstore paths.
>> * Use xc_domain_add_to_physmap_batch to do the page sharing.
>> * Set the refcount of the shared region accordingly
>>
>> Temporarily mark this as unsupported on x86 because calling p2m_add_foregin on
>> two domU's is currently not allowd on x86 (see the comments in
>> x86/mm/p2m.c:p2m_add_foregin 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 <blackskygg@gmail.com>
>>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: xen-devel@lists.xen.org
>> ---
>> V3:
>> * unmap the successfully mapped pages whenever rc != 0
>
>
> A general note: please properly capitalise the comments.
>
>> ---
>> 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 | 13 ++
>> tools/libxl/libxl_sshm.c | 395 +++++++++++++++++++++++++++++++++++++++++++
>> tools/libxl/libxl_x86.c | 18 ++
>> 7 files changed, 475 insertions(+), 1 deletion(-)
>> create mode 100644 tools/libxl/libxl_sshm.c
>>
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index 5a861f72cb..91bc70cda2 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
> [...]
>> +
>> +/* check if the sshm slave configs in @sshm overlap */
>> +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
>> + *
>> + * This functions maps
>> + * mater gfn: [@msshm->begin + @sshm->offset, @msshm->end + @sshm->offset)
>
> "master"
>
> This is confusing. What if offset is not page aligned?
No, it will always be page-aligned. This was documented in the man
page and also checked
in the corresponding libxlu_* function.
>
>> + * 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 */
>> +static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
>> + libxl_static_shm *sshm, libxl_static_shm *msshm,
>> + xen_pfn_t *mapped, unsigned int *nmapped)
>> +{
>> + int rc;
>> + int i;
>> + unsigned int num_mpages, num_spages, num_success, offset;
>> + 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;
>> + offset = sshm->offset >> XC_PAGE_SHIFT;
>> +
>> + /* Check range. Test offset < mpages first to avoid overflow */
>> + if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
>> + SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
>> + rc = ERROR_INVAL;
>> + goto out;
>> + }
>> +
>> + /* fill out the pfn's and do the mapping */
>> + 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_foreign,
>> + 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;
>> + const char *count_path, *count_string;
>> +
>> + count_path = GCSPRINTF("%s/users", sshm_path);
>> + rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
>> + if (rc) goto out;
>> + count = atoi(count_string);
>> +
>> + count_string = GCSPRINTF("%d", count+1);
>> + rc = libxl__xs_write_checked(gc, xt, count_path, count_string);
>> + if (rc) goto out;
>> +
>> + rc = 0;
>> +out:
>> + return rc;
>> +}
>> +
> [...]
>> +
>> + /* 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) {
>> + /* role 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]);
>> + }
>
> It is possible to get here when the first attempt of mapping has failed.
> The do_map function can return early before it modifies nmapped. At
> first glance this code snippet is buggy.
>
> I would suggest you change isretry to mapped and adjust the predicate
> for rolling back.
nmapped was initialized to 0, so even if do_map fails early (and the
only possible case
where do_map doesn't modify nmapped is when no page was actually mapped), the
value of nmapped stay correct and no pages will be rolled back.
>
>> + }
>> +
>> + 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] = "users";
>> + ents[11] = "1";
>> + ents[12] = NULL;
>> +
>> + /* could only be accessed by Dom0 */
>> + noperm.id = 0;
>> + noperm.perms = XS_PERM_NONE;
>> +
>
> The owner of the path is going to be Dom0 and all other domains have no
> permission, which matches the comment.
>
> Do you not want domid to be able to read? If you don't want it to read
> from the path, why not put in under some other paths like /libxl/?
>
I think this is to be discussed. But in my opinion, the target user won't need
this information because the address is often statically setup and hardcoded
in the simple bare metal apps running inside the DomU's, so I will currently
mark it as noperm and move it to /libxl as you suggested (secure by default).
If, the future, there are needs to read this information from within the DomU's,
we could easily move it back and set the permissions accordingly.
>> + 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;
>> +}
>> +
> [...]
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index 5f91fe4f92..450fb3d354 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -596,6 +596,24 @@ 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 */
>
> Move the */ to a new line please. And add "." after "p2m_add_foreign".
>
>> + return false;
>> +}
>> +
>> +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_X86_NORMAL;
>> + if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
>> + return ERROR_INVAL;
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * Local variables:
>> * mode: C
>> --
>> 2.14.2
>>
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 5/7] libxl: support unmapping static shared memory areas during domain destruction
2017-10-19 2:36 [PATCH v3 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (3 preceding siblings ...)
2017-10-19 2:36 ` [PATCH v3 4/7] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
@ 2017-10-19 2:36 ` Zhongze Liu
2017-11-01 15:55 ` Wei Liu
2017-10-19 2:36 ` [PATCH v3 6/7] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files Zhongze Liu
2017-10-19 2:36 ` [PATCH v3 7/7] docs: documentation about static shared memory regions Zhongze Liu
6 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-10-19 2:36 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
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: unmap the shared pages, and cleanup related xs entries.
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 <blackskygg@gmail.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
tools/libxl/libxl_domain.c | 5 +++
tools/libxl/libxl_internal.h | 2 +
tools/libxl/libxl_sshm.c | 105 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 112 insertions(+)
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 814f8128a1..ce90387173 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -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);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fc28191625..dc5937e712 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4358,6 +4358,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,
diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
index b20c37dab8..f977c7ec5b 100644
--- a/tools/libxl/libxl_sshm.c
+++ b/tools/libxl/libxl_sshm.c
@@ -86,6 +86,111 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
return 0;
}
+/* Decrease the refcount of an sshm. When refcount reaches 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/users", sshm_path);
+ if (libxl__xs_read_checked(gc, xt, count_path, &count_string))
+ return;
+ count = atoi(count_string);
+
+ if (--count == 0) {
+ libxl__xs_path_cleanup(gc, xt, sshm_path);
+ return;
+ }
+
+ count_string = GCSPRINTF("%d", count);
+ libxl__xs_write_checked(gc, xt, count_path, count_string);
+
+ 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;
+
+ 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);
+
+ 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);
+
+ isretry = false;
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &xt);
+ if (rc) goto out;
+
+ if (libxl__xs_read(gc, xt, dom_sshm_path)) {
+ sshm_ents = libxl__xs_directory(gc, xt, dom_sshm_path, &sshm_num);
+ if (!sshm_ents) continue;
+
+ for (i = 0; i < sshm_num; ++i) {
+ 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;
+ }
+
+ rc = 0;
+out:
+ libxl__xs_transaction_abort(gc, &xt);
+ return rc;
+}
+
/* libxl__sshm_do_map -- map pages into slave's physmap
*
* This functions maps
--
2.14.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 5/7] libxl: support unmapping static shared memory areas during domain destruction
2017-10-19 2:36 ` [PATCH v3 5/7] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
@ 2017-11-01 15:55 ` Wei Liu
2017-11-09 2:06 ` Zhongze Liu
0 siblings, 1 reply; 29+ messages in thread
From: Wei Liu @ 2017-11-01 15:55 UTC (permalink / raw)
To: Zhongze Liu
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On Thu, Oct 19, 2017 at 10:36:33AM +0800, Zhongze Liu wrote:
> 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: unmap the shared pages, and cleanup related xs entries.
> decrease the refcount of the sshm region, if the refcount reaches 0,
> cleanup the whole sshm path.
>
This appears to be in line with what we discussed.
I would like to see some explanation for: if one or more of the things
the code does fail half way, the system is still going to be in a
consistent state. Most notably, there isn't going to be any page leaked
with uncleaned refs.
> +
> + rc = libxl__xs_transaction_commit(gc, &xt);
> + if (!rc) break;
> + if (rc < 0) goto out;
> + isretry = true;
Indentation.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 5/7] libxl: support unmapping static shared memory areas during domain destruction
2017-11-01 15:55 ` Wei Liu
@ 2017-11-09 2:06 ` Zhongze Liu
2017-11-09 2:10 ` Zhongze Liu
0 siblings, 1 reply; 29+ messages in thread
From: Zhongze Liu @ 2017-11-09 2:06 UTC (permalink / raw)
Cc: Julien Grall, Stefano Stabellini, Ian Jackson, xen-devel
Hi Wei,
2017-11-01 23:55 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
> On Thu, Oct 19, 2017 at 10:36:33AM +0800, Zhongze Liu wrote:
>> 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: unmap the shared pages, and cleanup related xs entries.
>> decrease the refcount of the sshm region, if the refcount reaches 0,
>> cleanup the whole sshm path.
>>
>
> This appears to be in line with what we discussed.
>
> I would like to see some explanation for: if one or more of the things
> the code does fail half way, the system is still going to be in a
> consistent state. Most notably, there isn't going to be any page leaked
> with uncleaned refs.
>
The unmapping code is invoked during the domain destruction process, so
we can't roll back. Everything done here is a "best effort": even if the code
fails half way, what I could do is to report the errors and proceed anyway.
I might be totally wrong. So please correct me.
>> +
>> + rc = libxl__xs_transaction_commit(gc, &xt);
>> + if (!rc) break;
>> + if (rc < 0) goto out;
>> + isretry = true;
>
> Indentation.
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 5/7] libxl: support unmapping static shared memory areas during domain destruction
2017-11-09 2:06 ` Zhongze Liu
@ 2017-11-09 2:10 ` Zhongze Liu
0 siblings, 0 replies; 29+ messages in thread
From: Zhongze Liu @ 2017-11-09 2:10 UTC (permalink / raw)
To: Wei Liu; +Cc: Julien Grall, Stefano Stabellini, Ian Jackson, xen-devel
Oops, The address lines were somehow dropped by my mail client.
Adding Wei.
2017-11-09 10:06 GMT+08:00 Zhongze Liu <blackskygg@gmail.com>:
> Hi Wei,
>
> 2017-11-01 23:55 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
>> On Thu, Oct 19, 2017 at 10:36:33AM +0800, Zhongze Liu wrote:
>>> 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: unmap the shared pages, and cleanup related xs entries.
>>> decrease the refcount of the sshm region, if the refcount reaches 0,
>>> cleanup the whole sshm path.
>>>
>>
>> This appears to be in line with what we discussed.
>>
>> I would like to see some explanation for: if one or more of the things
>> the code does fail half way, the system is still going to be in a
>> consistent state. Most notably, there isn't going to be any page leaked
>> with uncleaned refs.
>>
>
> The unmapping code is invoked during the domain destruction process, so
> we can't roll back. Everything done here is a "best effort": even if the code
> fails half way, what I could do is to report the errors and proceed anyway.
>
> I might be totally wrong. So please correct me.
>
>>> +
>>> + rc = libxl__xs_transaction_commit(gc, &xt);
>>> + if (!rc) break;
>>> + if (rc < 0) goto out;
>>> + isretry = true;
>>
>> Indentation.
>
> Cheers,
>
> Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 6/7] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files
2017-10-19 2:36 [PATCH v3 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (4 preceding siblings ...)
2017-10-19 2:36 ` [PATCH v3 5/7] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
@ 2017-10-19 2:36 ` Zhongze Liu
2017-10-19 2:36 ` [PATCH v3 7/7] docs: documentation about static shared memory regions Zhongze Liu
6 siblings, 0 replies; 29+ messages in thread
From: Zhongze Liu @ 2017-10-19 2:36 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
Add the parsing utils for the newly introduced libxl_static_sshm struct
to the libxl/libxlu_* family. And add realated parsing code in xl to
parse the struct from xl config files. 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 <blackskygg@gmail.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
tools/libxl/Makefile | 2 +-
tools/libxl/libxlu_sshm.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxlutil.h | 6 ++
tools/xl/xl_parse.c | 24 +++++-
4 files changed, 240 insertions(+), 2 deletions(-)
create mode 100644 tools/libxl/libxlu_sshm.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 91bc70cda2..25e6f0bd7f 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -176,7 +176,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
- libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
+ libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
$(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
$(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog) $(CFLAGS_libxentoolcore)
diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
new file mode 100644
index 0000000000..5276ff9395
--- /dev/null
+++ b/tools/libxl/libxlu_sshm.c
@@ -0,0 +1,210 @@
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxlu_internal.h"
+#include "xenctrl.h"
+
+#include <ctype.h>
+
+#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
+#define WORD_RE "([_a-zA-Z0-9]+)"
+#define EQU_RE PARAM_RE(WORD_RE "\\s*=\\s*" WORD_RE)
+
+#define RET_INVAL(msg, curr_str) do { \
+ xlu__sshm_err(cfg, msg, curr_str); \
+ rc = EINVAL; \
+ goto out; \
+ } while(0)
+
+/* set a member in libxl_static_shm and report an error if it's respecified,
+ * @curr_str indicates the head of the remaining string. */
+#define SET_VAL(var, name, type, value, curr_str) do { \
+ if ((var) != LIBXL_SSHM_##type##_UNKNOWN && (var) != value) { \
+ RET_INVAL("\"" name "\" respecified", curr_str); \
+ } \
+ (var) = value; \
+ } while(0)
+
+
+static void xlu__sshm_err(XLU_Config *cfg, const char *msg,
+ const char *curr_str) {
+ fprintf(cfg->report,
+ "%s: config parsing error in shared_memory: %s at '%s'\n",
+ cfg->config_source, msg, curr_str);
+}
+
+static int parse_prot(XLU_Config *cfg, char *str, libxl_sshm_prot *prot)
+{
+ int rc;
+ libxl_sshm_prot new_prot;
+
+ if (!strcmp(str, "rw")) {
+ new_prot = LIBXL_SSHM_PROT_RW;
+ } else {
+ RET_INVAL("invalid permission flags", str);
+ }
+
+ SET_VAL(*prot, "permission flags", PROT, new_prot, str);
+
+ rc = 0;
+
+ out:
+ return rc;
+}
+
+static int parse_cachepolicy(XLU_Config *cfg, char *str,
+ libxl_sshm_cachepolicy *policy)
+{
+ int rc;
+ libxl_sshm_cachepolicy new_policy;
+
+ if (!strcmp(str, "ARM_normal")) {
+ new_policy = LIBXL_SSHM_CACHEPOLICY_ARM_NORMAL;
+ } else if (!strcmp(str, "x86_normal")) {
+ new_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
+ } else {
+ RET_INVAL("invalid cache policy", str);
+ }
+
+ SET_VAL(*policy, "cache policy", CACHEPOLICY, new_policy, str);
+ rc = 0;
+
+ out:
+ return rc;
+}
+
+/* handle key = value pairs */
+static int handle_equ(XLU_Config *cfg, char *key, char *val,
+ libxl_static_shm *sshm)
+{
+ int rc;
+
+ if (!strcmp(key, "id")) {
+ if (strlen(val) > LIBXL_SSHM_ID_MAXLEN) { RET_INVAL("id too long", val); }
+ if (sshm->id && !strcmp(sshm->id, val)) {
+ RET_INVAL("id respecified", val);
+ }
+
+ sshm->id = strdup(val);
+ if (!sshm->id) {
+ fprintf(stderr, "sshm parser out of memory\n");
+ rc = ENOMEM;
+ goto out;
+ }
+ } else if (!strcmp(key, "role")) {
+ libxl_sshm_role new_role;
+
+ if (!strcmp("master", val)) {
+ new_role = LIBXL_SSHM_ROLE_MASTER;
+ } else if (!strcmp("slave", val)) {
+ new_role = LIBXL_SSHM_ROLE_SLAVE;
+ } else {
+ RET_INVAL("invalid role", val);
+ }
+
+ SET_VAL(sshm->role, "role", ROLE, new_role, val);
+ } else if (!strcmp(key, "begin") ||
+ !strcmp(key, "end") ||
+ !strcmp(key, "offset")) {
+ char *endptr;
+ int base = 10;
+ uint64_t new_addr;
+
+ /* Could be in hex form. Note that we don't need to check the length here,
+ * for val[] is NULL-terminated */
+ if (val[0] == '0' && val[1] == 'x') { base = 16; }
+ new_addr = strtoull(val, &endptr, base);
+ if (errno == ERANGE || *endptr)
+ RET_INVAL("invalid begin/end/offset", val);
+ if (new_addr & ~XC_PAGE_MASK)
+ RET_INVAL("begin/end/offset is not a multiple of 4K", val);
+
+ /* begin or end */
+ if (key[0] == 'b') {
+ SET_VAL(sshm->begin, "beginning address", RANGE, new_addr, val);
+ } else if(key[0] == 'e'){
+ SET_VAL(sshm->end, "ending address", RANGE, new_addr, val);
+ } else {
+ SET_VAL(sshm->offset, "offset", RANGE, new_addr, val);
+ }
+ } else if (!strcmp(key, "prot")) {
+ rc = parse_prot(cfg, val, &sshm->prot);
+ if (rc) { goto out; }
+ } else if (!strcmp(key, "cache_policy")) {
+ rc = parse_cachepolicy(cfg, val, &sshm->cache_policy);
+ if (rc) { goto out; }
+ } else {
+ RET_INVAL("invalid option", key);
+ }
+
+ rc = 0;
+
+ out:
+ return rc;
+}
+
+int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
+ libxl_static_shm *sshm)
+{
+ int rc;
+ regex_t equ_rec;
+ char *buf2 = NULL, *ptr = NULL;
+ regmatch_t pmatch[3];
+
+ rc = regcomp(&equ_rec, EQU_RE, REG_EXTENDED);
+ if (rc) {
+ fprintf(stderr, "sshm parser failed to initialize\n");
+ goto out;
+ }
+
+ buf2 = ptr = strdup(spec);
+ if (!buf2) {
+ fprintf(stderr, "sshm parser out of memory\n");
+ rc = ENOMEM;
+ goto out;
+ }
+
+ /* main parsing loop */
+ while (true) {
+ if (!*ptr) { break; }
+ if (regexec(&equ_rec, ptr, 3, pmatch, 0))
+ RET_INVAL("unrecognized token", ptr);
+
+ ptr[pmatch[1].rm_eo] = '\0';
+ ptr[pmatch[2].rm_eo] = '\0';
+ rc = handle_equ(cfg, ptr + pmatch[1].rm_so,
+ ptr + pmatch[2].rm_so, sshm);
+ if (rc) { goto out; }
+
+ ptr += pmatch[0].rm_eo;
+ }
+
+ if (*ptr) { RET_INVAL("invalid syntax", ptr); }
+
+ /* do some early checks */
+ if (!sshm->id) {
+ RET_INVAL("id not specified", spec);
+ }
+ if (sshm->begin == LIBXL_SSHM_RANGE_UNKNOWN) {
+ RET_INVAL("begin address not specified", spec);
+ }
+ if (sshm->end == LIBXL_SSHM_RANGE_UNKNOWN) {
+ RET_INVAL("end address not specified", spec);
+ }
+ if (sshm->begin > sshm->end) {
+ RET_INVAL("begin address larger that end address", spec);
+ }
+
+ rc = 0;
+
+ out:
+ if (buf2) { free(buf2); }
+ regfree(&equ_rec);
+ return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index e81b644c01..ee39cb5bdc 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -118,6 +118,12 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str);
int xlu_vif_parse_rate(XLU_Config *cfg, const char *rate,
libxl_device_nic *nic);
+/*
+ * static shared memory specification parsing
+ */
+int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
+ libxl_static_shm *sshm);
+
#endif /* LIBXLUTIL_H */
/*
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 9a692d5ae6..bf425f0ff3 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -860,7 +860,7 @@ void parse_config_data(const char *config_source,
long l, vcpus = 0;
XLU_Config *config;
XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
- *usbctrls, *usbdevs, *p9devs, *vdispls;
+ *usbctrls, *usbdevs, *p9devs, *vdispls, *sshms;
XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
*mca_caps;
int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
@@ -1555,6 +1555,28 @@ void parse_config_data(const char *config_source,
}
}
+ if (!xlu_cfg_get_list (config, "static_shm", &sshms, 0, 0)) {
+ d_config->num_sshms = 0;
+ d_config->sshms = NULL;
+ while ((buf = xlu_cfg_get_listitem (sshms, d_config->num_sshms)) != NULL) {
+ libxl_static_shm *sshm;
+ char *buf2 = strdup(buf);
+ int ret;
+
+ sshm = ARRAY_EXTEND_INIT_NODEVID(d_config->sshms,
+ d_config->num_sshms,
+ libxl_static_shm_init);
+ ret = xlu_sshm_parse(config, buf2, sshm);
+ if (ret) {
+ fprintf(stderr,
+ "xl: Invalid argument for static_shm: %s", buf2);
+ exit(EXIT_FAILURE);
+ }
+
+ free(buf2);
+ }
+ }
+
if (!xlu_cfg_get_list(config, "p9", &p9devs, 0, 0)) {
libxl_device_p9 *p9;
char *security_model = NULL;
--
2.14.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 7/7] docs: documentation about static shared memory regions
2017-10-19 2:36 [PATCH v3 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (5 preceding siblings ...)
2017-10-19 2:36 ` [PATCH v3 6/7] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files Zhongze Liu
@ 2017-10-19 2:36 ` Zhongze Liu
6 siblings, 0 replies; 29+ messages in thread
From: Zhongze Liu @ 2017-10-19 2:36 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
Add docs to document the motivation, usage, use cases and other
relavant infomation about the static shared memory feature.
This is for the proposal "Allow setting up shared memory areas between VMs
from xl config file". See:
https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
docs/man/xl-static-shm-configuration.pod.5 | 257 +++++++++++++++++++++++++++++
docs/man/xl.cfg.pod.5.in | 8 +
docs/misc/xenstore-paths.markdown | 47 ++++++
3 files changed, 312 insertions(+)
create mode 100644 docs/man/xl-static-shm-configuration.pod.5
diff --git a/docs/man/xl-static-shm-configuration.pod.5 b/docs/man/xl-static-shm-configuration.pod.5
new file mode 100644
index 0000000000..f103499a87
--- /dev/null
+++ b/docs/man/xl-static-shm-configuration.pod.5
@@ -0,0 +1,257 @@
+=head1 NAME
+
+xl-static-shm-configuration - XL Static Shared Memeory Configuration Syntax
+
+
+(B<NOTE>: This is currently only available to ARM guests.)
+
+=head1 DESCRIPTION
+
+The static_shm option allows users to statically setup shared memory regions
+among a group of VMs, enabling guests without grant table support to do
+shm-based communication.
+
+Every shared region is:
+
+=over 4
+
+* Uniquely identified by a string that is no longer than 128 characters, which
+is called an B<identifier> in this document.
+
+* Backed by exactely one domain, which is called a B<master> domain, and all
+the other domains who are also sharing this region are called B<slave>s.
+
+=back
+
+=head1 SYNTAX
+
+This document specifies syntax of the static shared memory configuration in
+the xl config file. It has the following form:
+
+ static_shm = [ "SSHM_SPEC", "SSHM_SPEC", ... ]
+
+where each C<SSHM_SPEC> is in this form:
+
+ [<key>=<value>,]*
+
+Valid examples of C<SSHM_SPEC> are:
+
+ id=ID1, begin=0x100000, end=0x200000, role=master, cache_policy=x86_normal
+ id=ID1, offset = 0, begin=0x500000, end=0x600000, role=slave, prot=rw
+ id=ID2, begin=0x300000, end=0x400000, role=master
+ id=ID2, offset = 0x10000, begin=0x690000, end=0x800000, role=slave
+ id=ID2, offset = 0x10000, begin=0x690000, end=0x800000, role=slave
+
+These might be specified in the domain config file like this:
+
+ static_shm = ["id=ID2, offset = 0x10000, begin=0x690000, end=0x800000,
+role=slave"]
+
+
+More formally, the string is a series of comma-separated keyword/value
+pairs. Each parameter may be specified at most once. Default values apply if
+the parameter is not specified.
+
+=head1 Parameters
+
+=over 4
+
+=item B<id>
+
+=over 4
+
+=item Description
+
+The unique identifier of the shared memory region.
+
+Every identifier could appear only once in each xl config file.
+
+=item Supported values
+
+A string that contains alphanumerics and "_"s, and is no longer than 128
+characters.
+
+=item Default value
+
+None, this parameter is mandatory.
+
+=back
+
+=item B<begin>/B<end>
+
+=over 4
+
+=item Description
+
+The boundaries of the shared memory area.
+
+=item Supported values
+
+Same with B<offset>.
+
+=item Default Value
+
+None, this parameter is mandatory.
+
+=back
+
+=item B<offset>
+
+=over 4
+
+=item Description
+
+Can only appear when B<role> = slave. If set, the address mapping will not
+start from the beginning the backing memory region, but from the middle
+(B<offet> bytes away from the beginning) of it. See the graph below:
+
+With B<offset> = 0, the mapping will look like:
+
+ backing memory region: #########################################
+ | |
+ | |
+ | |
+ V V
+ slave's shared region: #########################
+
+With B<offset> > 0:
+
+ backing memory region: #########################################
+ |<-- offset -->|| |
+ | |
+ | |
+ V V
+ slave's memory region: #########################
+
+=item Supported values
+
+Decimals or hexadecimals with an optional prefix "0x", and should be the
+multiple of the hypervisor page granularity (currently 4K on both ARM and x86).
+
+=item Default value
+
+0x0
+
+=back
+
+=item B<role>
+
+=over 4
+
+=item Description
+
+The backing area would be taken from one domain, which we will mark
+as the "master domain", and this domain should be created prior to any
+other slave domains that depend on it.
+
+This arugment specifies the role of this domain.
+
+=item Supported values
+
+master, slave
+
+=item Default value
+
+slave
+
+=back
+
+=item B<prot>
+
+=over 4
+
+=item Description
+
+When B<role> = master, this means the largest set of stage-2 permission flags
+that can be granted to the slave domains. When B<role> = slave, this means the
+stage-2 permission flags of the shared memory area.
+
+=item Supported values
+
+Currently only 'rw' is supported.
+
+=item Default value
+
+rw
+
+=back
+
+=item B<cache_policy>
+
+=over 4
+
+=item Description
+
+The stage-2 cacheability/shareability attributes of the shared memory area.
+This can only appear when B<role> = master.
+
+=item Supported values
+
+Currently, only the following policy is supported:
+
+=over 4
+
+=item B<ARM_normal>
+
+Only applicable to ARM guests. This would mean Inner and Outer Write-Back
+Cacheable, and Inner Shareable.
+
+=back
+
+=item Default value
+
+ARM_normal
+
+=back
+
+=back
+
+=head1 TYPICAL USAGE
+
+A typical procedure of setting up a shared mem region among several VMs is:
+
+=over 4
+
+1. Add a static_shm option to the master domain's xl config file, assign an
+B<ID> to it and mark it's B<role> as master, and set up the boundaries, prot
+flag, and B<cache_policy> appropriately.
+
+2. Add a static_shm option to every slave domain's xl config file, set
+their B<ID> to the same value as the master's, and set up the B<offset>,
+boundaries and prot flag appropriately.
+
+3. Create the master domain.
+
+4. Create the slaves.
+
+=back
+
+Remember that the master domain must be created before any slave domains could
+be created, for the slaves depend on the memory pages backed by their master.
+
+=head1 Example
+
+Suppose that we have 3 domains: vm1~vm3. And we want to setup two shared
+regions, say, ID1 and ID2, among the three domains, with the following address
+mapping:
+
+ ID1: (vm1 : 0x100000~0x200000) <=====> (vm2 : 0x500000~0x600000)
+ ID2: (vm1 : 0x310000~0x400000) <=====> (vm3 : 0x690000~0x800000)
+
+According to the syntax defined above, the xl config files of the three domains
+should contains the following content:
+
+In xl config file of vm1:
+ static_shm = [ "id=ID1, begin=0x100000, end=0x200000, role=master,
+cache_policy=x86_normal, prot=rw",
+"id=ID2, begin=0x300000, end=0x400000, role=master" ]
+
+In xl config file of vm2:
+ static_shm = [ "id=ID1, offset=0, begin=0x500000, end=0x600000,
+role=slave, prot=rw" ]
+
+In xl config file of vm3:
+ static_shm = [ "id=ID2, offset=0x10000, begin=0x690000,
+end=0x800000, role=slave" ]
+
+After that, just create vm1 first, and then create vm2 and vm3 in any order.
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index b7b91d8627..21540ba15a 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -277,6 +277,14 @@ memory=8096 will report significantly less memory available for use
than a system with maxmem=8096 memory=8096 due to the memory overhead
of having to track the unused pages.
+=item B<static_shm=[ "SSHM_SPEC", "SSHM_SPEC", ... ]>
+
+Specifies the static shared memory regions of this guest. Static shared
+memory regions enables guests to communicate with each other through
+one or more shared memory regions, even without grant table support.
+Currently, this only works on ARM guests.
+See L<xl-static-shm-configuration(5)> for more details.
+
=back
=head3 Guest Virtual NUMA Configuration
diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 7be2592c74..e3fa8e29ea 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -174,6 +174,14 @@ than this amount of RAM.
The size of the video RAM this domain is configured with.
+#### ~/static_shm/[_a-zA-Z0-9]+/role = ("master"|"slave") []
+
+(Note: Currently, this will only appear on ARM guests.)
+
+The role of this domain in the static shared memory region whose id matches
+the `[_a-zA-Z0-9]+` part in the path. (Described in the manpage
+**xl-static-shm-configuration(5)**).
+
#### ~/device/suspend/event-channel = ""|EVTCHN [w]
The domain's suspend event channel. The toolstack will create this
@@ -610,6 +618,45 @@ for the toolstack to obtain e.g. the domain id of a xenstore domain.
Domain Id of the xenstore domain in case xenstore is provided via a
domain instead of a daemon in dom0.
+#### /local/static_shm/[_a-zA-Z0-9]+/* []
+
+(Note: Currently, this will only appear on ARM guests.)
+
+The following paths contain backing memory parameters of a static shared memory
+whose id matches the `[_a-zA-Z0-9]+` part in the path. Their formats and
+meanings are the same as those in an xl config file, described in the manpage
+**xl-static-shm-configuration(5)**.
+
+* begin/end: the boundary of the backing memory region.
+* prot: the largest set of stage-2 permission flags that can be granted to
+ the slave domains.
+* cache_policy: the stage-2 cacheability/shareability attributes of the backing
+ memory region.
+
+The following paths contain run-time information about the static shared memory
+region.
+
+* master: the domid of the backing domain.
+* slaves: information about the slaves that are sharing the region, see
+ ** /local/static_shm/[_a-zA-Z0-9]+/slaves/$DOMID/* ** below.
+* users: An integer. This is the reference count of the backing memory region,
+ including the master domain itself. When this value reachies 0, the backing
+ memory region will be freed.
+
+#### /local/staitc_shm/[_a-zA-Z0-9]+/slaves/$DOMID/* []
+
+(Note: Currently, this will only appear on ARM guests.)
+
+The following paths contain static shared memory region parameters of a slave
+domain. Their formats and meanings are the same as those in xl config files,
+described in the manpage **xl-static-shm-configuration(5)**.
+
+* begin/end: the boundary of the shared memory region.
+* prot: the stage-2 permission flags of the shared memory area.
+* offset: when mapping the backing memory region to the slave's memory space,
+ the mapping will start from offset bytes after the beginning of the backing
+ memory region.
+
[BLKIF]: http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,io,blkif.h.html
[FBIF]: http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,io,fbif.h.html
[HVMPARAMS]: http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,hvm,params.h.html
--
2.14.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread