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

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>, Xin Li <talons.lee@gmail.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Xin Li <xin.li@citrix.com>, Tim Deegan <tim@xen.org>,
	xen-devel@lists.xen.org, Ming Lu <ming.lu@citrix.com>,
	Daniel de Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Date: Fri, 29 Jun 2018 04:04:42 -0600	[thread overview]
Message-ID: <5B36043A02000078001CF0AC@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <03f7c8f0-b129-d787-5716-4efbc10100af@citrix.com>

>>> On 29.06.18 at 11:47, <andrew.cooper3@citrix.com> 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)?

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

And static.

>> +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.

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

>> +        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.

Jan

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