[PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Luwei Kang]

Hi All,

Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf
In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.

Introduction:
Intel Processor Trace (Intel PT) is an extension of Intel Architecture that captures information about software execution using dedicated hardware facilities that cause only minimal performance perturbation to the software being traced. Details on the Intel PT infrastructure and trace capabilities can be found in the Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.

The suite of architecture changes serve to simplify the process of virtualizing Intel PT for use by a guest software. There are two primary elements to this new architecture support for VMX support improvements made for Intel PT.
1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
  — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
2. Enabling use of EPT to redirect PT output.
  — This enables the VMM to elect to virtualize the PT output buffer using EPT. In this mode, the CPU will treat PT output addresses as Guest Physical Addresses (GPAs) and translate them using EPT. This means that Intel PT output reads (of the ToPA table) and writes (of trace output) can cause EPT violations, and other output events.

=======
Do some optimization in context switch compared with the first sent:
1. disable intercept only when PT is enabled in guest;
2. disable Intel PT and enable intercept MSRs when L1 guest VMXON;


Luwei Kang (7):
  x86: add a flag to enable Intel processor trace
  x86: configure vmcs for Intel processor trace virtualization
  x86: add intel proecessor trace support for cpuid
  x86: add intel processor trace context
  x86: Implement Intel Processor Trace context switch
  x86: Implement Intel Processor Trace MSRs read/write
  x86: Disable Intel Processor Trace when VMXON in L1 guest

 docs/misc/xen-command-line.markdown         |   7 +
 tools/libxc/xc_cpuid_x86.c                  |  12 +-
 xen/arch/x86/cpu/Makefile                   |   1 +
 xen/arch/x86/cpu/intel_pt.c                 | 197 ++++++++++++++++++++++++++++
 xen/arch/x86/cpuid.c                        |  22 ++++
 xen/arch/x86/domctl.c                       |   4 +
 xen/arch/x86/hvm/vmx/vmcs.c                 |  36 ++++-
 xen/arch/x86/hvm/vmx/vmx.c                  |  22 ++++
 xen/arch/x86/hvm/vmx/vvmx.c                 |   7 +-
 xen/include/asm-x86/cpufeature.h            |   1 +
 xen/include/asm-x86/cpuid.h                 |  12 +-
 xen/include/asm-x86/hvm/vmx/vmcs.h          |  12 ++
 xen/include/asm-x86/intel_pt.h              |  51 +++++++
 xen/include/asm-x86/msr-index.h             |  20 +++
 xen/include/public/arch-x86/cpufeatureset.h |   1 +
 15 files changed, 395 insertions(+), 10 deletions(-)
 create mode 100644 xen/arch/x86/cpu/intel_pt.c
 create mode 100644 xen/include/asm-x86/intel_pt.h

-- 
1.8.3.1


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

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

[Luwei Kang]

This patch add a flag to enable Intel PT (Intel processor trace).
Default value is 1 (enabled).

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 docs/misc/xen-command-line.markdown |  7 +++++++
 xen/arch/x86/cpu/Makefile           |  1 +
 xen/arch/x86/cpu/intel_pt.c         | 27 +++++++++++++++++++++++++++
 xen/include/asm-x86/intel_pt.h      | 26 ++++++++++++++++++++++++++
 4 files changed, 61 insertions(+)
 create mode 100644 xen/arch/x86/cpu/intel_pt.c
 create mode 100644 xen/include/asm-x86/intel_pt.h

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 781110d..95411cf 100644
--- 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.
+
 ### ioapic\_ack
 > `= old | new`
 
diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index 74f23ae..33d7a74 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -8,3 +8,4 @@ obj-y += intel.o
 obj-y += intel_cacheinfo.o
 obj-y += mwait-idle.o
 obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
+obj-y += intel_pt.o
diff --git a/xen/arch/x86/cpu/intel_pt.c b/xen/arch/x86/cpu/intel_pt.c
new file mode 100644
index 0000000..520e0ca
--- /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>
+
+/* intel_pt: Flag to enable Intel Processor Trace (default on). */
+bool_t __read_mostly opt_intel_pt = 1;
+boolean_param("intel_pt", opt_intel_pt);
diff --git a/xen/include/asm-x86/intel_pt.h b/xen/include/asm-x86/intel_pt.h
new file mode 100644
index 0000000..2a8b579
--- /dev/null
+++ b/xen/include/asm-x86/intel_pt.h
@@ -0,0 +1,26 @@
+/*
+ * intel_pt.h: Intel Processor Trace virtualization for HVM domain.
+ *
+ * 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>
+ */
+
+#ifndef __ASM_X86_HVM_INTEL_PT_H_
+#define __ASM_X86_HVM_INTEL_PT_H_
+
+extern bool_t opt_intel_pt;
+
+#endif /* __ASM_X86_HVM_INTEL_PT_H_ */
-- 
1.8.3.1


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

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

[Wei Liu]

On Tue, Jan 16, 2018 at 02:12:27AM +0800, Luwei Kang wrote:
> This patch add a flag to enable Intel PT (Intel processor trace).
> Default value is 1 (enabled).
> 
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---
>  docs/misc/xen-command-line.markdown |  7 +++++++
>  xen/arch/x86/cpu/Makefile           |  1 +
>  xen/arch/x86/cpu/intel_pt.c         | 27 +++++++++++++++++++++++++++
>  xen/include/asm-x86/intel_pt.h      | 26 ++++++++++++++++++++++++++
>  4 files changed, 61 insertions(+)
>  create mode 100644 xen/arch/x86/cpu/intel_pt.c
>  create mode 100644 xen/include/asm-x86/intel_pt.h
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 781110d..95411cf 100644
> --- 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.
> +
>  ### ioapic\_ack
>  > `= old | new`
>  

No document for this option?

> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
> index 74f23ae..33d7a74 100644
> --- a/xen/arch/x86/cpu/Makefile
> +++ b/xen/arch/x86/cpu/Makefile
> @@ -8,3 +8,4 @@ obj-y += intel.o
>  obj-y += intel_cacheinfo.o
>  obj-y += mwait-idle.o
>  obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
> +obj-y += intel_pt.o

Move this after intel_cacheinfo please.

We order things alphabetically.

> diff --git a/xen/arch/x86/cpu/intel_pt.c b/xen/arch/x86/cpu/intel_pt.c
> new file mode 100644
> index 0000000..520e0ca
> --- /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>

Please order the headers alphabetically.

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

Use plain bool and true please.

> +boolean_param("intel_pt", opt_intel_pt);
> diff --git a/xen/include/asm-x86/intel_pt.h b/xen/include/asm-x86/intel_pt.h
> new file mode 100644
> index 0000000..2a8b579
> --- /dev/null
> +++ b/xen/include/asm-x86/intel_pt.h
> @@ -0,0 +1,26 @@
> +/*
> + * intel_pt.h: Intel Processor Trace virtualization for HVM domain.
> + *
> + * 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>
> + */
> +
> +#ifndef __ASM_X86_HVM_INTEL_PT_H_
> +#define __ASM_X86_HVM_INTEL_PT_H_
> +
> +extern bool_t opt_intel_pt;
> +
> +#endif /* __ASM_X86_HVM_INTEL_PT_H_ */
> -- 
> 1.8.3.1
> 

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

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

[Kang, Luwei]

> > diff --git a/docs/misc/xen-command-line.markdown
> > b/docs/misc/xen-command-line.markdown
> > index 781110d..95411cf 100644
> > --- 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.
> > +
> >  ### ioapic\_ack
> >  > `= old | new`
> >
> 
> No document for this option?

Thanks for the code review. Will add a simple description here.

> 
> > diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
> > index 74f23ae..33d7a74 100644
> > --- a/xen/arch/x86/cpu/Makefile
> > +++ b/xen/arch/x86/cpu/Makefile
> > @@ -8,3 +8,4 @@ obj-y += intel.o
> >  obj-y += intel_cacheinfo.o
> >  obj-y += mwait-idle.o
> >  obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
> > +obj-y += intel_pt.o
> 
> Move this after intel_cacheinfo please.
> 
> We order things alphabetically.

Will check and reorder all the things in next version.

> > +
> > +/* intel_pt: Flag to enable Intel Processor Trace (default on). */
> > +bool_t __read_mostly opt_intel_pt = 1;
> 
> Use plain bool and true please.

Will fix it. BTW, I am thinking of make it "false" as default to reduce overhead.

Thanks,
Luwei Kang

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

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

[Wei Liu]

On Tue, Jan 16, 2018 at 02:12:27AM +0800, Luwei Kang wrote:
> This patch add a flag to enable Intel PT (Intel processor trace).
> Default value is 1 (enabled).
> 
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---
>  docs/misc/xen-command-line.markdown |  7 +++++++
>  xen/arch/x86/cpu/Makefile           |  1 +
>  xen/arch/x86/cpu/intel_pt.c         | 27 +++++++++++++++++++++++++++
>  xen/include/asm-x86/intel_pt.h      | 26 ++++++++++++++++++++++++++
>  4 files changed, 61 insertions(+)
>  create mode 100644 xen/arch/x86/cpu/intel_pt.c
>  create mode 100644 xen/include/asm-x86/intel_pt.h
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 781110d..95411cf 100644
> --- 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`
> +

After reading the manual a bit I think this option needs to be more
sophisticated. 

The series only implements guest-only tracing, while in the future we might
want host-only tracing and system wide tracing.

Even the other modes aren't implemented yet we should leave room for
them.

Wei.

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

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

[Kang, Luwei]

> > diff --git a/docs/misc/xen-command-line.markdown
> > b/docs/misc/xen-command-line.markdown
> > index 781110d..95411cf 100644
> > --- 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`
> > +
> 
> After reading the manual a bit I think this option needs to be more
> sophisticated.
> 
> The series only implements guest-only tracing, while in the future we might
> want host-only tracing and system wide tracing.
> 
> Even the other modes aren't implemented yet we should leave room for
> them.
> 

Hi Wei,
     Thanks for the review. So what about define guest mode like this and make the option as a string. Other mode can be added like 'system | host' in future.

### Intel\_pt
> `= guest`

<some description>

By the way, you mentioned in another said that,  "No document for this option here?". Do you mean the description is too simple in this patch or I need add some words in a another document?

Thanks,
Luwei Kang

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

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

[Wei Liu]

On Fri, Apr 27, 2018 at 08:22:00AM +0000, Kang, Luwei wrote:
> > > diff --git a/docs/misc/xen-command-line.markdown
> > > b/docs/misc/xen-command-line.markdown
> > > index 781110d..95411cf 100644
> > > --- 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`
> > > +
> > 
> > After reading the manual a bit I think this option needs to be more
> > sophisticated.
> > 
> > The series only implements guest-only tracing, while in the future we might
> > want host-only tracing and system wide tracing.
> > 
> > Even the other modes aren't implemented yet we should leave room for
> > them.
> > 
> 
> Hi Wei,
>      Thanks for the review. So what about define guest mode like this and make the option as a string. Other mode can be added like 'system | host' in future.
> 
> ### Intel\_pt
> > `= guest`
> 

I'm fine with this.

> <some description>
> 
> By the way, you mentioned in another said that,  "No document for this option here?". Do you mean the description is too simple in this patch or I need add some words in a another document?
> 

The one-liner provided in this patch is too terse.

You need to describe what this option is used for and how to use it. See
the other items in xen-command-line.markdown for example.

Wei.

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

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

[Jan Beulich]

>>> On 27.04.18 at 10:22, <luwei.kang@intel.com> wrote:
>> > diff --git a/docs/misc/xen-command-line.markdown
>> > b/docs/misc/xen-command-line.markdown
>> > index 781110d..95411cf 100644
>> > --- 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`
>> > +
>> 
>> After reading the manual a bit I think this option needs to be more
>> sophisticated.
>> 
>> The series only implements guest-only tracing, while in the future we might
>> want host-only tracing and system wide tracing.
>> 
>> Even the other modes aren't implemented yet we should leave room for
>> them.
>> 
> 
> Hi Wei,
>      Thanks for the review. So what about define guest mode like this and 
> make the option as a string. Other mode can be added like 'system | host' in 
> future.
> 
> ### Intel\_pt
>> `= guest`

That's still too simple, as you implement it for HVM only. While it's not clear
to me whether it could be done properly for PV, that path shouldn't be closed
unless it's crystal clear that it's impossible to ever happen.

Jan



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

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

[Kang, Luwei]

> >>> On 27.04.18 at 10:22, <luwei.kang@intel.com> wrote:
> >> > diff --git a/docs/misc/xen-command-line.markdown
> >> > b/docs/misc/xen-command-line.markdown
> >> > index 781110d..95411cf 100644
> >> > --- 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`
> >> > +
> >>
> >> After reading the manual a bit I think this option needs to be more
> >> sophisticated.
> >>
> >> The series only implements guest-only tracing, while in the future we
> >> might want host-only tracing and system wide tracing.
> >>
> >> Even the other modes aren't implemented yet we should leave room for
> >> them.
> >>
> >
> > Hi Wei,
> >      Thanks for the review. So what about define guest mode like this
> > and make the option as a string. Other mode can be added like 'system
> > | host' in future.
> >
> > ### Intel\_pt
> >> `= guest`
> 
> That's still too simple, as you implement it for HVM only. While it's not clear
> to me whether it could be done properly for PV, that path shouldn't be
> closed unless it's crystal clear that it's impossible to ever happen.

The great change in Intel Processor Trace improvements is CPU will treat PT output addresses as Guest Physical Addresses (GPAs) and translate them using EPT. So if EPT is not supported or disabled in PV guest, intel PT feature can't be exposed to guest.
System wide mode may have a little different, we can disable intel address translated by EPT and enable PT in dom0  to record the behavior of hypervisor and  guest. Although Intel PT is still enabled(IA32_RTIT_CTL.EN=1) in guest but actually guest don't aware this. This mode is not implemented in this patch set and may need more discussion in next step.

> 
> Jan
> 


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

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

[Jan Beulich]

>>> 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

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

[Kang, Luwei]

> > --- 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.
> 
Hi Jan,
   Thanks for you review. "ptrace" make me associate "strace", "ftrace". Although they are complete  different things but I think "ptrace" is not good enough to present "Intel Processor Trace".

Thanks,
Luwei Kang

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

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

[Jan Beulich]

>>> On 27.04.18 at 11:01, <luwei.kang@intel.com> wrote:
>    Thanks for you review. "ptrace" make me associate "strace", "ftrace". 
> Although they are complete  different things but I think "ptrace" is not good 
> enough to present "Intel Processor Trace".

Then how about ipt instead of just pt?

Jan



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

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

[Kang, Luwei]

> >>> On 27.04.18 at 11:01, <luwei.kang@intel.com> wrote:
> >    Thanks for you review. "ptrace" make me associate "strace", "ftrace".
> > Although they are complete  different things but I think "ptrace" is
> > not good enough to present "Intel Processor Trace".
> 
> Then how about ipt instead of just pt?

"ipt" is looks good to me.

Thanks,
Luwei Kang

> 
> Jan
> 


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

[PATCH RESEND v1 2/7] x86: configure vmcs for Intel processor trace virtualization

[Luwei Kang]

This patch configure VMCS to make Intel PT output address can be
treat as guest physical address and translated by EPT when
intel_pt option is true.
There have some constraint condition on VMCS configuration,
otherwise will cause VM entry failed.

1. If the “Guest PT uses Guest Physical Addresses” execution
   control is 1, the “Clear IA32_RTIT_CTL on exit” exit
   control and the “Load IA32_RTIT_CTL on entry” entry
   control must also be 1.
2. If the “Guest PT uses Guest Physical Addresses” execution
   control is 1, the "enable EPT" execution control must
   also be 1.

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c        | 36 +++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/hvm/vmx/vmcs.h |  7 +++++++
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index e7818ca..8d49a6b 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -40,6 +40,7 @@
 #include <asm/shadow.h>
 #include <asm/tboot.h>
 #include <asm/apic.h>
+#include <asm/intel_pt.h>
 
 static bool_t __read_mostly opt_vpid_enabled = 1;
 boolean_param("vpid", opt_vpid_enabled);
@@ -242,6 +243,9 @@ static int vmx_init_vmcs_config(void)
         rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
         if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL )
             opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
+        if ( _vmx_misc_cap & VMX_MISC_PT_ENABLE )
+            opt |= SECONDARY_EXEC_PT_USE_GPA |
+                   SECONDARY_EXEC_CONCEAL_PT_PIP;
         if ( opt_vpid_enabled )
             opt |= SECONDARY_EXEC_ENABLE_VPID;
         if ( opt_unrestricted_guest_enabled )
@@ -343,7 +347,8 @@ static int vmx_init_vmcs_config(void)
 
     min = VM_EXIT_ACK_INTR_ON_EXIT;
     opt = VM_EXIT_SAVE_GUEST_PAT | VM_EXIT_LOAD_HOST_PAT |
-          VM_EXIT_CLEAR_BNDCFGS;
+          VM_EXIT_CLEAR_BNDCFGS | VM_EXIT_CONCEAL_PT_PIP |
+          VM_EXIT_CLEAR_IA32_RTIT_CTL;
     min |= VM_EXIT_IA32E_MODE;
     _vmx_vmexit_control = adjust_vmx_controls(
         "VMExit Control", min, opt, MSR_IA32_VMX_EXIT_CTLS, &mismatch);
@@ -383,13 +388,28 @@ static int vmx_init_vmcs_config(void)
         _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
 
     min = 0;
-    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS;
+    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS |
+          VM_ENTRY_CONCEAL_PT_PIP | VM_ENTRY_LOAD_IA32_RTIT_CTL;
     _vmx_vmentry_control = adjust_vmx_controls(
         "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS, &mismatch);
 
     if ( mismatch )
         return -EINVAL;
 
+    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) ||
+         !(_vmx_secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA) ||
+         !(_vmx_vmexit_control & VM_EXIT_CLEAR_IA32_RTIT_CTL) ||
+         !(_vmx_vmentry_control & VM_ENTRY_LOAD_IA32_RTIT_CTL) )
+    {
+        _vmx_secondary_exec_control &= ~(SECONDARY_EXEC_PT_USE_GPA |
+                                         SECONDARY_EXEC_CONCEAL_PT_PIP);
+        _vmx_vmexit_control &= ~(VM_EXIT_CONCEAL_PT_PIP |
+                                 VM_EXIT_CLEAR_IA32_RTIT_CTL);
+        _vmx_vmentry_control &= ~(VM_ENTRY_CONCEAL_PT_PIP |
+                                  VM_ENTRY_LOAD_IA32_RTIT_CTL);
+        opt_intel_pt = 0;
+    }
+
     if ( !vmx_pin_based_exec_control )
     {
         /* First time through. */
@@ -1029,10 +1049,16 @@ static int construct_vmcs(struct vcpu *v)
         v->arch.hvm_vmx.secondary_exec_control &= 
             ~(SECONDARY_EXEC_ENABLE_EPT | 
               SECONDARY_EXEC_UNRESTRICTED_GUEST |
-              SECONDARY_EXEC_ENABLE_INVPCID);
+              SECONDARY_EXEC_ENABLE_INVPCID |
+              SECONDARY_EXEC_PT_USE_GPA |
+              SECONDARY_EXEC_CONCEAL_PT_PIP);
         vmexit_ctl &= ~(VM_EXIT_SAVE_GUEST_PAT |
-                        VM_EXIT_LOAD_HOST_PAT);
-        vmentry_ctl &= ~VM_ENTRY_LOAD_GUEST_PAT;
+                        VM_EXIT_LOAD_HOST_PAT |
+                        VM_EXIT_CONCEAL_PT_PIP |
+                        VM_EXIT_CLEAR_IA32_RTIT_CTL);
+        vmentry_ctl &= ~(VM_ENTRY_LOAD_GUEST_PAT |
+                         VM_ENTRY_CONCEAL_PT_PIP |
+                         VM_ENTRY_LOAD_IA32_RTIT_CTL);
     }
 
     /* Disable Virtualize x2APIC mode by default. */
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 8fb9e3c..bd8a128 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -220,6 +220,8 @@ extern u32 vmx_pin_based_exec_control;
 #define VM_EXIT_LOAD_HOST_EFER          0x00200000
 #define VM_EXIT_SAVE_PREEMPT_TIMER      0x00400000
 #define VM_EXIT_CLEAR_BNDCFGS           0x00800000
+#define VM_EXIT_CONCEAL_PT_PIP          0x01000000
+#define VM_EXIT_CLEAR_IA32_RTIT_CTL     0x02000000
 extern u32 vmx_vmexit_control;
 
 #define VM_ENTRY_IA32E_MODE             0x00000200
@@ -229,6 +231,8 @@ extern u32 vmx_vmexit_control;
 #define VM_ENTRY_LOAD_GUEST_PAT         0x00004000
 #define VM_ENTRY_LOAD_GUEST_EFER        0x00008000
 #define VM_ENTRY_LOAD_BNDCFGS           0x00010000
+#define VM_ENTRY_CONCEAL_PT_PIP         0x00020000
+#define VM_ENTRY_LOAD_IA32_RTIT_CTL     0x00040000
 extern u32 vmx_vmentry_control;
 
 #define SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES 0x00000001
@@ -247,7 +251,9 @@ extern u32 vmx_vmentry_control;
 #define SECONDARY_EXEC_ENABLE_VMCS_SHADOWING    0x00004000
 #define SECONDARY_EXEC_ENABLE_PML               0x00020000
 #define SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS   0x00040000
+#define SECONDARY_EXEC_CONCEAL_PT_PIP           0x00080000
 #define SECONDARY_EXEC_XSAVES                   0x00100000
+#define SECONDARY_EXEC_PT_USE_GPA               0x01000000
 #define SECONDARY_EXEC_TSC_SCALING              0x02000000
 extern u32 vmx_secondary_exec_control;
 
@@ -268,6 +274,7 @@ extern u32 vmx_secondary_exec_control;
 #define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 0x80000000000ULL
 extern u64 vmx_ept_vpid_cap;
 
+#define VMX_MISC_PT_ENABLE                      0x00004000
 #define VMX_MISC_CR3_TARGET                     0x01ff0000
 #define VMX_MISC_VMWRITE_ALL                    0x20000000
 
-- 
1.8.3.1


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

Re: [PATCH RESEND v1 2/7] x86: configure vmcs for Intel processor trace virtualization

[Jan Beulich]

>>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> This patch configure VMCS to make Intel PT output address can be
> treat as guest physical address and translated by EPT when
> intel_pt option is true.
> There have some constraint condition on VMCS configuration,
> otherwise will cause VM entry failed.
> 
> 1. If the “Guest PT uses Guest Physical Addresses” execution
>    control is 1, the “Clear IA32_RTIT_CTL on exit” exit
>    control and the “Load IA32_RTIT_CTL on entry” entry
>    control must also be 1.
> 2. If the “Guest PT uses Guest Physical Addresses” execution
>    control is 1, the "enable EPT" execution control must
>    also be 1.

What are the implications for a guest running with hap=0?

> @@ -383,13 +388,28 @@ static int vmx_init_vmcs_config(void)
>          _vmx_secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
>  
>      min = 0;
> -    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS;
> +    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS |
> +          VM_ENTRY_CONCEAL_PT_PIP | VM_ENTRY_LOAD_IA32_RTIT_CTL;
>      _vmx_vmentry_control = adjust_vmx_controls(
>          "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS, &mismatch);
>  
>      if ( mismatch )
>          return -EINVAL;
>  
> +    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) ||
> +         !(_vmx_secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA) ||
> +         !(_vmx_vmexit_control & VM_EXIT_CLEAR_IA32_RTIT_CTL) ||
> +         !(_vmx_vmentry_control & VM_ENTRY_LOAD_IA32_RTIT_CTL) )
> +    {
> +        _vmx_secondary_exec_control &= ~(SECONDARY_EXEC_PT_USE_GPA |
> +                                         SECONDARY_EXEC_CONCEAL_PT_PIP);
> +        _vmx_vmexit_control &= ~(VM_EXIT_CONCEAL_PT_PIP |
> +                                 VM_EXIT_CLEAR_IA32_RTIT_CTL);
> +        _vmx_vmentry_control &= ~(VM_ENTRY_CONCEAL_PT_PIP |
> +                                  VM_ENTRY_LOAD_IA32_RTIT_CTL);
> +        opt_intel_pt = 0;
> +    }

Besides clearing the flag here, shouldn't you also check it further up?

Jan


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

Re: [PATCH RESEND v1 2/7] x86: configure vmcs for Intel processor trace virtualization

[Kang, Luwei]

> > This patch configure VMCS to make Intel PT output address can be treat
> > as guest physical address and translated by EPT when intel_pt option
> > is true.
> > There have some constraint condition on VMCS configuration, otherwise
> > will cause VM entry failed.
> >
> > 1. If the “Guest PT uses Guest Physical Addresses” execution
> >    control is 1, the “Clear IA32_RTIT_CTL on exit” exit
> >    control and the “Load IA32_RTIT_CTL on entry” entry
> >    control must also be 1.
> > 2. If the “Guest PT uses Guest Physical Addresses” execution
> >    control is 1, the "enable EPT" execution control must
> >    also be 1.
> 
> What are the implications for a guest running with hap=0?
> 

Intel PT enabling in guest depend on EPT feature. So if hap=0 Intel PT will disabled in guest.

> > @@ -383,13 +388,28 @@ static int vmx_init_vmcs_config(void)
> >          _vmx_secondary_exec_control &=
> > ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
> >
> >      min = 0;
> > -    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS;
> > +    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS |
> > +          VM_ENTRY_CONCEAL_PT_PIP | VM_ENTRY_LOAD_IA32_RTIT_CTL;
> >      _vmx_vmentry_control = adjust_vmx_controls(
> >          "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS,
> > &mismatch);
> >
> >      if ( mismatch )
> >          return -EINVAL;
> >
> > +    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT)
> ||
> > +         !(_vmx_secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA)
> ||
> > +         !(_vmx_vmexit_control & VM_EXIT_CLEAR_IA32_RTIT_CTL) ||
> > +         !(_vmx_vmentry_control & VM_ENTRY_LOAD_IA32_RTIT_CTL) )
> > +    {
> > +        _vmx_secondary_exec_control &=
> ~(SECONDARY_EXEC_PT_USE_GPA |
> > +                                         SECONDARY_EXEC_CONCEAL_PT_PIP);
> > +        _vmx_vmexit_control &= ~(VM_EXIT_CONCEAL_PT_PIP |
> > +                                 VM_EXIT_CLEAR_IA32_RTIT_CTL);
> > +        _vmx_vmentry_control &= ~(VM_ENTRY_CONCEAL_PT_PIP |
> > +                                  VM_ENTRY_LOAD_IA32_RTIT_CTL);
> > +        opt_intel_pt = 0;
> > +    }
> 
> Besides clearing the flag here, shouldn't you also check it further up?

If " opt_intel_pt =0" represent user don't want to use this feature to all guest or hardware don't support it at all. If flag "opt_intel_pt " still true after this check represent the user want to use this feature and hardware have capability to support PT in guest.  This is depend on hardware capability and the parameter set of xen command line "ipt=1".

If " opt_intel_pt = 1" but a new guest created by "hap=0" in xl.cfg. Intel PT will be make off in this guest and set flag " pt->intel_pt_enabled = false". This flag is VM specific and can't affect create another new guest have Intel PT with "hap=1". I am not sure if this is what you concern.

Thanks,
Luwei Kang

> 
> Jan

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

Re: [PATCH RESEND v1 2/7] x86: configure vmcs for Intel processor trace virtualization

[Jan Beulich]

>>> On 28.04.18 at 03:07, <luwei.kang@intel.com> wrote:
>> > @@ -383,13 +388,28 @@ static int vmx_init_vmcs_config(void)
>> >          _vmx_secondary_exec_control &=
>> > ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
>> >
>> >      min = 0;
>> > -    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS;
>> > +    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS |
>> > +          VM_ENTRY_CONCEAL_PT_PIP | VM_ENTRY_LOAD_IA32_RTIT_CTL;
>> >      _vmx_vmentry_control = adjust_vmx_controls(
>> >          "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS,
>> > &mismatch);
>> >
>> >      if ( mismatch )
>> >          return -EINVAL;
>> >
>> > +    if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT)
>> ||
>> > +         !(_vmx_secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA)
>> ||
>> > +         !(_vmx_vmexit_control & VM_EXIT_CLEAR_IA32_RTIT_CTL) ||
>> > +         !(_vmx_vmentry_control & VM_ENTRY_LOAD_IA32_RTIT_CTL) )
>> > +    {
>> > +        _vmx_secondary_exec_control &=
>> ~(SECONDARY_EXEC_PT_USE_GPA |
>> > +                                         SECONDARY_EXEC_CONCEAL_PT_PIP);
>> > +        _vmx_vmexit_control &= ~(VM_EXIT_CONCEAL_PT_PIP |
>> > +                                 VM_EXIT_CLEAR_IA32_RTIT_CTL);
>> > +        _vmx_vmentry_control &= ~(VM_ENTRY_CONCEAL_PT_PIP |
>> > +                                  VM_ENTRY_LOAD_IA32_RTIT_CTL);
>> > +        opt_intel_pt = 0;
>> > +    }
>> 
>> Besides clearing the flag here, shouldn't you also check it further up?
> 
> If " opt_intel_pt =0" represent user don't want to use this feature to all 
> guest or hardware don't support it at all. If flag "opt_intel_pt " still true 
> after this check represent the user want to use this feature and hardware 
> have capability to support PT in guest.  This is depend on hardware 
> capability and the parameter set of xen command line "ipt=1".

I'm having some difficulty to follow this, so let me explain my point a little
further: If (part of) the required features is available in hardware, but
the user opted to not use IPT, wouldn't it be better for consistency to
turn off the individual IPT features (so that e.g. checks of them elsewhere
in the code won't go wrong), i.e. pretend the hardware doesn't support
them?

Jan



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

Re: [PATCH RESEND v1 2/7] x86: configure vmcs for Intel processor trace virtualization

[Kang, Luwei]

> >>> On 28.04.18 at 03:07, <luwei.kang@intel.com> wrote:
> >> > @@ -383,13 +388,28 @@ static int vmx_init_vmcs_config(void)
> >> >          _vmx_secondary_exec_control &=
> >> > ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
> >> >
> >> >      min = 0;
> >> > -    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS;
> >> > +    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS |
> >> > +          VM_ENTRY_CONCEAL_PT_PIP | VM_ENTRY_LOAD_IA32_RTIT_CTL;
> >> >      _vmx_vmentry_control = adjust_vmx_controls(
> >> >          "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS,
> >> > &mismatch);
> >> >
> >> >      if ( mismatch )
> >> >          return -EINVAL;
> >> >
> >> > +    if ( !(_vmx_secondary_exec_control &
> >> > + SECONDARY_EXEC_ENABLE_EPT)
> >> ||
> >> > +         !(_vmx_secondary_exec_control &
> >> > + SECONDARY_EXEC_PT_USE_GPA)
> >> ||
> >> > +         !(_vmx_vmexit_control & VM_EXIT_CLEAR_IA32_RTIT_CTL) ||
> >> > +         !(_vmx_vmentry_control & VM_ENTRY_LOAD_IA32_RTIT_CTL) )
> >> > +    {
> >> > +        _vmx_secondary_exec_control &=
> >> ~(SECONDARY_EXEC_PT_USE_GPA |
> >> > +                                         SECONDARY_EXEC_CONCEAL_PT_PIP);
> >> > +        _vmx_vmexit_control &= ~(VM_EXIT_CONCEAL_PT_PIP |
> >> > +                                 VM_EXIT_CLEAR_IA32_RTIT_CTL);
> >> > +        _vmx_vmentry_control &= ~(VM_ENTRY_CONCEAL_PT_PIP |
> >> > +                                  VM_ENTRY_LOAD_IA32_RTIT_CTL);
> >> > +        opt_intel_pt = 0;
> >> > +    }
> >>
> >> Besides clearing the flag here, shouldn't you also check it further up?
> >
> > If " opt_intel_pt =0" represent user don't want to use this feature to
> > all guest or hardware don't support it at all. If flag "opt_intel_pt "
> > still true after this check represent the user want to use this
> > feature and hardware have capability to support PT in guest.  This is
> > depend on hardware capability and the parameter set of xen command line "ipt=1".
> 
> I'm having some difficulty to follow this, so let me explain my point a little
> further: If (part of) the required features is available in hardware, but the user opted to not use IPT, wouldn't it be better for
> consistency to turn off the individual IPT features (so that e.g. checks of them elsewhere in the code won't go wrong), i.e. pretend
> the hardware doesn't support them?

If the hardware have the capability to enable IPT in guest but the user don't want to use it. We can set "intel_pt = 0" in XEN command line to disable this feature so that IPT will can't be detected in all guest. 

Thanks,
Luwei Kang

> 
> Jan
> 


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

Re: [PATCH RESEND v1 2/7] x86: configure vmcs for Intel processor trace virtualization

[Jan Beulich]

>>> On 02.05.18 at 09:22, <luwei.kang@intel.com> wrote:
>> >>> On 28.04.18 at 03:07, <luwei.kang@intel.com> wrote:
>> >> > @@ -383,13 +388,28 @@ static int vmx_init_vmcs_config(void)
>> >> >          _vmx_secondary_exec_control &=
>> >> > ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
>> >> >
>> >> >      min = 0;
>> >> > -    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS;
>> >> > +    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS |
>> >> > +          VM_ENTRY_CONCEAL_PT_PIP | VM_ENTRY_LOAD_IA32_RTIT_CTL;
>> >> >      _vmx_vmentry_control = adjust_vmx_controls(
>> >> >          "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS,
>> >> > &mismatch);
>> >> >
>> >> >      if ( mismatch )
>> >> >          return -EINVAL;
>> >> >
>> >> > +    if ( !(_vmx_secondary_exec_control &
>> >> > + SECONDARY_EXEC_ENABLE_EPT)
>> >> ||
>> >> > +         !(_vmx_secondary_exec_control &
>> >> > + SECONDARY_EXEC_PT_USE_GPA)
>> >> ||
>> >> > +         !(_vmx_vmexit_control & VM_EXIT_CLEAR_IA32_RTIT_CTL) ||
>> >> > +         !(_vmx_vmentry_control & VM_ENTRY_LOAD_IA32_RTIT_CTL) )
>> >> > +    {
>> >> > +        _vmx_secondary_exec_control &=
>> >> ~(SECONDARY_EXEC_PT_USE_GPA |
>> >> > +                                         SECONDARY_EXEC_CONCEAL_PT_PIP);
>> >> > +        _vmx_vmexit_control &= ~(VM_EXIT_CONCEAL_PT_PIP |
>> >> > +                                 VM_EXIT_CLEAR_IA32_RTIT_CTL);
>> >> > +        _vmx_vmentry_control &= ~(VM_ENTRY_CONCEAL_PT_PIP |
>> >> > +                                  VM_ENTRY_LOAD_IA32_RTIT_CTL);
>> >> > +        opt_intel_pt = 0;
>> >> > +    }
>> >>
>> >> Besides clearing the flag here, shouldn't you also check it further up?
>> >
>> > If " opt_intel_pt =0" represent user don't want to use this feature to
>> > all guest or hardware don't support it at all. If flag "opt_intel_pt "
>> > still true after this check represent the user want to use this
>> > feature and hardware have capability to support PT in guest.  This is
>> > depend on hardware capability and the parameter set of xen command line "ipt=1".
>> 
>> I'm having some difficulty to follow this, so let me explain my point a little
>> further: If (part of) the required features is available in hardware, but 
> the user opted to not use IPT, wouldn't it be better for
>> consistency to turn off the individual IPT features (so that e.g. checks of 
> them elsewhere in the code won't go wrong), i.e. pretend
>> the hardware doesn't support them?
> 
> If the hardware have the capability to enable IPT in guest but the user 
> don't want to use it. We can set "intel_pt = 0" in XEN command line to 
> disable this feature so that IPT will can't be detected in all guest. 

So we're moving in circles, it seems: Based on what you wrote, you appear
to agree to the abstract consideration. Yet my original question remains
unanswered: Besides clearing the flag here, shouldn't you also check it
further up?

Jan



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

Re: [PATCH RESEND v1 2/7] x86: configure vmcs for Intel processor trace virtualization

[Kang, Luwei]

> >> >>> On 28.04.18 at 03:07, <luwei.kang@intel.com> wrote:
> >> >> > @@ -383,13 +388,28 @@ static int vmx_init_vmcs_config(void)
> >> >> >          _vmx_secondary_exec_control &=
> >> >> > ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
> >> >> >
> >> >> >      min = 0;
> >> >> > -    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS;
> >> >> > +    opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS |
> >> >> > +          VM_ENTRY_CONCEAL_PT_PIP |
> >> >> > + VM_ENTRY_LOAD_IA32_RTIT_CTL;
> >> >> >      _vmx_vmentry_control = adjust_vmx_controls(
> >> >> >          "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS,
> >> >> > &mismatch);
> >> >> >
> >> >> >      if ( mismatch )
> >> >> >          return -EINVAL;
> >> >> >
> >> >> > +    if ( !(_vmx_secondary_exec_control &
> >> >> > + SECONDARY_EXEC_ENABLE_EPT)
> >> >> ||
> >> >> > +         !(_vmx_secondary_exec_control &
> >> >> > + SECONDARY_EXEC_PT_USE_GPA)
> >> >> ||
> >> >> > +         !(_vmx_vmexit_control & VM_EXIT_CLEAR_IA32_RTIT_CTL) ||
> >> >> > +         !(_vmx_vmentry_control & VM_ENTRY_LOAD_IA32_RTIT_CTL) )
> >> >> > +    {
> >> >> > +        _vmx_secondary_exec_control &=
> >> >> ~(SECONDARY_EXEC_PT_USE_GPA |
> >> >> > +                                         SECONDARY_EXEC_CONCEAL_PT_PIP);
> >> >> > +        _vmx_vmexit_control &= ~(VM_EXIT_CONCEAL_PT_PIP |
> >> >> > +                                 VM_EXIT_CLEAR_IA32_RTIT_CTL);
> >> >> > +        _vmx_vmentry_control &= ~(VM_ENTRY_CONCEAL_PT_PIP |
> >> >> > +                                  VM_ENTRY_LOAD_IA32_RTIT_CTL);
> >> >> > +        opt_intel_pt = 0;
> >> >> > +    }
> >> >>
> >> >> Besides clearing the flag here, shouldn't you also check it further up?
> >> >
> >> > If " opt_intel_pt =0" represent user don't want to use this feature
> >> > to all guest or hardware don't support it at all. If flag "opt_intel_pt "
> >> > still true after this check represent the user want to use this
> >> > feature and hardware have capability to support PT in guest.  This
> >> > is depend on hardware capability and the parameter set of xen command line "ipt=1".
> >>
> >> I'm having some difficulty to follow this, so let me explain my point
> >> a little
> >> further: If (part of) the required features is available in hardware,
> >> but
> > the user opted to not use IPT, wouldn't it be better for
> >> consistency to turn off the individual IPT features (so that e.g.
> >> checks of
> > them elsewhere in the code won't go wrong), i.e. pretend
> >> the hardware doesn't support them?
> >
> > If the hardware have the capability to enable IPT in guest but the
> > user don't want to use it. We can set "intel_pt = 0" in XEN command
> > line to disable this feature so that IPT will can't be detected in all guest.
> 
> So we're moving in circles, it seems: Based on what you wrote, you appear to agree to the abstract consideration. Yet my original
> question remains
> unanswered: Besides clearing the flag here, shouldn't you also check it further up?

I think we should clear what we set in VMCS (i.e. SECONDARY_EXEC_PT_USE_GPA, VM_EXIT_CLEAR_IA32_RTIT_CTL and VM_ENTRY_LOAD_IA32_RTIT_CTL), don't expose Intel PT to guest and emulate a #GP when guest access Intel PT MSRs (i.e. MSR_IA32_RTIT_*) if hardware support intel PT in guest but "IPT=0".

Thanks,
Luwei Kang

> 
> Jan
> 


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

[PATCH RESEND v1 3/7] x86: add intel proecessor trace support for cpuid

[Luwei Kang]

This patch add Intel processor trace support
for cpuid handling.

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 tools/libxc/xc_cpuid_x86.c                  | 12 ++++++++++--
 xen/arch/x86/cpuid.c                        | 22 ++++++++++++++++++++++
 xen/arch/x86/domctl.c                       |  4 ++++
 xen/include/asm-x86/cpufeature.h            |  1 +
 xen/include/asm-x86/cpuid.h                 | 12 +++++++++++-
 xen/include/public/arch-x86/cpufeatureset.h |  1 +
 6 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 25b922e..c39a9cf 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -38,7 +38,7 @@ enum {
 #define clear_feature(idx, dst) ((dst) &= ~bitmaskof(idx))
 #define set_feature(idx, dst)   ((dst) |=  bitmaskof(idx))
 
-#define DEF_MAX_BASE 0x0000000du
+#define DEF_MAX_BASE 0x00000014u
 #define DEF_MAX_INTELEXT  0x80000008u
 #define DEF_MAX_AMDEXT    0x8000001cu
 
@@ -471,6 +471,7 @@ static void xc_cpuid_hvm_policy(xc_interface *xch,
     case 0x00000002: /* Intel cache info (dumped by AMD policy) */
     case 0x00000004: /* Intel cache info (dumped by AMD policy) */
     case 0x0000000a: /* Architectural Performance Monitor Features */
+    case 0x00000014: /* Intel Processor Trace Features */
     case 0x80000002: /* Processor name string */
     case 0x80000003: /* ... continued         */
     case 0x80000004: /* ... continued         */
@@ -757,12 +758,19 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
                 continue;
         }
 
+        if ( input[0] == 0x14 )
+        {
+            input[1]++;
+            if ( input[1] == 1 )
+                continue;
+        }
+
         input[0]++;
         if ( !(input[0] & 0x80000000u) && (input[0] > base_max ) )
             input[0] = 0x80000000u;
 
         input[1] = XEN_CPUID_INPUT_UNUSED;
-        if ( (input[0] == 4) || (input[0] == 7) )
+        if ( (input[0] == 4) || (input[0] == 7) || (input[0] == 0x14) )
             input[1] = 0;
         else if ( input[0] == 0xd )
             input[1] = 1; /* Xen automatically calculates almost everything. */
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 5ee82d3..c3d56fd 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -9,6 +9,7 @@
 #include <asm/paging.h>
 #include <asm/processor.h>
 #include <asm/xstate.h>
+#include <asm/intel_pt.h>
 
 const uint32_t known_features[] = INIT_KNOWN_FEATURES;
 const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
@@ -487,7 +488,19 @@ void recalculate_cpuid_policy(struct domain *d)
             __clear_bit(X86_FEATURE_VMX, max_fs);
             __clear_bit(X86_FEATURE_SVM, max_fs);
         }
+
+        /*
+         * Hide Intel Processor trace feature when hardware not support
+         * PT-VMX or intel_pt option is disabled.
+         */
+        if ( !opt_intel_pt )
+        {
+            __clear_bit(X86_FEATURE_INTEL_PT, max_fs);
+            zero_leaves(p->intel_pt.raw, 0, ARRAY_SIZE(p->intel_pt.raw) - 1);
+        }
     }
+    else
+        zero_leaves(p->intel_pt.raw, 0, ARRAY_SIZE(p->intel_pt.raw) - 1);
 
     /*
      * Allow the toolstack to set HTT, X2APIC and CMP_LEGACY.  These bits
@@ -634,6 +647,15 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
             *res = p->feat.raw[subleaf];
             break;
 
+        case 0x14:
+            ASSERT(p->intel_pt.max_subleaf < ARRAY_SIZE(p->intel_pt.raw));
+            if ( subleaf > min_t(uint32_t, p->intel_pt.max_subleaf,
+                                 ARRAY_SIZE(p->intel_pt.raw) - 1) )
+                return;
+
+            *res = p->intel_pt.raw[subleaf];
+            break;
+
         case XSTATE_CPUID:
             if ( !p->basic.xsave || subleaf >= ARRAY_SIZE(p->xstate.raw) )
                 return;
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 5973d9f..493cca4 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -101,6 +101,10 @@ static int update_domain_cpuid_info(struct domain *d,
             p->feat.raw[ctl->input[1]] = leaf;
             break;
 
+        case 0x14:
+            p->intel_pt.raw[ctl->input[1]] = leaf;
+            break;
+
         case XSTATE_CPUID:
             p->xstate.raw[ctl->input[1]] = leaf;
             break;
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 84cc51d..8956667 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -95,6 +95,7 @@
 #define cpu_has_mpx             boot_cpu_has(X86_FEATURE_MPX)
 #define cpu_has_rdseed          boot_cpu_has(X86_FEATURE_RDSEED)
 #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
+#define cpu_has_intel_pt        boot_cpu_has(X86_FEATURE_INTEL_PT)
 #define cpu_has_sha             boot_cpu_has(X86_FEATURE_SHA)
 
 /* CPUID level 0x80000007.edx */
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index d2dd841..39f06aa 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -61,10 +61,11 @@ extern struct cpuidmasks cpuidmask_defaults;
 /* Whether or not cpuid faulting is available for the current domain. */
 DECLARE_PER_CPU(bool, cpuid_faulting_enabled);
 
-#define CPUID_GUEST_NR_BASIC      (0xdu + 1)
+#define CPUID_GUEST_NR_BASIC      (0x14u + 1)
 #define CPUID_GUEST_NR_FEAT       (0u + 1)
 #define CPUID_GUEST_NR_CACHE      (5u + 1)
 #define CPUID_GUEST_NR_XSTATE     (62u + 1)
+#define CPUID_GUEST_NR_INTEL_PT   (1u + 1)
 #define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1)
 #define CPUID_GUEST_NR_EXTD_AMD   (0x1cu + 1)
 #define CPUID_GUEST_NR_EXTD       MAX(CPUID_GUEST_NR_EXTD_INTEL, \
@@ -169,6 +170,15 @@ struct cpuid_policy
         } comp[CPUID_GUEST_NR_XSTATE];
     } xstate;
 
+    /* Structured feature leaf: 0x00000014[xx] */
+    union {
+        struct cpuid_leaf raw[CPUID_GUEST_NR_INTEL_PT];
+        struct {
+            /* Subleaf 0. */
+            uint32_t max_subleaf;
+        };
+    } intel_pt;
+
     /* Extended leaves: 0x800000xx */
     union {
         struct cpuid_leaf raw[CPUID_GUEST_NR_EXTD];
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index be6da8e..c35b061 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -215,6 +215,7 @@ XEN_CPUFEATURE(SMAP,          5*32+20) /*S  Supervisor Mode Access Prevention */
 XEN_CPUFEATURE(AVX512IFMA,    5*32+21) /*A  AVX-512 Integer Fused Multiply Add */
 XEN_CPUFEATURE(CLFLUSHOPT,    5*32+23) /*A  CLFLUSHOPT instruction */
 XEN_CPUFEATURE(CLWB,          5*32+24) /*A  CLWB instruction */
+XEN_CPUFEATURE(INTEL_PT,      5*32+25) /*H  Intel Processor Trace */
 XEN_CPUFEATURE(AVX512PF,      5*32+26) /*A  AVX-512 Prefetch Instructions */
 XEN_CPUFEATURE(AVX512ER,      5*32+27) /*A  AVX-512 Exponent & Reciprocal Instrs */
 XEN_CPUFEATURE(AVX512CD,      5*32+28) /*A  AVX-512 Conflict Detection Instrs */
-- 
1.8.3.1


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

Re: [PATCH RESEND v1 3/7] x86: add intel proecessor trace support for cpuid

[Konrad Rzeszutek Wilk]

On Tue, Jan 16, 2018 at 02:12:29AM +0800, Luwei Kang wrote:
> This patch add Intel processor trace support
> for cpuid handling.

The 0x14 usage screams of wanting an #define.

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

Re: [PATCH RESEND v1 3/7] x86: add intel proecessor trace support for cpuid

[Kang, Luwei]

> > This patch add Intel processor trace support for cpuid handling.
> 
> The 0x14 usage screams of wanting an #define.

Get it. Will define leaf 0x14 as a macro in next version. Thanks for the review.

Luwei Kang

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

[PATCH RESEND v1 4/7] x86: add intel processor trace context

[Luwei Kang]

This patch add Intel processor trace context
date structure for guest.

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 xen/include/asm-x86/hvm/vmx/vmcs.h |  3 +++
 xen/include/asm-x86/intel_pt.h     | 17 +++++++++++++++++
 xen/include/asm-x86/msr-index.h    | 20 ++++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index bd8a128..33ec3e6 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -20,6 +20,7 @@
 
 #include <asm/hvm/io.h>
 #include <irq_vectors.h>
+#include <asm/intel_pt.h>
 
 extern void vmcs_dump_vcpu(struct vcpu *v);
 extern void setup_vmcs_dump(void);
@@ -171,6 +172,8 @@ struct arch_vmx_struct {
      * pCPU and wakeup the related vCPU.
      */
     struct pi_blocking_vcpu pi_blocking;
+
+    struct pt_desc       pt_desc;
 };
 
 int vmx_create_vmcs(struct vcpu *v);
diff --git a/xen/include/asm-x86/intel_pt.h b/xen/include/asm-x86/intel_pt.h
index 2a8b579..909e22f 100644
--- a/xen/include/asm-x86/intel_pt.h
+++ b/xen/include/asm-x86/intel_pt.h
@@ -21,6 +21,23 @@
 #ifndef __ASM_X86_HVM_INTEL_PT_H_
 #define __ASM_X86_HVM_INTEL_PT_H_
 
+#include <asm/msr-index.h>
+
+struct pt_ctx {
+    u64 ctl;
+    u64 status;
+    u64 output_base;
+    u64 output_mask;
+    u64 cr3_match;
+    u64 addr[NUM_MSR_IA32_RTIT_ADDR];
+};
+
+struct pt_desc {
+    bool intel_pt_enabled;
+    unsigned int addr_num;
+    struct pt_ctx guest_pt_ctx;
+};
+
 extern bool_t opt_intel_pt;
 
 #endif /* __ASM_X86_HVM_INTEL_PT_H_ */
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index a834f3b..73c33be 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -529,4 +529,24 @@
 #define MSR_PKGC9_IRTL			0x00000634
 #define MSR_PKGC10_IRTL			0x00000635
 
+/* Intel PT MSRs */
+#define MSR_IA32_RTIT_CTL		0x00000570
+#define _MSR_IA32_RTIT_CTL_TRACEEN	0
+#define MSR_IA32_RTIT_CTL_TRACEEN	(1ULL << _MSR_IA32_RTIT_CTL_TRACEEN)
+#define _MSR_IA32_RTIT_CTL_TOPA		8
+#define MSR_IA32_RTIT_CTL_TOPA		(1ULL << _MSR_IA32_RTIT_CTL_TOPA)
+#define MSR_IA32_RTIT_STATUS		0x00000571
+#define MSR_IA32_RTIT_CR3_MATCH		0x00000572
+#define MSR_IA32_RTIT_OUTPUT_BASE	0x00000560
+#define MSR_IA32_RTIT_OUTPUT_MASK	0x00000561
+#define MSR_IA32_RTIT_ADDR0_A		0x00000580
+#define MSR_IA32_RTIT_ADDR0_B		0x00000581
+#define MSR_IA32_RTIT_ADDR1_A		0x00000582
+#define MSR_IA32_RTIT_ADDR1_B		0x00000583
+#define MSR_IA32_RTIT_ADDR2_A		0x00000584
+#define MSR_IA32_RTIT_ADDR2_B		0x00000585
+#define MSR_IA32_RTIT_ADDR3_A		0x00000586
+#define MSR_IA32_RTIT_ADDR3_B		0x00000587
+#define NUM_MSR_IA32_RTIT_ADDR		8
+
 #endif /* __ASM_MSR_INDEX_H */
-- 
1.8.3.1


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

Re: [PATCH RESEND v1 4/7] x86: add intel processor trace context

[Wei Liu]

On Tue, Jan 16, 2018 at 02:12:30AM +0800, Luwei Kang wrote:
>  
> +#include <asm/msr-index.h>
> +
> +struct pt_ctx {
> +    u64 ctl;
> +    u64 status;
> +    u64 output_base;
> +    u64 output_mask;
> +    u64 cr3_match;
> +    u64 addr[NUM_MSR_IA32_RTIT_ADDR];

uint64_t please.

> +};
> +
> +struct pt_desc {
> +    bool intel_pt_enabled;

Just "enabled" is fine.

> +    unsigned int addr_num;

num_addr.

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

Re: [PATCH RESEND v1 4/7] x86: add intel processor trace context

[Jan Beulich]

>>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -20,6 +20,7 @@
>  
>  #include <asm/hvm/io.h>
>  #include <irq_vectors.h>
> +#include <asm/intel_pt.h>
>  
>  extern void vmcs_dump_vcpu(struct vcpu *v);
>  extern void setup_vmcs_dump(void);
> @@ -171,6 +172,8 @@ struct arch_vmx_struct {
>       * pCPU and wakeup the related vCPU.
>       */
>      struct pi_blocking_vcpu pi_blocking;
> +
> +    struct pt_desc       pt_desc;

Perhaps better a pointer, to limit struct vcpu growth. The structure also
doesn't actually need allocating unless the feature is present and the
command line option allows its use. Once a pointer, you also
- don't need to include the header above.
- can size the allocation based on actual number of address MSRs 
  supported, instead of ...

> --- a/xen/include/asm-x86/intel_pt.h
> +++ b/xen/include/asm-x86/intel_pt.h
> @@ -21,6 +21,23 @@
>  #ifndef __ASM_X86_HVM_INTEL_PT_H_
>  #define __ASM_X86_HVM_INTEL_PT_H_
>  
> +#include <asm/msr-index.h>
> +
> +struct pt_ctx {
> +    u64 ctl;
> +    u64 status;
> +    u64 output_base;
> +    u64 output_mask;
> +    u64 cr3_match;
> +    u64 addr[NUM_MSR_IA32_RTIT_ADDR];

... this fixed value.

> +};
> +
> +struct pt_desc {
> +    bool intel_pt_enabled;
> +    unsigned int addr_num;
> +    struct pt_ctx guest_pt_ctx;
> +};

Any particular reason to make this two structures instead of one?

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -529,4 +529,24 @@
>  #define MSR_PKGC9_IRTL			0x00000634
>  #define MSR_PKGC10_IRTL			0x00000635
>  
> +/* Intel PT MSRs */
> +#define MSR_IA32_RTIT_CTL		0x00000570
> +#define _MSR_IA32_RTIT_CTL_TRACEEN	0

Please can you avoid further extending the set of name space violations?
Names starting with an underscore and an upper case letter are reserved.
Unless you really need the bit position values, defining just the mask
values ought to suffice.

> +#define MSR_IA32_RTIT_CTL_TRACEEN	(1ULL << _MSR_IA32_RTIT_CTL_TRACEEN)
> +#define _MSR_IA32_RTIT_CTL_TOPA		8
> +#define MSR_IA32_RTIT_CTL_TOPA		(1ULL << _MSR_IA32_RTIT_CTL_TOPA)
> +#define MSR_IA32_RTIT_STATUS		0x00000571
> +#define MSR_IA32_RTIT_CR3_MATCH		0x00000572
> +#define MSR_IA32_RTIT_OUTPUT_BASE	0x00000560
> +#define MSR_IA32_RTIT_OUTPUT_MASK	0x00000561
> +#define MSR_IA32_RTIT_ADDR0_A		0x00000580
> +#define MSR_IA32_RTIT_ADDR0_B		0x00000581
> +#define MSR_IA32_RTIT_ADDR1_A		0x00000582
> +#define MSR_IA32_RTIT_ADDR1_B		0x00000583
> +#define MSR_IA32_RTIT_ADDR2_A		0x00000584
> +#define MSR_IA32_RTIT_ADDR2_B		0x00000585
> +#define MSR_IA32_RTIT_ADDR3_A		0x00000586
> +#define MSR_IA32_RTIT_ADDR3_B		0x00000587

I'd prefer a single pair of

+#define MSR_IA32_RTIT_ADDR_A(n)		(0x00000580 + (n) * 2)
+#define MSR_IA32_RTIT_ADDR_B(n)		(0x00000581 + (n) * 2)

> +#define NUM_MSR_IA32_RTIT_ADDR		8

The 3-bit value in CPUID output can enumerate up to 7 pairs. I'm not convinced
hard coding a lower limit is desirable (unless I haven't been able to spot where
in the SDM such a lower limit is mandated).

Jan



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

Re: [PATCH RESEND v1 4/7] x86: add intel processor trace context

[Kang, Luwei]

> > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> > @@ -20,6 +20,7 @@
> >
> >  #include <asm/hvm/io.h>
> >  #include <irq_vectors.h>
> > +#include <asm/intel_pt.h>
> >
> >  extern void vmcs_dump_vcpu(struct vcpu *v);  extern void
> > setup_vmcs_dump(void); @@ -171,6 +172,8 @@ struct arch_vmx_struct {
> >       * pCPU and wakeup the related vCPU.
> >       */
> >      struct pi_blocking_vcpu pi_blocking;
> > +
> > +    struct pt_desc       pt_desc;
> 
> Perhaps better a pointer, to limit struct vcpu growth. The structure also
> doesn't actually need allocating unless the feature is present and the
> command line option allows its use. Once a pointer, you also
> - don't need to include the header above.
> - can size the allocation based on actual number of address MSRs
>   supported, instead of ...

Agree, will fix in next version.

> 
> > --- a/xen/include/asm-x86/intel_pt.h
> > +++ b/xen/include/asm-x86/intel_pt.h
> > @@ -21,6 +21,23 @@
> >  #ifndef __ASM_X86_HVM_INTEL_PT_H_
> >  #define __ASM_X86_HVM_INTEL_PT_H_
> >
> > +#include <asm/msr-index.h>
> > +
> > +struct pt_ctx {
> > +    u64 ctl;
> > +    u64 status;
> > +    u64 output_base;
> > +    u64 output_mask;
> > +    u64 cr3_match;
> > +    u64 addr[NUM_MSR_IA32_RTIT_ADDR];
> 
> ... this fixed value.
> 
> > +};
> > +
> > +struct pt_desc {
> > +    bool intel_pt_enabled;
> > +    unsigned int addr_num;
> > +    struct pt_ctx guest_pt_ctx;
> > +};
> 
> Any particular reason to make this two structures instead of one?

If add the support of host guest mode(trace hypervisor and guest independent), new item can be added in the structure like " struct pt_ctx host_pt_ctx".

> 
> > --- a/xen/include/asm-x86/msr-index.h
> > +++ b/xen/include/asm-x86/msr-index.h
> > @@ -529,4 +529,24 @@
> >  #define MSR_PKGC9_IRTL			0x00000634
> >  #define MSR_PKGC10_IRTL			0x00000635
> >
> > +/* Intel PT MSRs */
> > +#define MSR_IA32_RTIT_CTL		0x00000570
> > +#define _MSR_IA32_RTIT_CTL_TRACEEN	0
> 
> Please can you avoid further extending the set of name space violations?
> Names starting with an underscore and an upper case letter are reserved.
> Unless you really need the bit position values, defining just the mask values
> ought to suffice.

Get it. Will fix it.

> 
> > +#define MSR_IA32_RTIT_CTL_TRACEEN	(1ULL <<
> _MSR_IA32_RTIT_CTL_TRACEEN)
> > +#define _MSR_IA32_RTIT_CTL_TOPA		8
> > +#define MSR_IA32_RTIT_CTL_TOPA		(1ULL <<
> _MSR_IA32_RTIT_CTL_TOPA)
> > +#define MSR_IA32_RTIT_STATUS		0x00000571
> > +#define MSR_IA32_RTIT_CR3_MATCH		0x00000572
> > +#define MSR_IA32_RTIT_OUTPUT_BASE	0x00000560
> > +#define MSR_IA32_RTIT_OUTPUT_MASK	0x00000561
> > +#define MSR_IA32_RTIT_ADDR0_A		0x00000580
> > +#define MSR_IA32_RTIT_ADDR0_B		0x00000581
> > +#define MSR_IA32_RTIT_ADDR1_A		0x00000582
> > +#define MSR_IA32_RTIT_ADDR1_B		0x00000583
> > +#define MSR_IA32_RTIT_ADDR2_A		0x00000584
> > +#define MSR_IA32_RTIT_ADDR2_B		0x00000585
> > +#define MSR_IA32_RTIT_ADDR3_A		0x00000586
> > +#define MSR_IA32_RTIT_ADDR3_B		0x00000587
> 
> I'd prefer a single pair of
> 
> +#define MSR_IA32_RTIT_ADDR_A(n)		(0x00000580 + (n) * 2)
> +#define MSR_IA32_RTIT_ADDR_B(n)		(0x00000581 + (n) * 2)
> 
> > +#define NUM_MSR_IA32_RTIT_ADDR		8
> 
> The 3-bit value in CPUID output can enumerate up to 7 pairs. I'm not
> convinced hard coding a lower limit is desirable (unless I haven't been able to
> spot where in the SDM such a lower limit is mandated).

Agree, looks good to me.

Thanks,
Luwei Kang

> 
> Jan
> 


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

[PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Luwei Kang]

Load/Store Intel processor trace register in context switch.
MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
When Intel PT is supported in guest, we need load/restore
PT MSRs only when PT is enabled in guest.

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 xen/arch/x86/cpu/intel_pt.c        | 69 ++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c         |  4 +++
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 ++
 xen/include/asm-x86/intel_pt.h     |  4 +++
 4 files changed, 79 insertions(+)

diff --git a/xen/arch/x86/cpu/intel_pt.c b/xen/arch/x86/cpu/intel_pt.c
index 520e0ca..c0e9e68 100644
--- a/xen/arch/x86/cpu/intel_pt.c
+++ b/xen/arch/x86/cpu/intel_pt.c
@@ -21,7 +21,76 @@
 #include <xen/types.h>
 #include <xen/cache.h>
 #include <xen/init.h>
+#include <asm/hvm/vmx/vmx.h>
+#include <asm/intel_pt.h>
 
 /* intel_pt: Flag to enable Intel Processor Trace (default on). */
 bool_t __read_mostly opt_intel_pt = 1;
 boolean_param("intel_pt", opt_intel_pt);
+
+static inline void pt_load_msr(struct pt_ctx *ctx, u32 addr_num)
+{
+    u32 i;
+    wrmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
+    wrmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
+    wrmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
+    wrmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
+    for ( i = 0; i < addr_num; i++ )
+        wrmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]);
+}
+
+static inline void pt_save_msr(struct pt_ctx *ctx, u32 addr_num)
+{
+    u32 i;
+    rdmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
+    rdmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
+    rdmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
+    rdmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
+    for ( i = 0; i < addr_num; i++ )
+        rdmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]);
+}
+
+void pt_guest_enter(struct vcpu *v)
+{
+    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
+
+    if ( pt->intel_pt_enabled &&
+       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
+        pt_load_msr(&pt->guest_pt_ctx, pt->addr_num);
+}
+
+void pt_guest_exit(struct vcpu *v)
+{
+    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
+
+    if ( pt->intel_pt_enabled &&
+       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
+        pt_save_msr(&pt->guest_pt_ctx, pt->addr_num);
+}
+
+void pt_vcpu_init(struct vcpu *v)
+{
+    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
+    unsigned int eax, ebx, ecx, edx;
+
+    memset(pt, 0, sizeof(struct pt_desc));
+    pt->intel_pt_enabled = false;
+
+    if ( !cpu_has_intel_pt || !opt_intel_pt ||
+         !(v->arch.hvm_vmx.secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA) )
+        return;
+
+    /* get the number of address ranges */
+    if ( cpuid_eax(0x14) == 1 )
+        cpuid_count(0x14, 1, &eax, &ebx, &ecx, &edx);
+    else
+        return;
+
+    pt->addr_num = eax & 0x7;
+    pt->guest_pt_ctx.output_mask = 0x7F;
+    pt->intel_pt_enabled = true;
+
+    vmx_vmcs_enter(v);
+    __vmwrite(GUEST_IA32_RTIT_CTL, 0);
+    vmx_vmcs_exit(v);
+}
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index e036303..f386933 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -467,6 +467,8 @@ static int vmx_vcpu_initialise(struct vcpu *v)
     if ( v->vcpu_id == 0 )
         v->arch.user_regs.rax = 1;
 
+    pt_vcpu_init(v);
+
     return 0;
 }
 
@@ -3513,6 +3515,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
     __vmread(GUEST_RSP,    &regs->rsp);
     __vmread(GUEST_RFLAGS, &regs->rflags);
 
+    pt_guest_exit(v);
     hvm_invalidate_regs_fields(regs);
 
     if ( paging_mode_hap(v->domain) )
@@ -4281,6 +4284,7 @@ bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
         }
     }
 
+    pt_guest_enter(curr);
  out:
     if ( unlikely(curr->arch.hvm_vmx.lbr_fixup_enabled) )
         lbr_fixup();
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 33ec3e6..46c386f 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -421,6 +421,8 @@ enum vmcs_field {
     GUEST_PDPTE0                    = 0x0000280a,
 #define GUEST_PDPTE(n) (GUEST_PDPTE0 + (n) * 2) /* n = 0...3 */
     GUEST_BNDCFGS                   = 0x00002812,
+    GUEST_IA32_RTIT_CTL             = 0x00002814,
+    GUEST_IA32_RTIT_CTL_HIGH        = 0x00002815,
     HOST_PAT                        = 0x00002c00,
     HOST_EFER                       = 0x00002c02,
     HOST_PERF_GLOBAL_CTRL           = 0x00002c04,
diff --git a/xen/include/asm-x86/intel_pt.h b/xen/include/asm-x86/intel_pt.h
index 909e22f..9505c8f 100644
--- a/xen/include/asm-x86/intel_pt.h
+++ b/xen/include/asm-x86/intel_pt.h
@@ -40,4 +40,8 @@ struct pt_desc {
 
 extern bool_t opt_intel_pt;
 
+void pt_vcpu_init(struct vcpu *v);
+void pt_guest_enter(struct vcpu *v);
+void pt_guest_exit(struct vcpu *v);
+
 #endif /* __ASM_X86_HVM_INTEL_PT_H_ */
-- 
1.8.3.1


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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Wei Liu]

On Tue, Jan 16, 2018 at 02:12:31AM +0800, Luwei Kang wrote:
> Load/Store Intel processor trace register in context switch.
> MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
> When Intel PT is supported in guest, we need load/restore
> PT MSRs only when PT is enabled in guest.
> 
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>

Is there a reason to not use xsaves/xrstors when they are available?

> ---
>  xen/arch/x86/cpu/intel_pt.c        | 69 ++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/vmx/vmx.c         |  4 +++
>  xen/include/asm-x86/hvm/vmx/vmcs.h |  2 ++
>  xen/include/asm-x86/intel_pt.h     |  4 +++
>  4 files changed, 79 insertions(+)
> 
> diff --git a/xen/arch/x86/cpu/intel_pt.c b/xen/arch/x86/cpu/intel_pt.c
> index 520e0ca..c0e9e68 100644
> --- a/xen/arch/x86/cpu/intel_pt.c
> +++ b/xen/arch/x86/cpu/intel_pt.c
> @@ -21,7 +21,76 @@
>  #include <xen/types.h>
>  #include <xen/cache.h>
>  #include <xen/init.h>
> +#include <asm/hvm/vmx/vmx.h>
> +#include <asm/intel_pt.h>
>  
>  /* intel_pt: Flag to enable Intel Processor Trace (default on). */
>  bool_t __read_mostly opt_intel_pt = 1;
>  boolean_param("intel_pt", opt_intel_pt);
> +
> +static inline void pt_load_msr(struct pt_ctx *ctx, u32 addr_num)

Use unsigned int for addr_num.

> +{
> +    u32 i;

Use unsigned int and add a blank line.

> +    wrmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> +    wrmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> +    wrmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> +    wrmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> +    for ( i = 0; i < addr_num; i++ )
> +        wrmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]);
> +}
> +
> +static inline void pt_save_msr(struct pt_ctx *ctx, u32 addr_num)
> +{
> +    u32 i;

Ditto.

> +    rdmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> +    rdmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> +    rdmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> +    rdmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> +    for ( i = 0; i < addr_num; i++ )
> +        rdmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]);
> +}
> +
> +void pt_guest_enter(struct vcpu *v)
> +{
> +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> +
> +    if ( pt->intel_pt_enabled &&
> +       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
> +        pt_load_msr(&pt->guest_pt_ctx, pt->addr_num);
> +}
> +
> +void pt_guest_exit(struct vcpu *v)
> +{
> +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> +
> +    if ( pt->intel_pt_enabled &&
> +       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
> +        pt_save_msr(&pt->guest_pt_ctx, pt->addr_num);
> +}
> +
> +void pt_vcpu_init(struct vcpu *v)
> +{
> +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> +    unsigned int eax, ebx, ecx, edx;
> +
> +    memset(pt, 0, sizeof(struct pt_desc));
> +    pt->intel_pt_enabled = false;
> +
> +    if ( !cpu_has_intel_pt || !opt_intel_pt ||
> +         !(v->arch.hvm_vmx.secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA) )
> +        return;
> +
> +    /* get the number of address ranges */
> +    if ( cpuid_eax(0x14) == 1 )
> +        cpuid_count(0x14, 1, &eax, &ebx, &ecx, &edx);
> +    else
> +        return;
> +

        if ( cpuid_eax(0x14) != 1 )
            return;
        
        cpuid_count(....);

This reduces the indentation needed.

> +    pt->addr_num = eax & 0x7;
> +    pt->guest_pt_ctx.output_mask = 0x7F;
> +    pt->intel_pt_enabled = true;
> +
> +    vmx_vmcs_enter(v);
> +    __vmwrite(GUEST_IA32_RTIT_CTL, 0);
> +    vmx_vmcs_exit(v);
> +}
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index e036303..f386933 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -467,6 +467,8 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>      if ( v->vcpu_id == 0 )
>          v->arch.user_regs.rax = 1;
>  
> +    pt_vcpu_init(v);
> +
>      return 0;
>  }
>  
> @@ -3513,6 +3515,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>      __vmread(GUEST_RSP,    &regs->rsp);
>      __vmread(GUEST_RFLAGS, &regs->rflags);
>  
> +    pt_guest_exit(v);
>      hvm_invalidate_regs_fields(regs);
>  
>      if ( paging_mode_hap(v->domain) )
> @@ -4281,6 +4284,7 @@ bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
>          }
>      }
>  
> +    pt_guest_enter(curr);
>   out:
>      if ( unlikely(curr->arch.hvm_vmx.lbr_fixup_enabled) )
>          lbr_fixup();
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 33ec3e6..46c386f 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -421,6 +421,8 @@ enum vmcs_field {
>      GUEST_PDPTE0                    = 0x0000280a,
>  #define GUEST_PDPTE(n) (GUEST_PDPTE0 + (n) * 2) /* n = 0...3 */
>      GUEST_BNDCFGS                   = 0x00002812,
> +    GUEST_IA32_RTIT_CTL             = 0x00002814,
> +    GUEST_IA32_RTIT_CTL_HIGH        = 0x00002815,
>      HOST_PAT                        = 0x00002c00,
>      HOST_EFER                       = 0x00002c02,
>      HOST_PERF_GLOBAL_CTRL           = 0x00002c04,
> diff --git a/xen/include/asm-x86/intel_pt.h b/xen/include/asm-x86/intel_pt.h
> index 909e22f..9505c8f 100644
> --- a/xen/include/asm-x86/intel_pt.h
> +++ b/xen/include/asm-x86/intel_pt.h
> @@ -40,4 +40,8 @@ struct pt_desc {
>  
>  extern bool_t opt_intel_pt;
>  
> +void pt_vcpu_init(struct vcpu *v);
> +void pt_guest_enter(struct vcpu *v);
> +void pt_guest_exit(struct vcpu *v);
> +
>  #endif /* __ASM_X86_HVM_INTEL_PT_H_ */
> -- 
> 1.8.3.1
> 

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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Kang, Luwei]

> > Load/Store Intel processor trace register in context switch.
> > MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
> > When Intel PT is supported in guest, we need load/restore PT MSRs only
> > when PT is enabled in guest.
> >
> > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> 
> Is there a reason to not use xsaves/xrstors when they are available?
> 
There have two method to implement context switch(manual and xsave/xrstors).
The first method is more directly and also won't have any performance overhead if intel PT is disabled.
If use xsave/xrstors we need to check if it available and whether PT is supported  in XSS (CPUID.0D(ecx=1).ecx).
I will think about this scenario and may make an independent patch to enable it. 
But I think manual save/restore will also needed in case xsave/rstores not available, although I think this may can't be happened.

Thanks,
Luwei Kang

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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Wei Liu]

On Fri, Apr 27, 2018 at 08:53:30AM +0000, Kang, Luwei wrote:
> > > Load/Store Intel processor trace register in context switch.
> > > MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
> > > When Intel PT is supported in guest, we need load/restore PT MSRs only
> > > when PT is enabled in guest.
> > >
> > > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> > 
> > Is there a reason to not use xsaves/xrstors when they are available?
> > 
> There have two method to implement context switch(manual and xsave/xrstors).
> The first method is more directly and also won't have any performance overhead if intel PT is disabled.
> If use xsave/xrstors we need to check if it available and whether PT is supported  in XSS (CPUID.0D(ecx=1).ecx).
> I will think about this scenario and may make an independent patch to enable it. 

Fair enough.

I figure Xen doesn't understand xsaves (among other xsave* features) at
the moment so a dedicated series to enable that is required.

Wei.

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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Jan Beulich]

>>> On 02.05.18 at 17:19, <wei.liu2@citrix.com> wrote:
> On Fri, Apr 27, 2018 at 08:53:30AM +0000, Kang, Luwei wrote:
>> > > Load/Store Intel processor trace register in context switch.
>> > > MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
>> > > When Intel PT is supported in guest, we need load/restore PT MSRs only
>> > > when PT is enabled in guest.
>> > >
>> > > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
>> > 
>> > Is there a reason to not use xsaves/xrstors when they are available?
>> > 
>> There have two method to implement context switch(manual and xsave/xrstors).
>> The first method is more directly and also won't have any performance 
> overhead if intel PT is disabled.
>> If use xsave/xrstors we need to check if it available and whether PT is 
> supported  in XSS (CPUID.0D(ecx=1).ecx).
>> I will think about this scenario and may make an independent patch to enable 
> it. 
> 
> Fair enough.
> 
> I figure Xen doesn't understand xsaves (among other xsave* features) at
> the moment so a dedicated series to enable that is required.

I wouldn't be surprised if there were bugs, but if you look at xstate.c you'll
find a number of references to XSAVES.

Jan



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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Wei Liu]

On Wed, May 02, 2018 at 09:43:33AM -0600, Jan Beulich wrote:
> >>> On 02.05.18 at 17:19, <wei.liu2@citrix.com> wrote:
> > On Fri, Apr 27, 2018 at 08:53:30AM +0000, Kang, Luwei wrote:
> >> > > Load/Store Intel processor trace register in context switch.
> >> > > MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
> >> > > When Intel PT is supported in guest, we need load/restore PT MSRs only
> >> > > when PT is enabled in guest.
> >> > >
> >> > > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> >> > 
> >> > Is there a reason to not use xsaves/xrstors when they are available?
> >> > 
> >> There have two method to implement context switch(manual and xsave/xrstors).
> >> The first method is more directly and also won't have any performance 
> > overhead if intel PT is disabled.
> >> If use xsave/xrstors we need to check if it available and whether PT is 
> > supported  in XSS (CPUID.0D(ecx=1).ecx).
> >> I will think about this scenario and may make an independent patch to enable 
> > it. 
> > 
> > Fair enough.
> > 
> > I figure Xen doesn't understand xsaves (among other xsave* features) at
> > the moment so a dedicated series to enable that is required.
> 
> I wouldn't be surprised if there were bugs, but if you look at xstate.c you'll
> find a number of references to XSAVES.

What I meant was in xstate.c:xstate_init:

   /* Mask out features not currently understood by Xen. */
    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
            cpufeat_mask(X86_FEATURE_XSAVEC) |
            cpufeat_mask(X86_FEATURE_XGETBV1) |
            cpufeat_mask(X86_FEATURE_XSAVES));

Also PT is missing in state component definitions in x86-defns.h.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Andrew Cooper]

On 02/05/18 17:15, Wei Liu wrote:
> On Wed, May 02, 2018 at 09:43:33AM -0600, Jan Beulich wrote:
>>>>> On 02.05.18 at 17:19, <wei.liu2@citrix.com> wrote:
>>> On Fri, Apr 27, 2018 at 08:53:30AM +0000, Kang, Luwei wrote:
>>>>>> Load/Store Intel processor trace register in context switch.
>>>>>> MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
>>>>>> When Intel PT is supported in guest, we need load/restore PT MSRs only
>>>>>> when PT is enabled in guest.
>>>>>>
>>>>>> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
>>>>> Is there a reason to not use xsaves/xrstors when they are available?
>>>>>
>>>> There have two method to implement context switch(manual and xsave/xrstors).
>>>> The first method is more directly and also won't have any performance 
>>> overhead if intel PT is disabled.
>>>> If use xsave/xrstors we need to check if it available and whether PT is 
>>> supported  in XSS (CPUID.0D(ecx=1).ecx).
>>>> I will think about this scenario and may make an independent patch to enable 
>>> it. 
>>>
>>> Fair enough.
>>>
>>> I figure Xen doesn't understand xsaves (among other xsave* features) at
>>> the moment so a dedicated series to enable that is required.
>> I wouldn't be surprised if there were bugs, but if you look at xstate.c you'll
>> find a number of references to XSAVES.
> What I meant was in xstate.c:xstate_init:
>
>    /* Mask out features not currently understood by Xen. */
>     eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>             cpufeat_mask(X86_FEATURE_XSAVEC) |
>             cpufeat_mask(X86_FEATURE_XGETBV1) |
>             cpufeat_mask(X86_FEATURE_XSAVES));
>
> Also PT is missing in state component definitions in x86-defns.h.

PT is the first defined XSS state, and was introduced at the same time
as XSAVES.  Xen's xsave logic should be updated to use XSAVES/XRSTORS if
available (which, other than its privileged nature, works identically to
XSAVEC AFAICT).

If PT isn't enabled, then it won't be setting in XSS, and XSAVES will be
identical to XSAVEC, performance wise.  Having the manual rdmsr/wrmsr
context switch code is, however, surely going to be slower than using
XSAVES ?

~Andrew

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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Jan Beulich]

>>> On 02.05.18 at 18:51, <andrew.cooper3@citrix.com> wrote:
> On 02/05/18 17:15, Wei Liu wrote:
>> On Wed, May 02, 2018 at 09:43:33AM -0600, Jan Beulich wrote:
>>>>>> On 02.05.18 at 17:19, <wei.liu2@citrix.com> wrote:
>>>> On Fri, Apr 27, 2018 at 08:53:30AM +0000, Kang, Luwei wrote:
>>>>>>> Load/Store Intel processor trace register in context switch.
>>>>>>> MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
>>>>>>> When Intel PT is supported in guest, we need load/restore PT MSRs only
>>>>>>> when PT is enabled in guest.
>>>>>>>
>>>>>>> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
>>>>>> Is there a reason to not use xsaves/xrstors when they are available?
>>>>>>
>>>>> There have two method to implement context switch(manual and xsave/xrstors).
>>>>> The first method is more directly and also won't have any performance 
>>>> overhead if intel PT is disabled.
>>>>> If use xsave/xrstors we need to check if it available and whether PT is 
>>>> supported  in XSS (CPUID.0D(ecx=1).ecx).
>>>>> I will think about this scenario and may make an independent patch to enable 
> 
>>>> it. 
>>>>
>>>> Fair enough.
>>>>
>>>> I figure Xen doesn't understand xsaves (among other xsave* features) at
>>>> the moment so a dedicated series to enable that is required.
>>> I wouldn't be surprised if there were bugs, but if you look at xstate.c 
> you'll
>>> find a number of references to XSAVES.
>> What I meant was in xstate.c:xstate_init:
>>
>>    /* Mask out features not currently understood by Xen. */
>>     eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>>             cpufeat_mask(X86_FEATURE_XSAVEC) |
>>             cpufeat_mask(X86_FEATURE_XGETBV1) |
>>             cpufeat_mask(X86_FEATURE_XSAVES));
>>
>> Also PT is missing in state component definitions in x86-defns.h.
> 
> PT is the first defined XSS state, and was introduced at the same time
> as XSAVES.  Xen's xsave logic should be updated to use XSAVES/XRSTORS if
> available (which, other than its privileged nature, works identically to
> XSAVEC AFAICT).

We do that already (except that we don't "if available" but "if necessary",
i.e. if any state requiring XSAVES is to be saved/restored).

Jan



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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Jan Beulich]

>>> On 02.05.18 at 18:15, <wei.liu2@citrix.com> wrote:
> On Wed, May 02, 2018 at 09:43:33AM -0600, Jan Beulich wrote:
>> >>> On 02.05.18 at 17:19, <wei.liu2@citrix.com> wrote:
>> > On Fri, Apr 27, 2018 at 08:53:30AM +0000, Kang, Luwei wrote:
>> >> > > Load/Store Intel processor trace register in context switch.
>> >> > > MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
>> >> > > When Intel PT is supported in guest, we need load/restore PT MSRs only
>> >> > > when PT is enabled in guest.
>> >> > >
>> >> > > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
>> >> > 
>> >> > Is there a reason to not use xsaves/xrstors when they are available?
>> >> > 
>> >> There have two method to implement context switch(manual and 
> xsave/xrstors).
>> >> The first method is more directly and also won't have any performance 
>> > overhead if intel PT is disabled.
>> >> If use xsave/xrstors we need to check if it available and whether PT is 
>> > supported  in XSS (CPUID.0D(ecx=1).ecx).
>> >> I will think about this scenario and may make an independent patch to enable 
>> > it. 
>> > 
>> > Fair enough.
>> > 
>> > I figure Xen doesn't understand xsaves (among other xsave* features) at
>> > the moment so a dedicated series to enable that is required.
>> 
>> I wouldn't be surprised if there were bugs, but if you look at xstate.c you'll
>> find a number of references to XSAVES.
> 
> What I meant was in xstate.c:xstate_init:
> 
>    /* Mask out features not currently understood by Xen. */
>     eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>             cpufeat_mask(X86_FEATURE_XSAVEC) |
>             cpufeat_mask(X86_FEATURE_XGETBV1) |
>             cpufeat_mask(X86_FEATURE_XSAVES));

Well, this masks out things other than XSAVES and the three others we do
understand.

> Also PT is missing in state component definitions in x86-defns.h.

Right - that's what I'd expect the series to implement.

Jan



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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Wei Liu]

On Thu, May 03, 2018 at 01:26:17AM -0600, Jan Beulich wrote:
> >>> On 02.05.18 at 18:15, <wei.liu2@citrix.com> wrote:
> > On Wed, May 02, 2018 at 09:43:33AM -0600, Jan Beulich wrote:
> >> >>> On 02.05.18 at 17:19, <wei.liu2@citrix.com> wrote:
> >> > On Fri, Apr 27, 2018 at 08:53:30AM +0000, Kang, Luwei wrote:
> >> >> > > Load/Store Intel processor trace register in context switch.
> >> >> > > MSR IA32_RTIT_CTL is loaded/stored automatically from VMCS.
> >> >> > > When Intel PT is supported in guest, we need load/restore PT MSRs only
> >> >> > > when PT is enabled in guest.
> >> >> > >
> >> >> > > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> >> >> > 
> >> >> > Is there a reason to not use xsaves/xrstors when they are available?
> >> >> > 
> >> >> There have two method to implement context switch(manual and 
> > xsave/xrstors).
> >> >> The first method is more directly and also won't have any performance 
> >> > overhead if intel PT is disabled.
> >> >> If use xsave/xrstors we need to check if it available and whether PT is 
> >> > supported  in XSS (CPUID.0D(ecx=1).ecx).
> >> >> I will think about this scenario and may make an independent patch to enable 
> >> > it. 
> >> > 
> >> > Fair enough.
> >> > 
> >> > I figure Xen doesn't understand xsaves (among other xsave* features) at
> >> > the moment so a dedicated series to enable that is required.
> >> 
> >> I wouldn't be surprised if there were bugs, but if you look at xstate.c you'll
> >> find a number of references to XSAVES.
> > 
> > What I meant was in xstate.c:xstate_init:
> > 
> >    /* Mask out features not currently understood by Xen. */
> >     eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> >             cpufeat_mask(X86_FEATURE_XSAVEC) |
> >             cpufeat_mask(X86_FEATURE_XGETBV1) |
> >             cpufeat_mask(X86_FEATURE_XSAVES));
> 
> Well, this masks out things other than XSAVES and the three others we do
> understand.

D'oh, I read that wrong. Sorry for the noise.

Wei.

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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Jan Beulich]

>>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> --- a/xen/arch/x86/cpu/intel_pt.c
> +++ b/xen/arch/x86/cpu/intel_pt.c
> @@ -21,7 +21,76 @@
>  #include <xen/types.h>
>  #include <xen/cache.h>
>  #include <xen/init.h>
> +#include <asm/hvm/vmx/vmx.h>
> +#include <asm/intel_pt.h>
>  
>  /* intel_pt: Flag to enable Intel Processor Trace (default on). */
>  bool_t __read_mostly opt_intel_pt = 1;
>  boolean_param("intel_pt", opt_intel_pt);
> +
> +static inline void pt_load_msr(struct pt_ctx *ctx, u32 addr_num)

const

> +{
> +    u32 i;
> +    wrmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> +    wrmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> +    wrmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> +    wrmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> +    for ( i = 0; i < addr_num; i++ )
> +        wrmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]);
> +}
> +
> +static inline void pt_save_msr(struct pt_ctx *ctx, u32 addr_num)
> +{
> +    u32 i;
> +    rdmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> +    rdmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> +    rdmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> +    rdmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> +    for ( i = 0; i < addr_num; i++ )
> +        rdmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]);
> +}
> +
> +void pt_guest_enter(struct vcpu *v)

const

> +{
> +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> +
> +    if ( pt->intel_pt_enabled &&
> +       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
> +        pt_load_msr(&pt->guest_pt_ctx, pt->addr_num);
> +}
> +
> +void pt_guest_exit(struct vcpu *v)
> +{
> +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> +
> +    if ( pt->intel_pt_enabled &&
> +       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
> +        pt_save_msr(&pt->guest_pt_ctx, pt->addr_num);
> +}
> +
> +void pt_vcpu_init(struct vcpu *v)
> +{
> +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> +    unsigned int eax, ebx, ecx, edx;
> +
> +    memset(pt, 0, sizeof(struct pt_desc));

As long as this is a sub-structure of struct vcpu, this is unnecessary.
And once you switch to separate allocation, you should simply use
xzalloc().

> +    pt->intel_pt_enabled = false;

This is redundant with the memset() just done.

> +    if ( !cpu_has_intel_pt || !opt_intel_pt ||
> +         !(v->arch.hvm_vmx.secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA) )
> +        return;
> +
> +    /* get the number of address ranges */
> +    if ( cpuid_eax(0x14) == 1 )

Why would a max leaf above 1 not be okay?

> +        cpuid_count(0x14, 1, &eax, &ebx, &ecx, &edx);
> +    else
> +        return;

Please invert the condition in the if() and use "return" first,
rendering the "else" unnecessary.

> +    pt->addr_num = eax & 0x7;
> +    pt->guest_pt_ctx.output_mask = 0x7F;

Bad literal numbers. In the latter case you want a #define, while in the
former case you should be able to read the host CPUID policy (once
you've properly defined fields for it).

> +    pt->intel_pt_enabled = true;
> +
> +    vmx_vmcs_enter(v);
> +    __vmwrite(GUEST_IA32_RTIT_CTL, 0);
> +    vmx_vmcs_exit(v);

Wouldn't this better go into construct_vmcs(), especially if this isn't the
default (in which case the early returns above would leave the field with
a random value)?

> @@ -4281,6 +4284,7 @@ bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
>          }
>      }
>  
> +    pt_guest_enter(curr);
>   out:
>      if ( unlikely(curr->arch.hvm_vmx.lbr_fixup_enabled) )
>          lbr_fixup();

Doesn't your addition belong after the out label? Also note that the function
has two early return paths, one of which likely would need this added, too.

> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -421,6 +421,8 @@ enum vmcs_field {
>      GUEST_PDPTE0                    = 0x0000280a,
>  #define GUEST_PDPTE(n) (GUEST_PDPTE0 + (n) * 2) /* n = 0...3 */
>      GUEST_BNDCFGS                   = 0x00002812,
> +    GUEST_IA32_RTIT_CTL             = 0x00002814,
> +    GUEST_IA32_RTIT_CTL_HIGH        = 0x00002815,

Did you not notice that we don't have any *_HIGH enumerators (anymore)?

Jan


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

Re: [PATCH RESEND v1 5/7] x86: Implement Intel Processor Trace context switch

[Kang, Luwei]

> > --- a/xen/arch/x86/cpu/intel_pt.c
> > +++ b/xen/arch/x86/cpu/intel_pt.c
> > @@ -21,7 +21,76 @@
> >  #include <xen/types.h>
> >  #include <xen/cache.h>
> >  #include <xen/init.h>
> > +#include <asm/hvm/vmx/vmx.h>
> > +#include <asm/intel_pt.h>
> >
> >  /* intel_pt: Flag to enable Intel Processor Trace (default on). */
> > bool_t __read_mostly opt_intel_pt = 1;  boolean_param("intel_pt",
> > opt_intel_pt);
> > +
> > +static inline void pt_load_msr(struct pt_ctx *ctx, u32 addr_num)
> 
> const
> 
> > +{
> > +    u32 i;
> > +    wrmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> > +    wrmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> > +    wrmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> > +    wrmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> > +    for ( i = 0; i < addr_num; i++ )
> > +        wrmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]); }
> > +
> > +static inline void pt_save_msr(struct pt_ctx *ctx, u32 addr_num) {
> > +    u32 i;
> > +    rdmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> > +    rdmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> > +    rdmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> > +    rdmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> > +    for ( i = 0; i < addr_num; i++ )
> > +        rdmsrl(MSR_IA32_RTIT_ADDR0_A + i, ctx->addr[i]); }
> > +
> > +void pt_guest_enter(struct vcpu *v)
> 
> const
> 
> > +{
> > +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> > +
> > +    if ( pt->intel_pt_enabled &&
> > +       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
> > +        pt_load_msr(&pt->guest_pt_ctx, pt->addr_num); }
> > +
> > +void pt_guest_exit(struct vcpu *v)
> > +{
> > +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> > +
> > +    if ( pt->intel_pt_enabled &&
> > +       (pt->guest_pt_ctx.ctl & MSR_IA32_RTIT_CTL_TRACEEN) )
> > +        pt_save_msr(&pt->guest_pt_ctx, pt->addr_num); }
> > +
> > +void pt_vcpu_init(struct vcpu *v)
> > +{
> > +    struct pt_desc *pt = &v->arch.hvm_vmx.pt_desc;
> > +    unsigned int eax, ebx, ecx, edx;
> > +
> > +    memset(pt, 0, sizeof(struct pt_desc));
> 
> As long as this is a sub-structure of struct vcpu, this is unnecessary.
> And once you switch to separate allocation, you should simply use xzalloc().

Get it. Will fix it.

> 
> > +    pt->intel_pt_enabled = false;
> 
> This is redundant with the memset() just done.
> 
> > +    if ( !cpu_has_intel_pt || !opt_intel_pt ||
> > +         !(v->arch.hvm_vmx.secondary_exec_control &
> SECONDARY_EXEC_PT_USE_GPA) )
> > +        return;
> > +
> > +    /* get the number of address ranges */
> > +    if ( cpuid_eax(0x14) == 1 )
> 
> Why would a max leaf above 1 not be okay?

Yes, you are right. Will change to:
if ( cpuid_eax(0x14) == 0 )
    return;

> 
> > +        cpuid_count(0x14, 1, &eax, &ebx, &ecx, &edx);
> > +    else
> > +        return;
> 
> Please invert the condition in the if() and use "return" first, rendering the
> "else" unnecessary.
> 
> > +    pt->addr_num = eax & 0x7;
> > +    pt->guest_pt_ctx.output_mask = 0x7F;
> 
> Bad literal numbers. In the latter case you want a #define, while in the
> former case you should be able to read the host CPUID policy (once you've
> properly defined fields for it).

Get it. Will fix it.

> 
> > +    pt->intel_pt_enabled = true;
> > +
> > +    vmx_vmcs_enter(v);
> > +    __vmwrite(GUEST_IA32_RTIT_CTL, 0);
> > +    vmx_vmcs_exit(v);
> 
> Wouldn't this better go into construct_vmcs(), especially if this isn't the
> default (in which case the early returns above would leave the field with a
> random value)?

Will move this into construct_vmcs(). I didn't found any case cause this with a random value. 
I just think about another case, If we create two guest and all can enabled intel PT. we may need to re-initialize the value of GUEST_IA32_RTIT_CTL if a physical logic CPU switch vcpu from one guest to another vcpu from a different guest.

> 
> > @@ -4281,6 +4284,7 @@ bool vmx_vmenter_helper(const struct
> cpu_user_regs *regs)
> >          }
> >      }
> >
> > +    pt_guest_enter(curr);
> >   out:
> >      if ( unlikely(curr->arch.hvm_vmx.lbr_fixup_enabled) )
> >          lbr_fixup();
> 
> Doesn't your addition belong after the out label? Also note that the function
> has two early return paths, one of which likely would need this added, too.

Yes, will move this function into the out label.

> 
> > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> > @@ -421,6 +421,8 @@ enum vmcs_field {
> >      GUEST_PDPTE0                    = 0x0000280a,
> >  #define GUEST_PDPTE(n) (GUEST_PDPTE0 + (n) * 2) /* n = 0...3 */
> >      GUEST_BNDCFGS                   = 0x00002812,
> > +    GUEST_IA32_RTIT_CTL             = 0x00002814,
> > +    GUEST_IA32_RTIT_CTL_HIGH        = 0x00002815,
> 
> Did you not notice that we don't have any *_HIGH enumerators (anymore)?

Oh, get it. I will remove it in next version.

Thanks,
Luwei Kang

> 
> Jan


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

[PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Luwei Kang]

Intel PT MSRs read/write will not be intercepted when guest enabled
Intel PT. IA32_RTIT_CTL read/write will always cause a VM-Exit.

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 xen/arch/x86/cpu/intel_pt.c    | 101 +++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c     |  18 ++++++++
 xen/include/asm-x86/intel_pt.h |   4 ++
 3 files changed, 123 insertions(+)

diff --git a/xen/arch/x86/cpu/intel_pt.c b/xen/arch/x86/cpu/intel_pt.c
index c0e9e68..d530e57 100644
--- a/xen/arch/x86/cpu/intel_pt.c
+++ b/xen/arch/x86/cpu/intel_pt.c
@@ -28,6 +28,107 @@
 bool_t __read_mostly opt_intel_pt = 1;
 boolean_param("intel_pt", opt_intel_pt);
 
+
+static void intel_pt_disable_intercept_for_msr(u32 addr_num)
+{
+    struct vcpu *v = current;
+    int i;
+
+    vmx_clear_msr_intercept(v, MSR_IA32_RTIT_STATUS, VMX_MSR_RW);
+    vmx_clear_msr_intercept(v, MSR_IA32_RTIT_OUTPUT_BASE, VMX_MSR_RW);
+    vmx_clear_msr_intercept(v, MSR_IA32_RTIT_OUTPUT_MASK, VMX_MSR_RW);
+    vmx_clear_msr_intercept(v, MSR_IA32_RTIT_CR3_MATCH, VMX_MSR_RW);
+    for ( i = 0; i < addr_num; i++ )
+        vmx_clear_msr_intercept(v, MSR_IA32_RTIT_ADDR0_A + i, VMX_MSR_RW);
+}
+
+static void intel_pt_enable_intercept_for_msr(u32 addr_num)
+{
+    struct vcpu *v = current;
+    int i;
+
+    vmx_set_msr_intercept(v, MSR_IA32_RTIT_STATUS, VMX_MSR_RW);
+    vmx_set_msr_intercept(v, MSR_IA32_RTIT_OUTPUT_BASE, VMX_MSR_RW);
+    vmx_set_msr_intercept(v, MSR_IA32_RTIT_OUTPUT_MASK, VMX_MSR_RW);
+    vmx_set_msr_intercept(v, MSR_IA32_RTIT_CR3_MATCH, VMX_MSR_RW);
+    for ( i = 0; i < addr_num; i++ )
+        vmx_set_msr_intercept(v, MSR_IA32_RTIT_ADDR0_A + i, VMX_MSR_RW);
+}
+
+void pt_set_rtit_ctl(struct pt_desc *pt_desc, uint64_t msr_content)
+{
+    if (msr_content & MSR_IA32_RTIT_CTL_TRACEEN)
+        intel_pt_disable_intercept_for_msr(pt_desc->addr_num);
+    else
+        intel_pt_enable_intercept_for_msr(pt_desc->addr_num);
+
+    pt_desc->guest_pt_ctx.ctl = msr_content;
+
+    vmx_vmcs_enter(current);
+    __vmwrite(GUEST_IA32_RTIT_CTL, msr_content);
+    vmx_vmcs_exit(current);
+}
+
+int pt_do_rdmsr(unsigned int msr, uint64_t *msr_content)
+{
+    struct pt_desc *pt_desc = &current->arch.hvm_vmx.pt_desc;
+
+    if ( !opt_intel_pt )
+        return 1;
+
+    switch ( msr ) {
+    case MSR_IA32_RTIT_CTL:
+        *msr_content = pt_desc->guest_pt_ctx.ctl;
+        break;
+    case MSR_IA32_RTIT_STATUS:
+        *msr_content = pt_desc->guest_pt_ctx.status;
+        break;
+    case MSR_IA32_RTIT_OUTPUT_BASE:
+        *msr_content = pt_desc->guest_pt_ctx.output_base;
+        break;
+    case MSR_IA32_RTIT_OUTPUT_MASK:
+        *msr_content = pt_desc->guest_pt_ctx.output_mask;
+        break;
+    case MSR_IA32_RTIT_CR3_MATCH:
+        *msr_content = pt_desc->guest_pt_ctx.cr3_match;
+        break;
+    default:
+        *msr_content = pt_desc->guest_pt_ctx.addr[msr - MSR_IA32_RTIT_ADDR0_A];
+    }
+
+    return 0;
+}
+
+int pt_do_wrmsr(unsigned int msr, uint64_t msr_content)
+{
+    struct pt_desc *pt_desc = &current->arch.hvm_vmx.pt_desc;
+
+    if ( !opt_intel_pt )
+        return 1;
+
+    switch ( msr ) {
+    case MSR_IA32_RTIT_CTL:
+        pt_set_rtit_ctl(pt_desc, msr_content);
+        break;
+    case MSR_IA32_RTIT_STATUS:
+        pt_desc->guest_pt_ctx.status = msr_content;
+        break;
+    case MSR_IA32_RTIT_OUTPUT_BASE:
+        pt_desc->guest_pt_ctx.output_base = msr_content;
+        break;
+    case MSR_IA32_RTIT_OUTPUT_MASK:
+        pt_desc->guest_pt_ctx.output_mask = msr_content | 0x7F;
+        break;
+    case MSR_IA32_RTIT_CR3_MATCH:
+        pt_desc->guest_pt_ctx.cr3_match = msr_content;
+        break;
+    default:
+        pt_desc->guest_pt_ctx.addr[msr - MSR_IA32_RTIT_ADDR0_A] = msr_content;
+    }
+
+    return 0;
+}
+
 static inline void pt_load_msr(struct pt_ctx *ctx, u32 addr_num)
 {
     u32 i;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f386933..e6713fd 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2929,6 +2929,15 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         if ( vpmu_do_rdmsr(msr, msr_content) )
             goto gp_fault;
         break;
+    case MSR_IA32_RTIT_CTL:
+    case MSR_IA32_RTIT_STATUS:
+    case MSR_IA32_RTIT_OUTPUT_BASE:
+    case MSR_IA32_RTIT_OUTPUT_MASK:
+    case MSR_IA32_RTIT_CR3_MATCH:
+    case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
+        if ( pt_do_rdmsr(msr, msr_content) )
+            goto gp_fault;
+        break;
 
     default:
         if ( passive_domain_do_rdmsr(msr, msr_content) )
@@ -3149,6 +3158,15 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
          if ( vpmu_do_wrmsr(msr, msr_content, 0) )
             goto gp_fault;
         break;
+    case MSR_IA32_RTIT_CTL:
+    case MSR_IA32_RTIT_STATUS:
+    case MSR_IA32_RTIT_OUTPUT_BASE:
+    case MSR_IA32_RTIT_OUTPUT_MASK:
+    case MSR_IA32_RTIT_CR3_MATCH:
+    case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
+        if ( pt_do_wrmsr(msr, msr_content) )
+            goto gp_fault;
+        break;
 
     default:
         if ( passive_domain_do_wrmsr(msr, msr_content) )
diff --git a/xen/include/asm-x86/intel_pt.h b/xen/include/asm-x86/intel_pt.h
index 9505c8f..5d51a12 100644
--- a/xen/include/asm-x86/intel_pt.h
+++ b/xen/include/asm-x86/intel_pt.h
@@ -40,6 +40,10 @@ struct pt_desc {
 
 extern bool_t opt_intel_pt;
 
+int pt_do_rdmsr(unsigned int msr, uint64_t *pdata);
+int pt_do_wrmsr(unsigned int msr, uint64_t data);
+void pt_set_rtit_ctl(struct pt_desc *pt_desc, uint64_t msr_content);
+
 void pt_vcpu_init(struct vcpu *v);
 void pt_guest_enter(struct vcpu *v);
 void pt_guest_exit(struct vcpu *v);
-- 
1.8.3.1


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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Jan Beulich]

>>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> --- a/xen/arch/x86/cpu/intel_pt.c
> +++ b/xen/arch/x86/cpu/intel_pt.c
> @@ -28,6 +28,107 @@
>  bool_t __read_mostly opt_intel_pt = 1;
>  boolean_param("intel_pt", opt_intel_pt);
>  
> +
> +static void intel_pt_disable_intercept_for_msr(u32 addr_num)
> +{
> +    struct vcpu *v = current;
> +    int i;

unsigned int

> +    vmx_clear_msr_intercept(v, MSR_IA32_RTIT_STATUS, VMX_MSR_RW);
> +    vmx_clear_msr_intercept(v, MSR_IA32_RTIT_OUTPUT_BASE, VMX_MSR_RW);
> +    vmx_clear_msr_intercept(v, MSR_IA32_RTIT_OUTPUT_MASK, VMX_MSR_RW);
> +    vmx_clear_msr_intercept(v, MSR_IA32_RTIT_CR3_MATCH, VMX_MSR_RW);
> +    for ( i = 0; i < addr_num; i++ )
> +        vmx_clear_msr_intercept(v, MSR_IA32_RTIT_ADDR0_A + i, VMX_MSR_RW);

Are all of these expected to be frequently written? If not, keeping the intercept
enabled would allow to avoid the MSR reads right after a VM exit.

> +void pt_set_rtit_ctl(struct pt_desc *pt_desc, uint64_t msr_content)
> +{
> +    if (msr_content & MSR_IA32_RTIT_CTL_TRACEEN)

Style (missing blanks).

> +int pt_do_rdmsr(unsigned int msr, uint64_t *msr_content)
> +{
> +    struct pt_desc *pt_desc = &current->arch.hvm_vmx.pt_desc;
> +
> +    if ( !opt_intel_pt )
> +        return 1;
> +
> +    switch ( msr ) {

Style (brace placement).

But on the whole - I'm not sure we want any new MSR handling like this.
Properly integrating this with guest_{rd,wr}msr() would be much preferred.

And then - what about migrating a guest with the feature enabled? Even
if you choose to not support this initially, you'd at least need to fail
migration cleanly when the feature is in use.

Jan



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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Jan Beulich]

>>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> +int pt_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +{
> +    struct pt_desc *pt_desc = &current->arch.hvm_vmx.pt_desc;
> +
> +    if ( !opt_intel_pt )
> +        return 1;
> +
> +    switch ( msr ) {
> +    case MSR_IA32_RTIT_CTL:
> +        pt_set_rtit_ctl(pt_desc, msr_content);
> +        break;
> +    case MSR_IA32_RTIT_STATUS:
> +        pt_desc->guest_pt_ctx.status = msr_content;
> +        break;
> +    case MSR_IA32_RTIT_OUTPUT_BASE:
> +        pt_desc->guest_pt_ctx.output_base = msr_content;
> +        break;
> +    case MSR_IA32_RTIT_OUTPUT_MASK:
> +        pt_desc->guest_pt_ctx.output_mask = msr_content | 0x7F;
> +        break;
> +    case MSR_IA32_RTIT_CR3_MATCH:
> +        pt_desc->guest_pt_ctx.cr3_match = msr_content;
> +        break;
> +    default:
> +        pt_desc->guest_pt_ctx.addr[msr - MSR_IA32_RTIT_ADDR0_A] = msr_content;

At least these last ones need to have a canonical address check attached.

And there is one more thing I've not found throughout the series: EPT
violations and a few other VM exits have gained a new qualification bit,
indicating that it's not the current instruction which has caused the exit.
I can't imagine this to not require any change to the handling of such
exits - in particular, such exits must never be handled by invoking the
insn emulator. Aiui the only handling options here are to eliminate the
condition causing the exit, or to crash the guest. There's no way to
emulate the intended access.

Yet another apparently missing piece appears to be the corresponding
XSAVE handling.

Jan



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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Kang, Luwei]

> > +int pt_do_wrmsr(unsigned int msr, uint64_t msr_content) {
> > +    struct pt_desc *pt_desc = &current->arch.hvm_vmx.pt_desc;
> > +
> > +    if ( !opt_intel_pt )
> > +        return 1;
> > +
> > +    switch ( msr ) {
> > +    case MSR_IA32_RTIT_CTL:
> > +        pt_set_rtit_ctl(pt_desc, msr_content);
> > +        break;
> > +    case MSR_IA32_RTIT_STATUS:
> > +        pt_desc->guest_pt_ctx.status = msr_content;
> > +        break;
> > +    case MSR_IA32_RTIT_OUTPUT_BASE:
> > +        pt_desc->guest_pt_ctx.output_base = msr_content;
> > +        break;
> > +    case MSR_IA32_RTIT_OUTPUT_MASK:
> > +        pt_desc->guest_pt_ctx.output_mask = msr_content | 0x7F;
> > +        break;
> > +    case MSR_IA32_RTIT_CR3_MATCH:
> > +        pt_desc->guest_pt_ctx.cr3_match = msr_content;
> > +        break;
> > +    default:
> > +        pt_desc->guest_pt_ctx.addr[msr - MSR_IA32_RTIT_ADDR0_A] =
> > + msr_content;
> 
> At least these last ones need to have a canonical address check attached.

Get it. Will add address range number check and "goto gp_fault" if access unsupported MSRs.

> 
> And there is one more thing I've not found throughout the series: EPT violations and a few other VM exits have gained a new
> qualification bit, indicating that it's not the current instruction which has caused the exit.

Hi Jan,
    I don't quite understand here about EPT violations and other VM exit qualification bit. There may have an EPT violations when guest record trace to ToPA. Is this what is your concern? About new vm-exit qualification bit, do you mean there have new qualification bit for Intel PT?

> I can't imagine this to not require any change to the handling of such exits - in particular, such exits must never be handled by
> invoking the insn emulator. Aiui the only handling options here are to eliminate the condition causing the exit, or to crash the guest.
> There's no way to emulate the intended access.

Emulate which instructions? Can you give me an example?

Thanks,
Luwei Kang

> 
> Yet another apparently missing piece appears to be the corresponding XSAVE handling.
> 
> Jan
> 


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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Jan Beulich]

>>> On 03.05.18 at 07:22, <luwei.kang@intel.com> wrote:
>> And there is one more thing I've not found throughout the series: EPT 
> violations and a few other VM exits have gained a new
>> qualification bit, indicating that it's not the current instruction which 
> has caused the exit.
> 
>     I don't quite understand here about EPT violations and other VM exit 
> qualification bit. There may have an EPT violations when guest record trace 
> to ToPA. Is this what is your concern? About new vm-exit qualification bit, do 
> you mean there have new qualification bit for Intel PT?

Quoting the respective doc:

"4.2.2.1 VM Exits Due to Intel PT Output

 Treating PT output addresses as guest-physical addresses introduces the
 possibility of taking events on PT output reads and writes. Event possibilities
 include EPT violations, EPT misconfigurations, PML log-full VM exits, and APIC
 access VM exits.

 Exit Qualification

 Intel PT output reads and writes are asynchronous to instruction execution,
 as a result of the internal buffering of trace data. Trace packets are output
 some unpredictable number of cycles after the completion of the instructions
 or events that generated them. For this reason, any VM exit caused by Intel
 PT output will set the following new exit qualification bit."

>> I can't imagine this to not require any change to the handling of such exits 
> - in particular, such exits must never be handled by
>> invoking the insn emulator. Aiui the only handling options here are to 
> eliminate the condition causing the exit, or to crash the guest.
>> There's no way to emulate the intended access.
> 
> Emulate which instructions? Can you give me an example?

No instructions, as I've said (and hence no example). My point is you need to
make sure we don't _ever_ try to emulate the instruction at which guest state
points when this is an EPT violation (or misconfiguration) caused by Intel PT.

Jan



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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Kang, Luwei]

> >>> On 03.05.18 at 07:22, <luwei.kang@intel.com> wrote:
> >> And there is one more thing I've not found throughout the series: EPT
> > violations and a few other VM exits have gained a new
> >> qualification bit, indicating that it's not the current instruction
> >> which
> > has caused the exit.
> >
> >     I don't quite understand here about EPT violations and other VM
> > exit qualification bit. There may have an EPT violations when guest
> > record trace to ToPA. Is this what is your concern? About new vm-exit
> > qualification bit, do you mean there have new qualification bit for Intel PT?
> 
> Quoting the respective doc:
> 
> "4.2.2.1 VM Exits Due to Intel PT Output
> 
>  Treating PT output addresses as guest-physical addresses introduces the  possibility of taking events on PT output reads and writes.
> Event possibilities  include EPT violations, EPT misconfigurations, PML log-full VM exits, and APIC  access VM exits.
> 
>  Exit Qualification
> 
>  Intel PT output reads and writes are asynchronous to instruction execution,  as a result of the internal buffering of trace data. Trace
> packets are output  some unpredictable number of cycles after the completion of the instructions  or events that generated them.
> For this reason, any VM exit caused by Intel  PT output will set the following new exit qualification bit."

Hi Jan,
     Thanks for your clarification. Please correct me if I have something wrong. Guest may execute an instruction and this instruction may produce an PT packet save in PT output buffer. An EPT violation will be generated if the address of this PT buffer don't have EPT page table mapping, but this EPT violations shouldn't be handled by x86_emulate() because it no relate with the execute of this instruction.

     In that case, can we build the EPT map when set the output buffer address (IA32_RTIT_OUTPUT_BASE) and crash the guest if still happened EPT violation with Intel PT output buffer read/write exit qualification. Or add an exit qualification check before instruction emulation?

Thanks,
Luwei Kang

> 
> >> I can't imagine this to not require any change to the handling of
> >> such exits
> > - in particular, such exits must never be handled by
> >> invoking the insn emulator. Aiui the only handling options here are
> >> to
> > eliminate the condition causing the exit, or to crash the guest.
> >> There's no way to emulate the intended access.
> >
> > Emulate which instructions? Can you give me an example?
> 
> No instructions, as I've said (and hence no example). My point is you need to make sure we don't _ever_ try to emulate the
> instruction at which guest state points when this is an EPT violation (or misconfiguration) caused by Intel PT.
> 
> Jan
> 


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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Jan Beulich]

>>> On 03.05.18 at 11:40, <luwei.kang@intel.com> wrote:
>      Thanks for your clarification. Please correct me if I have something 
> wrong. Guest may execute an instruction and this instruction may produce an 
> PT packet save in PT output buffer. An EPT violation will be generated if the 
> address of this PT buffer don't have EPT page table mapping, but this EPT 
> violations shouldn't be handled by x86_emulate() because it no relate with 
> the execute of this instruction.

Plus - and that's very important - the EPT violation may be reported on some
later insn.

>      In that case, can we build the EPT map when set the output buffer 
> address (IA32_RTIT_OUTPUT_BASE) and crash the guest if still happened EPT 
> violation with Intel PT output buffer read/write exit qualification. Or add 
> an exit qualification check before instruction emulation?

Imo you should add an exit qualification check in any case. Depending
what else you do, finding the new bit set may imply crashing the domain
or doing something more sophisticated.

Jan



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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Kang, Luwei]

> >      Thanks for your clarification. Please correct me if I have
> > something wrong. Guest may execute an instruction and this instruction
> > may produce an PT packet save in PT output buffer. An EPT violation
> > will be generated if the address of this PT buffer don't have EPT page
> > table mapping, but this EPT violations shouldn't be handled by
> > x86_emulate() because it no relate with the execute of this instruction.
> 
> Plus - and that's very important - the EPT violation may be reported on some later insn.

Hi Jan,
You mean the "later instruction" is some new instruction in future hardware?  Is there have something need we attention or any different with current instruction which can lead EPT violation? Sorry, I didn't catch your concern. 

> 
> >      In that case, can we build the EPT map when set the output buffer
> > address (IA32_RTIT_OUTPUT_BASE) and crash the guest if still happened
> > EPT violation with Intel PT output buffer read/write exit
> > qualification. Or add an exit qualification check before instruction emulation?
> 
> Imo you should add an exit qualification check in any case. Depending what else you do, finding the new bit set may imply crashing
> the domain or doing something more sophisticated.

Do you mean add this check at the beginning of any specific "exit_reason" handler in vmx_vmexit_handler() function?
Another question is where to build this EPT mapping? Setting IA32_RTIT_OUTPUT_BASE or handled by EPT violation?

Thanks,
Luwei Kang

> 
> Jan
> 


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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Jan Beulich]

>>> On 04.05.18 at 05:53, <luwei.kang@intel.com> wrote:
>> >      Thanks for your clarification. Please correct me if I have
>> > something wrong. Guest may execute an instruction and this instruction
>> > may produce an PT packet save in PT output buffer. An EPT violation
>> > will be generated if the address of this PT buffer don't have EPT page
>> > table mapping, but this EPT violations shouldn't be handled by
>> > x86_emulate() because it no relate with the execute of this instruction.
>> 
>> Plus - and that's very important - the EPT violation may be reported on some 
> later insn.
> 
> You mean the "later instruction" is some new instruction in future hardware? 

No, I mean an instruction following later in the instruction stream.

>> >      In that case, can we build the EPT map when set the output buffer
>> > address (IA32_RTIT_OUTPUT_BASE) and crash the guest if still happened
>> > EPT violation with Intel PT output buffer read/write exit
>> > qualification. Or add an exit qualification check before instruction emulation?
>> 
>> Imo you should add an exit qualification check in any case. Depending what 
> else you do, finding the new bit set may imply crashing
>> the domain or doing something more sophisticated.
> 
> Do you mean add this check at the beginning of any specific "exit_reason" 
> handler in vmx_vmexit_handler() function?

That depends. Surely not for every exit reason, but only those for which this
new bit is valid (iirc exit qualifications differ per exit reason anyway, so you
can't unilaterally check the same bit everywhere). And even for those where
the bit is valid, I'm not sure you can decide in the exit handler alone what to
do if the bit is set. It may be necessary to propagate the flag down the call
tree.

> Another question is where to build this EPT mapping? Setting 
> IA32_RTIT_OUTPUT_BASE or handled by EPT violation?

I have no idea - that's more a question for you to answer yourself.

Jan



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

Re: [PATCH RESEND v1 6/7] x86: Implement Intel Processor Trace MSRs read/write

[Kang, Luwei]

> >>> On 04.05.18 at 05:53, <luwei.kang@intel.com> wrote:
> >> >      Thanks for your clarification. Please correct me if I have
> >> > something wrong. Guest may execute an instruction and this
> >> > instruction may produce an PT packet save in PT output buffer. An
> >> > EPT violation will be generated if the address of this PT buffer
> >> > don't have EPT page table mapping, but this EPT violations
> >> > shouldn't be handled by
> >> > x86_emulate() because it no relate with the execute of this instruction.
> >>
> >> Plus - and that's very important - the EPT violation may be reported
> >> on some
> > later insn.
> >
> > You mean the "later instruction" is some new instruction in future hardware?
> 
> No, I mean an instruction following later in the instruction stream.
> 
> >> >      In that case, can we build the EPT map when set the output
> >> > buffer address (IA32_RTIT_OUTPUT_BASE) and crash the guest if still
> >> > happened EPT violation with Intel PT output buffer read/write exit
> >> > qualification. Or add an exit qualification check before instruction emulation?
> >>
> >> Imo you should add an exit qualification check in any case. Depending
> >> what
> > else you do, finding the new bit set may imply crashing
> >> the domain or doing something more sophisticated.
> >
> > Do you mean add this check at the beginning of any specific "exit_reason"
> > handler in vmx_vmexit_handler() function?
> 
> That depends. Surely not for every exit reason, but only those for which this new bit is valid (iirc exit qualifications differ per exit
> reason anyway, so you can't unilaterally check the same bit everywhere). And even for those where the bit is valid, I'm not sure you
> can decide in the exit handler alone what to do if the bit is set. It may be necessary to propagate the flag down the call tree.
> 
> > Another question is where to build this EPT mapping? Setting
> > IA32_RTIT_OUTPUT_BASE or handled by EPT violation?
> 
> I have no idea - that's more a question for you to answer yourself.
> 

I make a draft patch for VM exit caused by Intel PT output (comments as annotation). It looks have little thing to deal with.
There have four case which may cause VM-exit by PT output (spec 5.2.2.1).
1. EPT violations. This because there don't have EPT mapping from GPA to HPA, I think this can be handled as usual. About MMIO, I think maybe we can make guest OS crash.
2. EPT misconfigurations. We may misconfigure EPT table for log the dirty page and MMIO access (Please correct me if have something wrong). I can't find there need to have some special need to be handled.
3. PML log-full. PT output may cause vm-exit for PML  log-full when trace record to a new page. But it looks don't need additional handle as well.
4. APIC access. Currently, I have no idea about how this relate with PT buffer write. When PT buffer is full, an PMI interrupt would be injected to this VM, but still have no direct relationship.

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c23983c..fbf272e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1873,6 +1873,18 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
          (npfec.write_access &&
           (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
     {
+        /*
+         * Don't need emulate and make guest crash when write to
+         * mmio address or a ram address without write permission.
+         * In fact, output buffer can set to be MMIO address (35.2.6.1),
+         * it need the support of a hardware PCI card which use for
+         * collect trace information. I am afraid it initialized to
+         * a valid general MMIO address which is used by a pass through
+         * device.
+         */
+        if ( npfec.pt_output )
+            goto out_put_gfn;
+
         if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         rc = 1;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 021cf33..ba4f979 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3249,6 +3249,7 @@ static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
         .write_access = q.write,
         .insn_fetch = q.fetch,
         .present = q.eff_read || q.eff_write || q.eff_exec,
+        .pt_output = q.pt_output,
     };

     if ( tb_init_done )
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 89619e4..70b6c5f 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -620,11 +620,13 @@ void vmx_pi_hooks_deassign(struct domain *d);
 typedef union ept_qual {
     unsigned long raw;
     struct {
-        bool read:1, write:1, fetch:1,
+        unsigned long read:1, write:1, fetch:1,
             eff_read:1, eff_write:1, eff_exec:1, /* eff_user_exec */:1,
             gla_valid:1,
-            gla_fault:1; /* Valid iff gla_valid. */
-        unsigned long /* pad */:55;
+            gla_fault:1, /* Valid iff gla_valid. */
+            :7,
+            pt_output:1; /* VM exit caused by Intel PT output */
+        unsigned long /* pad */:47;
     };
 } __transparent__ ept_qual_t;

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index e928551..221ffc3 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -228,6 +228,7 @@ struct npfec {
     unsigned int present:1;
     unsigned int gla_valid:1;
     unsigned int kind:2;  /* npfec_kind_t */
+    unsigned int ipt_output:1;
 };

 /* memflags: */






> Jan
> 


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

[PATCH RESEND v1 7/7] x86: Disable Intel Processor Trace when VMXON in L1 guest

[Luwei Kang]

Disable intel Processor Trace VMX operation(IA32_VMX_MISC[bit 14] is 0)
in L1 guest. As mentioned in SDM, on these type of processors, execution
of the VMXON instruction will  clears IA32_RTIT_CTL.TraceEn and any
attempt to write IA32_RTIT_CTL causes a general-protection xception (#GP).

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 885eab3..86ccfda 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1516,6 +1516,9 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     v->arch.hvm_vmx.launched = 0;
     vmsucceed(regs);
 
+    if ( v->arch.hvm_vmx.pt_desc.intel_pt_enabled )
+        pt_set_rtit_ctl(&v->arch.hvm_vmx.pt_desc, 0);
+
     return X86EMUL_OKAY;
 }
 
@@ -2140,8 +2143,8 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         data = hvm_cr4_guest_valid_bits(v, 0);
         break;
     case MSR_IA32_VMX_MISC:
-        /* Do not support CR3-target feature now */
-        data = host_data & ~VMX_MISC_CR3_TARGET;
+        /* Do not support CR3-target and PT VMX feature now */
+        data = host_data & ~(VMX_MISC_CR3_TARGET | VMX_MISC_PT_ENABLE);
         break;
     case MSR_IA32_VMX_EPT_VPID_CAP:
         data = nept_get_ept_vpid_cap();
-- 
1.8.3.1


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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Jan Beulich]

>>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> Luwei Kang (7):
>   x86: add a flag to enable Intel processor trace
>   x86: configure vmcs for Intel processor trace virtualization
>   x86: add intel proecessor trace support for cpuid
>   x86: add intel processor trace context
>   x86: Implement Intel Processor Trace context switch
>   x86: Implement Intel Processor Trace MSRs read/write
>   x86: Disable Intel Processor Trace when VMXON in L1 guest

How can this be a re-send of v1 when the original v1 consisted of
just 6 patches?

Jan


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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> >>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> > Luwei Kang (7):
> >   x86: add a flag to enable Intel processor trace
> >   x86: configure vmcs for Intel processor trace virtualization
> >   x86: add intel proecessor trace support for cpuid
> >   x86: add intel processor trace context
> >   x86: Implement Intel Processor Trace context switch
> >   x86: Implement Intel Processor Trace MSRs read/write
> >   x86: Disable Intel Processor Trace when VMXON in L1 guest
> 
> How can this be a re-send of v1 when the original v1 consisted of just 6 patches?
>

Yes, I make some change in Intel PT context switch(only save /load guest state when Intel PT is enabled in guest), Intel PT MSRs pass through strategy (only passthrough MSRs when PT is enabled in guest)  and  add a patch (patch 7) to disable Intel PT-VMX in nested.
Because of there just have some high level comments from community and above all changes is from my point of view. So I re-send this patch set still as v1.

Thanks,
Luwei Kang

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Jan Beulich]

>>> On 16.01.18 at 10:02, <luwei.kang@intel.com> wrote:
>> >>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
>> > Luwei Kang (7):
>> >   x86: add a flag to enable Intel processor trace
>> >   x86: configure vmcs for Intel processor trace virtualization
>> >   x86: add intel proecessor trace support for cpuid
>> >   x86: add intel processor trace context
>> >   x86: Implement Intel Processor Trace context switch
>> >   x86: Implement Intel Processor Trace MSRs read/write
>> >   x86: Disable Intel Processor Trace when VMXON in L1 guest
>> 
>> How can this be a re-send of v1 when the original v1 consisted of just 6 patches?
>>
> 
> Yes, I make some change in Intel PT context switch(only save /load guest 
> state when Intel PT is enabled in guest), Intel PT MSRs pass through strategy 
> (only passthrough MSRs when PT is enabled in guest)  and  add a patch (patch 
> 7) to disable Intel PT-VMX in nested.
> Because of there just have some high level comments from community and above 
> all changes is from my point of view. So I re-send this patch set still as v1.

Any change makes it a new version. If you don't want to use v2,
use v1.5 or v1.1 in such a case.

Jan


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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> >>> On 16.01.18 at 10:02, <luwei.kang@intel.com> wrote:
> >> >>> On 15.01.18 at 19:12, <luwei.kang@intel.com> wrote:
> >> > Luwei Kang (7):
> >> >   x86: add a flag to enable Intel processor trace
> >> >   x86: configure vmcs for Intel processor trace virtualization
> >> >   x86: add intel proecessor trace support for cpuid
> >> >   x86: add intel processor trace context
> >> >   x86: Implement Intel Processor Trace context switch
> >> >   x86: Implement Intel Processor Trace MSRs read/write
> >> >   x86: Disable Intel Processor Trace when VMXON in L1 guest
> >>
> >> How can this be a re-send of v1 when the original v1 consisted of just 6 patches?
> >>
> >
> > Yes, I make some change in Intel PT context switch(only save /load
> > guest state when Intel PT is enabled in guest), Intel PT MSRs pass
> > through strategy (only passthrough MSRs when PT is enabled in guest)
> > and  add a patch (patch
> > 7) to disable Intel PT-VMX in nested.
> > Because of there just have some high level comments from community and
> > above all changes is from my point of view. So I re-send this patch set still as v1.
> 
> Any change makes it a new version. If you don't want to use v2, use v1.5 or v1.1 in such a case.
> 

Get it.

Thanks,
Luwei Kang


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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Wei Liu]

On Tue, Jan 16, 2018 at 02:12:26AM +0800, Luwei Kang wrote:
> Hi All,
> 
> Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf
> In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> 
> Introduction:
> Intel Processor Trace (Intel PT) is an extension of Intel Architecture that captures information about software execution using dedicated hardware facilities that cause only minimal performance perturbation to the software being traced. Details on the Intel PT infrastructure and trace capabilities can be found in the Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> 
> The suite of architecture changes serve to simplify the process of virtualizing Intel PT for use by a guest software. There are two primary elements to this new architecture support for VMX support improvements made for Intel PT.
> 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
>   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> 2. Enabling use of EPT to redirect PT output.
>   — This enables the VMM to elect to virtualize the PT output buffer using EPT. In this mode, the CPU will treat PT output addresses as Guest Physical Addresses (GPAs) and translate them using EPT. This means that Intel PT output reads (of the ToPA table) and writes (of trace output) can cause EPT violations, and other output events.
> 

A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
says:

"If a VMM emulates an element of processor state by taking a VM
exit on reads and/or writes to that piece of state, and the state
element impacts Intel PT packet generation or values, it may be
incumbent upon the VMM to insert or modify the output trace data."

The immediately follows that paragraph is an example of CR3 causing
vmexit which leads to missing packet. IIRC Xen does that, however the
code as is doesn't seem to handle that at all.

Another thing is Xen's vmevent allows intercepting several other traced
states. It seems that a more generic framework is needed to make PT work
with vmevent subsystem? What is your thought on that?

Wei.

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> > Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> > https://software.intel.com/sites/default/files/managed/c5/15/architect
> > ure-instruction-set-extensions-programming-reference.pdf
> > In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> >
> > Introduction:
> > Intel Processor Trace (Intel PT) is an extension of Intel Architecture that captures information about software execution using
> dedicated hardware facilities that cause only minimal performance perturbation to the software being traced. Details on the Intel PT
> infrastructure and trace capabilities can be found in the Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> >
> > The suite of architecture changes serve to simplify the process of virtualizing Intel PT for use by a guest software. There are two
> primary elements to this new architecture support for VMX support improvements made for Intel PT.
> > 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
> >   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> > 2. Enabling use of EPT to redirect PT output.
> >   — This enables the VMM to elect to virtualize the PT output buffer using EPT. In this mode, the CPU will treat PT output
> addresses as Guest Physical Addresses (GPAs) and translate them using EPT. This means that Intel PT output reads (of the ToPA
> table) and writes (of trace output) can cause EPT violations, and other output events.
> >
> 
> A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
> says:
> 
> "If a VMM emulates an element of processor state by taking a VM exit on reads and/or writes to that piece of state, and the state
> element impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output trace
> data."
> 
> The immediately follows that paragraph is an example of CR3 causing vmexit which leads to missing packet. IIRC Xen does that,
> however the code as is doesn't seem to handle that at all.

Yes, I need add some code on this. I propose if this can be handled by hardware but...

> 
> Another thing is Xen's vmevent allows intercepting several other traced states. It seems that a more generic framework is needed to
> make PT work with vmevent subsystem? What is your thought on that?

Hi Wei,
    I am not fully clear what is the "vmevent subsystem" and what is your mean of " several other traced states ". 
    I guess vmevent is use VPMU collect performance counter? and save/load vpmu MSRs when it's scheduled?

Thanks,
Luwei Kang

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Razvan Cojocaru]

On 05/03/2018 07:06 AM, Kang, Luwei wrote:
>> Another thing is Xen's vmevent allows intercepting several other traced states. It seems that a more generic framework is needed to
>> make PT work with vmevent subsystem? What is your thought on that?
> 
> Hi Wei,
>      I am not fully clear what is the "vmevent subsystem" and what is your mean of " several other traced states ".
>      I guess vmevent is use VPMU collect performance counter? and save/load vpmu MSRs when it's scheduled?

No, vm_event is the part of Xen that is responsible for telling 
userspace applications when things they subscribe to happen, among which 
are control register (CR) and MSR writes, EPT violations, breakpoints, 
and so on.

Please see tools/tests/xen-access/xen-access.c for a quick introduction.


Thanks,
Razvan

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Wei Liu]

On Thu, May 03, 2018 at 04:06:57AM +0000, Kang, Luwei wrote:
> > > Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> > > https://software.intel.com/sites/default/files/managed/c5/15/architect
> > > ure-instruction-set-extensions-programming-reference.pdf
> > > In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> > >
> > > Introduction:
> > > Intel Processor Trace (Intel PT) is an extension of Intel Architecture that captures information about software execution using
> > dedicated hardware facilities that cause only minimal performance perturbation to the software being traced. Details on the Intel PT
> > infrastructure and trace capabilities can be found in the Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> > >
> > > The suite of architecture changes serve to simplify the process of virtualizing Intel PT for use by a guest software. There are two
> > primary elements to this new architecture support for VMX support improvements made for Intel PT.
> > > 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
> > >   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> > > 2. Enabling use of EPT to redirect PT output.
> > >   — This enables the VMM to elect to virtualize the PT output buffer using EPT. In this mode, the CPU will treat PT output
> > addresses as Guest Physical Addresses (GPAs) and translate them using EPT. This means that Intel PT output reads (of the ToPA
> > table) and writes (of trace output) can cause EPT violations, and other output events.
> > >
> > 
> > A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
> > says:
> > 
> > "If a VMM emulates an element of processor state by taking a VM exit on reads and/or writes to that piece of state, and the state
> > element impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output trace
> > data."
> > 
> > The immediately follows that paragraph is an example of CR3 causing vmexit which leads to missing packet. IIRC Xen does that,
> > however the code as is doesn't seem to handle that at all.
> 
> Yes, I need add some code on this. I propose if this can be handled by hardware but...
> 
> > 
> > Another thing is Xen's vmevent allows intercepting several other traced states. It seems that a more generic framework is needed to
> > make PT work with vmevent subsystem? What is your thought on that?
> 
> Hi Wei,
>     I am not fully clear what is the "vmevent subsystem" and what is your mean of " several other traced states ". 
>     I guess vmevent is use VPMU collect performance counter? and save/load vpmu MSRs when it's scheduled?

See Razvan's reply.

I suppose your first step would be to make Xen able to insert new
records to guest's trace buffer. The end result would be a set of
functions to do that. We would need that even without consideration of
vmevent because Xen can choose to intercept any of the traced state as
it evolves.

Then we can see about how to hook that up to vmevent subsystem -- at
this point I think it will become a specialised case of what Xen already
does.

Wei.

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> > > > Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> > > > https://software.intel.com/sites/default/files/managed/c5/15/archi
> > > > tect ure-instruction-set-extensions-programming-reference.pdf
> > > > In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> > > >
> > > > Introduction:
> > > > Intel Processor Trace (Intel PT) is an extension of Intel
> > > > Architecture that captures information about software execution
> > > > using
> > > dedicated hardware facilities that cause only minimal performance
> > > perturbation to the software being traced. Details on the Intel PT infrastructure and trace capabilities can be found in the Intel
> 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> > > >
> > > > The suite of architecture changes serve to simplify the process of
> > > > virtualizing Intel PT for use by a guest software. There are two
> > > primary elements to this new architecture support for VMX support improvements made for Intel PT.
> > > > 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
> > > >   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> > > > 2. Enabling use of EPT to redirect PT output.
> > > >   — This enables the VMM to elect to virtualize the PT output
> > > > buffer using EPT. In this mode, the CPU will treat PT output
> > > addresses as Guest Physical Addresses (GPAs) and translate them
> > > using EPT. This means that Intel PT output reads (of the ToPA
> > > table) and writes (of trace output) can cause EPT violations, and other output events.
> > > >
> > >
> > > A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
> > > says:
> > >
> > > "If a VMM emulates an element of processor state by taking a VM exit
> > > on reads and/or writes to that piece of state, and the state element
> > > impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output trace data."
> > >
> > > The immediately follows that paragraph is an example of CR3 causing
> > > vmexit which leads to missing packet. IIRC Xen does that, however the code as is doesn't seem to handle that at all.
> >
> > Yes, I need add some code on this. I propose if this can be handled by hardware but...
> >
> > >
> > > Another thing is Xen's vmevent allows intercepting several other
> > > traced states. It seems that a more generic framework is needed to make PT work with vmevent subsystem? What is your
> thought on that?
> >
> > Hi Wei,
> >     I am not fully clear what is the "vmevent subsystem" and what is your mean of " several other traced states ".
> >     I guess vmevent is use VPMU collect performance counter? and save/load vpmu MSRs when it's scheduled?
> 
> See Razvan's reply.
> 
> I suppose your first step would be to make Xen able to insert new records to guest's trace buffer. The end result would be a set of
> functions to do that. We would need that even without consideration of vmevent because Xen can choose to intercept any of the
> traced state as it evolves.
> 
> Then we can see about how to hook that up to vmevent subsystem -- at this point I think it will become a specialised case of what
> Xen already does.
> 

I briefly go through the source code of vm_event and find some function like hvm_monitor_msr(), hvm_monitor_cpuid(), hvm_monitor_cr(). I guess cpuid and msr event can work with current source code and don't need do anything.
The only things I can think of is as your mentioned " insert new records to guest's trace buffer ". But vmm may can't catch this event because it write by hardware. Or we trap the buffer write operation by making the trace buffer write protect or misconfiguration? 

Thanks,
Luwei Kang

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> > Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> > https://software.intel.com/sites/default/files/managed/c5/15/architect
> > ure-instruction-set-extensions-programming-reference.pdf
> > In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> >
> > Introduction:
> > Intel Processor Trace (Intel PT) is an extension of Intel Architecture that captures information about software execution using
> dedicated hardware facilities that cause only minimal performance perturbation to the software being traced. Details on the Intel PT
> infrastructure and trace capabilities can be found in the Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> >
> > The suite of architecture changes serve to simplify the process of virtualizing Intel PT for use by a guest software. There are two
> primary elements to this new architecture support for VMX support improvements made for Intel PT.
> > 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
> >   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> > 2. Enabling use of EPT to redirect PT output.
> >   — This enables the VMM to elect to virtualize the PT output buffer using EPT. In this mode, the CPU will treat PT output
> addresses as Guest Physical Addresses (GPAs) and translate them using EPT. This means that Intel PT output reads (of the ToPA
> table) and writes (of trace output) can cause EPT violations, and other output events.
> >
> 
> A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
> says:
> 
> "If a VMM emulates an element of processor state by taking a VM exit on reads and/or writes to that piece of state, and the state
> element impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output trace
> data."
> 
> The immediately follows that paragraph is an example of CR3 causing vmexit which leads to missing packet. IIRC Xen does that,
> however the code as is doesn't seem to handle that at all.

Hi Wei,
    Intel PT can be exposed to guest only when EPT is enabled. In that case, CPU_BASED_CR3_LOAD_EXITING and CPU_BASED_CR3_STORE_EXITING would be clear, so "MOV CR3 " will not cause a vm-exit. It looks like don't need emulate the missing PIP by writing it into the guest output buffer.

Thanks,
Luwei Kang

> 
> Another thing is Xen's vmevent allows intercepting several other traced states. It seems that a more generic framework is needed to
> make PT work with vmevent subsystem? What is your thought on that?
> 
> Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Andrew Cooper]

On 03/05/18 10:49, Kang, Luwei wrote:
>>> Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
>>> https://software.intel.com/sites/default/files/managed/c5/15/architect
>>> ure-instruction-set-extensions-programming-reference.pdf
>>> In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
>>>
>>> Introduction:
>>> Intel Processor Trace (Intel PT) is an extension of Intel Architecture that captures information about software execution using
>> dedicated hardware facilities that cause only minimal performance perturbation to the software being traced. Details on the Intel PT
>> infrastructure and trace capabilities can be found in the Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.
>>> The suite of architecture changes serve to simplify the process of virtualizing Intel PT for use by a guest software. There are two
>> primary elements to this new architecture support for VMX support improvements made for Intel PT.
>>> 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
>>>   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
>>> 2. Enabling use of EPT to redirect PT output.
>>>   — This enables the VMM to elect to virtualize the PT output buffer using EPT. In this mode, the CPU will treat PT output
>> addresses as Guest Physical Addresses (GPAs) and translate them using EPT. This means that Intel PT output reads (of the ToPA
>> table) and writes (of trace output) can cause EPT violations, and other output events.
>> A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
>> says:
>>
>> "If a VMM emulates an element of processor state by taking a VM exit on reads and/or writes to that piece of state, and the state
>> element impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output trace
>> data."
>>
>> The immediately follows that paragraph is an example of CR3 causing vmexit which leads to missing packet. IIRC Xen does that,
>> however the code as is doesn't seem to handle that at all.
> Hi Wei,
>     Intel PT can be exposed to guest only when EPT is enabled. In that case, CPU_BASED_CR3_LOAD_EXITING and CPU_BASED_CR3_STORE_EXITING would be clear, so "MOV CR3 " will not cause a vm-exit. It looks like don't need emulate the missing PIP by writing it into the guest output buffer.

With introspection, the guest mov to cr3 instruction might be on a page
protected with NX at the EPT level, at which point it traps for
inspection and will be completed with emulation, to avoid the overhead
of changing EPT permissions, singlestepping the guest, then reinstating
the NX protection.

Basically, any and all actions could end up requiring emulation, based
on the safety decisions of the introspection logic.

~Andrew

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> >>> Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> >>> https://software.intel.com/sites/default/files/managed/c5/15/archite
> >>> ct ure-instruction-set-extensions-programming-reference.pdf
> >>> In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> >>>
> >>> Introduction:
> >>> Intel Processor Trace (Intel PT) is an extension of Intel
> >>> Architecture that captures information about software execution
> >>> using
> >> dedicated hardware facilities that cause only minimal performance
> >> perturbation to the software being traced. Details on the Intel PT infrastructure and trace capabilities can be found in the Intel 64
> and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> >>> The suite of architecture changes serve to simplify the process of
> >>> virtualizing Intel PT for use by a guest software. There are two
> >> primary elements to this new architecture support for VMX support improvements made for Intel PT.
> >>> 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
> >>>   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> >>> 2. Enabling use of EPT to redirect PT output.
> >>>   — This enables the VMM to elect to virtualize the PT output buffer
> >>> using EPT. In this mode, the CPU will treat PT output
> >> addresses as Guest Physical Addresses (GPAs) and translate them using
> >> EPT. This means that Intel PT output reads (of the ToPA
> >> table) and writes (of trace output) can cause EPT violations, and other output events.
> >> A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
> >> says:
> >>
> >> "If a VMM emulates an element of processor state by taking a VM exit
> >> on reads and/or writes to that piece of state, and the state element
> >> impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output trace data."
> >>
> >> The immediately follows that paragraph is an example of CR3 causing
> >> vmexit which leads to missing packet. IIRC Xen does that, however the code as is doesn't seem to handle that at all.
> > Hi Wei,
> >     Intel PT can be exposed to guest only when EPT is enabled. In that case, CPU_BASED_CR3_LOAD_EXITING and
> CPU_BASED_CR3_STORE_EXITING would be clear, so "MOV CR3 " will not cause a vm-exit. It looks like don't need emulate the
> missing PIP by writing it into the guest output buffer.
> 
> With introspection, the guest mov to cr3 instruction might be on a page protected with NX at the EPT level, at which point it traps
> for inspection and will be completed with emulation, to avoid the overhead of changing EPT permissions, singlestepping the guest,
> then reinstating the NX protection.
> 
> Basically, any and all actions could end up requiring emulation, based on the safety decisions of the introspection logic.

Thanks for your clarification.  I will emulate the missing PIP packet in vmx_vmexit_handler() -> "case EXIT_REASON_CR_ACCESS"

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> >>> Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> >>> https://software.intel.com/sites/default/files/managed/c5/15/archite
> >>> ct ure-instruction-set-extensions-programming-reference.pdf
> >>> In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> >>>
> >>> Introduction:
> >>> Intel Processor Trace (Intel PT) is an extension of Intel
> >>> Architecture that captures information about software execution
> >>> using
> >> dedicated hardware facilities that cause only minimal performance
> >> perturbation to the software being traced. Details on the Intel PT infrastructure and trace capabilities can be found in the Intel 64
> and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> >>> The suite of architecture changes serve to simplify the process of
> >>> virtualizing Intel PT for use by a guest software. There are two
> >> primary elements to this new architecture support for VMX support improvements made for Intel PT.
> >>> 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
> >>>   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> >>> 2. Enabling use of EPT to redirect PT output.
> >>>   — This enables the VMM to elect to virtualize the PT output buffer
> >>> using EPT. In this mode, the CPU will treat PT output
> >> addresses as Guest Physical Addresses (GPAs) and translate them using
> >> EPT. This means that Intel PT output reads (of the ToPA
> >> table) and writes (of trace output) can cause EPT violations, and other output events.
> >> A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
> >> says:
> >>
> >> "If a VMM emulates an element of processor state by taking a VM exit
> >> on reads and/or writes to that piece of state, and the state element
> >> impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output trace data."
> >>
> >> The immediately follows that paragraph is an example of CR3 causing
> >> vmexit which leads to missing packet. IIRC Xen does that, however the code as is doesn't seem to handle that at all.
> > Hi Wei,
> >     Intel PT can be exposed to guest only when EPT is enabled. In that case, CPU_BASED_CR3_LOAD_EXITING and
> CPU_BASED_CR3_STORE_EXITING would be clear, so "MOV CR3 " will not cause a vm-exit. It looks like don't need emulate the
> missing PIP by writing it into the guest output buffer.
> 
> With introspection, the guest mov to cr3 instruction might be on a page protected with NX at the EPT level, at which point it traps
> for inspection and will be completed with emulation, to avoid the overhead of changing EPT permissions, singlestepping the guest,
> then reinstating the NX protection.
> 
> Basically, any and all actions could end up requiring emulation, based on the safety decisions of the introspection logic.

Hi Andrew, 
     As you mentioned in previous mail and emphasized in community call. Any instruction might be on a page protected with NX at the EPT level. So it looks like that almost all the Trace packet need to be emulated. For example, TNT(taken/not-taken) might be emulate for branch instruction, TIP(target IP) might be emulate for branch, interrupt, exception and so on. Is that right?

Thanks,
Luwei Kang

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Andrew Cooper]

On 10/05/18 10:26, Kang, Luwei wrote:
>>>>> Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
>>>>> https://software.intel.com/sites/default/files/managed/c5/15/archite
>>>>> ct ure-instruction-set-extensions-programming-reference.pdf
>>>>> In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
>>>>>
>>>>> Introduction:
>>>>> Intel Processor Trace (Intel PT) is an extension of Intel
>>>>> Architecture that captures information about software execution
>>>>> using
>>>> dedicated hardware facilities that cause only minimal performance
>>>> perturbation to the software being traced. Details on the Intel PT infrastructure and trace capabilities can be found in the Intel 64
>> and IA-32 Architectures Software Developer’s Manual, Volume 3C.
>>>>> The suite of architecture changes serve to simplify the process of
>>>>> virtualizing Intel PT for use by a guest software. There are two
>>>> primary elements to this new architecture support for VMX support improvements made for Intel PT.
>>>>> 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
>>>>>   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
>>>>> 2. Enabling use of EPT to redirect PT output.
>>>>>   — This enables the VMM to elect to virtualize the PT output buffer
>>>>> using EPT. In this mode, the CPU will treat PT output
>>>> addresses as Guest Physical Addresses (GPAs) and translate them using
>>>> EPT. This means that Intel PT output reads (of the ToPA
>>>> table) and writes (of trace output) can cause EPT violations, and other output events.
>>>> A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
>>>> says:
>>>>
>>>> "If a VMM emulates an element of processor state by taking a VM exit
>>>> on reads and/or writes to that piece of state, and the state element
>>>> impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output trace data."
>>>>
>>>> The immediately follows that paragraph is an example of CR3 causing
>>>> vmexit which leads to missing packet. IIRC Xen does that, however the code as is doesn't seem to handle that at all.
>>> Hi Wei,
>>>     Intel PT can be exposed to guest only when EPT is enabled. In that case, CPU_BASED_CR3_LOAD_EXITING and
>> CPU_BASED_CR3_STORE_EXITING would be clear, so "MOV CR3 " will not cause a vm-exit. It looks like don't need emulate the
>> missing PIP by writing it into the guest output buffer.
>>
>> With introspection, the guest mov to cr3 instruction might be on a page protected with NX at the EPT level, at which point it traps
>> for inspection and will be completed with emulation, to avoid the overhead of changing EPT permissions, singlestepping the guest,
>> then reinstating the NX protection.
>>
>> Basically, any and all actions could end up requiring emulation, based on the safety decisions of the introspection logic.
> Hi Andrew, 
>      As you mentioned in previous mail and emphasized in community call. Any instruction might be on a page protected with NX at the EPT level. So it looks like that almost all the Trace packet need to be emulated. For example, TNT(taken/not-taken) might be emulate for branch instruction, TIP(target IP) might be emulate for branch, interrupt, exception and so on. Is that right?

Yes.  Then again, this information is readily available from the
emulator.  What we probably need (although I've not put much thought
into this) is to accumulate a list of trace events during emulation,
then insert them into the trace log only when we retire the instruction.

~Andrew

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> >>>>> Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> >>>>> https://software.intel.com/sites/default/files/managed/c5/15/archi
> >>>>> te ct ure-instruction-set-extensions-programming-reference.pdf
> >>>>> In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> >>>>>
> >>>>> Introduction:
> >>>>> Intel Processor Trace (Intel PT) is an extension of Intel
> >>>>> Architecture that captures information about software execution
> >>>>> using
> >>>> dedicated hardware facilities that cause only minimal performance
> >>>> perturbation to the software being traced. Details on the Intel PT
> >>>> infrastructure and trace capabilities can be found in the Intel 64
> >> and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> >>>>> The suite of architecture changes serve to simplify the process of
> >>>>> virtualizing Intel PT for use by a guest software. There are two
> >>>> primary elements to this new architecture support for VMX support improvements made for Intel PT.
> >>>>> 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
> >>>>>   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> >>>>> 2. Enabling use of EPT to redirect PT output.
> >>>>>   — This enables the VMM to elect to virtualize the PT output
> >>>>> buffer using EPT. In this mode, the CPU will treat PT output
> >>>> addresses as Guest Physical Addresses (GPAs) and translate them
> >>>> using EPT. This means that Intel PT output reads (of the ToPA
> >>>> table) and writes (of trace output) can cause EPT violations, and other output events.
> >>>> A high level question, SDM vol 3 "Emulation of Intel PT Traced State"
> >>>> says:
> >>>>
> >>>> "If a VMM emulates an element of processor state by taking a VM
> >>>> exit on reads and/or writes to that piece of state, and the state
> >>>> element impacts Intel PT packet generation or values, it may be incumbent upon the VMM to insert or modify the output
> trace data."
> >>>>
> >>>> The immediately follows that paragraph is an example of CR3 causing
> >>>> vmexit which leads to missing packet. IIRC Xen does that, however the code as is doesn't seem to handle that at all.
> >>> Hi Wei,
> >>>     Intel PT can be exposed to guest only when EPT is enabled. In
> >>> that case, CPU_BASED_CR3_LOAD_EXITING and
> >> CPU_BASED_CR3_STORE_EXITING would be clear, so "MOV CR3 " will not
> >> cause a vm-exit. It looks like don't need emulate the missing PIP by writing it into the guest output buffer.
> >>
> >> With introspection, the guest mov to cr3 instruction might be on a
> >> page protected with NX at the EPT level, at which point it traps for
> >> inspection and will be completed with emulation, to avoid the overhead of changing EPT permissions, singlestepping the guest,
> then reinstating the NX protection.
> >>
> >> Basically, any and all actions could end up requiring emulation, based on the safety decisions of the introspection logic.
> > Hi Andrew,
> >      As you mentioned in previous mail and emphasized in community call. Any instruction might be on a page protected with NX
> at the EPT level. So it looks like that almost all the Trace packet need to be emulated. For example, TNT(taken/not-taken) might be
> emulate for branch instruction, TIP(target IP) might be emulate for branch, interrupt, exception and so on. Is that right?
> 
> Yes.  Then again, this information is readily available from the emulator.  What we probably need (although I've not put much
> thought into this) is to accumulate a list of trace events during emulation, then insert them into the trace log only when we retire
> the instruction.

Hi,
    I think this may take some time to add a function list to emulate (build) all type of packets. Furthermore, we need to consider all the condition of one packet generate scenario and get the information which needed by these packets. It looks like we need to emulate all the behaviors of what hardware to do.
    Can we move this task (about introspection) to next stage? Or mask off Intel PT when use introspection?

Thanks,
Luwei Kang

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Konrad Rzeszutek Wilk]

On Tue, Jan 16, 2018 at 02:12:26AM +0800, Luwei Kang wrote:
> Hi All,
> 
> Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf
> In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.

Now Chapter 4.
> 
> Introduction:
> Intel Processor Trace (Intel PT) is an extension of Intel Architecture that captures information about software execution using dedicated hardware facilities that cause only minimal performance perturbation to the software being traced. Details on the Intel PT infrastructure and trace capabilities can be found in the Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> 
> The suite of architecture changes serve to simplify the process of virtualizing Intel PT for use by a guest software. There are two primary elements to this new architecture support for VMX support improvements made for Intel PT.
> 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
>   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> 2. Enabling use of EPT to redirect PT output.
>   — This enables the VMM to elect to virtualize the PT output buffer using EPT. In this mode, the CPU will treat PT output addresses as Guest Physical Addresses (GPAs) and translate them using EPT. This means that Intel PT output reads (of the ToPA table) and writes (of trace output) can cause EPT violations, and other output events.
> 

How does one test this functionality in Linux? As in does 'perf' take advantage of the Processor Trace
functionality? 

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

Re: [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Kang, Luwei]

> On Tue, Jan 16, 2018 at 02:12:26AM +0800, Luwei Kang wrote:
> > Hi All,
> >
> > Here is a patch-series which adding Processor Trace enabling in XEN guest. You can get It's software developer manuals from:
> > https://software.intel.com/sites/default/files/managed/c5/15/architect
> > ure-instruction-set-extensions-programming-reference.pdf
> > In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.
> 
> Now Chapter 4.

Yes, will update this description. Thanks.

> >
> > Introduction:
> > Intel Processor Trace (Intel PT) is an extension of Intel Architecture that captures information about software execution using
> dedicated hardware facilities that cause only minimal performance perturbation to the software being traced. Details on the Intel PT
> infrastructure and trace capabilities can be found in the Intel 64 and IA-32 Architectures Software Developer’s Manual, Volume 3C.
> >
> > The suite of architecture changes serve to simplify the process of virtualizing Intel PT for use by a guest software. There are two
> primary elements to this new architecture support for VMX support improvements made for Intel PT.
> > 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
> >   — This serves to speed and simplify the process of disabling trace on VM exit, and restoring it on VM entry.
> > 2. Enabling use of EPT to redirect PT output.
> >   — This enables the VMM to elect to virtualize the PT output buffer using EPT. In this mode, the CPU will treat PT output
> addresses as Guest Physical Addresses (GPAs) and translate them using EPT. This means that Intel PT output reads (of the ToPA
> table) and writes (of trace output) can cause EPT violations, and other output events.
> >
> 
> How does one test this functionality in Linux? As in does 'perf' take advantage of the Processor Trace functionality?

We can test this feature by "perf" tools in Linux. 

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