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

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

Hello Jan,

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, June 29, 2018 6:05 PM
> To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Xin Li
> <talons.lee@xxxxxxxxx>
> Cc: 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>; Daniel de Graaf <dgdegra@xxxxxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
> 
> >>> On 29.06.18 at 11:47, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 29/06/18 10:28, Xin Li wrote:
> >> --- a/docs/misc/xen-command-line.markdown
> >> +++ b/docs/misc/xen-command-line.markdown
> >> @@ -865,6 +865,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 | silo | flask`
> >
> > This should be just "dummy | flask" in this patch, and extended with
> > silo in the next path.  Also, options in this file should be sorted
> > alphabetically, so ### xsm should be near the end, rather than beside 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.  No special restriction will be 
> >> applied.
> >> +  it's also used when XSM is compiled out.
> >> +enum xsm_bootparam __read_mostly xsm_bootparam =
> >> +XSM_BOOTPARAM_DUMMY;
> 
> So why "dummy" instead of "none" (or one of the boolean false strings)?

It seems dummy is not fully stub. (some check by XSM_* classification)
So we want to keep this "dummy" check, and override it.

> 
> > This should be __initdata rather than __read_mostly.  It is safe to be
> > discarded after boot.
> 
> And static.

OK. Done.

> 
> >> +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
> >> +        xsm_bootparam = XSM_BOOTPARAM_INVALID;
> >> +
> >> +    return 0;
> >
> > else
> >     rc = -EINVAL;
> >
> > return rc;
> >
> > As a result, the core command line infrastructure will inform the user
> > if they passed an unrecognised option.
> >
> > ~Andrew
> >
> >> +}
> >> +
> >> +custom_param("xsm", parse_xsm_param);
> 
> Please avoid the blank line above here - in the majority of similar cases, we
> have the custom_param() immediately follow the parsing function.
OK. Done.

> 
> >> @@ -57,7 +81,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:
> >> +        /* empty */
> 
> I'm not sure of the value of this comment.
I just want avoid an empty switch case.

> 
> >> +        break;
> >> +
> >> +    case XSM_BOOTPARAM_FLASK:
> >> +        flask_init(policy_buffer, policy_size);
> >> +        break;
> >> +
> >> +    default:
> >> +        printk("XSM: Invalid value for xsm= boot parameter.\n");
> >> +    }
> 
> Please don't omit the "break;" here.
Sure, done.

> 
> Jan
> 

Best regards

Xin(Talons) Li

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