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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/04/15 06:46, Kai Huang wrote:
>
>
> 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;

The most concise way of doing this is:

static bool_t __read_mostly opt_pml_enabled = 0;

static void parse_ept_param(char *s)
{
  ...
}
custom_param("ept", parse_ept_param);

~Andrew