Re: [PATCH 02/10] VMX: New parameter to control PML enabling

[Kai Huang <kai.huang@linux.intel.com>]

On 03/28/2015 04:42 AM, Andrew Cooper wrote:
> On 27/03/15 02:35, Kai Huang wrote:
>> A top level EPT parameter "ept=<options>" and a sub boolean "pml_enable" are
>> added to control PML. Other booleans can be further added for any other EPT
>> related features.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> Please patch docs/misc/xen-command-line.markdown as well.  See the
> existing "psr" option as a similar example.
>
> Also, as indicated in patch 1, I think patches 1 and 2 need swapping in
> the series.
>
>> ---
>>   xen/arch/x86/hvm/vmx/vmcs.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 2f645fe..9b20a4b 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -50,6 +50,16 @@ boolean_param("unrestricted_guest", opt_unrestricted_guest_enabled);
>>   static bool_t __read_mostly opt_apicv_enabled = 1;
>>   boolean_param("apicv", opt_apicv_enabled);
>>   
>> +static void parse_ept_param(char *s);
>> +/*
>> + * The 'ept' parameter controls functionalities that depend on, or impact the
>> + * EPT mechanism. Optional comma separated value may contain:
>> + *
>> + *  pml                 Enable PML
>> + */
>> +custom_param("ept", parse_ept_param);
> It is common to put the custom_param() call below parse_ept_param() so
> you don't need to forward-declare the function.  The comment can happily
> live at the top of parse_ept_param().
Hi Andrew,

Looks it's better to keep parse_ept_param() below custom_param(), as 
simply moving parse_ept_param() above custom_param() results in below 
error (I also changed pml_enable to opt_pml_enabled), as it references 
opt_pml_enabled variable, which is defined below custom_param(). 
Actually for "iommu=<options>" parameter, parse_iommu_param() was also 
placed below custom_param().

What do you think?

vmcs.c: In function ‘parse_ept_param’:
vmcs.c:74:13: error: ‘opt_pml_enabled’ undeclared (first use in this 
function)
              opt_pml_enabled = val;
              ^
vmcs.c:74:13: note: each undeclared identifier is reported only once for 
each function it appears in
vmcs.c: At top level:
vmcs.c:81:29: error: ‘opt_pml_enabled’ defined but not used 
[-Werror=unused-variable]
  static bool_t __read_mostly opt_pml_enabled = 0;

Thanks,
-Kai

>
>> +static bool_t __read_mostly pml_enable = 0;
>> +
>>   /*
>>    * These two parameters are used to config the controls for Pause-Loop Exiting:
>>    * ple_gap:    upper bound on the amount of time between two successive
>> @@ -92,6 +102,28 @@ DEFINE_PER_CPU(bool_t, vmxon);
>>   static u32 vmcs_revision_id __read_mostly;
>>   u64 __read_mostly vmx_basic_msr;
>>   
>> +/* Copied from parse_iommu_param */
> Not a useful comment, as it is likely to diverge in the future.
>
>> +static void parse_ept_param(char *s)
> __init
>
> ~Andrew
>
>> +{
>> +    char *ss;
>> +    int val;
>> +
>> +    do {
>> +        val = !!strncmp(s, "no-", 3);
>> +        if ( !val )
>> +            s += 3;
>> +
>> +        ss = strchr(s, ',');
>> +        if ( ss )
>> +            *ss = '\0';
>> +
>> +        if ( !strcmp(s, "pml") )
>> +            pml_enable = val;
>> +
>> +        s = ss + 1;
>> +    } while ( ss );
>> +}
>> +
>>   static void __init vmx_display_features(void)
>>   {
>>       int printed = 0;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel