* [PATCH v9 01/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
@ 2021-02-01 23:26 ` Andrew Cooper
2021-02-04 21:23 ` Andrew Cooper
2021-02-01 23:26 ` [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter Andrew Cooper
` (10 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:26 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall, Paul Durrant,
Michał Leszczyński, Hubert Jasudowicz, Tamas K Lengyel
A guest's default number of grant frames is 64, and XENMEM_acquire_resource
will reject an attempt to map more than 32 frames. This limit is caused by
the size of mfn_list[] on the stack.
Fix mapping of arbitrary size requests by looping over batches of 32 in
acquire_resource(), and using hypercall continuations when necessary.
To start with, break _acquire_resource() out of acquire_resource() to cope
with type-specific dispatching, and update the return semantics to indicate
the number of mfns returned. Update gnttab_acquire_resource() and x86's
arch_acquire_resource() to match these new semantics.
Have do_memory_op() pass start_extent into acquire_resource() so it can pick
up where it left off after a continuation, and loop over batches of 32 until
all the work is done, or a continuation needs to occur.
compat_memory_op() is a bit more complicated, because it also has to marshal
frame_list in the XLAT buffer. Have it account for continuation information
itself and hide details from the upper layer, so it can marshal the buffer in
chunks if necessary.
With these fixes in place, it is now possible to map the whole grant table for
a guest.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v9:
* Crash domain rather than returning late with -ERANGE/-EFAULT.
v8:
* nat => cmp change in the start_extent check.
* Rebase over 'frame' and ARM/IOREQ series.
v3:
* Spelling fixes
---
xen/common/compat/memory.c | 114 +++++++++++++++++++++++++++++++++--------
xen/common/grant_table.c | 3 ++
xen/common/memory.c | 124 +++++++++++++++++++++++++++++++++------------
3 files changed, 187 insertions(+), 54 deletions(-)
diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index 834c5e19d1..c43fa97cf1 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -402,23 +402,10 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
case XENMEM_acquire_resource:
{
xen_pfn_t *xen_frame_list = NULL;
- unsigned int max_nr_frames;
if ( copy_from_guest(&cmp.mar, compat, 1) )
return -EFAULT;
- /*
- * The number of frames handled is currently limited to a
- * small number by the underlying implementation, so the
- * scratch space should be sufficient for bouncing the
- * frame addresses.
- */
- max_nr_frames = (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) /
- sizeof(*xen_frame_list);
-
- if ( cmp.mar.nr_frames > max_nr_frames )
- return -E2BIG;
-
/* Marshal the frame list in the remainder of the xlat space. */
if ( !compat_handle_is_null(cmp.mar.frame_list) )
xen_frame_list = (xen_pfn_t *)(nat.mar + 1);
@@ -432,6 +419,28 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
if ( xen_frame_list && cmp.mar.nr_frames )
{
+ unsigned int xlat_max_frames =
+ (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) /
+ sizeof(*xen_frame_list);
+
+ if ( start_extent >= cmp.mar.nr_frames )
+ return -EINVAL;
+
+ /*
+ * Adjust nat to account for work done on previous
+ * continuations, leaving cmp pristine. Hide the continaution
+ * from the native code to prevent double accounting.
+ */
+ nat.mar->nr_frames -= start_extent;
+ nat.mar->frame += start_extent;
+ cmd &= MEMOP_CMD_MASK;
+
+ /*
+ * If there are two many frames to fit within the xlat buffer,
+ * we'll need to loop to marshal them all.
+ */
+ nat.mar->nr_frames = min(nat.mar->nr_frames, xlat_max_frames);
+
/*
* frame_list is an input for translated guests, and an output
* for untranslated guests. Only copy in for translated guests.
@@ -444,14 +453,14 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
cmp.mar.nr_frames) ||
__copy_from_compat_offset(
compat_frame_list, cmp.mar.frame_list,
- 0, cmp.mar.nr_frames) )
+ start_extent, nat.mar->nr_frames) )
return -EFAULT;
/*
* Iterate backwards over compat_frame_list[] expanding
* compat_pfn_t to xen_pfn_t in place.
*/
- for ( int x = cmp.mar.nr_frames - 1; x >= 0; --x )
+ for ( int x = nat.mar->nr_frames - 1; x >= 0; --x )
xen_frame_list[x] = compat_frame_list[x];
}
}
@@ -600,9 +609,11 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
case XENMEM_acquire_resource:
{
DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
+ unsigned int done;
if ( compat_handle_is_null(cmp.mar.frame_list) )
{
+ ASSERT(split == 0 && rc == 0);
if ( __copy_field_to_guest(
guest_handle_cast(compat,
compat_mem_acquire_resource_t),
@@ -611,6 +622,21 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
break;
}
+ if ( split < 0 )
+ {
+ /* Continuation occurred. */
+ ASSERT(rc != XENMEM_acquire_resource);
+ done = cmd >> MEMOP_EXTENT_SHIFT;
+ }
+ else
+ {
+ /* No continuation. */
+ ASSERT(rc == 0);
+ done = nat.mar->nr_frames;
+ }
+
+ ASSERT(done <= nat.mar->nr_frames);
+
/*
* frame_list is an input for translated guests, and an output for
* untranslated guests. Only copy out for untranslated guests.
@@ -626,21 +652,67 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
*/
BUILD_BUG_ON(sizeof(compat_pfn_t) > sizeof(xen_pfn_t));
- for ( i = 0; i < cmp.mar.nr_frames; i++ )
+ rc = 0;
+ for ( i = 0; i < done; i++ )
{
compat_pfn_t frame = xen_frame_list[i];
if ( frame != xen_frame_list[i] )
- return -ERANGE;
+ {
+ rc = -ERANGE;
+ break;
+ }
compat_frame_list[i] = frame;
}
- if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
- compat_frame_list,
- cmp.mar.nr_frames) )
- return -EFAULT;
+ if ( !rc && __copy_to_compat_offset(
+ cmp.mar.frame_list, start_extent,
+ compat_frame_list, done) )
+ rc = -EFAULT;
+
+ if ( rc )
+ {
+ if ( split < 0 )
+ {
+ gdprintk(XENLOG_ERR,
+ "Cannot cancel continuation: %ld\n", rc);
+ domain_crash(current->domain);
+ }
+ return rc;
+ }
+ }
+
+ start_extent += done;
+
+ /* Completely done. */
+ if ( start_extent == cmp.mar.nr_frames )
+ break;
+
+ /*
+ * Done a "full" batch, but we were limited by space in the xlat
+ * area. Go around the loop again without necesserily returning
+ * to guest context.
+ */
+ if ( done == nat.mar->nr_frames )
+ {
+ split = 1;
+ break;
}
+
+ /* Explicit continuation request from a higher level. */
+ if ( done < nat.mar->nr_frames )
+ return hypercall_create_continuation(
+ __HYPERVISOR_memory_op, "ih",
+ op | (start_extent << MEMOP_EXTENT_SHIFT), compat);
+
+ /*
+ * Well... Somethings gone wrong with the two levels of chunking.
+ * My condolences to whomever next has to debug this mess.
+ */
+ ASSERT_UNREACHABLE();
+ domain_crash(current->domain);
+ split = 0;
break;
}
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 280b7969b6..b95403695f 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4053,6 +4053,9 @@ int gnttab_acquire_resource(
for ( i = 0; i < nr_frames; ++i )
mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
+ /* Success. Passed nr_frames back to the caller. */
+ rc = nr_frames;
+
out:
grant_write_unlock(gt);
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 01cab7e493..128718b31c 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1118,23 +1118,41 @@ static int acquire_ioreq_server(struct domain *d,
mfn_list[i] = mfn_x(mfn);
}
- return 0;
+ /* Success. Passed nr_frames back to the caller. */
+ return nr_frames;
#else
return -EOPNOTSUPP;
#endif
}
+/*
+ * Returns -errno on error, or positive in the range [1, nr_frames] on
+ * success. Returning less than nr_frames contitutes a request for a
+ * continuation. Callers can depend on frame + nr_frames not overflowing.
+ */
+static int _acquire_resource(
+ struct domain *d, unsigned int type, unsigned int id, unsigned int frame,
+ unsigned int nr_frames, xen_pfn_t mfn_list[])
+{
+ switch ( type )
+ {
+ case XENMEM_resource_grant_table:
+ return gnttab_acquire_resource(d, id, frame, nr_frames, mfn_list);
+
+ case XENMEM_resource_ioreq_server:
+ return acquire_ioreq_server(d, id, frame, nr_frames, mfn_list);
+
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static int acquire_resource(
- XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
+ XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg,
+ unsigned long start_extent)
{
struct domain *d, *currd = current->domain;
xen_mem_acquire_resource_t xmar;
- /*
- * The mfn_list and gfn_list (below) arrays are ok on stack for the
- * moment since they are small, but if they need to grow in future
- * use-cases then per-CPU arrays or heap allocations may be required.
- */
- xen_pfn_t mfn_list[32];
unsigned int max_frames;
int rc;
@@ -1147,9 +1165,6 @@ static int acquire_resource(
if ( xmar.pad != 0 )
return -EINVAL;
- if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
- return -E2BIG;
-
/*
* The ABI is rather unfortunate. nr_frames (and therefore the total size
* of the resource) is 32bit, while frame (the offset within the resource
@@ -1179,7 +1194,7 @@ static int acquire_resource(
if ( guest_handle_is_null(xmar.frame_list) )
{
- if ( xmar.nr_frames )
+ if ( xmar.nr_frames || start_extent )
goto out;
xmar.nr_frames = max_frames;
@@ -1187,30 +1202,47 @@ static int acquire_resource(
goto out;
}
- do {
- switch ( xmar.type )
- {
- case XENMEM_resource_grant_table:
- rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, xmar.nr_frames,
- mfn_list);
- break;
+ /*
+ * Limiting nr_frames at (UINT_MAX >> MEMOP_EXTENT_SHIFT) isn't ideal. If
+ * it ever becomes a practical problem, we can switch to mutating
+ * xmar.{frame,nr_frames,frame_list} in guest memory.
+ */
+ rc = -EINVAL;
+ if ( start_extent >= xmar.nr_frames ||
+ xmar.nr_frames > (UINT_MAX >> MEMOP_EXTENT_SHIFT) )
+ goto out;
- case XENMEM_resource_ioreq_server:
- rc = acquire_ioreq_server(d, xmar.id, xmar.frame, xmar.nr_frames,
- mfn_list);
- break;
+ /* Adjust for work done on previous continuations. */
+ xmar.nr_frames -= start_extent;
+ xmar.frame += start_extent;
+ guest_handle_add_offset(xmar.frame_list, start_extent);
- default:
- rc = -EOPNOTSUPP;
- break;
- }
+ do {
+ /*
+ * Arbitrary size. Not too much stack space, and a reasonable stride
+ * for continuation checks.
+ */
+ xen_pfn_t mfn_list[32];
+ unsigned int todo = MIN(ARRAY_SIZE(mfn_list), xmar.nr_frames), done;
- if ( rc )
+ rc = _acquire_resource(d, xmar.type, xmar.id, xmar.frame,
+ todo, mfn_list);
+ if ( rc < 0 )
+ goto out;
+
+ done = rc;
+ rc = 0;
+ if ( done == 0 || done > todo )
+ {
+ ASSERT_UNREACHABLE();
+ rc = -EINVAL;
goto out;
+ }
+ /* Adjust guest frame_list appropriately. */
if ( !paging_mode_translate(currd) )
{
- if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
+ if ( copy_to_guest(xmar.frame_list, mfn_list, done) )
rc = -EFAULT;
}
else
@@ -1218,10 +1250,10 @@ static int acquire_resource(
xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
unsigned int i;
- if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
+ if ( copy_from_guest(gfn_list, xmar.frame_list, done) )
rc = -EFAULT;
- for ( i = 0; !rc && i < xmar.nr_frames; i++ )
+ for ( i = 0; !rc && i < done; i++ )
{
rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
_mfn(mfn_list[i]));
@@ -1230,7 +1262,32 @@ static int acquire_resource(
rc = -EIO;
}
}
- } while ( 0 );
+
+ if ( rc )
+ goto out;
+
+ xmar.nr_frames -= done;
+ xmar.frame += done;
+ guest_handle_add_offset(xmar.frame_list, done);
+ start_extent += done;
+
+ /*
+ * Explicit continuation request from _acquire_resource(), or we've
+ * still got more work to do.
+ */
+ if ( done < todo ||
+ (xmar.nr_frames && hypercall_preempt_check()) )
+ {
+ rc = hypercall_create_continuation(
+ __HYPERVISOR_memory_op, "lh",
+ XENMEM_acquire_resource | (start_extent << MEMOP_EXTENT_SHIFT),
+ arg);
+ goto out;
+ }
+
+ } while ( xmar.nr_frames );
+
+ rc = 0;
out:
rcu_unlock_domain(d);
@@ -1697,7 +1754,8 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
case XENMEM_acquire_resource:
rc = acquire_resource(
- guest_handle_cast(arg, xen_mem_acquire_resource_t));
+ guest_handle_cast(arg, xen_mem_acquire_resource_t),
+ start_extent);
break;
default:
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v9 01/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource
2021-02-01 23:26 ` [PATCH v9 01/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource Andrew Cooper
@ 2021-02-04 21:23 ` Andrew Cooper
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-04 21:23 UTC (permalink / raw)
To: Xen-devel
Cc: George Dunlap, Ian Jackson, Jan Beulich, Stefano Stabellini,
Wei Liu, Julien Grall, Paul Durrant,
Michał Leszczyński, Hubert Jasudowicz, Tamas K Lengyel
On 01/02/2021 23:26, Andrew Cooper wrote:
> A guest's default number of grant frames is 64, and XENMEM_acquire_resource
> will reject an attempt to map more than 32 frames. This limit is caused by
> the size of mfn_list[] on the stack.
>
> Fix mapping of arbitrary size requests by looping over batches of 32 in
> acquire_resource(), and using hypercall continuations when necessary.
>
> To start with, break _acquire_resource() out of acquire_resource() to cope
> with type-specific dispatching, and update the return semantics to indicate
> the number of mfns returned. Update gnttab_acquire_resource() and x86's
> arch_acquire_resource() to match these new semantics.
>
> Have do_memory_op() pass start_extent into acquire_resource() so it can pick
> up where it left off after a continuation, and loop over batches of 32 until
> all the work is done, or a continuation needs to occur.
>
> compat_memory_op() is a bit more complicated, because it also has to marshal
> frame_list in the XLAT buffer. Have it account for continuation information
> itself and hide details from the upper layer, so it can marshal the buffer in
> chunks if necessary.
>
> With these fixes in place, it is now possible to map the whole grant table for
> a guest.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
>
> v9:
> * Crash domain rather than returning late with -ERANGE/-EFAULT.
>
> v8:
> * nat => cmp change in the start_extent check.
> * Rebase over 'frame' and ARM/IOREQ series.
>
> v3:
> * Spelling fixes
> ---
> xen/common/compat/memory.c | 114 +++++++++++++++++++++++++++++++++--------
> xen/common/grant_table.c | 3 ++
> xen/common/memory.c | 124 +++++++++++++++++++++++++++++++++------------
> 3 files changed, 187 insertions(+), 54 deletions(-)
Attempt at release-ack paperwork.
This is a bugfix for an issue which doesn't manifest by in-tree default
callers, but does manifest when using the
xenforeignmemory_map_resource() interface in the expected manner.
The hypercall is made of a metadata structure, and an array of frames.
The bug is that Xen only tolerates a maximum of 32 frames, and the
bugfix is to accept an arbitrary number of frames.
What can go wrong (other than the theoretical base case of everything,
seeing as we're talking about C in system context)?
The bugfix is basically "do { chunk_of_32(); } while ( !done );", so
we're adding in an extra loop into the hypervisor. We could fail to
terminate the loop (possible livelock in the hypervisor), or we could
incorrectly marshal the buffer (guest kernel might receive junk instead
of the mapping they expected).
The majority of the complexity actually comes from the fact there are
two nested loops, one in the compat layer doing 32=>64 (and back)
marshalling, and one in the main layer, looping over chunks of 32
frames. Therefore, the same risks apply at both layers.
I am certain the code is not bug free. The compat layer here is
practically impossible to follow, and has (self inflicted) patterns
where we have to crash the guest rather than raise a clean failure, due
to an inability to unwind the fact that the upper layer decided to issue
a continuation.
There is also one bit where I literally had to give up, and put this
logic in:
> + /*
> + * Well... Somethings gone wrong with the two levels of chunking.
> + * My condolences to whomever next has to debug this mess.
> + */
> + ASSERT_UNREACHABLE();
> + domain_crash(current->domain);
> + split = 0;
> break;
Mitigations to these risks are thus:
* Explicit use of failsafe coding patterns, will break out of the loops
and pass -EINVAL back to the caller, or crashing the domain when we
can't figure out how to pass an error back safely.
* This codepath codepath gets used multiple times on every single VM
boot, so will get ample testing from the in-tree caller point of view,
as soon as OSSTest starts running.
* The IPT series (which discovered this mess to start with) shows that,
in addition to the in-tree paths working, the >32 frame mappings appear
to work correctly.
* An in-tree unit test exercising this codepath in a way which
demonstrates this bug. Further work planned for this test.
* Some incredibly invasive Xen+XTF testing to prove the correctness of
the marshalling. Not suitable for committing, but available for
inspection/query. In particular, this covers aspects of the logic with
won't get any practical testing elsewhere.
Overall, if there are bugs, they're very likely to be spotted by OSSTest
in short order.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
2021-02-01 23:26 ` [PATCH v9 01/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource Andrew Cooper
@ 2021-02-01 23:26 ` Andrew Cooper
2021-02-02 9:04 ` Jan Beulich
2021-02-01 23:26 ` [PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter Andrew Cooper
` (9 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:26 UTC (permalink / raw)
To: Xen-devel
Cc: Michał Leszczyński, Andrew Cooper, Jan Beulich,
Roger Pau Monné,
Wei Liu, Anthony PERARD, Tamas K Lengyel
From: Michał Leszczyński <michal.leszczynski@cert.pl>
To use vmtrace, buffers of a suitable size need allocating, and different
tasks will want different sizes.
Add a domain creation parameter, and audit it appropriately in the
{arch_,}sanitise_domain_config() functions.
For now, the x86 specific auditing is tuned to Processor Trace running in
Single Output mode, which requires a single contiguous range of memory.
The size is given an arbitrary limit of 64M which is expected to be enough for
anticipated usecases, but not large enough to get into long-running-hypercall
problems.
Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
When support for later generations of IPT get added, we can in principle start
to use ToTP which is a scatter list of smaller trace regions to use, if we
need to massively up the buffer size available.
v9:
* Drop misleading comments in vmtrace_alloc_buffer(). Memory still gets
leaked in theoretical corner cases, but the pattern needs fixing across the
board when we figure out a solution.
v8:
* Rename vmtrace_frames to vmtrace_size. Reposition to fill a hole.
* Rename vmtrace.buf to vmtrace.pg.
* Rework the refcounting logic and comment it *very* clearly.
v7:
* Major chop&change within the series.
* Use the name 'vmtrace' consistently.
* Use the (new) common vcpu_teardown() functionality, rather than leaving a
latent memory leak on ARM.
---
xen/arch/x86/domain.c | 23 ++++++++++++++++
xen/common/domain.c | 64 +++++++++++++++++++++++++++++++++++++++++++++
xen/include/public/domctl.h | 3 +++
xen/include/xen/sched.h | 6 +++++
4 files changed, 96 insertions(+)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index b9ba04633e..6c7ee25f3b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -660,6 +660,29 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ if ( config->vmtrace_size )
+ {
+ unsigned int size = config->vmtrace_size;
+
+ ASSERT(vmtrace_available); /* Checked by common code. */
+
+ /*
+ * For now, vmtrace is restricted to HVM guests, and using a
+ * power-of-2 buffer between 4k and 64M in size.
+ */
+ if ( !hvm )
+ {
+ dprintk(XENLOG_INFO, "vmtrace not supported for PV\n");
+ return -EINVAL;
+ }
+
+ if ( size < PAGE_SIZE || size > MB(64) || (size & (size - 1)) )
+ {
+ dprintk(XENLOG_INFO, "Unsupported vmtrace size: %#x\n", size);
+ return -EINVAL;
+ }
+ }
+
return 0;
}
diff --git a/xen/common/domain.c b/xen/common/domain.c
index d1e94d88cf..b6f8d2f536 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -132,6 +132,56 @@ static void vcpu_info_reset(struct vcpu *v)
v->vcpu_info_mfn = INVALID_MFN;
}
+static void vmtrace_free_buffer(struct vcpu *v)
+{
+ const struct domain *d = v->domain;
+ struct page_info *pg = v->vmtrace.pg;
+ unsigned int i;
+
+ if ( !pg )
+ return;
+
+ v->vmtrace.pg = NULL;
+
+ for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
+ {
+ put_page_alloc_ref(&pg[i]);
+ put_page_and_type(&pg[i]);
+ }
+}
+
+static int vmtrace_alloc_buffer(struct vcpu *v)
+{
+ struct domain *d = v->domain;
+ struct page_info *pg;
+ unsigned int i;
+
+ if ( !d->vmtrace_size )
+ return 0;
+
+ pg = alloc_domheap_pages(d, get_order_from_bytes(d->vmtrace_size),
+ MEMF_no_refcount);
+ if ( !pg )
+ return -ENOMEM;
+
+ for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
+ if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
+ goto refcnt_err;
+
+ /*
+ * We must only let vmtrace_free_buffer() take any action in the success
+ * case when we've taken all the refs it intends to drop.
+ */
+ v->vmtrace.pg = pg;
+ return 0;
+
+ refcnt_err:
+ while ( i-- )
+ put_page_and_type(&pg[i]);
+
+ return -ENODATA;
+}
+
/*
* Release resources held by a vcpu. There may or may not be live references
* to the vcpu, and it may or may not be fully constructed.
@@ -140,6 +190,8 @@ static void vcpu_info_reset(struct vcpu *v)
*/
static int vcpu_teardown(struct vcpu *v)
{
+ vmtrace_free_buffer(v);
+
return 0;
}
@@ -201,6 +253,9 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id)
if ( sched_init_vcpu(v) != 0 )
goto fail_wq;
+ if ( vmtrace_alloc_buffer(v) != 0 )
+ goto fail_wq;
+
if ( arch_vcpu_create(v) != 0 )
goto fail_sched;
@@ -449,6 +504,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
}
}
+ if ( config->vmtrace_size && !vmtrace_available )
+ {
+ dprintk(XENLOG_INFO, "vmtrace requested but not available\n");
+ return -EINVAL;
+ }
+
return arch_sanitise_domain_config(config);
}
@@ -474,7 +535,10 @@ struct domain *domain_create(domid_t domid,
ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
if ( config )
+ {
d->options = config->flags;
+ d->vmtrace_size = config->vmtrace_size;
+ }
/* Sort out our idea of is_control_domain(). */
d->is_privileged = is_priv;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 666aeb71bf..88a5b1ef5d 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -95,6 +95,9 @@ struct xen_domctl_createdomain {
int32_t max_grant_frames;
int32_t max_maptrack_frames;
+ /* Per-vCPU buffer size in bytes. 0 to disable. */
+ uint32_t vmtrace_size;
+
struct xen_arch_domainconfig arch;
};
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 06dba1a397..bc78a09a53 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -272,6 +272,10 @@ struct vcpu
/* vPCI per-vCPU area, used to store data for long running operations. */
struct vpci_vcpu vpci;
+ struct {
+ struct page_info *pg; /* One contiguous allocation of d->vmtrace_size */
+ } vmtrace;
+
struct arch_vcpu arch;
#ifdef CONFIG_IOREQ_SERVER
@@ -547,6 +551,8 @@ struct domain
unsigned int guest_request_sync : 1;
} monitor;
+ unsigned int vmtrace_size; /* Buffer size in bytes, or 0 to disable. */
+
#ifdef CONFIG_ARGO
/* Argo interdomain communication support */
struct argo_domain *argo;
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter
2021-02-01 23:26 ` [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter Andrew Cooper
@ 2021-02-02 9:04 ` Jan Beulich
2021-02-03 16:04 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2021-02-02 9:04 UTC (permalink / raw)
To: Andrew Cooper
Cc: Michał Leszczyński, Roger Pau Monné,
Wei Liu, Anthony PERARD, Tamas K Lengyel, Xen-devel
On 02.02.2021 00:26, Andrew Cooper wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -132,6 +132,56 @@ static void vcpu_info_reset(struct vcpu *v)
> v->vcpu_info_mfn = INVALID_MFN;
> }
>
> +static void vmtrace_free_buffer(struct vcpu *v)
> +{
> + const struct domain *d = v->domain;
> + struct page_info *pg = v->vmtrace.pg;
> + unsigned int i;
> +
> + if ( !pg )
> + return;
> +
> + v->vmtrace.pg = NULL;
> +
> + for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
> + {
> + put_page_alloc_ref(&pg[i]);
> + put_page_and_type(&pg[i]);
> + }
> +}
> +
> +static int vmtrace_alloc_buffer(struct vcpu *v)
> +{
> + struct domain *d = v->domain;
> + struct page_info *pg;
> + unsigned int i;
> +
> + if ( !d->vmtrace_size )
> + return 0;
> +
> + pg = alloc_domheap_pages(d, get_order_from_bytes(d->vmtrace_size),
> + MEMF_no_refcount);
> + if ( !pg )
> + return -ENOMEM;
> +
> + for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
> + if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
> + goto refcnt_err;
> +
> + /*
> + * We must only let vmtrace_free_buffer() take any action in the success
> + * case when we've taken all the refs it intends to drop.
> + */
> + v->vmtrace.pg = pg;
> + return 0;
> +
> + refcnt_err:
> + while ( i-- )
> + put_page_and_type(&pg[i]);
> +
> + return -ENODATA;
Would you mind at least logging how many pages may be leaked
here? I also don't understand why you don't call
put_page_alloc_ref() in the loop - that's fine to do prior to
the put_page_and_type(), and will at least limit the leak.
The buffer size here typically isn't insignificant, and it
may be helpful to not unnecessarily defer the freeing to
relinquish_resources() (assuming we will make that one also
traverse the list of "extra" pages, but I understand that's
not going to happen for 4.15 anymore anyway).
Additionally, while I understand you're not in favor of the
comments we have on all three similar code paths, I think
replicating their comments here would help easily spotting
(by grep-ing e.g. for "fishy") all instances that will need
adjusting once we have figured a better overall solution.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter
2021-02-02 9:04 ` Jan Beulich
@ 2021-02-03 16:04 ` Andrew Cooper
2021-02-04 11:11 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2021-02-03 16:04 UTC (permalink / raw)
To: Jan Beulich
Cc: Michał Leszczyński, Roger Pau Monné,
Wei Liu, Anthony PERARD, Tamas K Lengyel, Xen-devel
On 02/02/2021 09:04, Jan Beulich wrote:
> On 02.02.2021 00:26, Andrew Cooper wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -132,6 +132,56 @@ static void vcpu_info_reset(struct vcpu *v)
>> v->vcpu_info_mfn = INVALID_MFN;
>> }
>>
>> +static void vmtrace_free_buffer(struct vcpu *v)
>> +{
>> + const struct domain *d = v->domain;
>> + struct page_info *pg = v->vmtrace.pg;
>> + unsigned int i;
>> +
>> + if ( !pg )
>> + return;
>> +
>> + v->vmtrace.pg = NULL;
>> +
>> + for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
>> + {
>> + put_page_alloc_ref(&pg[i]);
>> + put_page_and_type(&pg[i]);
>> + }
>> +}
>> +
>> +static int vmtrace_alloc_buffer(struct vcpu *v)
>> +{
>> + struct domain *d = v->domain;
>> + struct page_info *pg;
>> + unsigned int i;
>> +
>> + if ( !d->vmtrace_size )
>> + return 0;
>> +
>> + pg = alloc_domheap_pages(d, get_order_from_bytes(d->vmtrace_size),
>> + MEMF_no_refcount);
>> + if ( !pg )
>> + return -ENOMEM;
>> +
>> + for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
>> + if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
>> + goto refcnt_err;
>> +
>> + /*
>> + * We must only let vmtrace_free_buffer() take any action in the success
>> + * case when we've taken all the refs it intends to drop.
>> + */
>> + v->vmtrace.pg = pg;
>> + return 0;
>> +
>> + refcnt_err:
>> + while ( i-- )
>> + put_page_and_type(&pg[i]);
>> +
>> + return -ENODATA;
> Would you mind at least logging how many pages may be leaked
> here? I also don't understand why you don't call
> put_page_alloc_ref() in the loop - that's fine to do prior to
> the put_page_and_type(), and will at least limit the leak.
> The buffer size here typically isn't insignificant, and it
> may be helpful to not unnecessarily defer the freeing to
> relinquish_resources() (assuming we will make that one also
> traverse the list of "extra" pages, but I understand that's
> not going to happen for 4.15 anymore anyway).
>
> Additionally, while I understand you're not in favor of the
> comments we have on all three similar code paths, I think
> replicating their comments here would help easily spotting
> (by grep-ing e.g. for "fishy") all instances that will need
> adjusting once we have figured a better overall solution.
How is:
for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
/*
* The domain can't possibly know about this page yet, so
failure
* here is a clear indication of something fishy going on.
*/
goto refcnt_err;
/*
* We must only let vmtrace_free_buffer() take any action in the success
* case when we've taken all the refs it intends to drop.
*/
v->vmtrace.pg = pg;
return 0;
refcnt_err:
/*
* We can theoretically reach this point if someone has taken 2^43
refs on
* the frames in the time the above loop takes to execute, or
someone has
* made a blind decrease reservation hypercall and managed to pick the
* right mfn. Free the memory we safely can, and leak the rest.
*/
while ( i-- )
{
put_page_alloc_ref(&pg[i]);
put_page_and_type(&pg[i]);
}
return -ENODATA;
this?
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter
2021-02-03 16:04 ` Andrew Cooper
@ 2021-02-04 11:11 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2021-02-04 11:11 UTC (permalink / raw)
To: Andrew Cooper
Cc: Michał Leszczyński, Roger Pau Monné,
Wei Liu, Anthony PERARD, Tamas K Lengyel, Xen-devel
On 03.02.2021 17:04, Andrew Cooper wrote:
> On 02/02/2021 09:04, Jan Beulich wrote:
>> On 02.02.2021 00:26, Andrew Cooper wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -132,6 +132,56 @@ static void vcpu_info_reset(struct vcpu *v)
>>> v->vcpu_info_mfn = INVALID_MFN;
>>> }
>>>
>>> +static void vmtrace_free_buffer(struct vcpu *v)
>>> +{
>>> + const struct domain *d = v->domain;
>>> + struct page_info *pg = v->vmtrace.pg;
>>> + unsigned int i;
>>> +
>>> + if ( !pg )
>>> + return;
>>> +
>>> + v->vmtrace.pg = NULL;
>>> +
>>> + for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
>>> + {
>>> + put_page_alloc_ref(&pg[i]);
>>> + put_page_and_type(&pg[i]);
>>> + }
>>> +}
>>> +
>>> +static int vmtrace_alloc_buffer(struct vcpu *v)
>>> +{
>>> + struct domain *d = v->domain;
>>> + struct page_info *pg;
>>> + unsigned int i;
>>> +
>>> + if ( !d->vmtrace_size )
>>> + return 0;
>>> +
>>> + pg = alloc_domheap_pages(d, get_order_from_bytes(d->vmtrace_size),
>>> + MEMF_no_refcount);
>>> + if ( !pg )
>>> + return -ENOMEM;
>>> +
>>> + for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
>>> + if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
>>> + goto refcnt_err;
>>> +
>>> + /*
>>> + * We must only let vmtrace_free_buffer() take any action in the success
>>> + * case when we've taken all the refs it intends to drop.
>>> + */
>>> + v->vmtrace.pg = pg;
>>> + return 0;
>>> +
>>> + refcnt_err:
>>> + while ( i-- )
>>> + put_page_and_type(&pg[i]);
>>> +
>>> + return -ENODATA;
>> Would you mind at least logging how many pages may be leaked
>> here? I also don't understand why you don't call
>> put_page_alloc_ref() in the loop - that's fine to do prior to
>> the put_page_and_type(), and will at least limit the leak.
>> The buffer size here typically isn't insignificant, and it
>> may be helpful to not unnecessarily defer the freeing to
>> relinquish_resources() (assuming we will make that one also
>> traverse the list of "extra" pages, but I understand that's
>> not going to happen for 4.15 anymore anyway).
>>
>> Additionally, while I understand you're not in favor of the
>> comments we have on all three similar code paths, I think
>> replicating their comments here would help easily spotting
>> (by grep-ing e.g. for "fishy") all instances that will need
>> adjusting once we have figured a better overall solution.
>
> How is:
>
> for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
> if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
> /*
> * The domain can't possibly know about this page yet, so
> failure
> * here is a clear indication of something fishy going on.
> */
> goto refcnt_err;
>
> /*
> * We must only let vmtrace_free_buffer() take any action in the success
> * case when we've taken all the refs it intends to drop.
> */
> v->vmtrace.pg = pg;
> return 0;
>
> refcnt_err:
> /*
> * We can theoretically reach this point if someone has taken 2^43
> refs on
> * the frames in the time the above loop takes to execute, or
> someone has
> * made a blind decrease reservation hypercall and managed to pick the
> * right mfn. Free the memory we safely can, and leak the rest.
> */
> while ( i-- )
> {
> put_page_alloc_ref(&pg[i]);
> put_page_and_type(&pg[i]);
> }
>
> return -ENODATA;
>
> this?
Much better, thanks. Remains the question of logging the
suspected leak of perhaps many pages. But either way
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
2021-02-01 23:26 ` [PATCH v9 01/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource Andrew Cooper
2021-02-01 23:26 ` [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter Andrew Cooper
@ 2021-02-01 23:26 ` Andrew Cooper
2021-02-02 12:16 ` Ian Jackson
2021-02-01 23:26 ` [PATCH v9 04/11] xen/memory: Add a vmtrace_buf resource type Andrew Cooper
` (8 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:26 UTC (permalink / raw)
To: Xen-devel
Cc: Michał Leszczyński, Andrew Cooper, Ian Jackson,
Wei Liu, Anthony PERARD, Tamas K Lengyel
From: Michał Leszczyński <michal.leszczynski@cert.pl>
Allow to specify the size of per-vCPU trace buffer upon
domain creation. This is zero by default (meaning: not enabled).
Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v8:
* Rebase over vmtrace_size change.
v7:
* Use the name 'vmtrace' consistently.
---
docs/man/xl.cfg.5.pod.in | 9 +++++++++
tools/golang/xenlight/helpers.gen.go | 2 ++
tools/golang/xenlight/types.gen.go | 1 +
tools/include/libxl.h | 7 +++++++
tools/libs/light/libxl_create.c | 1 +
tools/libs/light/libxl_types.idl | 4 ++++
tools/xl/xl_parse.c | 4 ++++
7 files changed, 28 insertions(+)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 7cdb8595d3..040374dcd6 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -681,6 +681,15 @@ Windows).
If this option is not specified then it will default to B<false>.
+=item B<vmtrace_buf_kb=KBYTES>
+
+Specifies the size of vmtrace buffer that would be allocated for each
+vCPU belonging to this domain. Disabled (i.e. B<vmtrace_buf_kb=0>) by
+default.
+
+B<NOTE>: Acceptable values are platform specific. For Intel Processor
+Trace, this value must be a power of 2 between 4k and 16M.
+
=back
=head2 Devices
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 63e2876463..4c60d27a9c 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1114,6 +1114,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
x.Altp2M = Altp2MMode(xc.altp2m)
+x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
return nil}
@@ -1589,6 +1590,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
+xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
return nil
}
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index 5851c38057..cb13002fdb 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -514,6 +514,7 @@ GicVersion GicVersion
Vuart VuartType
}
Altp2M Altp2MMode
+VmtraceBufKb int
}
type domainBuildInfoTypeUnion interface {
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index f48d0c5e8a..a7b673e89d 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -489,6 +489,13 @@
#define LIBXL_HAVE_PHYSINFO_CAP_VMTRACE 1
/*
+ * LIBXL_HAVE_VMTRACE_BUF_KB indicates that libxl_domain_create_info has a
+ * vmtrace_buf_kb parameter, which allows to enable pre-allocation of
+ * processor tracing buffers of given size.
+ */
+#define LIBXL_HAVE_VMTRACE_BUF_KB 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 9848d65f36..46f68da697 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -607,6 +607,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
.max_evtchn_port = b_info->event_channels,
.max_grant_frames = b_info->max_grant_frames,
.max_maptrack_frames = b_info->max_maptrack_frames,
+ .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
};
if (info->type != LIBXL_DOMAIN_TYPE_PV) {
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index dacb7df6b7..5b85a7419f 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -648,6 +648,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
# supported by x86 HVM and ARM support is planned.
("altp2m", libxl_altp2m_mode),
+ # Size of preallocated vmtrace trace buffers (in KBYTES).
+ # Use zero value to disable this feature.
+ ("vmtrace_buf_kb", integer),
+
], dir=DIR_IN,
copy_deprecated_fn="libxl__domain_build_info_copy_deprecated",
)
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 867e4d068a..1893cfc086 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1863,6 +1863,10 @@ void parse_config_data(const char *config_source,
}
}
+ if (!xlu_cfg_get_long(config, "vmtrace_buf_kb", &l, 1) && l) {
+ b_info->vmtrace_buf_kb = l;
+ }
+
if (!xlu_cfg_get_list(config, "ioports", &ioports, &num_ioports, 0)) {
b_info->num_ioports = num_ioports;
b_info->ioports = calloc(num_ioports, sizeof(*b_info->ioports));
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter
2021-02-01 23:26 ` [PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter Andrew Cooper
@ 2021-02-02 12:16 ` Ian Jackson
2021-02-02 12:17 ` Ian Jackson
0 siblings, 1 reply; 22+ messages in thread
From: Ian Jackson @ 2021-02-02 12:16 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Michał Leszczyński, Wei Liu, Anthony PERARD,
Tamas K Lengyel
Andrew Cooper writes ("[PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter"):
> From: Michał Leszczyński <michal.leszczynski@cert.pl>
>
> Allow to specify the size of per-vCPU trace buffer upon
> domain creation. This is zero by default (meaning: not enabled).
...
Wearing my maintainer/reviewer hat:
Release risk assessment for this patch:
* This contains golang changes which might break the build or need
updates to golang generated files. This ought to be detected by
our tests so we can fix it. At this stage of the release that is
probably OK. The risk of actually shipping a broken build is low.
* The patch introduces a new libxl config parameter. That has API
and UI implications. But it is a very small change and the
semantics are fairly obvious. The name likewise is fine. So I am
very comfortable with recommending this late addition to these
APIs.
* The patch contains buffer size handling code. In the general case
that might produce a risk of buffer overruns. But at least here in
this patch this is actually just the configured size of a buffer,
and actual length/use checks are done elsewhere, so this is is not
a real risk.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter
2021-02-02 12:16 ` Ian Jackson
@ 2021-02-02 12:17 ` Ian Jackson
0 siblings, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2021-02-02 12:17 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel, Michał Leszczyński, Wei Liu,
Anthony PERARD, Tamas K Lengyel
Ian Jackson writes ("Re: [PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter"):
> Andrew Cooper writes ("[PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter"):
> > From: Michał Leszczyński <michal.leszczynski@cert.pl>
> >
> > Allow to specify the size of per-vCPU trace buffer upon
> > domain creation. This is zero by default (meaning: not enabled).
> ...
>
> Wearing my maintainer/reviewer hat:
>
> Release risk assessment for this patch:
>
> * This contains golang changes which might break the build or need
> updates to golang generated files. This ought to be detected by
> our tests so we can fix it. At this stage of the release that is
> probably OK. The risk of actually shipping a broken build is low.
>
> * The patch introduces a new libxl config parameter. That has API
> and UI implications. But it is a very small change and the
> semantics are fairly obvious. The name likewise is fine. So I am
> very comfortable with recommending this late addition to these
> APIs.
>
> * The patch contains buffer size handling code. In the general case
> that might produce a risk of buffer overruns. But at least here in
> this patch this is actually just the configured size of a buffer,
> and actual length/use checks are done elsewhere, so this is is not
> a real risk.
Consequently, wearing my RM hat, this patch:
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v9 04/11] xen/memory: Add a vmtrace_buf resource type
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (2 preceding siblings ...)
2021-02-01 23:26 ` [PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter Andrew Cooper
@ 2021-02-01 23:26 ` Andrew Cooper
2021-02-01 23:26 ` [PATCH v9 05/11] x86/vmx: Add Intel Processor Trace support Andrew Cooper
` (7 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:26 UTC (permalink / raw)
To: Xen-devel
Cc: Michał Leszczyński, Andrew Cooper, Jan Beulich,
Roger Pau Monné,
Wei Liu, Tamas K Lengyel
From: Michał Leszczyński <michal.leszczynski@cert.pl>
Allow to map processor trace buffer using acquire_resource().
Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v8:
* Rebase over 'fault' and ARM/IOREQ series.
v7:
* Rebase over changes elsewhere in the series
---
xen/common/memory.c | 29 +++++++++++++++++++++++++++++
xen/include/public/memory.h | 1 +
2 files changed, 30 insertions(+)
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 128718b31c..fada97a79f 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1085,6 +1085,9 @@ static unsigned int resource_max_frames(const struct domain *d,
case XENMEM_resource_ioreq_server:
return ioreq_server_max_frames(d);
+ case XENMEM_resource_vmtrace_buf:
+ return d->vmtrace_size >> PAGE_SHIFT;
+
default:
return -EOPNOTSUPP;
}
@@ -1125,6 +1128,29 @@ static int acquire_ioreq_server(struct domain *d,
#endif
}
+static int acquire_vmtrace_buf(
+ struct domain *d, unsigned int id, unsigned int frame,
+ unsigned int nr_frames, xen_pfn_t mfn_list[])
+{
+ const struct vcpu *v = domain_vcpu(d, id);
+ unsigned int i;
+ mfn_t mfn;
+
+ if ( !v )
+ return -ENOENT;
+
+ if ( !v->vmtrace.pg ||
+ (frame + nr_frames) > (d->vmtrace_size >> PAGE_SHIFT) )
+ return -EINVAL;
+
+ mfn = page_to_mfn(v->vmtrace.pg);
+
+ for ( i = 0; i < nr_frames; i++ )
+ mfn_list[i] = mfn_x(mfn) + frame + i;
+
+ return nr_frames;
+}
+
/*
* Returns -errno on error, or positive in the range [1, nr_frames] on
* success. Returning less than nr_frames contitutes a request for a
@@ -1142,6 +1168,9 @@ static int _acquire_resource(
case XENMEM_resource_ioreq_server:
return acquire_ioreq_server(d, id, frame, nr_frames, mfn_list);
+ case XENMEM_resource_vmtrace_buf:
+ return acquire_vmtrace_buf(d, id, frame, nr_frames, mfn_list);
+
default:
return -EOPNOTSUPP;
}
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 020c79d757..50e73eef98 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -625,6 +625,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
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v9 05/11] x86/vmx: Add Intel Processor Trace support
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (3 preceding siblings ...)
2021-02-01 23:26 ` [PATCH v9 04/11] xen/memory: Add a vmtrace_buf resource type Andrew Cooper
@ 2021-02-01 23:26 ` Andrew Cooper
2021-02-01 23:26 ` [PATCH v9 06/11] xen/domctl: Add XEN_DOMCTL_vmtrace_op Andrew Cooper
` (6 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:26 UTC (permalink / raw)
To: Xen-devel
Cc: Michał Leszczyński, Andrew Cooper, Jan Beulich,
Roger Pau Monné,
Wei Liu, Jun Nakajima, Kevin Tian, Tamas K Lengyel
From: Michał Leszczyński <michal.leszczynski@cert.pl>
Add CPUID/MSR enumeration details for Processor Trace. For now, we will only
support its use inside VMX operation. Fill in the vmtrace_available boolean
to activate the newly introduced common infrastructure for allocating trace
buffers.
For now, Processor Trace is going to be operated in Single Output mode behind
the guests back. Add the MSRs to struct vcpu_msrs, and set up the buffer
limit in vmx_init_ipt() as it is fixed for the lifetime of the domain.
Context switch the most of the MSRs in and out of vCPU context, but the main
control register needs to reside in the MSR load/save lists. Explicitly pull
the msrs pointer out into a local variable, because the optimiser cannot keep
it live across the memory clobbers in the MSR accesses.
Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v8:
* Plumb bsp boolean down into vmx_init_vmcs_config()
* Rename vmx_init_pt() to vmx_init_ipt()
* Rebase over vmtrace_size/pg renames.
v7:
* Major chop&change within the series.
* Move MSRs to vcpu_msrs, which is where they'll definitely want to live when
we offer PT to VMs for their own use.
---
tools/libs/light/libxl_cpuid.c | 1 +
tools/misc/xen-cpuid.c | 2 +-
xen/arch/x86/hvm/vmx/vmcs.c | 19 +++++++++++++---
xen/arch/x86/hvm/vmx/vmx.c | 34 ++++++++++++++++++++++++++++-
xen/include/asm-x86/cpufeature.h | 1 +
xen/include/asm-x86/hvm/vmx/vmcs.h | 4 ++++
xen/include/asm-x86/msr.h | 32 +++++++++++++++++++++++++++
xen/include/public/arch-x86/cpufeatureset.h | 1 +
8 files changed, 89 insertions(+), 5 deletions(-)
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 259612834e..289c59c742 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -188,6 +188,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
{"avx512-ifma", 0x00000007, 0, CPUID_REG_EBX, 21, 1},
{"clflushopt", 0x00000007, 0, CPUID_REG_EBX, 23, 1},
{"clwb", 0x00000007, 0, CPUID_REG_EBX, 24, 1},
+ {"proc-trace", 0x00000007, 0, CPUID_REG_EBX, 25, 1},
{"avx512pf", 0x00000007, 0, CPUID_REG_EBX, 26, 1},
{"avx512er", 0x00000007, 0, CPUID_REG_EBX, 27, 1},
{"avx512cd", 0x00000007, 0, CPUID_REG_EBX, 28, 1},
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index c81aa93055..2d04162d8d 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -106,7 +106,7 @@ static const char *const str_7b0[32] =
[18] = "rdseed", [19] = "adx",
[20] = "smap", [21] = "avx512-ifma",
[22] = "pcommit", [23] = "clflushopt",
- [24] = "clwb", [25] = "pt",
+ [24] = "clwb", [25] = "proc-trace",
[26] = "avx512pf", [27] = "avx512er",
[28] = "avx512cd", [29] = "sha",
[30] = "avx512bw", [31] = "avx512vl",
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 164535f8f0..f9f9bc18cd 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -243,7 +243,7 @@ static bool_t cap_check(const char *name, u32 expected, u32 saw)
return saw != expected;
}
-static int vmx_init_vmcs_config(void)
+static int vmx_init_vmcs_config(bool bsp)
{
u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
u32 _vmx_pin_based_exec_control;
@@ -291,6 +291,20 @@ static int vmx_init_vmcs_config(void)
_vmx_cpu_based_exec_control &=
~(CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING);
+ rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
+
+ /* Check whether IPT is supported in VMX operation. */
+ if ( bsp )
+ vmtrace_available = cpu_has_proc_trace &&
+ (_vmx_misc_cap & VMX_MISC_PROC_TRACE);
+ else if ( vmtrace_available &&
+ !(_vmx_misc_cap & VMX_MISC_PROC_TRACE) )
+ {
+ printk("VMX: IPT capabilities differ between CPU%u and BSP\n",
+ smp_processor_id());
+ return -EINVAL;
+ }
+
if ( _vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS )
{
min = 0;
@@ -305,7 +319,6 @@ static int vmx_init_vmcs_config(void)
SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS |
SECONDARY_EXEC_XSAVES |
SECONDARY_EXEC_TSC_SCALING);
- rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL )
opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
if ( opt_vpid_enabled )
@@ -715,7 +728,7 @@ static int _vmx_cpu_up(bool bsp)
wrmsr(MSR_IA32_FEATURE_CONTROL, eax, 0);
}
- if ( (rc = vmx_init_vmcs_config()) != 0 )
+ if ( (rc = vmx_init_vmcs_config(bsp)) != 0 )
return rc;
INIT_LIST_HEAD(&this_cpu(active_vmcs_list));
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2d4475ee3d..12b961113e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -428,6 +428,20 @@ static void vmx_domain_relinquish_resources(struct domain *d)
vmx_free_vlapic_mapping(d);
}
+static void vmx_init_ipt(struct vcpu *v)
+{
+ unsigned int size = v->domain->vmtrace_size;
+
+ if ( !size )
+ return;
+
+ /* Checked by domain creation logic. */
+ ASSERT(v->vmtrace.pg);
+ ASSERT(size >= PAGE_SIZE && (size & (size - 1)) == 0);
+
+ v->arch.msrs->rtit.output_limit = size - 1;
+}
+
static int vmx_vcpu_initialise(struct vcpu *v)
{
int rc;
@@ -470,6 +484,7 @@ static int vmx_vcpu_initialise(struct vcpu *v)
}
vmx_install_vlapic_mapping(v);
+ vmx_init_ipt(v);
return 0;
}
@@ -508,22 +523,39 @@ static void vmx_restore_host_msrs(void)
static void vmx_save_guest_msrs(struct vcpu *v)
{
+ struct vcpu_msrs *msrs = v->arch.msrs;
+
/*
* We cannot cache SHADOW_GS_BASE while the VCPU runs, as it can
* be updated at any time via SWAPGS, which we cannot trap.
*/
v->arch.hvm.vmx.shadow_gs = read_gs_shadow();
+
+ if ( v->arch.hvm.vmx.ipt_active )
+ {
+ rdmsrl(MSR_RTIT_OUTPUT_MASK, msrs->rtit.output_mask);
+ rdmsrl(MSR_RTIT_STATUS, msrs->rtit.status);
+ }
}
static void vmx_restore_guest_msrs(struct vcpu *v)
{
+ const struct vcpu_msrs *msrs = v->arch.msrs;
+
write_gs_shadow(v->arch.hvm.vmx.shadow_gs);
wrmsrl(MSR_STAR, v->arch.hvm.vmx.star);
wrmsrl(MSR_LSTAR, v->arch.hvm.vmx.lstar);
wrmsrl(MSR_SYSCALL_MASK, v->arch.hvm.vmx.sfmask);
if ( cpu_has_msr_tsc_aux )
- wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
+ wrmsr_tsc_aux(msrs->tsc_aux);
+
+ if ( v->arch.hvm.vmx.ipt_active )
+ {
+ wrmsrl(MSR_RTIT_OUTPUT_BASE, page_to_maddr(v->vmtrace.pg));
+ wrmsrl(MSR_RTIT_OUTPUT_MASK, msrs->rtit.output_mask);
+ wrmsrl(MSR_RTIT_STATUS, msrs->rtit.status);
+ }
}
void vmx_update_cpu_exec_control(struct vcpu *v)
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index f62e526a96..33b2257888 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -105,6 +105,7 @@
#define cpu_has_clwb boot_cpu_has(X86_FEATURE_CLWB)
#define cpu_has_avx512er boot_cpu_has(X86_FEATURE_AVX512ER)
#define cpu_has_avx512cd boot_cpu_has(X86_FEATURE_AVX512CD)
+#define cpu_has_proc_trace boot_cpu_has(X86_FEATURE_PROC_TRACE)
#define cpu_has_sha boot_cpu_has(X86_FEATURE_SHA)
#define cpu_has_avx512bw boot_cpu_has(X86_FEATURE_AVX512BW)
#define cpu_has_avx512vl boot_cpu_has(X86_FEATURE_AVX512VL)
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 906810592f..8073af323b 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -156,6 +156,9 @@ struct vmx_vcpu {
/* Do we need to tolerate a spurious EPT_MISCONFIG VM exit? */
bool_t ept_spurious_misconfig;
+ /* Processor Trace configured and enabled for the vcpu. */
+ bool ipt_active;
+
/* Is the guest in real mode? */
uint8_t vmx_realmode;
/* Are we emulating rather than VMENTERing? */
@@ -283,6 +286,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_PROC_TRACE 0x00004000
#define VMX_MISC_CR3_TARGET 0x01ff0000
#define VMX_MISC_VMWRITE_ALL 0x20000000
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 16f95e7344..1d3eca9063 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -306,6 +306,38 @@ struct vcpu_msrs
};
} misc_features_enables;
+ /*
+ * 0x00000560 ... 57x - MSR_RTIT_*
+ *
+ * "Real Time Instruction Trace", now called Processor Trace.
+ *
+ * These MSRs are not exposed to guests. They are controlled by Xen
+ * behind the scenes, when vmtrace is enabled for the domain.
+ *
+ * MSR_RTIT_OUTPUT_BASE not stored here. It is fixed per vcpu, and
+ * derived from v->vmtrace.buf.
+ */
+ struct {
+ /*
+ * Placed in the MSR load/save lists. Only modified by hypercall in
+ * the common case.
+ */
+ uint64_t ctl;
+
+ /*
+ * Updated by hardware in non-root mode. Synchronised here on vcpu
+ * context switch.
+ */
+ uint64_t status;
+ union {
+ uint64_t output_mask;
+ struct {
+ uint32_t output_limit;
+ uint32_t output_offset;
+ };
+ };
+ } rtit;
+
/* 0x00000da0 - MSR_IA32_XSS */
struct {
uint64_t raw;
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 6f7efaad6d..a501479820 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -217,6 +217,7 @@ XEN_CPUFEATURE(SMAP, 5*32+20) /*S Supervisor Mode Access Prevention */
XEN_CPUFEATURE(AVX512_IFMA, 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(PROC_TRACE, 5*32+25) /* 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 */
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v9 06/11] xen/domctl: Add XEN_DOMCTL_vmtrace_op
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (4 preceding siblings ...)
2021-02-01 23:26 ` [PATCH v9 05/11] x86/vmx: Add Intel Processor Trace support Andrew Cooper
@ 2021-02-01 23:26 ` Andrew Cooper
2021-02-01 23:26 ` [PATCH v9 07/11] tools/libxc: Add xc_vmtrace_* functions Andrew Cooper
` (5 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:26 UTC (permalink / raw)
To: Xen-devel
Cc: Michał Leszczyński, Andrew Cooper, Jan Beulich,
Roger Pau Monné,
Wei Liu, Jun Nakajima, Kevin Tian, Tamas K Lengyel
From: Michał Leszczyński <michal.leszczynski@cert.pl>
Implement an interface to configure and control tracing operations. Reuse the
existing SETDEBUGGING flask vector rather than inventing a new one.
Userspace using this interface is going to need platform specific knowledge
anyway to interpret the contents of the trace buffer. While some operations
(e.g. enable/disable) can reasonably be generic, others cannot. Provide an
explicitly-platform specific pair of get/set operations to reduce API churn as
new options get added/enabled.
For the VMX specific Processor Trace implementation, tolerate reading and
modifying a safe subset of bits in CTL, STATUS and OUTPUT_MASK. This permits
userspace to control the content which gets logged, but prevents modification
of details such as the position/size of the output buffer.
Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v9:
* Drop platform-specific access to MSR_RTIT_OUTPUT_MASK. It's not necessary
for current usecases, and will simplify adding ToPA support in the future.
v8:
* Reposition mask constants.
v7:
* Major chop&change within the series.
---
xen/arch/x86/domctl.c | 55 +++++++++++++++++
xen/arch/x86/hvm/vmx/vmx.c | 135 ++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/hvm/hvm.h | 63 ++++++++++++++++++++
xen/include/public/domctl.h | 35 +++++++++++
xen/xsm/flask/hooks.c | 1 +
5 files changed, 289 insertions(+)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b28cfe9817..b464465230 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -155,6 +155,55 @@ 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)
+{
+ struct vcpu *v;
+ int rc;
+
+ if ( !d->vmtrace_size || d == current->domain /* No vcpu_pause() */ )
+ return -EINVAL;
+
+ ASSERT(is_hvm_domain(d)); /* Restricted by domain creation logic. */
+
+ v = domain_vcpu(d, op->vcpu);
+ if ( !v )
+ return -ENOENT;
+
+ vcpu_pause(v);
+ switch ( op->cmd )
+ {
+ case XEN_DOMCTL_vmtrace_enable:
+ case XEN_DOMCTL_vmtrace_disable:
+ case XEN_DOMCTL_vmtrace_reset_and_enable:
+ rc = hvm_vmtrace_control(
+ v, op->cmd != XEN_DOMCTL_vmtrace_disable,
+ op->cmd == XEN_DOMCTL_vmtrace_reset_and_enable);
+ break;
+
+ case XEN_DOMCTL_vmtrace_output_position:
+ rc = hvm_vmtrace_output_position(v, &op->value);
+ if ( rc >= 0 )
+ rc = 0;
+ break;
+
+ case XEN_DOMCTL_vmtrace_get_option:
+ rc = hvm_vmtrace_get_option(v, op->key, &op->value);
+ break;
+
+ case XEN_DOMCTL_vmtrace_set_option:
+ rc = hvm_vmtrace_set_option(v, op->key, op->value);
+ break;
+
+ default:
+ rc = -EOPNOTSUPP;
+ break;
+ }
+ vcpu_unpause(v);
+
+ return rc;
+}
+
#define MAX_IOPORTS 0x10000
long arch_do_domctl(
@@ -1320,6 +1369,12 @@ long arch_do_domctl(
domain_unpause(d);
break;
+ case XEN_DOMCTL_vmtrace_op:
+ ret = do_vmtrace_op(d, &domctl->u.vmtrace_op, u_domctl);
+ if ( !ret )
+ copyback = true;
+ break;
+
default:
ret = iommu_do_domctl(domctl, d, u_domctl);
break;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 12b961113e..beb5692b8b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2261,6 +2261,137 @@ static bool vmx_get_pending_event(struct vcpu *v, struct x86_event *info)
return true;
}
+/*
+ * We only let vmtrace agents see and modify a subset of bits in MSR_RTIT_CTL.
+ * These all pertain to data-emitted into the trace buffer(s). Must not
+ * include controls pertaining to the structure/position of the trace
+ * buffer(s).
+ */
+#define RTIT_CTL_MASK \
+ (RTIT_CTL_TRACE_EN | RTIT_CTL_OS | RTIT_CTL_USR | RTIT_CTL_TSC_EN | \
+ RTIT_CTL_DIS_RETC | RTIT_CTL_BRANCH_EN)
+
+/*
+ * Status bits restricted to the first-gen subset (i.e. no further CPUID
+ * requirements.)
+ */
+#define RTIT_STATUS_MASK \
+ (RTIT_STATUS_FILTER_EN | RTIT_STATUS_CONTEXT_EN | RTIT_STATUS_TRIGGER_EN | \
+ RTIT_STATUS_ERROR | RTIT_STATUS_STOPPED)
+
+static int vmtrace_get_option(struct vcpu *v, uint64_t key, uint64_t *output)
+{
+ const struct vcpu_msrs *msrs = v->arch.msrs;
+
+ switch ( key )
+ {
+ case MSR_RTIT_CTL:
+ *output = msrs->rtit.ctl & RTIT_CTL_MASK;
+ break;
+
+ case MSR_RTIT_STATUS:
+ *output = msrs->rtit.status & RTIT_STATUS_MASK;
+ break;
+
+ default:
+ *output = 0;
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int vmtrace_set_option(struct vcpu *v, uint64_t key, uint64_t value)
+{
+ struct vcpu_msrs *msrs = v->arch.msrs;
+ bool new_en, old_en = msrs->rtit.ctl & RTIT_CTL_TRACE_EN;
+
+ switch ( key )
+ {
+ case MSR_RTIT_CTL:
+ if ( value & ~RTIT_CTL_MASK )
+ return -EINVAL;
+
+ msrs->rtit.ctl &= ~RTIT_CTL_MASK;
+ msrs->rtit.ctl |= (value & RTIT_CTL_MASK);
+ break;
+
+ case MSR_RTIT_STATUS:
+ if ( value & ~RTIT_STATUS_MASK )
+ return -EINVAL;
+
+ msrs->rtit.status &= ~RTIT_STATUS_MASK;
+ msrs->rtit.status |= (value & RTIT_STATUS_MASK);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ new_en = msrs->rtit.ctl & RTIT_CTL_TRACE_EN;
+
+ /* ctl.trace_en changed => update MSR load/save lists appropriately. */
+ if ( !old_en && new_en )
+ {
+ if ( vmx_add_guest_msr(v, MSR_RTIT_CTL, msrs->rtit.ctl) ||
+ vmx_add_host_load_msr(v, MSR_RTIT_CTL, 0) )
+ {
+ /*
+ * The only failure cases here are failing the
+ * singleton-per-domain memory allocation, or exceeding the space
+ * in the allocation. We could unwind in principle, but there is
+ * nothing userspace can usefully do to continue using this VM.
+ */
+ domain_crash(v->domain);
+ return -ENXIO;
+ }
+ }
+ else if ( old_en && !new_en )
+ {
+ vmx_del_msr(v, MSR_RTIT_CTL, VMX_MSR_GUEST);
+ vmx_del_msr(v, MSR_RTIT_CTL, VMX_MSR_HOST);
+ }
+
+ return 0;
+}
+
+static int vmtrace_control(struct vcpu *v, bool enable, bool reset)
+{
+ struct vcpu_msrs *msrs = v->arch.msrs;
+ uint64_t new_ctl;
+ int rc;
+
+ /*
+ * Absolutely nothing good will come of Xen's and userspace's idea of
+ * whether ipt is enabled getting out of sync.
+ */
+ if ( v->arch.hvm.vmx.ipt_active == enable )
+ return -EINVAL;
+
+ if ( reset )
+ {
+ msrs->rtit.status = 0;
+ msrs->rtit.output_offset = 0;
+ }
+
+ new_ctl = msrs->rtit.ctl & ~RTIT_CTL_TRACE_EN;
+ if ( enable )
+ new_ctl |= RTIT_CTL_TRACE_EN;
+
+ rc = vmtrace_set_option(v, MSR_RTIT_CTL, new_ctl);
+ if ( rc )
+ return rc;
+
+ v->arch.hvm.vmx.ipt_active = enable;
+
+ return 0;
+}
+
+static int vmtrace_output_position(struct vcpu *v, uint64_t *pos)
+{
+ *pos = v->arch.msrs->rtit.output_offset;
+ return v->arch.hvm.vmx.ipt_active;
+}
+
static struct hvm_function_table __initdata vmx_function_table = {
.name = "VMX",
.cpu_up_prepare = vmx_cpu_up_prepare,
@@ -2316,6 +2447,10 @@ static struct hvm_function_table __initdata vmx_function_table = {
.altp2m_vcpu_update_vmfunc_ve = vmx_vcpu_update_vmfunc_ve,
.altp2m_vcpu_emulate_ve = vmx_vcpu_emulate_ve,
.altp2m_vcpu_emulate_vmfunc = vmx_vcpu_emulate_vmfunc,
+ .vmtrace_control = vmtrace_control,
+ .vmtrace_output_position = vmtrace_output_position,
+ .vmtrace_set_option = vmtrace_set_option,
+ .vmtrace_get_option = vmtrace_get_option,
.tsc_scaling = {
.max_ratio = VMX_TSC_MULTIPLIER_MAX,
},
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 334bd573b9..960ec03917 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -214,6 +214,12 @@ struct hvm_function_table {
bool_t (*altp2m_vcpu_emulate_ve)(struct vcpu *v);
int (*altp2m_vcpu_emulate_vmfunc)(const struct cpu_user_regs *regs);
+ /* vmtrace */
+ int (*vmtrace_control)(struct vcpu *v, bool enable, bool reset);
+ int (*vmtrace_output_position)(struct vcpu *v, uint64_t *pos);
+ int (*vmtrace_set_option)(struct vcpu *v, uint64_t key, uint64_t value);
+ int (*vmtrace_get_option)(struct vcpu *v, uint64_t key, uint64_t *value);
+
/*
* Parameters and callbacks for hardware-assisted TSC scaling,
* which are valid only when the hardware feature is available.
@@ -655,6 +661,41 @@ static inline bool altp2m_vcpu_emulate_ve(struct vcpu *v)
return false;
}
+static inline int hvm_vmtrace_control(struct vcpu *v, bool enable, bool reset)
+{
+ if ( hvm_funcs.vmtrace_control )
+ return hvm_funcs.vmtrace_control(v, enable, reset);
+
+ return -EOPNOTSUPP;
+}
+
+/* Returns -errno, or a boolean of whether tracing is currently active. */
+static inline int hvm_vmtrace_output_position(struct vcpu *v, uint64_t *pos)
+{
+ if ( hvm_funcs.vmtrace_output_position )
+ return hvm_funcs.vmtrace_output_position(v, pos);
+
+ return -EOPNOTSUPP;
+}
+
+static inline int hvm_vmtrace_set_option(
+ struct vcpu *v, uint64_t key, uint64_t value)
+{
+ if ( hvm_funcs.vmtrace_set_option )
+ return hvm_funcs.vmtrace_set_option(v, key, value);
+
+ return -EOPNOTSUPP;
+}
+
+static inline int hvm_vmtrace_get_option(
+ struct vcpu *v, uint64_t key, uint64_t *value)
+{
+ if ( hvm_funcs.vmtrace_get_option )
+ return hvm_funcs.vmtrace_get_option(v, key, value);
+
+ return -EOPNOTSUPP;
+}
+
/*
* This must be defined as a macro instead of an inline function,
* because it uses 'struct vcpu' and 'struct domain' which have
@@ -751,6 +792,28 @@ static inline bool hvm_has_set_descriptor_access_exiting(void)
return false;
}
+static inline int hvm_vmtrace_control(struct vcpu *v, bool enable, bool reset)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int hvm_vmtrace_output_position(struct vcpu *v, uint64_t *pos)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int hvm_vmtrace_set_option(
+ struct vcpu *v, uint64_t key, uint64_t value)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int hvm_vmtrace_get_option(
+ struct vcpu *v, uint64_t key, uint64_t *value)
+{
+ return -EOPNOTSUPP;
+}
+
#define is_viridian_domain(d) ((void)(d), false)
#define is_viridian_vcpu(v) ((void)(v), false)
#define has_viridian_time_ref_count(d) ((void)(d), false)
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 88a5b1ef5d..4dbf107785 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1135,6 +1135,39 @@ struct xen_domctl_vuart_op {
*/
};
+/* XEN_DOMCTL_vmtrace_op: Perform VM tracing operations. */
+struct xen_domctl_vmtrace_op {
+ uint32_t cmd; /* IN */
+ uint32_t vcpu; /* IN */
+ uint64_aligned_t key; /* IN - @cmd specific data. */
+ uint64_aligned_t value; /* IN/OUT - @cmd specific data. */
+
+ /*
+ * General enable/disable of tracing.
+ *
+ * XEN_DOMCTL_vmtrace_reset_and_enable is provided as optimisation for
+ * common usecases, which want to reset status and position information
+ * when turning tracing back on.
+ */
+#define XEN_DOMCTL_vmtrace_enable 1
+#define XEN_DOMCTL_vmtrace_disable 2
+#define XEN_DOMCTL_vmtrace_reset_and_enable 3
+
+ /* Obtain the current output position within the buffer. Fills @value. */
+#define XEN_DOMCTL_vmtrace_output_position 4
+
+ /*
+ * Get/Set platform specific configuration.
+ *
+ * For Intel Processor Trace, @key/@value are interpreted as MSR
+ * reads/writes to MSR_RTIT_*, filtered to a safe subset.
+ */
+#define XEN_DOMCTL_vmtrace_get_option 5
+#define XEN_DOMCTL_vmtrace_set_option 6
+};
+typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
+
struct xen_domctl {
uint32_t cmd;
#define XEN_DOMCTL_createdomain 1
@@ -1219,6 +1252,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
@@ -1279,6 +1313,7 @@ struct xen_domctl {
struct xen_domctl_monitor_op monitor_op;
struct xen_domctl_psr_alloc psr_alloc;
struct xen_domctl_vuart_op vuart_op;
+ struct xen_domctl_vmtrace_op vmtrace_op;
uint8_t pad[128];
} u;
};
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 11784d7425..3b7313b949 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -703,6 +703,7 @@ static int flask_domctl(struct domain *d, int cmd)
return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__VM_EVENT);
case XEN_DOMCTL_debug_op:
+ case XEN_DOMCTL_vmtrace_op:
case XEN_DOMCTL_gdbsx_guestmemio:
case XEN_DOMCTL_gdbsx_pausevcpu:
case XEN_DOMCTL_gdbsx_unpausevcpu:
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v9 07/11] tools/libxc: Add xc_vmtrace_* functions
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (5 preceding siblings ...)
2021-02-01 23:26 ` [PATCH v9 06/11] xen/domctl: Add XEN_DOMCTL_vmtrace_op Andrew Cooper
@ 2021-02-01 23:26 ` Andrew Cooper
2021-02-01 23:27 ` [PATCH v9 08/11] tools/misc: Add xen-vmtrace tool Andrew Cooper
` (4 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:26 UTC (permalink / raw)
To: Xen-devel
Cc: Michał Leszczyński, Andrew Cooper, Ian Jackson,
Wei Liu, Tamas K Lengyel
From: Michał Leszczyński <michal.leszczynski@cert.pl>
Add functions in libxc that use the new XEN_DOMCTL_vmtrace interface.
Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v7:
* Use the name 'vmtrace' consistently.
---
tools/include/xenctrl.h | 73 ++++++++++++++++++++++++
tools/libs/ctrl/Makefile | 1 +
tools/libs/ctrl/xc_vmtrace.c | 128 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 202 insertions(+)
create mode 100644 tools/libs/ctrl/xc_vmtrace.c
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 3796425e1e..0efcdae8b4 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1583,6 +1583,79 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, xc_cpumap_t mask);
int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
+/**
+ * Enable vmtrace for given vCPU.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid domain identifier
+ * @parm vcpu vcpu identifier
+ * @return 0 on success, -1 on failure
+ */
+int xc_vmtrace_enable(xc_interface *xch, uint32_t domid, uint32_t vcpu);
+
+/**
+ * Enable vmtrace for given vCPU.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid domain identifier
+ * @parm vcpu vcpu identifier
+ * @return 0 on success, -1 on failure
+ */
+int xc_vmtrace_disable(xc_interface *xch, uint32_t domid, uint32_t vcpu);
+
+/**
+ * Enable vmtrace for a given vCPU, along with resetting status/offset
+ * details.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid domain identifier
+ * @parm vcpu vcpu identifier
+ * @return 0 on success, -1 on failure
+ */
+int xc_vmtrace_reset_and_enable(xc_interface *xch, uint32_t domid,
+ uint32_t vcpu);
+
+/**
+ * Get current output position inside the trace buffer.
+ *
+ * Repeated calls will return different values if tracing is enabled. It is
+ * platform specific what happens when the buffer fills completely.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid domain identifier
+ * @parm vcpu vcpu identifier
+ * @parm pos current output position in bytes
+ * @return 0 on success, -1 on failure
+ */
+int xc_vmtrace_output_position(xc_interface *xch, uint32_t domid,
+ uint32_t vcpu, uint64_t *pos);
+
+/**
+ * Get platform specific vmtrace options.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid domain identifier
+ * @parm vcpu vcpu identifier
+ * @parm key platform-specific input
+ * @parm value platform-specific output
+ * @return 0 on success, -1 on failure
+ */
+int xc_vmtrace_get_option(xc_interface *xch, uint32_t domid,
+ uint32_t vcpu, uint64_t key, uint64_t *value);
+
+/**
+ * Set platform specific vntvmtrace options.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid domain identifier
+ * @parm vcpu vcpu identifier
+ * @parm key platform-specific input
+ * @parm value platform-specific input
+ * @return 0 on success, -1 on failure
+ */
+int xc_vmtrace_set_option(xc_interface *xch, uint32_t domid,
+ uint32_t vcpu, uint64_t key, uint64_t value);
+
int xc_domctl(xc_interface *xch, struct xen_domctl *domctl);
int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
index 6106e36c49..ce9ecae710 100644
--- a/tools/libs/ctrl/Makefile
+++ b/tools/libs/ctrl/Makefile
@@ -22,6 +22,7 @@ SRCS-y += xc_pm.c
SRCS-y += xc_cpu_hotplug.c
SRCS-y += xc_resume.c
SRCS-y += xc_vm_event.c
+SRCS-y += xc_vmtrace.c
SRCS-y += xc_monitor.c
SRCS-y += xc_mem_paging.c
SRCS-y += xc_mem_access.c
diff --git a/tools/libs/ctrl/xc_vmtrace.c b/tools/libs/ctrl/xc_vmtrace.c
new file mode 100644
index 0000000000..602502367f
--- /dev/null
+++ b/tools/libs/ctrl/xc_vmtrace.c
@@ -0,0 +1,128 @@
+/******************************************************************************
+ * xc_vmtrace.c
+ *
+ * API for manipulating hardware tracing features
+ *
+ * Copyright (c) 2020, Michal Leszczynski
+ *
+ * Copyright 2020 CERT Polska. All rights reserved.
+ * Use is subject to license terms.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "xc_private.h"
+
+int xc_vmtrace_enable(
+ xc_interface *xch, uint32_t domid, uint32_t vcpu)
+{
+ struct xen_domctl domctl = {
+ .cmd = XEN_DOMCTL_vmtrace_op,
+ .domain = domid,
+ .u.vmtrace_op = {
+ .cmd = XEN_DOMCTL_vmtrace_enable,
+ .vcpu = vcpu,
+ },
+ };
+
+ return do_domctl(xch, &domctl);
+}
+
+int xc_vmtrace_disable(
+ xc_interface *xch, uint32_t domid, uint32_t vcpu)
+{
+ struct xen_domctl domctl = {
+ .cmd = XEN_DOMCTL_vmtrace_op,
+ .domain = domid,
+ .u.vmtrace_op = {
+ .cmd = XEN_DOMCTL_vmtrace_disable,
+ .vcpu = vcpu,
+ },
+ };
+
+ return do_domctl(xch, &domctl);
+}
+
+int xc_vmtrace_reset_and_enable(
+ xc_interface *xch, uint32_t domid, uint32_t vcpu)
+{
+ struct xen_domctl domctl = {
+ .cmd = XEN_DOMCTL_vmtrace_op,
+ .domain = domid,
+ .u.vmtrace_op = {
+ .cmd = XEN_DOMCTL_vmtrace_reset_and_enable,
+ .vcpu = vcpu,
+ },
+ };
+
+ return do_domctl(xch, &domctl);
+}
+
+int xc_vmtrace_output_position(
+ xc_interface *xch, uint32_t domid, uint32_t vcpu, uint64_t *pos)
+{
+ struct xen_domctl domctl = {
+ .cmd = XEN_DOMCTL_vmtrace_op,
+ .domain = domid,
+ .u.vmtrace_op = {
+ .cmd = XEN_DOMCTL_vmtrace_output_position,
+ .vcpu = vcpu,
+ },
+ };
+ int rc = do_domctl(xch, &domctl);
+
+ if ( !rc )
+ *pos = domctl.u.vmtrace_op.value;
+
+ return rc;
+}
+
+int xc_vmtrace_get_option(
+ xc_interface *xch, uint32_t domid, uint32_t vcpu,
+ uint64_t key, uint64_t *value)
+{
+ struct xen_domctl domctl = {
+ .cmd = XEN_DOMCTL_vmtrace_op,
+ .domain = domid,
+ .u.vmtrace_op = {
+ .cmd = XEN_DOMCTL_vmtrace_get_option,
+ .vcpu = vcpu,
+ .key = key,
+ },
+ };
+ int rc = do_domctl(xch, &domctl);
+
+ if ( !rc )
+ *value = domctl.u.vmtrace_op.value;
+
+ return rc;
+}
+
+int xc_vmtrace_set_option(
+ xc_interface *xch, uint32_t domid, uint32_t vcpu,
+ uint64_t key, uint64_t value)
+{
+ struct xen_domctl domctl = {
+ .cmd = XEN_DOMCTL_vmtrace_op,
+ .domain = domid,
+ .u.vmtrace_op = {
+ .cmd = XEN_DOMCTL_vmtrace_set_option,
+ .vcpu = vcpu,
+ .key = key,
+ .value = value,
+ },
+ };
+
+ return do_domctl(xch, &domctl);
+}
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v9 08/11] tools/misc: Add xen-vmtrace tool
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (6 preceding siblings ...)
2021-02-01 23:26 ` [PATCH v9 07/11] tools/libxc: Add xc_vmtrace_* functions Andrew Cooper
@ 2021-02-01 23:27 ` Andrew Cooper
2021-02-01 23:27 ` [PATCH v9 09/11] xen/vmtrace: support for VM forks Andrew Cooper
` (3 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:27 UTC (permalink / raw)
To: Xen-devel
Cc: Michał Leszczyński, Andrew Cooper, Ian Jackson,
Wei Liu, Tamas K Lengyel
From: Michał Leszczyński <michal.leszczynski@cert.pl>
Add an demonstration tool that uses xc_vmtrace_* calls in order
to manage external IPT monitoring for DomU.
Signed-off-by: Michał Leszczyński <michal.leszczynski@cert.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <iwj@xenproject.org>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v9:
* Fix truncated data on clean exit
v8:
* Switch to being a build-only target
---
tools/misc/.gitignore | 1 +
tools/misc/Makefile | 7 ++
tools/misc/xen-vmtrace.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 174 insertions(+)
create mode 100644 tools/misc/xen-vmtrace.c
diff --git a/tools/misc/.gitignore b/tools/misc/.gitignore
index b2c3b21f57..ce6f937d0c 100644
--- a/tools/misc/.gitignore
+++ b/tools/misc/.gitignore
@@ -1,3 +1,4 @@
xen-access
xen-memshare
xen-ucode
+xen-vmtrace
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 912c5d4f0e..2b683819d4 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -50,6 +50,10 @@ TARGETS_COPY += xenpvnetboot
# Everything which needs to be built
TARGETS_BUILD := $(filter-out $(TARGETS_COPY),$(TARGETS_ALL))
+# ... including build-only targets
+TARGETS_BUILD-$(CONFIG_X86) += xen-vmtrace
+TARGETS_BUILD += $(TARGETS_BUILD-y)
+
.PHONY: all build
all build: $(TARGETS_BUILD)
@@ -90,6 +94,9 @@ xen-hvmcrash: xen-hvmcrash.o
xen-memshare: xen-memshare.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+xen-vmtrace: xen-vmtrace.o
+ $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
+
xenperf: xenperf.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
diff --git a/tools/misc/xen-vmtrace.c b/tools/misc/xen-vmtrace.c
new file mode 100644
index 0000000000..cc58a0707b
--- /dev/null
+++ b/tools/misc/xen-vmtrace.c
@@ -0,0 +1,166 @@
+/******************************************************************************
+ * tools/vmtrace.c
+ *
+ * Demonstrative tool for collecting Intel Processor Trace data from Xen.
+ * Could be used to externally monitor a given vCPU in given DomU.
+ *
+ * Copyright (C) 2020 by CERT Polska - NASK PIB
+ *
+ * Authors: Michał Leszczyński, michal.leszczynski@cert.pl
+ * Date: June, 2020
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; under version 2 of the License.
+ *
+ * This program is distributed in the hope that 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/>.
+ */
+
+#include <err.h>
+#include <errno.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+
+#include <xenctrl.h>
+#include <xenforeignmemory.h>
+
+#define MSR_RTIT_CTL 0x00000570
+#define RTIT_CTL_OS (1 << 2)
+#define RTIT_CTL_USR (1 << 3)
+#define RTIT_CTL_BRANCH_EN (1 << 13)
+
+static xc_interface *xch;
+static xenforeignmemory_handle *fh;
+static uint32_t domid, vcpu;
+static size_t size;
+static char *buf;
+
+static sig_atomic_t interrupted = 0;
+static void int_handler(int signum)
+{
+ interrupted = 1;
+}
+
+static int get_more_data(void)
+{
+ static uint64_t last_pos;
+ uint64_t pos;
+
+ if ( xc_vmtrace_output_position(xch, domid, vcpu, &pos) )
+ {
+ perror("xc_vmtrace_output_position()");
+ return -1;
+ }
+
+ if ( pos > last_pos )
+ fwrite(buf + last_pos, pos - last_pos, 1, stdout);
+ else if ( pos < last_pos )
+ {
+ /* buffer wrapped */
+ fwrite(buf + last_pos, size - last_pos, 1, stdout);
+ fwrite(buf, pos, 1, stdout);
+ }
+
+ last_pos = pos;
+ return 0;
+}
+
+int main(int argc, char **argv)
+{
+ int rc, exit = 1;
+ xenforeignmemory_resource_handle *fres = NULL;
+
+ if ( signal(SIGINT, int_handler) == SIG_ERR )
+ err(1, "Failed to register signal handler\n");
+
+ if ( argc != 3 )
+ {
+ fprintf(stderr, "Usage: %s <domid> <vcpu_id>\n", argv[0]);
+ fprintf(stderr, "It's recommended to redirect thisprogram's output to file\n");
+ fprintf(stderr, "or to pipe it's output to xxd or other program.\n");
+ return 1;
+ }
+
+ domid = atoi(argv[1]);
+ vcpu = atoi(argv[2]);
+
+ xch = xc_interface_open(NULL, NULL, 0);
+ fh = xenforeignmemory_open(NULL, 0);
+
+ if ( !xch )
+ err(1, "xc_interface_open()");
+ if ( !fh )
+ err(1, "xenforeignmemory_open()");
+
+ rc = xenforeignmemory_resource_size(
+ fh, domid, XENMEM_resource_vmtrace_buf, vcpu, &size);
+ if ( rc )
+ err(1, "xenforeignmemory_resource_size()");
+
+ fres = xenforeignmemory_map_resource(
+ fh, domid, XENMEM_resource_vmtrace_buf, vcpu,
+ 0, size >> XC_PAGE_SHIFT, (void **)&buf, PROT_READ, 0);
+ if ( !fres )
+ err(1, "xenforeignmemory_map_resource()");
+
+ if ( xc_vmtrace_set_option(
+ xch, domid, vcpu, MSR_RTIT_CTL,
+ RTIT_CTL_BRANCH_EN | RTIT_CTL_USR | RTIT_CTL_OS) )
+ {
+ perror("xc_vmtrace_set_option()");
+ goto out;
+ }
+
+ if ( xc_vmtrace_enable(xch, domid, vcpu) )
+ {
+ perror("xc_vmtrace_enable()");
+ goto out;
+ }
+
+ while ( !interrupted )
+ {
+ xc_dominfo_t dominfo;
+
+ if ( get_more_data() )
+ goto out;
+
+ usleep(1000 * 100);
+
+ if ( xc_domain_getinfo(xch, domid, 1, &dominfo) != 1 ||
+ dominfo.domid != domid || dominfo.shutdown )
+ {
+ if ( get_more_data() )
+ goto out;
+ break;
+ }
+ }
+
+ exit = 0;
+
+ out:
+ if ( xc_vmtrace_disable(xch, domid, vcpu) )
+ perror("xc_vmtrace_disable()");
+
+ if ( fres && xenforeignmemory_unmap_resource(fh, fres) )
+ perror("xenforeignmemory_unmap_resource()");
+
+ return exit;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v9 09/11] xen/vmtrace: support for VM forks
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (7 preceding siblings ...)
2021-02-01 23:27 ` [PATCH v9 08/11] tools/misc: Add xen-vmtrace tool Andrew Cooper
@ 2021-02-01 23:27 ` Andrew Cooper
2021-02-01 23:27 ` [PATCH v9 10/11] x86/vm_event: Carry the vmtrace buffer position in vm_event Andrew Cooper
` (2 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:27 UTC (permalink / raw)
To: Xen-devel
Cc: Tamas K Lengyel, Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Jun Nakajima, Kevin Tian, Michał Leszczyński,
Tamas K Lengyel
From: Tamas K Lengyel <tamas.lengyel@intel.com>
Implement vmtrace_reset_pt function. Properly set IPT
state for VM forks.
Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v7:
* New
---
tools/misc/xen-vmtrace.c | 2 +-
xen/arch/x86/hvm/vmx/vmx.c | 11 +++++++++++
xen/arch/x86/mm/mem_sharing.c | 3 +++
xen/include/asm-x86/hvm/hvm.h | 9 +++++++++
4 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/tools/misc/xen-vmtrace.c b/tools/misc/xen-vmtrace.c
index cc58a0707b..7572e880c5 100644
--- a/tools/misc/xen-vmtrace.c
+++ b/tools/misc/xen-vmtrace.c
@@ -43,7 +43,7 @@ static uint32_t domid, vcpu;
static size_t size;
static char *buf;
-static sig_atomic_t interrupted = 0;
+static sig_atomic_t interrupted;
static void int_handler(int signum)
{
interrupted = 1;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index beb5692b8b..faba95d057 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2392,6 +2392,16 @@ static int vmtrace_output_position(struct vcpu *v, uint64_t *pos)
return v->arch.hvm.vmx.ipt_active;
}
+static int vmtrace_reset(struct vcpu *v)
+{
+ if ( !v->arch.hvm.vmx.ipt_active )
+ return -EINVAL;
+
+ v->arch.msrs->rtit.output_offset = 0;
+ v->arch.msrs->rtit.status = 0;
+ return 0;
+}
+
static struct hvm_function_table __initdata vmx_function_table = {
.name = "VMX",
.cpu_up_prepare = vmx_cpu_up_prepare,
@@ -2451,6 +2461,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
.vmtrace_output_position = vmtrace_output_position,
.vmtrace_set_option = vmtrace_set_option,
.vmtrace_get_option = vmtrace_get_option,
+ .vmtrace_reset = vmtrace_reset,
.tsc_scaling = {
.max_ratio = VMX_TSC_MULTIPLIER_MAX,
},
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index adaeab4612..00ada05c10 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1632,6 +1632,8 @@ static int copy_vcpu_settings(struct domain *cd, const struct domain *d)
copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn);
}
+ hvm_vmtrace_reset(cd_vcpu);
+
/*
* TODO: to support VMs with PV interfaces copy additional
* settings here, such as PV timers.
@@ -1782,6 +1784,7 @@ static int fork(struct domain *cd, struct domain *d)
cd->max_pages = d->max_pages;
*cd->arch.cpuid = *d->arch.cpuid;
*cd->arch.msr = *d->arch.msr;
+ cd->vmtrace_size = d->vmtrace_size;
cd->parent = d;
}
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 960ec03917..150746de66 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -219,6 +219,7 @@ struct hvm_function_table {
int (*vmtrace_output_position)(struct vcpu *v, uint64_t *pos);
int (*vmtrace_set_option)(struct vcpu *v, uint64_t key, uint64_t value);
int (*vmtrace_get_option)(struct vcpu *v, uint64_t key, uint64_t *value);
+ int (*vmtrace_reset)(struct vcpu *v);
/*
* Parameters and callbacks for hardware-assisted TSC scaling,
@@ -696,6 +697,14 @@ static inline int hvm_vmtrace_get_option(
return -EOPNOTSUPP;
}
+static inline int hvm_vmtrace_reset(struct vcpu *v)
+{
+ if ( hvm_funcs.vmtrace_reset )
+ return hvm_funcs.vmtrace_reset(v);
+
+ return -EOPNOTSUPP;
+}
+
/*
* This must be defined as a macro instead of an inline function,
* because it uses 'struct vcpu' and 'struct domain' which have
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v9 10/11] x86/vm_event: Carry the vmtrace buffer position in vm_event
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (8 preceding siblings ...)
2021-02-01 23:27 ` [PATCH v9 09/11] xen/vmtrace: support for VM forks Andrew Cooper
@ 2021-02-01 23:27 ` Andrew Cooper
2021-02-01 23:27 ` [PATCH v9 11/11] x86/vm_event: add response flag to reset vmtrace buffer Andrew Cooper
2021-02-02 12:20 ` [PATCH v9 00/11] acquire_resource size and external IPT monitoring Ian Jackson
11 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:27 UTC (permalink / raw)
To: Xen-devel
Cc: Tamas K Lengyel, Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Jun Nakajima, Kevin Tian, Michał Leszczyński,
Tamas K Lengyel
From: Tamas K Lengyel <tamas.lengyel@intel.com>
Add vmtrace_pos field to x86 regs in vm_event. Initialized to ~0 if
vmtrace is not in use.
Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
v8:
* Use 'vmtrace' consistently.
v7:
* New
---
xen/arch/x86/vm_event.c | 3 +++
xen/include/public/vm_event.h | 7 +++++++
2 files changed, 10 insertions(+)
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 848d69c1b0..36272e9316 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -251,6 +251,9 @@ void vm_event_fill_regs(vm_event_request_t *req)
req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
req->data.regs.x86.dr6 = ctxt.dr6;
+
+ if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.vmtrace_pos) != 1 )
+ req->data.regs.x86.vmtrace_pos = ~0;
#endif
}
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 141ea024a3..147dc3ea73 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -223,6 +223,13 @@ struct vm_event_regs_x86 {
*/
uint64_t npt_base;
+ /*
+ * Current position in the vmtrace buffer, or ~0 if vmtrace is not active.
+ *
+ * For Intel Processor Trace, it is the upper half of MSR_RTIT_OUTPUT_MASK.
+ */
+ uint64_t vmtrace_pos;
+
uint32_t cs_base;
uint32_t ss_base;
uint32_t ds_base;
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v9 11/11] x86/vm_event: add response flag to reset vmtrace buffer
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (9 preceding siblings ...)
2021-02-01 23:27 ` [PATCH v9 10/11] x86/vm_event: Carry the vmtrace buffer position in vm_event Andrew Cooper
@ 2021-02-01 23:27 ` Andrew Cooper
2021-02-02 12:20 ` [PATCH v9 00/11] acquire_resource size and external IPT monitoring Ian Jackson
11 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-01 23:27 UTC (permalink / raw)
To: Xen-devel; +Cc: Tamas K Lengyel
From: Tamas K Lengyel <tamas.lengyel@intel.com>
Allow resetting the vmtrace buffer in response to a vm_event. This can be used
to optimize a use-case where detecting a looped vmtrace buffer is important.
Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/vm_event.c | 7 +++++++
xen/common/vm_event.c | 3 +++
xen/include/asm-arm/vm_event.h | 6 ++++++
xen/include/asm-x86/vm_event.h | 2 ++
xen/include/public/vm_event.h | 4 ++++
5 files changed, 22 insertions(+)
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 36272e9316..8f73a73e2e 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -300,6 +300,13 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
};
}
+void vm_event_reset_vmtrace(struct vcpu *v)
+{
+#ifdef CONFIG_HVM
+ hvm_vmtrace_reset(v);
+#endif
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 127f2d58f1..44d542f23e 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -424,6 +424,9 @@ static int vm_event_resume(struct domain *d, struct vm_event_domain *ved)
if ( rsp.flags & VM_EVENT_FLAG_GET_NEXT_INTERRUPT )
vm_event_monitor_next_interrupt(v);
+ if ( rsp.flags & VM_EVENT_FLAG_RESET_VMTRACE )
+ vm_event_reset_vmtrace(v);
+
if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
vm_event_vcpu_unpause(v);
}
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 14d1d341cc..abe7db1970 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -58,4 +58,10 @@ void vm_event_sync_event(struct vcpu *v, bool value)
/* Not supported on ARM. */
}
+static inline
+void vm_event_reset_vmtrace(struct vcpu *v)
+{
+ /* Not supported on ARM. */
+}
+
#endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 785e741fba..0756124075 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -54,4 +54,6 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp);
void vm_event_sync_event(struct vcpu *v, bool value);
+void vm_event_reset_vmtrace(struct vcpu *v);
+
#endif /* __ASM_X86_VM_EVENT_H__ */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 147dc3ea73..36135ba4f1 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -123,6 +123,10 @@
* Set if the event comes from a nested VM and thus npt_base is valid.
*/
#define VM_EVENT_FLAG_NESTED_P2M (1 << 12)
+/*
+ * Reset the vmtrace buffer (if vmtrace is enabled)
+ */
+#define VM_EVENT_FLAG_RESET_VMTRACE (1 << 13)
/*
* Reasons for the vm event request
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v9 00/11] acquire_resource size and external IPT monitoring
2021-02-01 23:26 [PATCH v9 00/11] acquire_resource size and external IPT monitoring Andrew Cooper
` (10 preceding siblings ...)
2021-02-01 23:27 ` [PATCH v9 11/11] x86/vm_event: add response flag to reset vmtrace buffer Andrew Cooper
@ 2021-02-02 12:20 ` Ian Jackson
2021-02-02 12:44 ` Andrew Cooper
2021-02-02 20:19 ` Andrew Cooper
11 siblings, 2 replies; 22+ messages in thread
From: Ian Jackson @ 2021-02-02 12:20 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné,
Wei Liu, Anthony PERARD, Jun Nakajima, Kevin Tian,
Michał Leszczyński, Tamas K Lengyel
Andrew Cooper writes ("[PATCH v9 00/11] acquire_resource size and external IPT monitoring"):
...
> Therefore, I'd like to request a release exception.
Thanks for this writeup.
There is discussion here of the upside of granting an exception, which
certainly seems substantial enough to give this serious consideration.
> It [is] fairly isolated in terms of interactions with the rest of
> Xen, so the changes of a showstopper affecting other features is
> very slim.
This is encouraging (optimistic, even) but very general. I would like
to see a frank and detailed assessment of the downside risks, ideally
based on analysis of the individual patches.
When I say a "frank and detailed assessment" I'm hoping to have a list
of the specific design and code changes that pose a risk to non-IPT
configurations, in decreasing order of risk.
For each one there should be brief discussion of the measures that
have exist to control that risk (eg, additional review, additional
testing), and a characterisation of the resulting risk (both in terms
of likelihood and severity of the resulting bug).
All risks that would come to a diligent reviewer's mind should be
mentioned and explicitly delath with, even if it is immediately clear
that they are not a real risk.
Do you think that would be feasible ? We would want to make a
decision ASAP so it would have to be done quickly too - in the next
few days and certainly by the end of the week.
Since you mentioned patch 1 and asserted it didn't need a release-ack,
I looked at it in a little more detail. It seems to contain a
moderate amount of (fairly localised) restructuring. IDK whether
XENMEM_acquire_resource is used by non-IPT configurations but I didn't
see an assertion anywhere that it isn't.
I appreciate that whether something is "straightforward" on the one
hand, vs involving "substantial refactoring" on the ohter, or this is
a matter of judgement, which I have left up to the commiters during
this part of the freeze. But for the record my view is that this
patch is not a "straightforward bugfix" and needs a release ack.
To give you an idea of what kind of thing I am looking for in a risk
assessment, I have written one up for
[PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter
Ideally I would like to go through a similar process for the other
patches.
I appreciate that this is rather a more throrough process than we have
adopted in the past.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v9 00/11] acquire_resource size and external IPT monitoring
2021-02-02 12:20 ` [PATCH v9 00/11] acquire_resource size and external IPT monitoring Ian Jackson
@ 2021-02-02 12:44 ` Andrew Cooper
2021-02-02 20:19 ` Andrew Cooper
1 sibling, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2021-02-02 12:44 UTC (permalink / raw)
To: Ian Jackson
Cc: Xen-devel, Jan Beulich, Roger Pau Monné,
Wei Liu, Anthony PERARD, Jun Nakajima, Kevin Tian,
Michał Leszczyński, Tamas K Lengyel
On 02/02/2021 12:20, Ian Jackson wrote:
> Since you mentioned patch 1 and asserted it didn't need a release-ack,
> I looked at it in a little more detail. It seems to contain a
> moderate amount of (fairly localised) restructuring. IDK whether
> XENMEM_acquire_resource is used by non-IPT configurations but I didn't
> see an assertion anywhere that it isn't.
Acquire resource is used by Qemu/demu/varstored/etc (for IO emulation)
and the the domain builder (seeding the grant table with
xenstore/console details).
None of these usecases used a size calculation, and made blind mapping
calls of 1 page in size.
IPT is the first usecase to want to map more than a single page in one go.
> I appreciate that whether something is "straightforward" on the one
> hand, vs involving "substantial refactoring" on the ohter, or this is
> a matter of judgement, which I have left up to the commiters during
> this part of the freeze. But for the record my view is that this
> patch is not a "straightforward bugfix" and needs a release ack.
I have extensive testing, demonstrating the bug already present in
staging (unable to map the guests whole grant table in default
configurations), and demonstrating the correctness of the fix.
Some of this testing (specifically, the toos/tests/* binary) is
something I plan to fix up over the ARM IOERQ and other series, and
submit later this week. It will demonstrate the current bug in staging,
and show it fixed with patch 1 committed. (This is something I want to
become an autotest in due course.)
Other parts of this testing cannot be submitted. To get the compat
layer correct, I needed an XTF test and modified Xen which had a known
pattern, to check the marshalling logic didn't lose anything when a
continuation hit an interesting. I can talk you through these tests and
assert that I have run them, but its not testing logic we can commit
into Xen, and its not anything which gets tested by OSSTest because we
don't test 32bit PVH dom0's in anger.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v9 00/11] acquire_resource size and external IPT monitoring
2021-02-02 12:20 ` [PATCH v9 00/11] acquire_resource size and external IPT monitoring Ian Jackson
2021-02-02 12:44 ` Andrew Cooper
@ 2021-02-02 20:19 ` Andrew Cooper
2021-02-05 15:36 ` [PATCH v9 00/11] acquire_resource size and external IPT monitoring [and 1 more messages] Ian Jackson
1 sibling, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2021-02-02 20:19 UTC (permalink / raw)
To: Ian Jackson
Cc: Xen-devel, Jan Beulich, Roger Pau Monné,
Wei Liu, Anthony PERARD, Jun Nakajima, Kevin Tian,
Michał Leszczyński, Tamas K Lengyel
On 02/02/2021 12:20, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH v9 00/11] acquire_resource size and external IPT monitoring"):
> ...
>> Therefore, I'd like to request a release exception.
> Thanks for this writeup.
>
> There is discussion here of the upside of granting an exception, which
> certainly seems substantial enough to give this serious consideration.
>
>> It [is] fairly isolated in terms of interactions with the rest of
>> Xen, so the changes of a showstopper affecting other features is
>> very slim.
> This is encouraging (optimistic, even) but very general. I would like
> to see a frank and detailed assessment of the downside risks, ideally
> based on analysis of the individual patches.
>
> When I say a "frank and detailed assessment" I'm hoping to have a list
> of the specific design and code changes that pose a risk to non-IPT
> configurations, in decreasing order of risk.
>
> For each one there should be brief discussion of the measures that
> have exist to control that risk (eg, additional review, additional
> testing), and a characterisation of the resulting risk (both in terms
> of likelihood and severity of the resulting bug).
>
> All risks that would come to a diligent reviewer's mind should be
> mentioned and explicitly delath with, even if it is immediately clear
> that they are not a real risk.
>
> Do you think that would be feasible ? We would want to make a
> decision ASAP so it would have to be done quickly too - in the next
> few days and certainly by the end of the week.
Honestly, I think this is an unreasonably large paperwork expectation,
particularly for changes this-clearly isolated in terms of functionality.
I'm going to explicitly disregard build/compile issues because we're not
even at code freeze or -rc1 yet, with multiple weeks yet before any
potential release, and loads of tooling.
Patch 2 adds a new domain creation parameter, which is an internal
tools/xen interface. Default is off, and it needs explicit opting in to
(patch 3), so it will get all the testing it needs in an OSSTest smoke run.
This patch does introduce one new use of a preexisting refcounting
pattern currently under discussion. It many leak memory in theoretical
circumstances, not practical ones. Work to figure out how to unbreak
this pattern is in progress, and orthogonal, and needs applying uniformly
Patch 4 adds a new resource type, which is an API/ABI with userspace.
It is a new type/index so has no current users.
Patch 5 adds enumeration for the IPT feature in hardware, as well as
context switching logic. All context switching changes are behind an
opt-in flag, so a smoke run will be sufficient to prove no adverse
interaction in !vmtrace case.
Patch 6 adds a new domctl and subops for controlling vmtrace. All brand
new functionality with no users, and bounded by the opt-ins from patch 2
and 5.
Patch 7 adds libxc library functions wrapping the domctl of patch 6. No
users.
Patch 8 is example code demonstrating how to use all of the new
functionality. It is built, but not installed.
Patch 9 extends the existing VMFork feature to cope with VMs configured
with this new functionality. It is a no-op for regular VMs.
Patch 10 extends vm_event requests with additional optional metadata
about the tail pointer of data in the vmtrace buffer. Doesn't alter the
behaviour for regular VMs.
Patch 11 extends vm_event responses with an optional request to reset
the vmtrace buffer position. No users, and a no-op for regular VMs.
All of this new functionality is off-by-default and needs an explicit
opt in, for any behavioural changes to occur. While there is no
guarantee that the implementation of the new functionality is perfect,
the development of it has found and fixed a whole slew bugs elsewhere in
Xen, and the new functionality does have extensive testing itself.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v9 00/11] acquire_resource size and external IPT monitoring [and 1 more messages]
2021-02-02 20:19 ` Andrew Cooper
@ 2021-02-05 15:36 ` Ian Jackson
0 siblings, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2021-02-05 15:36 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné,
Wei Liu, Anthony PERARD, Jun Nakajima, Kevin Tian,
Michał Leszczyński, Tamas K Lengyel, George Dunlap,
Stefano Stabellini, Julien Grall, Paul Durrant,
Hubert Jasudowicz
Thanks, Andy, for those writeups. I still have substantial misgivings
I don't feel confident. However, I don't think continuing attempts to
try to understand and/or mitigate this risk will be helpful. I need
to make a decision now.
I think there are significant downsides to either choice here. At
this stage of the freeze I am going to err on the side of saying
"yes", so, for the whole series:
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Provied this is committed by the end of Monday at the very latest. I
would appreciate it if it could be committed today.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread