Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <1540368316-12998-7-git-send-email-luwei.kang@intel.com>
References: <1540368316-12998-1-git-send-email-luwei.kang@intel.com> <1540368316-12998-7-git-send-email-luwei.kang@intel.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Wed, 24 Oct 2018 09:18:12 -0700
Message-ID: <CALMp9eSNr_AedOLv+gcqayFQSpwxQnVdsJQSWG1qixwNUmQXzw@mail.gmail.com>
Subject: Re: [PATCH v13 06/12] KVM: x86: Add Intel PT virtualization work mode
To:     Luwei Kang <luwei.kang@intel.com>
Cc:     kvm list <kvm@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Joerg Roedel <joro@8bytes.org>, songliubraving@fb.com,
        Peter Zijlstra <peterz@infradead.org>,
        alexander.shishkin@linux.intel.com, kstewart@linuxfoundation.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        mattst88@gmail.com,
        Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Marc Orr <marcorr@google.com>, Uros Bizjak <ubizjak@gmail.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Chao Peng <chao.p.peng@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Oct 24, 2018 at 1:05 AM, Luwei Kang <luwei.kang@intel.com> wrote:
> From: Chao Peng <chao.p.peng@linux.intel.com>
>
> Intel Processor Trace virtualization can be work in one
> of 2 possible modes:
>
> a. System-Wide mode (default):
>    When the host configures Intel PT to collect trace packets
>    of the entire system, it can leave the relevant VMX controls
>    clear to allow VMX-specific packets to provide information
>    across VMX transitions.
>    KVM guest will not aware this feature in this mode and both
>    host and KVM guest trace will output to host buffer.
>
> b. Host-Guest mode:
>    Host can configure trace-packet generation while in
>    VMX non-root operation for guests and root operation
>    for native executing normally.
>    Intel PT will be exposed to KVM guest in this mode, and
>    the trace output to respective buffer of host and guest.
>    In this mode, tht status of PT will be saved and disabled
>    before VM-entry and restored after VM-exit if trace
>    a virtual machine.
>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---

> +#define SECONDARY_EXEC_PT_USE_GPA              0x01000000
> +#define VM_EXIT_CLEAR_IA32_RTIT_CTL            0x02000000
> +#define VM_ENTRY_LOAD_IA32_RTIT_CTL            0x00040000

Where are all of these bits documented? I'm looking at the latest SDM,
volume 3 (325384-067US), and none of these bits aredocumented there.

> +       GUEST_IA32_RTIT_CTL             = 0x00002814,
> +       GUEST_IA32_RTIT_CTL_HIGH        = 0x00002815,

Where is this VMCS field documented?

> +/* Default is SYSTEM mode. */
> +static int __read_mostly pt_mode = PT_MODE_SYSTEM;
> +module_param(pt_mode, int, S_IRUGO);

As a module parameter, this doesn't allow much flexibility. Is it
possible to make this decision per-VM, using a VM capability that can
be set by userspace? (In that case, it may make sense to have a module
parameter which allows/disallows the per-VM capability.)


> +static inline bool cpu_has_vmx_intel_pt(void)
> +{
> +       u64 vmx_msr;
> +
> +       rdmsrl(MSR_IA32_VMX_MISC, vmx_msr);
> +       return !!(vmx_msr & MSR_IA32_VMX_MISC_INTEL_PT);
> +}

Instead of the rdmsr here, wouldn't it be better to cache the
IA32_VMX_MISC MSR in vmcs_config?
Nit: throughout this change, the '!!' isn't necessary when casting an
integer type to bool.