| Message ID | 20210903190629.11917-10-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xsm: refactoring xsm hooks | expand |
On 03/09/2021 20:06, Daniel P. Smith wrote: > SILO implements a few XSM hooks to extended the decision logic beyond > what is defined in the dummy/default policy. For each of the hooks, it > falls back to the dummy/default policy. The fall back is done a slight > round-about way. "done in a slightly" ? > This commit makes the direct call to the default policy's > logic, xsm_default_action(). > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > xen/xsm/silo.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c > index 6db793f35c..56a330a831 100644 > --- a/xen/xsm/silo.c > +++ b/xen/xsm/silo.c > @@ -17,6 +17,7 @@ > * You should have received a copy of the GNU General Public License along with > * this program; If not, see <http://www.gnu.org/licenses/>. > */ > +#include <xsm/xsm-core.h> > #include <xsm/dummy.h> > > /* > @@ -43,7 +44,7 @@ static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn, > else > { > if ( silo_mode_dom_check(d1, d2) ) > - rc = xsm_evtchn_unbound(d1, chn, id2); > + rc = xsm_default_action(XSM_TARGET, current->domain, d1); > rcu_unlock_domain(d2); > } > > @@ -54,7 +55,7 @@ static int silo_evtchn_interdomain(struct domain *d1, struct evtchn *chan1, > struct domain *d2, struct evtchn *chan2) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_evtchn_interdomain(d1, chan1, d2, chan2); > + return xsm_default_action(XSM_HOOK, d1, d2); > return -EPERM; > } > > @@ -62,21 +63,21 @@ static int silo_grant_mapref(struct domain *d1, struct domain *d2, > uint32_t flags) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_grant_mapref(d1, d2, flags); > + return xsm_default_action(XSM_HOOK, d1, d2); > return -EPERM; > } > > static int silo_grant_transfer(struct domain *d1, struct domain *d2) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_grant_transfer(d1, d2); > + return xsm_default_action(XSM_HOOK, d1, d2); > return -EPERM; > } > > static int silo_grant_copy(struct domain *d1, struct domain *d2) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_grant_copy(d1, d2); > + return xsm_default_action(XSM_HOOK, d1, d2); > return -EPERM; > } > > @@ -86,14 +87,14 @@ static int silo_argo_register_single_source(const struct domain *d1, > const struct domain *d2) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_argo_register_single_source(d1, d2); > + return 0; > return -EPERM; > } > > static int silo_argo_send(const struct domain *d1, const struct domain *d2) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_argo_send(d1, d2); > + return 0; Shouldn't these be XSM_HOOK too? Or should all other XSM_HOOK's be short-circuted to 0? The asymmetry here seems weird. ~Andrew
On 9/6/21 2:55 PM, Andrew Cooper wrote: > On 03/09/2021 20:06, Daniel P. Smith wrote: >> SILO implements a few XSM hooks to extended the decision logic beyond >> what is defined in the dummy/default policy. For each of the hooks, it >> falls back to the dummy/default policy. The fall back is done a slight >> round-about way. > > "done in a slightly" ? Ack. >> This commit makes the direct call to the default policy's >> logic, xsm_default_action(). >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> --- >> xen/xsm/silo.c | 15 ++++++++------- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c >> index 6db793f35c..56a330a831 100644 >> --- a/xen/xsm/silo.c >> +++ b/xen/xsm/silo.c >> @@ -17,6 +17,7 @@ >> * You should have received a copy of the GNU General Public License along with >> * this program; If not, see <http://www.gnu.org/licenses/>. >> */ >> +#include <xsm/xsm-core.h> >> #include <xsm/dummy.h> >> >> /* >> @@ -43,7 +44,7 @@ static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn, >> else >> { >> if ( silo_mode_dom_check(d1, d2) ) >> - rc = xsm_evtchn_unbound(d1, chn, id2); >> + rc = xsm_default_action(XSM_TARGET, current->domain, d1); >> rcu_unlock_domain(d2); >> } >> >> @@ -54,7 +55,7 @@ static int silo_evtchn_interdomain(struct domain *d1, struct evtchn *chan1, >> struct domain *d2, struct evtchn *chan2) >> { >> if ( silo_mode_dom_check(d1, d2) ) >> - return xsm_evtchn_interdomain(d1, chan1, d2, chan2); >> + return xsm_default_action(XSM_HOOK, d1, d2); >> return -EPERM; >> } >> >> @@ -62,21 +63,21 @@ static int silo_grant_mapref(struct domain *d1, struct domain *d2, >> uint32_t flags) >> { >> if ( silo_mode_dom_check(d1, d2) ) >> - return xsm_grant_mapref(d1, d2, flags); >> + return xsm_default_action(XSM_HOOK, d1, d2); >> return -EPERM; >> } >> >> static int silo_grant_transfer(struct domain *d1, struct domain *d2) >> { >> if ( silo_mode_dom_check(d1, d2) ) >> - return xsm_grant_transfer(d1, d2); >> + return xsm_default_action(XSM_HOOK, d1, d2); >> return -EPERM; >> } >> >> static int silo_grant_copy(struct domain *d1, struct domain *d2) >> { >> if ( silo_mode_dom_check(d1, d2) ) >> - return xsm_grant_copy(d1, d2); >> + return xsm_default_action(XSM_HOOK, d1, d2); >> return -EPERM; >> } >> >> @@ -86,14 +87,14 @@ static int silo_argo_register_single_source(const struct domain *d1, >> const struct domain *d2) >> { >> if ( silo_mode_dom_check(d1, d2) ) >> - return xsm_argo_register_single_source(d1, d2); >> + return 0; >> return -EPERM; >> } >> >> static int silo_argo_send(const struct domain *d1, const struct domain *d2) >> { >> if ( silo_mode_dom_check(d1, d2) ) >> - return xsm_argo_send(d1, d2); >> + return 0; > > Shouldn't these be XSM_HOOK too? Or should all other XSM_HOOK's be > short-circuted to 0? > > The asymmetry here seems weird. It makes more sense when you follow the approach, which was to duplicate the body of the dummy hook instead of making a call to the hook which would then call the function pointer to the dummy hook. The definition for the argo dummy hooks is to return 0. In the future these other calls may well have XSM_HOOk replaced with the proper role expected. Since all argo checks just return 0, this reflects there is no logic rules in xsm_default_action to determine argo accesses. Of course this is on my list todo and when the dummy hook is fixed, these would be synchronized. With that said, converting over to XSM_HOOK does provide the equivalent and would provide consistency within the context of this file. Basically a long winded way of saying, ack. v/r, dps
On 03.09.2021 21:06, Daniel P. Smith wrote: > SILO implements a few XSM hooks to extended the decision logic beyond > what is defined in the dummy/default policy. For each of the hooks, it > falls back to the dummy/default policy. The fall back is done a slight > round-about way. This commit makes the direct call to the default policy's > logic, xsm_default_action(). Again it's not clear to me what you're finding wrong here. The way it's done is not as direct as it could be, but going through the extra layer allows the functions to document things at the same time. You lose not only that documentation, but also ... > @@ -43,7 +44,7 @@ static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn, > else > { > if ( silo_mode_dom_check(d1, d2) ) > - rc = xsm_evtchn_unbound(d1, chn, id2); > + rc = xsm_default_action(XSM_TARGET, current->domain, d1); ... will need to sync changes to the dummy xsm_evtchn_unbound(), no matter how unlikely such may be, back to here. This would be quite easy to forget. But maybe I'm overlooking something where how things are really gets in the way of something you mean to do in the remaining two patches (or later)? > static int silo_grant_copy(struct domain *d1, struct domain *d2) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_grant_copy(d1, d2); > + return xsm_default_action(XSM_HOOK, d1, d2); > return -EPERM; > } > > @@ -86,14 +87,14 @@ static int silo_argo_register_single_source(const struct domain *d1, > const struct domain *d2) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_argo_register_single_source(d1, d2); > + return 0; > return -EPERM; > } > > static int silo_argo_send(const struct domain *d1, const struct domain *d2) > { > if ( silo_mode_dom_check(d1, d2) ) > - return xsm_argo_send(d1, d2); > + return 0; > return -EPERM; > } This would then also avoid introducing the anomaly observed by Andrew here. And in fact the Argo dummy functions may be a good example where a change might happen down the road - them being all empty doesn't seem quite right to me. Jan
On 9/9/21 11:45 AM, Jan Beulich wrote: > On 03.09.2021 21:06, Daniel P. Smith wrote: >> SILO implements a few XSM hooks to extended the decision logic beyond >> what is defined in the dummy/default policy. For each of the hooks, it >> falls back to the dummy/default policy. The fall back is done a slight >> round-about way. This commit makes the direct call to the default policy's >> logic, xsm_default_action(). > > Again it's not clear to me what you're finding wrong here. The way it's > done is not as direct as it could be, but going through the extra layer > allows the functions to document things at the same time. You lose not > only that documentation, but also ... It is only for six calls, thus I figured the slight overhead would be worth cutting out the indirection. If now one is worried about the extra indirection, than I can adjust to call the default's handlers.
diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c index 6db793f35c..56a330a831 100644 --- a/xen/xsm/silo.c +++ b/xen/xsm/silo.c @@ -17,6 +17,7 @@ * You should have received a copy of the GNU General Public License along with * this program; If not, see <http://www.gnu.org/licenses/>. */ +#include <xsm/xsm-core.h> #include <xsm/dummy.h> /* @@ -43,7 +44,7 @@ static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn, else { if ( silo_mode_dom_check(d1, d2) ) - rc = xsm_evtchn_unbound(d1, chn, id2); + rc = xsm_default_action(XSM_TARGET, current->domain, d1); rcu_unlock_domain(d2); } @@ -54,7 +55,7 @@ static int silo_evtchn_interdomain(struct domain *d1, struct evtchn *chan1, struct domain *d2, struct evtchn *chan2) { if ( silo_mode_dom_check(d1, d2) ) - return xsm_evtchn_interdomain(d1, chan1, d2, chan2); + return xsm_default_action(XSM_HOOK, d1, d2); return -EPERM; } @@ -62,21 +63,21 @@ static int silo_grant_mapref(struct domain *d1, struct domain *d2, uint32_t flags) { if ( silo_mode_dom_check(d1, d2) ) - return xsm_grant_mapref(d1, d2, flags); + return xsm_default_action(XSM_HOOK, d1, d2); return -EPERM; } static int silo_grant_transfer(struct domain *d1, struct domain *d2) { if ( silo_mode_dom_check(d1, d2) ) - return xsm_grant_transfer(d1, d2); + return xsm_default_action(XSM_HOOK, d1, d2); return -EPERM; } static int silo_grant_copy(struct domain *d1, struct domain *d2) { if ( silo_mode_dom_check(d1, d2) ) - return xsm_grant_copy(d1, d2); + return xsm_default_action(XSM_HOOK, d1, d2); return -EPERM; } @@ -86,14 +87,14 @@ static int silo_argo_register_single_source(const struct domain *d1, const struct domain *d2) { if ( silo_mode_dom_check(d1, d2) ) - return xsm_argo_register_single_source(d1, d2); + return 0; return -EPERM; } static int silo_argo_send(const struct domain *d1, const struct domain *d2) { if ( silo_mode_dom_check(d1, d2) ) - return xsm_argo_send(d1, d2); + return 0; return -EPERM; }
SILO implements a few XSM hooks to extended the decision logic beyond what is defined in the dummy/default policy. For each of the hooks, it falls back to the dummy/default policy. The fall back is done a slight round-about way. This commit makes the direct call to the default policy's logic, xsm_default_action(). Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/xsm/silo.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)