Re: [PATCH v4 09/11] silo: remove circular xsm hook call

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >   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