From: Andrew Cooper <andrew.cooper3@citrix.com>
To: "Michał Leszczyński" <michal.leszczynski@cert.pl>,
xen-devel@lists.xenproject.org
Cc: "Kevin Tian" <kevin.tian@intel.com>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Julien Grall" <julien@xen.org>,
"Jun Nakajima" <jun.nakajima@intel.com>, "Wei Liu" <wl@xen.org>,
"Ian Jackson" <ian.jackson@eu.citrix.com>,
"George Dunlap" <george.dunlap@citrix.com>,
"Kang, Luwei" <luwei.kang@intel.com>,
"Jan Beulich" <jbeulich@suse.com>,
"Tamas K Lengyel" <tamas.lengyel@intel.com>,
"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v3 4/7] x86/vmx: add do_vmtrace_op
Date: Tue, 23 Jun 2020 12:54:38 +0100 [thread overview]
Message-ID: <250cd39e-aa51-5397-93f9-b863e4f51269@citrix.com> (raw)
In-Reply-To: <97440747.11443782.1592849498089.JavaMail.zimbra@cert.pl>
On 22/06/2020 19:11, Michał Leszczyński wrote:
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 5bb47583b3..5899df52c3 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -58,6 +58,7 @@
> #include <asm/monitor.h>
> #include <asm/hvm/emulate.h>
> #include <asm/hvm/hvm.h>
> +#include <asm/hvm/vmx/vmx.h>
You cannot include this header file, because...
> #include <asm/hvm/vpt.h>
> #include <asm/hvm/support.h>
> #include <asm/hvm/cacheattr.h>
> @@ -606,6 +607,57 @@ static int hvm_print_line(
> return X86EMUL_OKAY;
> }
>
> +static int vmtrace_alloc_buffers(struct vcpu *v, uint64_t size)
> +{
> + struct page_info *pg;
> + struct pt_state *pt;
> +
> + if ( size < PAGE_SIZE || size > GB(4) || (size & (size - 1)) )
> + {
> + /*
> + * We don't accept trace buffer size smaller than single page
> + * and the upper bound is defined as 4GB in the specification.
> + * The buffer size must be also a power of 2.
> + */
> + return -EINVAL;
> + }
> +
> + if ( vmx_add_host_load_msr(v, MSR_RTIT_CTL, 0) )
> + return -EFAULT;
... this will explode on AMD hardware, as will ...
> +
> + pg = alloc_domheap_pages(v->domain, get_order_from_bytes(size),
> + MEMF_no_refcount);
> +
> + if ( !pg )
> + return -ENOMEM;
> +
> + pt = xzalloc(struct pt_state);
> +
> + if ( !pt )
> + return -ENOMEM;
> +
> + pt->output_base = page_to_maddr(pg);
> + pt->output_mask.raw = size - 1;
> +
> + v->arch.hvm.vmx.pt_state = pt;
... this. Both by reaching into the wrong half of the vmx/svm union.
(Also for the acquire resource in mm.c)
> @@ -5101,6 +5265,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> rc = current->hcall_compat ? compat_altp2m_op(arg) : do_altp2m_op(arg);
> break;
>
> + case HVMOP_vmtrace:
> + rc = do_vmtrace_op(arg);
> + break;
In my feedback on v1, I specifically recommended domctl, because hvmop
is incompatible with a future expansion to PV guests.
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 59bdc28c89..054892befe 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -92,6 +92,7 @@ struct xen_domctl_createdomain {
> uint32_t max_evtchn_port;
> int32_t max_grant_frames;
> int32_t max_maptrack_frames;
> + uint64_t vmtrace_pt_size;
For now, we have very limited space (128 bytes total) for this
structure. This will change in the future with the tools ABI changes,
but uint64_t is total overkill.
Julien/Stefano: For ARM CoreSight, are the trace buffers required to be
a power of two size, and/or is this a reasonable implementation
restriction you'd be willing to live with?
If so, we can get away with a uint8_t vmtrace_order, using 0 for
"nothing", 1 for 8k, 2 for 16k etc. (This does rule out allocating a 4k
buffer, but shifting the number scheme to be order-1 is a no-go
complexity wise, and the only other alternative is an explicit CDF flag
for vmtrace).
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 0a91bfa749..22f6185e01 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -300,6 +300,6 @@
> #define XEN_HVM_MCA_CAP_LMCE (xen_mk_ullong(1) << 0)
> #define XEN_HVM_MCA_CAP_MASK XEN_HVM_MCA_CAP_LMCE
>
> -#define HVM_NR_PARAMS 39
> +#define HVM_NR_PARAMS 40
This hunk is now stale, and can be dropped.
>
> #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index dbd35305df..f823c784c3 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -620,6 +620,7 @@ struct xen_mem_acquire_resource {
>
> #define XENMEM_resource_ioreq_server 0
> #define XENMEM_resource_grant_table 1
> +#define XENMEM_resource_vmtrace_buf 2
>
> /*
> * IN - a type-specific resource identifier, which must be zero
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ac53519d7f..48f0a61bbd 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -457,6 +457,10 @@ struct domain
> unsigned pbuf_idx;
> spinlock_t pbuf_lock;
>
> + /* Used by vmtrace features */
> + spinlock_t vmtrace_lock;
> + uint64_t vmtrace_pt_size;
Overall, the moving parts of this series needs to split out into rather
more patches.
First, in patch 3, the hvm_funcs.pt_supported isn't the place for that
to live. You want a global "bool vmtrace_supported" in common/domain.c
which vmx_init_vmcs_config() sets, and the ARM code can set in the
future when CoreSight is added.
Next, you want a patch in isolation which adds vmtrace_pt_size (or
whatever it ends up being) to createdomain, where all
allocation/deallocation logic lives in common/domain.c. The spinlock
(if its needed, but I don't think it is) wants initialising early in
domain_create(), alongside d->pbuf_lock, and you also need an extra
clause in sanitise_domain_config() which rejects a vmtrace setting if
vmtrace isn't supported. You'll need to put the struct page_info *
pointer to the memory allocation in struct vcpu, and adjust the vcpu
create/destroy logic appropriately.
Next, you want a patch doing the acquire resource logic for userspace to
map the buffers.
Next, you want a patch to introduce a domctl with the various runtime
enable/disable settings which were in an hvmop here.
Next, you want a patch to do the VMX plumbing, both at create, and runtime.
This ought to lay the logic out in a way which is extendable to x86 PV
guests and ARM CoreSight, and oughtn't to explode when creating guests
on non-Intel hardware.
Thanks,
~Andrew
next prev parent reply other threads:[~2020-06-23 11:55 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-22 18:06 [PATCH v3 0/7] Implement support for external IPT monitoring Michał Leszczyński
2020-06-22 18:10 ` [PATCH v3 1/7] memory: batch processing in acquire_resource() Michał Leszczyński
2020-06-22 18:10 ` [PATCH v3 2/7] x86/vmx: add Intel PT MSR definitions Michał Leszczyński
2020-06-22 18:11 ` [PATCH v3 3/7] x86/vmx: add IPT cpu feature Michał Leszczyński
2020-06-22 18:11 ` [PATCH v3 4/7] x86/vmx: add do_vmtrace_op Michał Leszczyński
2020-06-23 11:54 ` Andrew Cooper [this message]
2020-06-29 9:52 ` Michał Leszczyński
2020-06-22 18:12 ` [PATCH v3 5/7] tools/libxc: add xc_vmtrace_* functions Michał Leszczyński
2020-06-26 11:50 ` Wei Liu
2020-06-22 18:12 ` [PATCH v3 6/7] tools/libxl: add vmtrace_pt_size parameter Michał Leszczyński
2020-06-26 11:52 ` Wei Liu
2020-06-22 18:12 ` [PATCH v3 7/7] tools/proctrace: add proctrace tool Michał Leszczyński
2020-06-23 9:35 ` Tamas K Lengyel
2020-06-26 11:48 ` Wei Liu
2020-06-26 13:24 ` Ian Jackson
2020-06-29 15:27 ` Tamas K Lengyel
2020-07-01 11:01 ` Ian Jackson
2020-06-22 18:19 ` [PATCH v3 0/7] Implement support for external IPT monitoring Michał Leszczyński
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=250cd39e-aa51-5397-93f9-b863e4f51269@citrix.com \
--to=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=jun.nakajima@intel.com \
--cc=kevin.tian@intel.com \
--cc=luwei.kang@intel.com \
--cc=michal.leszczynski@cert.pl \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=tamas.lengyel@intel.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).