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

["Xin Li (Talons)" <xin.li@xxxxxxxxxx>]

Hello Jan,

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, July 3, 2018 3:34 PM
> To: Xin Li <talons.lee@xxxxxxxxx>; Daniel de Graaf <dgdegra@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Ming Lu
> <ming.lu@xxxxxxxxxx>; Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Xin Li (Talons) <xin.li@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxx; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 03.07.18 at 03:26, <talons.lee@xxxxxxxxx> 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.

OK.

> 
> > +/*
> > + * 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)
OK.

static bool silo_mode_dom_check(const struct domain *ldom,
                                                        const struct domain 
*rdom)
{
    const struct domain *cur_dom = current->domain;

    return (is_control_domain(cur_dom) || is_control_domain(ldom) ||
            is_control_domain(rdom) || ldom == rdom);
}
> 
> > +    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_evtchn_unbound(struct domain *d1, struct evtchn *chn,
                               domid_t id2)
{   
    int rc = -EPERM;
    struct domain *d2;
    
    if ( id2 == DOMID_SELF )
        id2 = current->domain->domain_id;
    d2 = rcu_lock_domain_by_id(id2);
    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )
        rc = dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
    rcu_unlock_domain(d2);
    
    return rc;
}

> 
> > +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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel