Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm

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

Thanks Jan.

> >>> On 29.09.18 at 11:22, <talons.lee@gmail.com> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -899,6 +899,19 @@ hardware domain is architecture dependent.
> >  Note that specifying zero as domU value means zero, while for dom0 it
> > means  to use the default.
> >
> > +### xsm
> > +> `= dummy | flask`
> > +
> > +> Default: `dummy`
> > +
> > +Specify which XSM module should be enabled.  This option is only
> > +available if the hypervisor was compiled with XSM support.
> > +
> > +* `dummy`: this is the default choice.  Basic restriction for common
> > +deployment
> > +  (the dummy module) will be applied.  it's also used when XSM is compiled
> out.
> 
> "It's" (upper case I).
> 
OK. Done.

> > +* `flask`: this is the policy based access control.  To choose this,
> > +the
> > +  separated option in kconfig must also be enabled.
> 
> Perhaps better "separate" (but I'm not a native speaker)?
> 
I prefer separated, as I checked here: https://english.stackexchange.com/questions/13144/separated-versus-separate
But generally both are OK I think.

> > @@ -154,6 +154,17 @@ config XSM_FLASK_POLICY
> >
> >  	  If unsure, say Y.
> >
> > +choice
> > +	prompt "Default XSM implementation"
> > +	depends on XSM
> > +	default XSM_FLASK_DEFAULT if XSM_FLASK
> > +	default XSM_DUMMY_DEFAULT
> > +	config XSM_DUMMY_DEFAULT
> > +		bool "Match non-XSM behavior"
> > +	config XSM_FLASK_DEFAULT
> > +		bool "FLux Advanced Security Kernel" if XSM_FLASK endchoice
> 
> Personally I'd prefer XSM_DEFAULT_*; not sure what others think.
> 
I would like to keep this style, because I see
"
        default "credit" if SCHED_CREDIT_DEFAULT
        default "credit2" if SCHED_CREDIT2_DEFAULT
" in config file.

> > --- a/xen/xsm/xsm_core.c
> > +++ b/xen/xsm/xsm_core.c
> > @@ -31,6 +31,37 @@
> >
> >  struct xsm_operations *xsm_ops;
> >
> > +enum xsm_bootparam {
> > +    XSM_BOOTPARAM_DUMMY,
> > +    XSM_BOOTPARAM_FLASK,
> 
> Considering the #ifdef-s further down, should this perhaps also be put in "#ifdef
> CONFIG_XSM_FLASK", to avoid it mistakenly getting used when the code was
> compiled out?
> 
I would like to avoid #ifdef here, because I prefer the same enum value in different config.

> > +};
> > +
> > +static enum xsm_bootparam __initdata xsm_bootparam = #ifdef
> > +CONFIG_XSM_FLASK_DEFAULT
> > +    XSM_BOOTPARAM_FLASK;
> > +#else
> > +    XSM_BOOTPARAM_DUMMY;
> > +#endif
> > +
> > +static int __init parse_xsm_param(const char *s) {
> > +    int rc = 0;
> > +
> > +    if ( !strcmp(s, "dummy") )
> > +        xsm_bootparam = XSM_BOOTPARAM_DUMMY; #ifdef
> CONFIG_XSM_FLASK
> > +    else if ( !strcmp(s, "flask") )
> > +        xsm_bootparam = XSM_BOOTPARAM_FLASK; #endif
> > +    else {
> 
> Style (brace goes on its own line).

OK. Done.

> 
> > +        printk("XSM: can't parse boot parameter xsm=%s\n", s);
> 
> Again, not being a native speaker, "can't parse" sounds odd. Why not just
> "unknown" or "unknown or unsupported"?

OK. Done.

> 
> > @@ -57,7 +88,20 @@ static int __init xsm_core_init(const void
> *policy_buffer, size_t policy_size)
> >      }
> >
> >      xsm_ops = &dummy_xsm_ops;
> > -    flask_init(policy_buffer, policy_size);
> > +
> > +    switch ( xsm_bootparam )
> > +    {
> > +    case XSM_BOOTPARAM_DUMMY:
> > +        break;
> > +
> > +    case XSM_BOOTPARAM_FLASK:
> > +        flask_init(policy_buffer, policy_size);
> > +        break;
> > +
> > +    default:
> > +        printk("XSM: Invalid value for xsm= boot parameter\n");
> > +        break;
> 
> What situation is this covering, when all enumerators already have their case
> label? Simply ASSERT_UNREACHABLE()?
> 

OK. Done.

> Jan
> 


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