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

[Kai Huang <kaih.linux@xxxxxxxxx>]

On Thu, Apr 2, 2015 at 5:58 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxx>
>>> 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);
>
Sure. Thanks.


> ~Andrew
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



-- 
Thanks,
-Kai

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel