From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 02/10] VMX: New parameter to control PML enabling Date: Thu, 2 Apr 2015 10:58:43 +0100 Message-ID: <551D12D3.10401@citrix.com> References: <1427423754-11841-1-git-send-email-kai.huang@linux.intel.com> <1427423754-11841-3-git-send-email-kai.huang@linux.intel.com> <5515C09A.9040303@citrix.com> <551CD7A2.9020409@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <551CD7A2.9020409@linux.intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Kai Huang , jbeulich@suse.com, tim@xen.org, kevin.tian@intel.com, yang.z.zhang@intel.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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=3D" 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 >> 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 =3D 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=3D" parameter, parse_iommu_param() was also > placed below custom_param(). > > What do you think? > > vmcs.c: In function =91parse_ept_param=92: > vmcs.c:74:13: error: =91opt_pml_enabled=92 undeclared (first use in this > function) > opt_pml_enabled =3D 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: =91opt_pml_enabled=92 defined but not used > [-Werror=3Dunused-variable] > static bool_t __read_mostly opt_pml_enabled =3D 0; The most concise way of doing this is: static bool_t __read_mostly opt_pml_enabled =3D 0; static void parse_ept_param(char *s) { ... } custom_param("ept", parse_ept_param); ~Andrew