Re: [PATCH RESEND v1 1/7] x86: add a flag to enable Intel processor trace

From: "Jan Beulich" <JBeulich@suse.com>
To: Luwei Kang <luwei.kang@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>,
	xen-devel@lists.xen.org, Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH RESEND v1 1/7] x86: add a flag to enable Intel processor trace
Date: Thu, 26 Apr 2018 06:29:46 -0600	[thread overview]
Message-ID: <5AE1C63A02000078001BEC72@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <1516039953-2988-2-git-send-email-luwei.kang@intel.com>

>>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1009,6 +1009,13 @@ debug hypervisor only).
>  ### idle\_latency\_factor
>  > `= <integer>`
>  
> +### intel\_pt
> +> `= <boolean>`
> +
> +> Default: `true`
> +
> +Flag to enable Intel Processor Trace.

I agree with what Wei has said. In addition please use dashes in preference to
underscores for both command line options and file names (neither of the two
is constrained by C identifier naming restrictions). That said, I'm afraid "pt" is
an acronym we commonly associate with pass-through, so for all of file names,
command line option, and identifiers I'd like to ask for an alternative to be found.
"ptrace" may be an option, as - other than e.g. Linux - we don't associate any
meaning to it.

> --- /dev/null
> +++ b/xen/arch/x86/cpu/intel_pt.c
> @@ -0,0 +1,27 @@
> +/*
> + * intel_pt.c: Support Intel Processor Trace Virtualization.
> + *
> + * Copyright (c) 2018, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Author: Luwei Kang <luwei.kang@intel.com>
> + */
> +
> +#include <xen/types.h>
> +#include <xen/cache.h>
> +#include <xen/init.h>

Why the last one?

> +/* intel_pt: Flag to enable Intel Processor Trace (default on). */
> +bool_t __read_mostly opt_intel_pt = 1;

bool and true please. However, I'm not convinced we want this feature on
by default, even less so right from the first patch in the series.

Jan

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