From: "Roger Pau Monné" <roger.pau@citrix.com>
To: "Michał Leszczyński" <michal.leszczynski@cert.pl>
Cc: Julien Grall <julien@xen.org>,
Stefano Stabellini <sstabellini@kernel.org>,
tamas.lengyel@intel.com, Wei Liu <wl@xen.org>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
luwei.kang@intel.com, Jan Beulich <jbeulich@suse.com>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v6 09/11] x86/domctl: add XEN_DOMCTL_vmtrace_op
Date: Wed, 15 Jul 2020 19:32:02 +0200 [thread overview]
Message-ID: <20200715173202.GG7191@Air-de-Roger> (raw)
In-Reply-To: <a9899858dba4a7e22a0256cff734399bff348adb.1594150543.git.michal.leszczynski@cert.pl>
On Tue, Jul 07, 2020 at 09:39:48PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@cert.pl>
>
> Implement domctl to manage the runtime state of
> processor trace feature.
>
> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>
> ---
> xen/arch/x86/domctl.c | 50 +++++++++++++++++++++++++++++++++++++
> xen/include/public/domctl.h | 28 +++++++++++++++++++++
> 2 files changed, 78 insertions(+)
>
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 6f2c69788d..6132499db4 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -322,6 +322,50 @@ void arch_get_domain_info(const struct domain *d,
> info->arch_config.emulation_flags = d->arch.emulation_flags;
> }
>
> +static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
> + XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> + int rc;
> + struct vcpu *v;
> +
> + if ( op->pad1 || op->pad2 )
> + return -EINVAL;
> +
> + if ( !vmtrace_supported )
> + return -EOPNOTSUPP;
> +
> + if ( !is_hvm_domain(d) )
> + return -EOPNOTSUPP;
You can join both if into a single one if they both return
-EOPNOTSUPP.
> +
> + if ( op->vcpu >= d->max_vcpus )
> + return -EINVAL;
You can remove this check and just use the return value of domain_vcpu
instead, if it's NULL the vCPU ID is wrong.
> +
> + v = domain_vcpu(d, op->vcpu);
> + rc = 0;
No need to init rc to 0, as it would be unconditionally initialized in
the switch below due to the default case.
> +
> + switch ( op->cmd )
> + {
> + case XEN_DOMCTL_vmtrace_pt_enable:
> + case XEN_DOMCTL_vmtrace_pt_disable:
> + vcpu_pause(v);
> + rc = vmtrace_control_pt(v, op->cmd == XEN_DOMCTL_vmtrace_pt_enable);
> + vcpu_unpause(v);
> + break;
> +
> + case XEN_DOMCTL_vmtrace_pt_get_offset:
> + rc = vmtrace_get_pt_offset(v, &op->offset, &op->size);
In order to get consistent values here the vCPU should be paused, or
else you just get stale values from the last vmexit?
Maybe that's fine for your use case?
> +
> + if ( !rc && d->is_dying )
> + rc = ENODATA;
This check here seems weird, if this is really required shouldn't it
be done before attempting to fetch the data?
> + break;
> +
> + default:
> + rc = -EOPNOTSUPP;
> + }
> +
> + return rc;
> +}
> +
> #define MAX_IOPORTS 0x10000
>
> long arch_do_domctl(
> @@ -337,6 +381,12 @@ long arch_do_domctl(
> switch ( domctl->cmd )
> {
>
> + case XEN_DOMCTL_vmtrace_op:
> + ret = do_vmtrace_op(d, &domctl->u.vmtrace_op, u_domctl);
> + if ( !ret )
> + copyback = true;
> + break;
Nit: new additions would usually got at the end of the switch.
> +
> case XEN_DOMCTL_shadow_op:
> ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
> if ( ret == -ERESTART )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 7681675a94..73c7ccbd16 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1136,6 +1136,30 @@ struct xen_domctl_vuart_op {
> */
> };
>
> +/* XEN_DOMCTL_vmtrace_op: Perform VM tracing related operation */
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
> +struct xen_domctl_vmtrace_op {
> + /* IN variable */
> + uint32_t cmd;
> +/* Enable/disable external vmtrace for given domain */
> +#define XEN_DOMCTL_vmtrace_pt_enable 1
> +#define XEN_DOMCTL_vmtrace_pt_disable 2
> +#define XEN_DOMCTL_vmtrace_pt_get_offset 3
> + domid_t domain;
> + uint16_t pad1;
> + uint32_t vcpu;
> + uint16_t pad2;
pad2 should be a uint32_t? Or else there's a padding field added by
the compiler? (maybe I'm missing something, I haven't checked with
pahole).
> +
> + /* OUT variable */
> + uint64_aligned_t size;
> + uint64_aligned_t offset;
> +};
> +typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
> +
> +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> +
> struct xen_domctl {
> uint32_t cmd;
> #define XEN_DOMCTL_createdomain 1
> @@ -1217,6 +1241,7 @@ struct xen_domctl {
> #define XEN_DOMCTL_vuart_op 81
> #define XEN_DOMCTL_get_cpu_policy 82
> #define XEN_DOMCTL_set_cpu_policy 83
> +#define XEN_DOMCTL_vmtrace_op 84
> #define XEN_DOMCTL_gdbsx_guestmemio 1000
> #define XEN_DOMCTL_gdbsx_pausevcpu 1001
> #define XEN_DOMCTL_gdbsx_unpausevcpu 1002
> @@ -1277,6 +1302,9 @@ struct xen_domctl {
> struct xen_domctl_monitor_op monitor_op;
> struct xen_domctl_psr_alloc psr_alloc;
> struct xen_domctl_vuart_op vuart_op;
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> + struct xen_domctl_vmtrace_op vmtrace_op;
> +#endif
No need for the preprocessor conditionals here, the whole domctl.h is
only to be used by the tools or Xen, and is already properly guarded
(see the #error at the top of the file).
Thanks.
next prev parent reply other threads:[~2020-07-15 17:32 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-07 19:39 [PATCH v6 00/11] Implement support for external IPT monitoring Michał Leszczyński
2020-07-07 19:39 ` [PATCH v6 01/11] memory: batch processing in acquire_resource() Michał Leszczyński
2020-07-15 9:36 ` Roger Pau Monné
2020-07-15 12:13 ` Jan Beulich
2020-07-16 8:14 ` Roger Pau Monné
2020-07-15 12:28 ` Jan Beulich
2020-07-17 14:16 ` Julien Grall
2020-07-07 19:39 ` [PATCH v6 02/11] x86/vmx: add Intel PT MSR definitions Michał Leszczyński
2020-07-07 19:39 ` [PATCH v6 03/11] x86/vmx: add IPT cpu feature Michał Leszczyński
2020-07-15 10:02 ` Roger Pau Monné
2020-08-07 14:22 ` Jan Beulich
2020-07-07 19:39 ` [PATCH v6 04/11] common: add vmtrace_pt_size domain parameter Michał Leszczyński
2020-07-15 15:08 ` Roger Pau Monné
2020-08-07 14:29 ` Jan Beulich
2020-07-07 19:39 ` [PATCH v6 05/11] tools/libxl: add vmtrace_pt_size parameter Michał Leszczyński
2020-07-15 15:17 ` Roger Pau Monné
2020-07-07 19:39 ` [PATCH v6 06/11] x86/hvm: processor trace interface in HVM Michał Leszczyński
2020-07-15 15:43 ` Roger Pau Monné
2020-08-07 14:37 ` Jan Beulich
2020-07-07 19:39 ` [PATCH v6 07/11] x86/vmx: implement IPT in VMX Michał Leszczyński
2020-07-15 16:04 ` Roger Pau Monné
2020-08-07 15:00 ` Jan Beulich
2020-07-07 19:39 ` [PATCH v6 08/11] x86/mm: add vmtrace_buf resource type Michał Leszczyński
2020-07-15 17:20 ` Roger Pau Monné
2020-07-07 19:39 ` [PATCH v6 09/11] x86/domctl: add XEN_DOMCTL_vmtrace_op Michał Leszczyński
2020-07-15 17:32 ` Roger Pau Monné [this message]
2020-08-27 14:54 ` Jan Beulich
2020-07-07 19:39 ` [PATCH v6 10/11] tools/libxc: add xc_vmtrace_* functions Michał Leszczyński
2020-07-16 8:26 ` Roger Pau Monné
2020-07-07 19:39 ` [PATCH v6 11/11] tools/proctrace: add proctrace tool Michał Leszczyński
2020-07-16 8:42 ` Roger Pau Monné
2020-07-14 13:11 ` [PATCH v6 00/11] Implement support for external IPT monitoring Michał Leszczyński
2020-07-14 15:05 ` Roger Pau Monné
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=20200715173202.GG7191@Air-de-Roger \
--to=roger.pau@citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=luwei.kang@intel.com \
--cc=michal.leszczynski@cert.pl \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.