Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

From: "Jan Beulich" <JBeulich@suse.com>
To: Xin Li <talons.lee@gmail.com>, Daniel de Graaf <dgdegra@tycho.nsa.gov>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>,
	xen-devel@lists.xen.org, Xin Li <xin.li@citrix.com>,
	Ming Lu <ming.lu@citrix.com>
Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
Date: Tue, 03 Jul 2018 01:33:54 -0600	[thread overview]
Message-ID: <5B3B26E202000078001D0114@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20180703012629.507-2-xin.li@citrix.com>

>>> On 03.07.18 at 03:26, <talons.lee@gmail.com> wrote:
> v2
> To further discuss:
> 1) is the new Kconfig option XSM_SILO necessary?
> we can handle SILO similar as DUMMY, using exsting CONFIG_XSM.
> 
> 2) explain "unmediated communication channel"

I'm confused: As said in the reply to patch 1, this section is generally
expected to contain information on what has changed from the prior
version. I take it the above item instead related to the "To further
discuss" sub-heading.

> 3) is it OK to use the indirect call dummy_xsm_ops.evtchn_unbound?

I'm not convinced it was worthwhile to send v2 with all of these still
open.

> +/*
> + * Check if inter-domain communication is allowed.
> + * Return true when pass check.
> + */
> +static bool silo_mode_dom_check(struct domain *ldom, struct domain *rdom)
> +{
> +    struct domain *cur_dom = current->domain;

const (three times altogether)

> +    return (is_control_domain(cur_dom) || is_control_domain(ldom) ||
> +            is_control_domain(rdom) || ldom == rdom);
> +}
> +
> +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> +                               domid_t id2)
> +{
> +    int rc = -EPERM;
> +    struct domain *d2 = rcu_lock_domain_by_id(id2);
> +    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )

Blank line please between declaration(s) and statement(s). And
const on the local variable declaration again.

Also, is DOMID_SELF really not allowed here for id2? I don't think
so, looking at e.g. evtchn_alloc_unbound().

> +static int silo_grant_copy(struct domain *d1, struct domain *d2)
> +{
> +    if ( silo_mode_dom_check(d1, d2) )
> +        return dummy_xsm_ops.grant_copy(d1, d2);
> +    return -EPERM;
> +}

I know transitive grants are a bad child, but they shouldn't be left out
altogether in deciding what SILO mode is going to mean. In fact it looks
to me as if there was no XSM check at all for the second half of a
transitive grant copy's domain handling (the recursive
acquire_grant_for_copy() call), which would mean that the fencing
SILO mode looks to mean to establish wouldn't be complete. Daniel?

Speaking of completeness: What about TMEM? Isn't one of the two
pool types also meant to allow page sharing between domains?

Jan

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