* [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-21 6:53 ` Jan Beulich
2021-09-24 14:51 ` Dario Faggioli
2021-09-20 17:25 ` [PATCH v2 02/12] xen/memory: Remove tail padding from TRC_MEM_* records Andrew Cooper
` (15 subsequent siblings)
16 siblings, 2 replies; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall, Dario Faggioli,
Meng Xu
In the case that 'extra' isn't a multiple of uint32_t, the calculation rounds
the number of bytes up, causing later logic to read unrelated bytes beyond the
end of the object.
Also, asserting that the object is within TRACE_EXTRA_MAX, but truncating it
in release builds is rude. Instead, reject any out-of-spec records, leaving
enough of a message to identify the faulty caller.
There is one buggy race record, TRC_RTDS_BUDGET_BURN. As it must remain
__packed (as cur_budget is misaligned), change bool has_extratime to uint32_t
to compensate.
The new printk() can also be hit by HVMOP_xentrace, although no over-read is
possible. This has no business being a hypercall in the first place, as it
can't be used outside of custom local debugging, so extend the uint32_t
requirement to HVMOP_xentrace too.
Signed-off-by: Andrew Cooper <andrew.cooper3@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: Dario Faggioli <dfaggioli@suse.com>
CC: Meng Xu <mengxu@cis.upenn.edu>
v2:
* Adjust HVMOP_xentrace to avoid hitting the printk()
* Fix TRC_RTDS_BUDGET_BURN
* Adjust wording in __trace_var()
---
xen/arch/x86/hvm/hvm.c | 5 +++--
xen/common/sched/rt.c | 24 +++++++++++++-----------
xen/common/trace.c | 21 ++++++++++-----------
3 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7b48a1b925bb..09cf6330ad26 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5063,8 +5063,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( copy_from_guest(&tr, arg, 1 ) )
return -EFAULT;
- if ( tr.extra_bytes > sizeof(tr.extra)
- || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
+ if ( tr.extra_bytes % sizeof(uint32_t) ||
+ tr.extra_bytes > sizeof(tr.extra) ||
+ tr.event >> TRC_SUBCLS_SHIFT )
return -EINVAL;
/* Cycles will be taken at the vmexit and vmenter */
diff --git a/xen/common/sched/rt.c b/xen/common/sched/rt.c
index c24cd2ac3200..c58edca0de84 100644
--- a/xen/common/sched/rt.c
+++ b/xen/common/sched/rt.c
@@ -968,18 +968,20 @@ burn_budget(const struct scheduler *ops, struct rt_unit *svc, s_time_t now)
/* TRACE */
{
struct __packed {
- unsigned unit:16, dom:16;
+ uint16_t unit, dom;
uint64_t cur_budget;
- int delta;
- unsigned priority_level;
- bool has_extratime;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- d.cur_budget = (uint64_t) svc->cur_budget;
- d.delta = delta;
- d.priority_level = svc->priority_level;
- d.has_extratime = svc->flags & RTDS_extratime;
+ uint32_t delta;
+ uint32_t priority_level;
+ uint32_t has_extratime;
+ } d = {
+ .unit = svc->unit->unit_id,
+ .dom = svc->unit->domain->domain_id,
+ .cur_budget = svc->cur_budget,
+ .delta = delta,
+ .priority_level = svc->priority_level,
+ .has_extratime = !!(svc->flags & RTDS_extratime),
+ };
+
trace_var(TRC_RTDS_BUDGET_BURN, 1,
sizeof(d),
(unsigned char *) &d);
diff --git a/xen/common/trace.c b/xen/common/trace.c
index a2a389a1c7c3..4297ff505fb9 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -686,22 +686,21 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
unsigned long flags;
u32 bytes_to_tail, bytes_to_wrap;
unsigned int rec_size, total_size;
- unsigned int extra_word;
bool_t started_below_highwater;
if( !tb_init_done )
return;
- /* Convert byte count into word count, rounding up */
- extra_word = (extra / sizeof(u32));
- if ( (extra % sizeof(u32)) != 0 )
- extra_word++;
-
- ASSERT(extra_word <= TRACE_EXTRA_MAX);
- extra_word = min_t(int, extra_word, TRACE_EXTRA_MAX);
-
- /* Round size up to nearest word */
- extra = extra_word * sizeof(u32);
+ /*
+ * extra data needs to be an exact multiple of uint32_t to prevent the
+ * later logic over-reading the object. Reject out-of-spec records. Any
+ * failure here is an error in the caller.
+ */
+ if ( extra % sizeof(uint32_t) ||
+ extra / sizeof(uint32_t) > TRACE_EXTRA_MAX )
+ return printk_once(XENLOG_WARNING
+ "Trace event %#x bad size %u, discarding\n",
+ event, extra);
if ( (tb_event_mask & event) == 0 )
return;
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-20 17:25 ` [PATCH v2 01/12] xen/trace: Don't over-read trace objects Andrew Cooper
@ 2021-09-21 6:53 ` Jan Beulich
2021-09-21 17:51 ` Andrew Cooper
2021-09-24 14:51 ` Dario Faggioli
1 sibling, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-09-21 6:53 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Dario Faggioli, Meng Xu, Xen-devel
On 20.09.2021 19:25, Andrew Cooper wrote:
> In the case that 'extra' isn't a multiple of uint32_t, the calculation rounds
> the number of bytes up, causing later logic to read unrelated bytes beyond the
> end of the object.
>
> Also, asserting that the object is within TRACE_EXTRA_MAX, but truncating it
> in release builds is rude. Instead, reject any out-of-spec records, leaving
> enough of a message to identify the faulty caller.
>
> There is one buggy race record, TRC_RTDS_BUDGET_BURN. As it must remain
Nit: I guess s/race/trace/ ?
> __packed (as cur_budget is misaligned), change bool has_extratime to uint32_t
> to compensate.
>
> The new printk() can also be hit by HVMOP_xentrace, although no over-read is
> possible. This has no business being a hypercall in the first place, as it
> can't be used outside of custom local debugging, so extend the uint32_t
> requirement to HVMOP_xentrace too.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Two remarks (plus further not directly related ones), nevertheless:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5063,8 +5063,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( copy_from_guest(&tr, arg, 1 ) )
> return -EFAULT;
>
> - if ( tr.extra_bytes > sizeof(tr.extra)
> - || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
> + if ( tr.extra_bytes % sizeof(uint32_t) ||
> + tr.extra_bytes > sizeof(tr.extra) ||
> + tr.event >> TRC_SUBCLS_SHIFT )
> return -EINVAL;
Despite this being a function that supposedly no-one is to really
use, you're breaking the interface here when really there would be a
way to be backwards compatible: Instead of failing, pad the data to
a 32-bit boundary. As the interface struct is large enough, this
would look to be as simple as a memset() plus aligning extra_bytes
upwards. Otherwise the deliberate breaking of potential existing
callers wants making explicit in the respective paragraph of the
description.
As an aside I think at the very least the case block wants enclosing
in "#ifdef CONFIG_TRACEBUFFER", such that builds without the support
would indicate so to callers (albeit that indication would then be
accompanied by a bogus log message in debug builds).
Seeing the adjacent HVMOP_get_time I also wonder who the intended
users of that one are.
> --- a/xen/common/sched/rt.c
> +++ b/xen/common/sched/rt.c
> @@ -968,18 +968,20 @@ burn_budget(const struct scheduler *ops, struct rt_unit *svc, s_time_t now)
> /* TRACE */
> {
> struct __packed {
> - unsigned unit:16, dom:16;
> + uint16_t unit, dom;
> uint64_t cur_budget;
> - int delta;
> - unsigned priority_level;
> - bool has_extratime;
> - } d;
> - d.dom = svc->unit->domain->domain_id;
> - d.unit = svc->unit->unit_id;
> - d.cur_budget = (uint64_t) svc->cur_budget;
> - d.delta = delta;
> - d.priority_level = svc->priority_level;
> - d.has_extratime = svc->flags & RTDS_extratime;
> + uint32_t delta;
The original field was plain int, and aiui for a valid reason. I
don't see why you couldn't use int32_t here.
Feel free to retain the R-b if you decide to make the two suggested
changes which are directly related to your change here.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-21 6:53 ` Jan Beulich
@ 2021-09-21 17:51 ` Andrew Cooper
2021-09-22 7:01 ` Jan Beulich
0 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-21 17:51 UTC (permalink / raw)
To: Jan Beulich
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Dario Faggioli, Meng Xu, Xen-devel
On 21/09/2021 07:53, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
>> In the case that 'extra' isn't a multiple of uint32_t, the calculation rounds
>> the number of bytes up, causing later logic to read unrelated bytes beyond the
>> end of the object.
>>
>> Also, asserting that the object is within TRACE_EXTRA_MAX, but truncating it
>> in release builds is rude. Instead, reject any out-of-spec records, leaving
>> enough of a message to identify the faulty caller.
>>
>> There is one buggy race record, TRC_RTDS_BUDGET_BURN. As it must remain
> Nit: I guess s/race/trace/ ?
Oops yes.
>
>> __packed (as cur_budget is misaligned), change bool has_extratime to uint32_t
>> to compensate.
>>
>> The new printk() can also be hit by HVMOP_xentrace, although no over-read is
>> possible. This has no business being a hypercall in the first place, as it
>> can't be used outside of custom local debugging, so extend the uint32_t
>> requirement to HVMOP_xentrace too.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Two remarks (plus further not directly related ones), nevertheless:
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5063,8 +5063,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>> if ( copy_from_guest(&tr, arg, 1 ) )
>> return -EFAULT;
>>
>> - if ( tr.extra_bytes > sizeof(tr.extra)
>> - || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
>> + if ( tr.extra_bytes % sizeof(uint32_t) ||
>> + tr.extra_bytes > sizeof(tr.extra) ||
>> + tr.event >> TRC_SUBCLS_SHIFT )
>> return -EINVAL;
> Despite this being a function that supposedly no-one is to really
> use, you're breaking the interface here when really there would be a
> way to be backwards compatible: Instead of failing, pad the data to
> a 32-bit boundary. As the interface struct is large enough, this
> would look to be as simple as a memset() plus aligning extra_bytes
> upwards. Otherwise the deliberate breaking of potential existing
> callers wants making explicit in the respective paragraph of the
> description.
It is discussed, along with a justification for why an ABI change is fine.
But I could do
tr.extra_bytes = ROUNDUP(tr.extra_bytes, sizeof(uint32_t));
if you'd really prefer.
> As an aside I think at the very least the case block wants enclosing
> in "#ifdef CONFIG_TRACEBUFFER", such that builds without the support
> would indicate so to callers (albeit that indication would then be
> accompanied by a bogus log message in debug builds).
That message really needs deleting.
As a better alternative,
if ( !IS_ENABLED(CONFIG_TRACEBUFFER) )
return -EOPNOTSUPP;
The call to __trace_var() is safe in !CONFIG_TRACEBUFFER builds.
>
> Seeing the adjacent HVMOP_get_time I also wonder who the intended
> users of that one are.
There is a very large amount of junk in hvm_op(), and to a first
approximation, I would include HVMOP_get_time in that category.
But c/s b91391491c58ac40a935e10cf0703b87d8733c38 explains why it is
necessary. This just goes to demonstrate how broken our time handling
is. I'll add this to the pile of things needing fixing in ABI-v2.
>
>> --- a/xen/common/sched/rt.c
>> +++ b/xen/common/sched/rt.c
>> @@ -968,18 +968,20 @@ burn_budget(const struct scheduler *ops, struct rt_unit *svc, s_time_t now)
>> /* TRACE */
>> {
>> struct __packed {
>> - unsigned unit:16, dom:16;
>> + uint16_t unit, dom;
>> uint64_t cur_budget;
>> - int delta;
>> - unsigned priority_level;
>> - bool has_extratime;
>> - } d;
>> - d.dom = svc->unit->domain->domain_id;
>> - d.unit = svc->unit->unit_id;
>> - d.cur_budget = (uint64_t) svc->cur_budget;
>> - d.delta = delta;
>> - d.priority_level = svc->priority_level;
>> - d.has_extratime = svc->flags & RTDS_extratime;
>> + uint32_t delta;
> The original field was plain int, and aiui for a valid reason. I
> don't see why you couldn't use int32_t here.
delta can't be negative, because there is a check earlier in the function.
What is a problem is the 63=>32 bit truncation, and uint32_t here is
half as bad as int32_t.
~Andrew
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-21 17:51 ` Andrew Cooper
@ 2021-09-22 7:01 ` Jan Beulich
2021-09-22 12:58 ` Andrew Cooper
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-09-22 7:01 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Dario Faggioli, Meng Xu, Xen-devel
On 21.09.2021 19:51, Andrew Cooper wrote:
> On 21/09/2021 07:53, Jan Beulich wrote:
>> On 20.09.2021 19:25, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -5063,8 +5063,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>> if ( copy_from_guest(&tr, arg, 1 ) )
>>> return -EFAULT;
>>>
>>> - if ( tr.extra_bytes > sizeof(tr.extra)
>>> - || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
>>> + if ( tr.extra_bytes % sizeof(uint32_t) ||
>>> + tr.extra_bytes > sizeof(tr.extra) ||
>>> + tr.event >> TRC_SUBCLS_SHIFT )
>>> return -EINVAL;
>> Despite this being a function that supposedly no-one is to really
>> use, you're breaking the interface here when really there would be a
>> way to be backwards compatible: Instead of failing, pad the data to
>> a 32-bit boundary. As the interface struct is large enough, this
>> would look to be as simple as a memset() plus aligning extra_bytes
>> upwards. Otherwise the deliberate breaking of potential existing
>> callers wants making explicit in the respective paragraph of the
>> description.
>
> It is discussed, along with a justification for why an ABI change is fine.
What you say is "This has no business being a hypercall in the first
place", yet to me that's not a justification to break an interface.
It is part of the ABI, so disallowing what was previously allowed
may break people's (questionable, yes) code.
> But I could do
>
> tr.extra_bytes = ROUNDUP(tr.extra_bytes, sizeof(uint32_t));
>
> if you'd really prefer.
I would, indeed, and as said ideally alongside clearing the excess
bytes in the buffer.
>> As an aside I think at the very least the case block wants enclosing
>> in "#ifdef CONFIG_TRACEBUFFER", such that builds without the support
>> would indicate so to callers (albeit that indication would then be
>> accompanied by a bogus log message in debug builds).
>
> That message really needs deleting.
>
> As a better alternative,
>
> if ( !IS_ENABLED(CONFIG_TRACEBUFFER) )
> return -EOPNOTSUPP;
>
> The call to __trace_var() is safe in !CONFIG_TRACEBUFFER builds.
Fine with me (I'm inclined to say of course).
>> Seeing the adjacent HVMOP_get_time I also wonder who the intended
>> users of that one are.
>
> There is a very large amount of junk in hvm_op(), and to a first
> approximation, I would include HVMOP_get_time in that category.
>
> But c/s b91391491c58ac40a935e10cf0703b87d8733c38 explains why it is
> necessary. This just goes to demonstrate how broken our time handling
> is. I'll add this to the pile of things needing fixing in ABI-v2.
Urgh.
>>> --- a/xen/common/sched/rt.c
>>> +++ b/xen/common/sched/rt.c
>>> @@ -968,18 +968,20 @@ burn_budget(const struct scheduler *ops, struct rt_unit *svc, s_time_t now)
>>> /* TRACE */
>>> {
>>> struct __packed {
>>> - unsigned unit:16, dom:16;
>>> + uint16_t unit, dom;
>>> uint64_t cur_budget;
>>> - int delta;
>>> - unsigned priority_level;
>>> - bool has_extratime;
>>> - } d;
>>> - d.dom = svc->unit->domain->domain_id;
>>> - d.unit = svc->unit->unit_id;
>>> - d.cur_budget = (uint64_t) svc->cur_budget;
>>> - d.delta = delta;
>>> - d.priority_level = svc->priority_level;
>>> - d.has_extratime = svc->flags & RTDS_extratime;
>>> + uint32_t delta;
>> The original field was plain int, and aiui for a valid reason. I
>> don't see why you couldn't use int32_t here.
>
> delta can't be negative, because there is a check earlier in the function.
Oh, yes, didn't spot that.
> What is a problem is the 63=>32 bit truncation, and uint32_t here is
> half as bad as int32_t.
Agreed. Whether the truncation is an issue in practice is questionable,
as I wouldn't expect budget to be consumed in multiple-second individual
steps. But I didn't check whether this scheduler might allow a vCPU to
run for this long all in one go.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-22 7:01 ` Jan Beulich
@ 2021-09-22 12:58 ` Andrew Cooper
2021-09-22 13:32 ` Jan Beulich
2021-09-24 14:35 ` Dario Faggioli
0 siblings, 2 replies; 52+ messages in thread
From: Andrew Cooper @ 2021-09-22 12:58 UTC (permalink / raw)
To: Jan Beulich
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Dario Faggioli, Meng Xu, Xen-devel
On 22/09/2021 08:01, Jan Beulich wrote:
> On 21.09.2021 19:51, Andrew Cooper wrote:
>> On 21/09/2021 07:53, Jan Beulich wrote:
>>> On 20.09.2021 19:25, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -5063,8 +5063,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>> if ( copy_from_guest(&tr, arg, 1 ) )
>>>> return -EFAULT;
>>>>
>>>> - if ( tr.extra_bytes > sizeof(tr.extra)
>>>> - || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
>>>> + if ( tr.extra_bytes % sizeof(uint32_t) ||
>>>> + tr.extra_bytes > sizeof(tr.extra) ||
>>>> + tr.event >> TRC_SUBCLS_SHIFT )
>>>> return -EINVAL;
>>> Despite this being a function that supposedly no-one is to really
>>> use, you're breaking the interface here when really there would be a
>>> way to be backwards compatible: Instead of failing, pad the data to
>>> a 32-bit boundary. As the interface struct is large enough, this
>>> would look to be as simple as a memset() plus aligning extra_bytes
>>> upwards. Otherwise the deliberate breaking of potential existing
>>> callers wants making explicit in the respective paragraph of the
>>> description.
>> It is discussed, along with a justification for why an ABI change is fine.
> What you say is "This has no business being a hypercall in the first
> place", yet to me that's not a justification to break an interface.
No, but "cannot be used outside of custom debugging" means there are no
users in practice, and therefore it really doesn't matter.
> It is part of the ABI, so disallowing what was previously allowed
> may break people's (questionable, yes) code.
>
>> But I could do
>>
>> tr.extra_bytes = ROUNDUP(tr.extra_bytes, sizeof(uint32_t));
>>
>> if you'd really prefer.
> I would, indeed, and as said ideally alongside clearing the excess
> bytes in the buffer.
Why? The entire structure is copied out of guest memory, with a fixed size.
It's not Xen's fault/problem if the VM didn't initialise it correctly,
and an explicit ROUNDUP() here maintains the current behaviour.
>>>> --- a/xen/common/sched/rt.c
>>>> +++ b/xen/common/sched/rt.c
>>>> @@ -968,18 +968,20 @@ burn_budget(const struct scheduler *ops, struct rt_unit *svc, s_time_t now)
>>>> /* TRACE */
>>>> {
>>>> struct __packed {
>>>> - unsigned unit:16, dom:16;
>>>> + uint16_t unit, dom;
>>>> uint64_t cur_budget;
>>>> - int delta;
>>>> - unsigned priority_level;
>>>> - bool has_extratime;
>>>> - } d;
>>>> - d.dom = svc->unit->domain->domain_id;
>>>> - d.unit = svc->unit->unit_id;
>>>> - d.cur_budget = (uint64_t) svc->cur_budget;
>>>> - d.delta = delta;
>>>> - d.priority_level = svc->priority_level;
>>>> - d.has_extratime = svc->flags & RTDS_extratime;
>>>> + uint32_t delta;
>>> The original field was plain int, and aiui for a valid reason. I
>>> don't see why you couldn't use int32_t here.
>> delta can't be negative, because there is a check earlier in the function.
> Oh, yes, didn't spot that.
>
>> What is a problem is the 63=>32 bit truncation, and uint32_t here is
>> half as bad as int32_t.
> Agreed. Whether the truncation is an issue in practice is questionable,
> as I wouldn't expect budget to be consumed in multiple-second individual
> steps. But I didn't check whether this scheduler might allow a vCPU to
> run for this long all in one go.
I expect it's marginal too. Honestly, its not a bug I care to fix right
about now. I could leave a /* TODO: truncation? */ in place so whomever
encounters weird behaviour from this trace record has a bit more help of
where to look?
~Andrew
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-22 12:58 ` Andrew Cooper
@ 2021-09-22 13:32 ` Jan Beulich
2021-09-24 14:35 ` Dario Faggioli
1 sibling, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-09-22 13:32 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Dario Faggioli, Meng Xu, Xen-devel
On 22.09.2021 14:58, Andrew Cooper wrote:
> On 22/09/2021 08:01, Jan Beulich wrote:
>> On 21.09.2021 19:51, Andrew Cooper wrote:
>>> On 21/09/2021 07:53, Jan Beulich wrote:
>>>> On 20.09.2021 19:25, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -5063,8 +5063,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>> if ( copy_from_guest(&tr, arg, 1 ) )
>>>>> return -EFAULT;
>>>>>
>>>>> - if ( tr.extra_bytes > sizeof(tr.extra)
>>>>> - || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
>>>>> + if ( tr.extra_bytes % sizeof(uint32_t) ||
>>>>> + tr.extra_bytes > sizeof(tr.extra) ||
>>>>> + tr.event >> TRC_SUBCLS_SHIFT )
>>>>> return -EINVAL;
>>>> Despite this being a function that supposedly no-one is to really
>>>> use, you're breaking the interface here when really there would be a
>>>> way to be backwards compatible: Instead of failing, pad the data to
>>>> a 32-bit boundary. As the interface struct is large enough, this
>>>> would look to be as simple as a memset() plus aligning extra_bytes
>>>> upwards. Otherwise the deliberate breaking of potential existing
>>>> callers wants making explicit in the respective paragraph of the
>>>> description.
>>> It is discussed, along with a justification for why an ABI change is fine.
>> What you say is "This has no business being a hypercall in the first
>> place", yet to me that's not a justification to break an interface.
>
> No, but "cannot be used outside of custom debugging" means there are no
> users in practice, and therefore it really doesn't matter.
>
>> It is part of the ABI, so disallowing what was previously allowed
>> may break people's (questionable, yes) code.
>>
>>> But I could do
>>>
>>> tr.extra_bytes = ROUNDUP(tr.extra_bytes, sizeof(uint32_t));
>>>
>>> if you'd really prefer.
>> I would, indeed, and as said ideally alongside clearing the excess
>> bytes in the buffer.
>
> Why? The entire structure is copied out of guest memory, with a fixed size.
>
> It's not Xen's fault/problem if the VM didn't initialise it correctly,
> and an explicit ROUNDUP() here maintains the current behaviour.
What I don't understand is what you derive "didn't initialise it correctly"
from. The public header says nothing. The data field being of type uint8_t[]
may very well suggest that any size is fine. Propagating rubbish instead of
predictable values (zeroes) seems wrong to me.
Anyway - I'm not going to insist; I merely think we should be as forgiving
as possible in situations like this.
>>>>> --- a/xen/common/sched/rt.c
>>>>> +++ b/xen/common/sched/rt.c
>>>>> @@ -968,18 +968,20 @@ burn_budget(const struct scheduler *ops, struct rt_unit *svc, s_time_t now)
>>>>> /* TRACE */
>>>>> {
>>>>> struct __packed {
>>>>> - unsigned unit:16, dom:16;
>>>>> + uint16_t unit, dom;
>>>>> uint64_t cur_budget;
>>>>> - int delta;
>>>>> - unsigned priority_level;
>>>>> - bool has_extratime;
>>>>> - } d;
>>>>> - d.dom = svc->unit->domain->domain_id;
>>>>> - d.unit = svc->unit->unit_id;
>>>>> - d.cur_budget = (uint64_t) svc->cur_budget;
>>>>> - d.delta = delta;
>>>>> - d.priority_level = svc->priority_level;
>>>>> - d.has_extratime = svc->flags & RTDS_extratime;
>>>>> + uint32_t delta;
>>>> The original field was plain int, and aiui for a valid reason. I
>>>> don't see why you couldn't use int32_t here.
>>> delta can't be negative, because there is a check earlier in the function.
>> Oh, yes, didn't spot that.
>>
>>> What is a problem is the 63=>32 bit truncation, and uint32_t here is
>>> half as bad as int32_t.
>> Agreed. Whether the truncation is an issue in practice is questionable,
>> as I wouldn't expect budget to be consumed in multiple-second individual
>> steps. But I didn't check whether this scheduler might allow a vCPU to
>> run for this long all in one go.
>
> I expect it's marginal too. Honestly, its not a bug I care to fix right
> about now. I could leave a /* TODO: truncation? */ in place so whomever
> encounters weird behaviour from this trace record has a bit more help of
> where to look?
Would seem reasonable to me, but really needs to be answered by the
maintainers of this code.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-22 12:58 ` Andrew Cooper
2021-09-22 13:32 ` Jan Beulich
@ 2021-09-24 14:35 ` Dario Faggioli
1 sibling, 0 replies; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 14:35 UTC (permalink / raw)
To: Jan Beulich, andrew.cooper3
Cc: julien, George.Dunlap, sstabellini, xen-devel, iwj, wl, mengxu
[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]
On Wed, 2021-09-22 at 13:58 +0100, Andrew Cooper wrote:
> On 22/09/2021 08:01, Jan Beulich wrote:
>
> >
> > Agreed. Whether the truncation is an issue in practice is
> > questionable,
> > as I wouldn't expect budget to be consumed in multiple-second
> > individual
> > steps. But I didn't check whether this scheduler might allow a vCPU
> > to
> > run for this long all in one go.
>
> I expect it's marginal too.
>
It is indeed.
> Honestly, its not a bug I care to fix right
> about now. I could leave a /* TODO: truncation? */ in place so
> whomever
> encounters weird behaviour from this trace record has a bit more help
> of
> where to look?
>
Sure, that's fine for me.
Thanks and Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-20 17:25 ` [PATCH v2 01/12] xen/trace: Don't over-read trace objects Andrew Cooper
2021-09-21 6:53 ` Jan Beulich
@ 2021-09-24 14:51 ` Dario Faggioli
2021-09-27 7:51 ` Jan Beulich
1 sibling, 1 reply; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 14:51 UTC (permalink / raw)
To: andrew.cooper3, xen-devel
Cc: julien, George.Dunlap, sstabellini, Jan Beulich, iwj, wl, mengxu
[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]
On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
> There is one buggy race record, TRC_RTDS_BUDGET_BURN. As it must
> remain
> __packed (as cur_budget is misaligned), change bool has_extratime to
> uint32_t
> to compensate.
>
Mmm... maybe my understanding of data alignment inside structs is a bit
lacking, but what the actual issue here, and what would we need to do
to fix it (where, by fix, I mean us being able to get rid of the
`__packed`)?
If rearranging fields is not enough, we can think about making
priority_level and has_extratime smaller, or even combining them in
just one field and decode the information in xentrace.
Of course, I can send a patch for that myself, even as a followup of
this series when it's in, as soon as we agree about the best way
forward.
Thanks and Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-24 14:51 ` Dario Faggioli
@ 2021-09-27 7:51 ` Jan Beulich
2021-09-30 8:07 ` Dario Faggioli
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-09-27 7:51 UTC (permalink / raw)
To: Dario Faggioli
Cc: julien, George.Dunlap, sstabellini, iwj, wl, mengxu,
andrew.cooper3, xen-devel
On 24.09.2021 16:51, Dario Faggioli wrote:
> On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
>
>> There is one buggy race record, TRC_RTDS_BUDGET_BURN. As it must
>> remain
>> __packed (as cur_budget is misaligned), change bool has_extratime to
>> uint32_t
>> to compensate.
>>
> Mmm... maybe my understanding of data alignment inside structs is a bit
> lacking, but what the actual issue here, and what would we need to do
> to fix it (where, by fix, I mean us being able to get rid of the
> `__packed`)?
>
> If rearranging fields is not enough, we can think about making
> priority_level and has_extratime smaller, or even combining them in
> just one field and decode the information in xentrace.
I guess Andrew has tried to avoid re-arranging field order so that
the consumer side doesn't need to also change.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-27 7:51 ` Jan Beulich
@ 2021-09-30 8:07 ` Dario Faggioli
2021-12-03 16:29 ` Andrew Cooper
0 siblings, 1 reply; 52+ messages in thread
From: Dario Faggioli @ 2021-09-30 8:07 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, wl, iwj, sstabellini, mengxu, xen-devel, andrew.cooper3,
George.Dunlap
[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]
On Mon, 2021-09-27 at 09:51 +0200, Jan Beulich wrote:
> On 24.09.2021 16:51, Dario Faggioli wrote:
> > On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
> >
> > > There is one buggy race record, TRC_RTDS_BUDGET_BURN. As it must
> > > remain
> > > __packed (as cur_budget is misaligned), change bool has_extratime
> > > to
> > > uint32_t
> > > to compensate.
> > >
> > Mmm... maybe my understanding of data alignment inside structs is a
> > bit
> > lacking, but what the actual issue here, and what would we need to
> > do
> > to fix it (where, by fix, I mean us being able to get rid of the
> > `__packed`)?
> >
> > If rearranging fields is not enough, we can think about making
> > priority_level and has_extratime smaller, or even combining them in
> > just one field and decode the information in xentrace.
>
> I guess Andrew has tried to avoid re-arranging field order so that
> the consumer side doesn't need to also change.
>
Right, but is it really worth it, in this case?
Aren't we (very very likely) in control, by having them here in the
tree, of all the consumers? And is is this a stable ABI?
Also, both xentrace_format and xenalyze are being modified in this
series anyway...
Maybe there's still something I'm missing, but if we've getting rid of
those ugly bitfields and __packed attributes, it seems to me that doing
it completely --i.e., including in this case-- is worth the small
change in the tools.
Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects
2021-09-30 8:07 ` Dario Faggioli
@ 2021-12-03 16:29 ` Andrew Cooper
0 siblings, 0 replies; 52+ messages in thread
From: Andrew Cooper @ 2021-12-03 16:29 UTC (permalink / raw)
To: Dario Faggioli, Jan Beulich
Cc: julien, wl, iwj, sstabellini, mengxu, xen-devel, andrew.cooper3,
George.Dunlap
On 30/09/2021 09:07, Dario Faggioli wrote:
> On Mon, 2021-09-27 at 09:51 +0200, Jan Beulich wrote:
>> On 24.09.2021 16:51, Dario Faggioli wrote:
>>> On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
>>>
>>>> There is one buggy race record, TRC_RTDS_BUDGET_BURN. As it must
>>>> remain
>>>> __packed (as cur_budget is misaligned), change bool has_extratime
>>>> to
>>>> uint32_t
>>>> to compensate.
>>>>
>>> Mmm... maybe my understanding of data alignment inside structs is a
>>> bit
>>> lacking, but what the actual issue here, and what would we need to
>>> do
>>> to fix it (where, by fix, I mean us being able to get rid of the
>>> `__packed`)?
>>>
>>> If rearranging fields is not enough, we can think about making
>>> priority_level and has_extratime smaller, or even combining them in
>>> just one field and decode the information in xentrace.
>> I guess Andrew has tried to avoid re-arranging field order so that
>> the consumer side doesn't need to also change.
>>
> Right, but is it really worth it, in this case?
>
> Aren't we (very very likely) in control, by having them here in the
> tree, of all the consumers? And is is this a stable ABI?
>
> Also, both xentrace_format and xenalyze are being modified in this
> series anyway...
>
> Maybe there's still something I'm missing, but if we've getting rid of
> those ugly bitfields and __packed attributes, it seems to me that doing
> it completely --i.e., including in this case-- is worth the small
> change in the tools.
This patch needs backporting to stable trees. We shouldn't be changing
the ABI, even if its stability is unclear.
Which means patch 2 needs altering to avoid ABI changes. /sigh
~Andre
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 02/12] xen/memory: Remove tail padding from TRC_MEM_* records
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
2021-09-20 17:25 ` [PATCH v2 01/12] xen/trace: Don't over-read trace objects Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-24 16:50 ` Dario Faggioli
2021-09-20 17:25 ` [PATCH v2 03/12] xen/credit2: Remove tail padding from TRC_CSCHED2_* records Andrew Cooper
` (14 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall, Dario Faggioli
Four TRC_MEM_* records supply custom structures with tail padding, leaking
stack rubble into the trace buffer. Three of the records were fine in 32-bit
builds of Xen, due to the relaxed alignment of 64-bit integers, but
POD_SUPERPAGE_SPLITER was broken right from the outset.
We could pack the datastructures to remove the padding, but xentrace_format
has no way of rendering the upper half of a 16-bit field. Instead, expand all
16-bit fields to 32-bit.
For POD_SUPERPAGE_SPLINTER, introduce an order field as it is relevant
information, and to match DECREASE_RESERVATION, and so it doesn't require a
__packed attribute to drop tail padding.
Update xenalyze's structures to match, and introduce xentrace_format rendering
which was absent previously.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.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: Dario Faggioli <dfaggioli@suse.com>
The xentrace_format script isn't remotely Py3 compatible, and was another
script missed by our previous efforts.
---
tools/xentrace/formats | 4 ++++
tools/xentrace/xenalyze.c | 12 ++++++------
xen/arch/x86/mm/p2m-pod.c | 17 +++++++++--------
xen/common/memory.c | 4 ++--
4 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index deac4d8598b0..0fcc327a4078 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -136,6 +136,10 @@
0x0010f001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_map [ domid = %(1)d ]
0x0010f002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_unmap [ domid = %(1)d ]
0x0010f003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_transfer [ domid = %(1)d ]
+0x0010f005 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) decrease_reservation [ d%(3)d gfn 0x%(2)08x%(1)08x, order %(4)u ]
+0x0010f010 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) pod_populate [ d%(5)d gfn 0x%(2)08x%(1)08x => mfn 0x%(4)08x%(3)08x, order %(6)u ]
+0x0010f011 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) pod_zero_reclaim [ d%(5)d gfn 0x%(2)08x%(1)08x => mfn 0x%(4)08x%(3)08x, order %(6)u ]
+0x0010f012 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) pod_superpage_splinter [ d%(3)d gfn 0x%(2)08x%(1)08x, order %(4)u ]
0x00201001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ eip = 0x%(1)08x, eax = 0x%(2)08x ]
0x00201101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ rip = 0x%(2)08x%(1)08x, eax = 0x%(3)08x ]
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 5de167031e01..12dcca964645 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -8121,7 +8121,7 @@ void mem_pod_zero_reclaim_process(struct pcpu_info *p)
struct {
uint64_t gfn, mfn;
- int d:16,order:16;
+ uint32_t d, order;
} *r = (typeof(r))ri->d;
if ( v && v->hvm.vmexit_valid )
@@ -8171,7 +8171,7 @@ void mem_pod_populate_process(struct pcpu_info *p)
struct {
uint64_t gfn, mfn;
- int d:16,order:16;
+ uint32_t d, order;
} *r = (typeof(r))ri->d;
if ( opt.dump_all )
@@ -8204,14 +8204,14 @@ void mem_pod_superpage_splinter_process(struct pcpu_info *p)
struct {
uint64_t gfn;
- int d:16;
+ uint32_t d, order;
} *r = (typeof(r))ri->d;
if ( opt.dump_all )
{
- printf(" %s pod_spage_splinter d%d g %llx\n",
+ printf(" %s pod_spage_splinter d%d o%d g %"PRIx64"\n",
ri->dump_header,
- r->d, (unsigned long long)r->gfn);
+ r->d, r->order, r->gfn);
}
}
@@ -8255,7 +8255,7 @@ void mem_decrease_reservation_process(struct pcpu_info *p)
struct {
uint64_t gfn;
- int d:16,order:16;
+ uint32_t d, order;
} *r = (typeof(r))ri->d;
if ( opt.dump_all )
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 8abc57265c10..90f02ae765f6 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -819,8 +819,8 @@ p2m_pod_zero_check_superpage(struct p2m_domain *p2m, gfn_t gfn)
if ( tb_init_done )
{
struct {
- u64 gfn, mfn;
- int d:16,order:16;
+ uint64_t gfn, mfn;
+ uint32_t d, order;
} t;
t.gfn = gfn_x(gfn);
@@ -987,8 +987,8 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t *gfns, unsigned int count
if ( tb_init_done )
{
struct {
- u64 gfn, mfn;
- int d:16,order:16;
+ uint64_t gfn, mfn;
+ uint32_t d, order;
} t;
t.gfn = gfn_x(gfns[i]);
@@ -1217,8 +1217,8 @@ p2m_pod_demand_populate(struct p2m_domain *p2m, gfn_t gfn,
if ( tb_init_done )
{
struct {
- u64 gfn, mfn;
- int d:16,order:16;
+ uint64_t gfn, mfn;
+ uint32_t d, order;
} t;
t.gfn = gfn_x(gfn);
@@ -1260,12 +1260,13 @@ p2m_pod_demand_populate(struct p2m_domain *p2m, gfn_t gfn,
if ( tb_init_done )
{
struct {
- u64 gfn;
- int d:16;
+ uint64_t gfn;
+ uint32_t d, order;
} t;
t.gfn = gfn_x(gfn);
t.d = d->domain_id;
+ t.order = order;
__trace_var(TRC_MEM_POD_SUPERPAGE_SPLINTER, 0, sizeof(t), &t);
}
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 63642278fda9..8fd88ccb70bf 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -450,8 +450,8 @@ static void decrease_reservation(struct memop_args *a)
if ( tb_init_done )
{
struct {
- u64 gfn;
- int d:16,order:16;
+ uint64_t gfn;
+ uint32_t d, order;
} t;
t.gfn = gmfn;
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 02/12] xen/memory: Remove tail padding from TRC_MEM_* records
2021-09-20 17:25 ` [PATCH v2 02/12] xen/memory: Remove tail padding from TRC_MEM_* records Andrew Cooper
@ 2021-09-24 16:50 ` Dario Faggioli
0 siblings, 0 replies; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 16:50 UTC (permalink / raw)
To: andrew.cooper3, xen-devel
Cc: julien, George.Dunlap, sstabellini, Jan Beulich, iwj, wl
[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]
On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
> Four TRC_MEM_* records supply custom structures with tail padding,
> leaking
> stack rubble into the trace buffer. Three of the records were fine
> in 32-bit
> builds of Xen, due to the relaxed alignment of 64-bit integers, but
> POD_SUPERPAGE_SPLITER was broken right from the outset.
>
> We could pack the datastructures to remove the padding, but
> xentrace_format
> has no way of rendering the upper half of a 16-bit field. Instead,
> expand all
> 16-bit fields to 32-bit.
>
> For POD_SUPERPAGE_SPLINTER, introduce an order field as it is
> relevant
> information, and to match DECREASE_RESERVATION, and so it doesn't
> require a
> __packed attribute to drop tail padding.
>
> Update xenalyze's structures to match, and introduce xentrace_format
> rendering
> which was absent previously.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
Thanks and Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 03/12] xen/credit2: Remove tail padding from TRC_CSCHED2_* records
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
2021-09-20 17:25 ` [PATCH v2 01/12] xen/trace: Don't over-read trace objects Andrew Cooper
2021-09-20 17:25 ` [PATCH v2 02/12] xen/memory: Remove tail padding from TRC_MEM_* records Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-24 16:54 ` Dario Faggioli
2021-09-20 17:25 ` [PATCH v2 04/12] x86/hvm: Reduce stack usage from HVMTRACE_ND() Andrew Cooper
` (13 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall, Dario Faggioli
All three of these records have tail padding, leaking stack rubble into the
trace buffer. Introduce an explicit _pad field and have the compiler zero the
padding automatically.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.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: Dario Faggioli <dfaggioli@suse.com>
---
xen/common/sched/credit2.c | 47 +++++++++++++++++++++++++++-------------------
1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index 6396b38e044c..41312158ec63 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -1106,12 +1106,14 @@ _runq_assign(struct csched2_unit *svc, struct csched2_runqueue_data *rqd)
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- unsigned rqi:16;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- d.rqi=rqd->id;
+ uint16_t unit, dom;
+ uint16_t rqi, _pad;
+ } d = {
+ .unit = svc->unit->unit_id,
+ .dom = svc->unit->domain->domain_id,
+ .rqi = rqd->id,
+ };
+
__trace_var(TRC_CSCHED2_RUNQ_ASSIGN, 1,
sizeof(d),
(unsigned char *)&d);
@@ -1336,13 +1338,17 @@ update_runq_load(const struct scheduler *ops,
{
struct {
uint64_t rq_avgload, b_avgload;
- unsigned rq_load:16, rq_id:8, shift:8;
- } d;
- d.rq_id = rqd->id;
- d.rq_load = rqd->load;
- d.rq_avgload = rqd->avgload;
- d.b_avgload = rqd->b_avgload;
- d.shift = P;
+ uint16_t rq_load;
+ uint8_t rq_id, shift;
+ uint32_t _pad;
+ } d = {
+ .rq_avgload = rqd->avgload,
+ .b_avgload = rqd->b_avgload,
+ .rq_load = rqd->load,
+ .rq_id = rqd->id,
+ .shift = P,
+ };
+
__trace_var(TRC_CSCHED2_UPDATE_RUNQ_LOAD, 1,
sizeof(d),
(unsigned char *)&d);
@@ -2799,12 +2805,15 @@ static void balance_load(const struct scheduler *ops, int cpu, s_time_t now)
{
struct {
uint64_t lb_avgload, ob_avgload;
- unsigned lrq_id:16, orq_id:16;
- } d;
- d.lrq_id = st.lrqd->id;
- d.lb_avgload = st.lrqd->b_avgload;
- d.orq_id = st.orqd->id;
- d.ob_avgload = st.orqd->b_avgload;
+ uint16_t lrq_id, orq_id;
+ uint32_t _pad;
+ } d = {
+ .lb_avgload = st.lrqd->b_avgload,
+ .ob_avgload = st.orqd->b_avgload,
+ .lrq_id = st.lrqd->id,
+ .orq_id = st.orqd->id,
+ };
+
__trace_var(TRC_CSCHED2_LOAD_BALANCE, 1,
sizeof(d),
(unsigned char *)&d);
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 03/12] xen/credit2: Remove tail padding from TRC_CSCHED2_* records
2021-09-20 17:25 ` [PATCH v2 03/12] xen/credit2: Remove tail padding from TRC_CSCHED2_* records Andrew Cooper
@ 2021-09-24 16:54 ` Dario Faggioli
0 siblings, 0 replies; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 16:54 UTC (permalink / raw)
To: andrew.cooper3, xen-devel
Cc: julien, George.Dunlap, sstabellini, Jan Beulich, iwj, wl
[-- Attachment #1: Type: text/plain, Size: 708 bytes --]
On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
> All three of these records have tail padding, leaking stack rubble
> into the
> trace buffer. Introduce an explicit _pad field and have the compiler
> zero the
> padding automatically.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
Thanks and Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 04/12] x86/hvm: Reduce stack usage from HVMTRACE_ND()
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (2 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 03/12] xen/credit2: Remove tail padding from TRC_CSCHED2_* records Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-21 11:00 ` Jan Beulich
2021-09-20 17:25 ` [PATCH v2 05/12] x86/hvm: Remove duplicate calls caused by tracing Andrew Cooper
` (12 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu
It is pointless to write all 6 entries and only consume the useful subset.
bloat-o-meter shows quite how obscene the overhead is in vmx_vmexit_handler(),
weighing in at 12% of the function arranging unread zeroes on the stack, and
8% for svm_vmexit_handler().
add/remove: 0/0 grow/shrink: 0/20 up/down: 0/-1929 (-1929)
Function old new delta
hvm_msr_write_intercept 1049 1033 -16
vmx_enable_intr_window 238 214 -24
svm_enable_intr_window 337 313 -24
hvmemul_write_xcr 115 91 -24
hvmemul_write_cr 350 326 -24
hvmemul_read_xcr 115 91 -24
hvmemul_read_cr 146 122 -24
hvm_mov_to_cr 438 414 -24
hvm_mov_from_cr 253 229 -24
vmx_intr_assist 1150 1118 -32
svm_intr_assist 459 427 -32
hvm_rdtsc_intercept 138 106 -32
hvm_msr_read_intercept 898 866 -32
vmx_vmenter_helper 1142 1094 -48
vmx_inject_event 813 765 -48
svm_vmenter_helper 238 187 -51
hvm_hlt 197 146 -51
svm_inject_event 1678 1614 -64
svm_vmexit_handler 5880 5392 -488
vmx_vmexit_handler 7281 6438 -843
Total: Before=3644277, After=3642348, chg -0.05%
Adjust all users of HVMTRACE_ND(), using TRC_PAR_LONG() where appropriate
instead of opencoding it.
The 0 case needs a little help. All object in C must have a unique address
and _d is passed by pointer. Explicitly permit the optimiser to drop the
array.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
v2:
* Adjust callers of HVMTRACE_ND() too
* Drop _d[] for the 0 case.
Normally I wouldn't recommend patches like this for backport, but
{vmx,svm}_vmexit_handler() are fastpaths and this is a *lot* of I-cache lines
dropped...
---
xen/arch/x86/hvm/svm/svm.c | 8 +++-----
xen/arch/x86/hvm/vmx/vmx.c | 9 ++++-----
xen/include/asm-x86/hvm/trace.h | 28 ++++++++++------------------
3 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index afb1ccb342c2..f0e10dec046e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1052,7 +1052,7 @@ void svm_vmenter_helper(const struct cpu_user_regs *regs)
if ( unlikely(tb_init_done) )
HVMTRACE_ND(VMENTRY,
nestedhvm_vcpu_in_guestmode(curr) ? TRC_HVM_NESTEDFLAG : 0,
- 1/*cycles*/, 0, 0, 0, 0, 0, 0, 0);
+ 1/*cycles*/);
svm_sync_vmcb(curr, vmcb_needs_vmsave);
@@ -2565,12 +2565,10 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
if ( hvm_long_mode_active(v) )
HVMTRACE_ND(VMEXIT64, vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0,
- 1/*cycles*/, 3, exit_reason,
- regs->eip, regs->rip >> 32, 0, 0, 0);
+ 1/*cycles*/, exit_reason, TRC_PAR_LONG(regs->rip));
else
HVMTRACE_ND(VMEXIT, vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0,
- 1/*cycles*/, 2, exit_reason,
- regs->eip, 0, 0, 0, 0);
+ 1/*cycles*/, exit_reason, regs->eip);
if ( vcpu_guestmode )
{
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b0a42d05f86a..d403e2d8060a 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3864,11 +3864,10 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
__vmread(VM_EXIT_REASON, &exit_reason);
if ( hvm_long_mode_active(v) )
- HVMTRACE_ND(VMEXIT64, 0, 1/*cycles*/, 3, exit_reason,
- regs->eip, regs->rip >> 32, 0, 0, 0);
+ HVMTRACE_ND(VMEXIT64, 0, 1/*cycles*/, exit_reason,
+ TRC_PAR_LONG(regs->rip));
else
- HVMTRACE_ND(VMEXIT, 0, 1/*cycles*/, 2, exit_reason,
- regs->eip, 0, 0, 0, 0);
+ HVMTRACE_ND(VMEXIT, 0, 1/*cycles*/, exit_reason, regs->eip);
perfc_incra(vmexits, exit_reason);
@@ -4645,7 +4644,7 @@ bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
if ( unlikely(curr->arch.hvm.vmx.lbr_flags & LBR_FIXUP_MASK) )
lbr_fixup();
- HVMTRACE_ND(VMENTRY, 0, 1/*cycles*/, 0, 0, 0, 0, 0, 0, 0);
+ HVMTRACE_ND(VMENTRY, 0, 1/*cycles*/);
__vmwrite(GUEST_RIP, regs->rip);
__vmwrite(GUEST_RSP, regs->rsp);
diff --git a/xen/include/asm-x86/hvm/trace.h b/xen/include/asm-x86/hvm/trace.h
index 5cd459b855b7..145b59f6ac65 100644
--- a/xen/include/asm-x86/hvm/trace.h
+++ b/xen/include/asm-x86/hvm/trace.h
@@ -67,38 +67,30 @@
#define TRACE_2_LONG_4D(_e, d1, d2, d3, d4, ...) \
TRACE_6D(_e, d1, d2, d3, d4)
-#define HVMTRACE_ND(evt, modifier, cycles, count, d1, d2, d3, d4, d5, d6) \
+#define HVMTRACE_ND(evt, modifier, cycles, ...) \
do { \
if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt ) \
{ \
- struct { \
- u32 d[6]; \
- } _d; \
- _d.d[0]=(d1); \
- _d.d[1]=(d2); \
- _d.d[2]=(d3); \
- _d.d[3]=(d4); \
- _d.d[4]=(d5); \
- _d.d[5]=(d6); \
+ uint32_t _d[] = { __VA_ARGS__ }; \
__trace_var(TRC_HVM_ ## evt | (modifier), cycles, \
- sizeof(*_d.d) * count, &_d); \
+ sizeof(_d), sizeof(_d) ? _d : NULL); \
} \
} while(0)
#define HVMTRACE_6D(evt, d1, d2, d3, d4, d5, d6) \
- HVMTRACE_ND(evt, 0, 0, 6, d1, d2, d3, d4, d5, d6)
+ HVMTRACE_ND(evt, 0, 0, d1, d2, d3, d4, d5, d6)
#define HVMTRACE_5D(evt, d1, d2, d3, d4, d5) \
- HVMTRACE_ND(evt, 0, 0, 5, d1, d2, d3, d4, d5, 0)
+ HVMTRACE_ND(evt, 0, 0, d1, d2, d3, d4, d5)
#define HVMTRACE_4D(evt, d1, d2, d3, d4) \
- HVMTRACE_ND(evt, 0, 0, 4, d1, d2, d3, d4, 0, 0)
+ HVMTRACE_ND(evt, 0, 0, d1, d2, d3, d4)
#define HVMTRACE_3D(evt, d1, d2, d3) \
- HVMTRACE_ND(evt, 0, 0, 3, d1, d2, d3, 0, 0, 0)
+ HVMTRACE_ND(evt, 0, 0, d1, d2, d3)
#define HVMTRACE_2D(evt, d1, d2) \
- HVMTRACE_ND(evt, 0, 0, 2, d1, d2, 0, 0, 0, 0)
+ HVMTRACE_ND(evt, 0, 0, d1, d2)
#define HVMTRACE_1D(evt, d1) \
- HVMTRACE_ND(evt, 0, 0, 1, d1, 0, 0, 0, 0, 0)
+ HVMTRACE_ND(evt, 0, 0, d1)
#define HVMTRACE_0D(evt) \
- HVMTRACE_ND(evt, 0, 0, 0, 0, 0, 0, 0, 0, 0)
+ HVMTRACE_ND(evt, 0, 0)
#define HVMTRACE_LONG_1D(evt, d1) \
HVMTRACE_2D(evt ## 64, (d1) & 0xFFFFFFFF, (d1) >> 32)
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 04/12] x86/hvm: Reduce stack usage from HVMTRACE_ND()
2021-09-20 17:25 ` [PATCH v2 04/12] x86/hvm: Reduce stack usage from HVMTRACE_ND() Andrew Cooper
@ 2021-09-21 11:00 ` Jan Beulich
2021-09-21 15:38 ` Andrew Cooper
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-09-21 11:00 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 20.09.2021 19:25, Andrew Cooper wrote:
> v2:
> * Adjust callers of HVMTRACE_ND() too
What does this refer to? The sole difference to v1 that I can spot
is ...
> * Drop _d[] for the 0 case.
... the one corresponding to this line, i.e. ...
> --- a/xen/include/asm-x86/hvm/trace.h
> +++ b/xen/include/asm-x86/hvm/trace.h
> @@ -67,38 +67,30 @@
> #define TRACE_2_LONG_4D(_e, d1, d2, d3, d4, ...) \
> TRACE_6D(_e, d1, d2, d3, d4)
>
> -#define HVMTRACE_ND(evt, modifier, cycles, count, d1, d2, d3, d4, d5, d6) \
> +#define HVMTRACE_ND(evt, modifier, cycles, ...) \
> do { \
> if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt ) \
> { \
> - struct { \
> - u32 d[6]; \
> - } _d; \
> - _d.d[0]=(d1); \
> - _d.d[1]=(d2); \
> - _d.d[2]=(d3); \
> - _d.d[3]=(d4); \
> - _d.d[4]=(d5); \
> - _d.d[5]=(d6); \
> + uint32_t _d[] = { __VA_ARGS__ }; \
> __trace_var(TRC_HVM_ ## evt | (modifier), cycles, \
> - sizeof(*_d.d) * count, &_d); \
> + sizeof(_d), sizeof(_d) ? _d : NULL); \
... the addition of a conditional operator here (which I assume was
something a particular compiler didn't like in v1). FAOD - I'm fine
with the change, but I fear I'm overlooking something (again).
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 04/12] x86/hvm: Reduce stack usage from HVMTRACE_ND()
2021-09-21 11:00 ` Jan Beulich
@ 2021-09-21 15:38 ` Andrew Cooper
2021-09-21 15:40 ` Jan Beulich
0 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-21 15:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 21/09/2021 12:00, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
>> v2:
>> * Adjust callers of HVMTRACE_ND() too
> What does this refer to? The sole difference to v1 that I can spot
> is ...
Oh - its me who was confused.
I thought I had failed to include the changes in vmx.c/svm.c in v1. In
which case, no change to that in v2.
>> * Drop _d[] for the 0 case.
> ... the one corresponding to this line, i.e. ...
>
>> --- a/xen/include/asm-x86/hvm/trace.h
>> +++ b/xen/include/asm-x86/hvm/trace.h
>> @@ -67,38 +67,30 @@
>> #define TRACE_2_LONG_4D(_e, d1, d2, d3, d4, ...) \
>> TRACE_6D(_e, d1, d2, d3, d4)
>>
>> -#define HVMTRACE_ND(evt, modifier, cycles, count, d1, d2, d3, d4, d5, d6) \
>> +#define HVMTRACE_ND(evt, modifier, cycles, ...) \
>> do { \
>> if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt ) \
>> { \
>> - struct { \
>> - u32 d[6]; \
>> - } _d; \
>> - _d.d[0]=(d1); \
>> - _d.d[1]=(d2); \
>> - _d.d[2]=(d3); \
>> - _d.d[3]=(d4); \
>> - _d.d[4]=(d5); \
>> - _d.d[5]=(d6); \
>> + uint32_t _d[] = { __VA_ARGS__ }; \
>> __trace_var(TRC_HVM_ ## evt | (modifier), cycles, \
>> - sizeof(*_d.d) * count, &_d); \
>> + sizeof(_d), sizeof(_d) ? _d : NULL); \
> ... the addition of a conditional operator here (which I assume was
> something a particular compiler didn't like in v1).
And was covered in the commit message:
> The 0 case needs a little help. All object in C must have a unique address
> and _d is passed by pointer. Explicitly permit the optimiser to drop the
> array.
> FAOD - I'm fine
> with the change, but I fear I'm overlooking something (again).
Thanks,
~Andrew
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 04/12] x86/hvm: Reduce stack usage from HVMTRACE_ND()
2021-09-21 15:38 ` Andrew Cooper
@ 2021-09-21 15:40 ` Jan Beulich
0 siblings, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-09-21 15:40 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 21.09.2021 17:38, Andrew Cooper wrote:
> On 21/09/2021 12:00, Jan Beulich wrote:
>> On 20.09.2021 19:25, Andrew Cooper wrote:
>>> v2:
>>> * Adjust callers of HVMTRACE_ND() too
>> What does this refer to? The sole difference to v1 that I can spot
>> is ...
>
> Oh - its me who was confused.
>
> I thought I had failed to include the changes in vmx.c/svm.c in v1. In
> which case, no change to that in v2.
Good:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> * Drop _d[] for the 0 case.
>> ... the one corresponding to this line, i.e. ...
>>
>>> --- a/xen/include/asm-x86/hvm/trace.h
>>> +++ b/xen/include/asm-x86/hvm/trace.h
>>> @@ -67,38 +67,30 @@
>>> #define TRACE_2_LONG_4D(_e, d1, d2, d3, d4, ...) \
>>> TRACE_6D(_e, d1, d2, d3, d4)
>>>
>>> -#define HVMTRACE_ND(evt, modifier, cycles, count, d1, d2, d3, d4, d5, d6) \
>>> +#define HVMTRACE_ND(evt, modifier, cycles, ...) \
>>> do { \
>>> if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt ) \
>>> { \
>>> - struct { \
>>> - u32 d[6]; \
>>> - } _d; \
>>> - _d.d[0]=(d1); \
>>> - _d.d[1]=(d2); \
>>> - _d.d[2]=(d3); \
>>> - _d.d[3]=(d4); \
>>> - _d.d[4]=(d5); \
>>> - _d.d[5]=(d6); \
>>> + uint32_t _d[] = { __VA_ARGS__ }; \
>>> __trace_var(TRC_HVM_ ## evt | (modifier), cycles, \
>>> - sizeof(*_d.d) * count, &_d); \
>>> + sizeof(_d), sizeof(_d) ? _d : NULL); \
>> ... the addition of a conditional operator here (which I assume was
>> something a particular compiler didn't like in v1).
>
> And was covered in the commit message:
>
>> The 0 case needs a little help. All object in C must have a unique address
>> and _d is passed by pointer. Explicitly permit the optimiser to drop the
>> array.
Right, I had associated text and change this way. It was really just
the revision log which was confusing.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 05/12] x86/hvm: Remove duplicate calls caused by tracing
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (3 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 04/12] x86/hvm: Reduce stack usage from HVMTRACE_ND() Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-21 12:18 ` Jan Beulich
2021-09-20 17:25 ` [PATCH v2 06/12] xen/credit2: Clean up trace handling Andrew Cooper
` (11 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu
1) vpic_ack_pending_irq() calls vlapic_accept_pic_intr() twice, once in the
TRACE_2D() instantiation and once "for real". Make the call only once.
2) vlapic_accept_pic_intr() similarly calls __vlapic_accept_pic_intr() twice,
although this is more complicated to disentangle.
v cannot be NULL because it has already been dereferenced in the function,
causing the ternary expression to always call __vlapic_accept_pic_intr().
However, the return expression of the function takes care to skip the call
if this vCPU isn't the PIC target. As __vlapic_accept_pic_intr() is far
from trivial, make the TRACE_2D() semantics match the return semantics by
only calling __vlapic_accept_pic_intr() when the vCPU is the PIC target.
3) hpet_set_timer() duplicates calls to hpet_tick_to_ns(). Pull the logic out
which simplifies both the TRACE and create_periodic_time() calls.
4) lapic_rearm() makes multiple calls to vlapic_lvtt_period(). Pull it out
into a local variable.
vlapic_accept_pic_intr() is called on every VMEntry, so this is a reduction in
VMEntry complexity across the board.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
v2:
* New
---
xen/arch/x86/hvm/hpet.c | 15 +++++++++------
xen/arch/x86/hvm/vlapic.c | 23 +++++++++++++++--------
xen/arch/x86/hvm/vpic.c | 9 +++++----
3 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index ee756abb824d..475c3f8bf471 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -240,6 +240,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
uint64_t tn_cmp, cur_tick, diff;
unsigned int irq;
unsigned int oneshot;
+ s_time_t diff_ns, period_ns = 0;
ASSERT(tn < HPET_TIMER_NUM);
ASSERT(rw_is_write_locked(&h->lock));
@@ -311,13 +312,15 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
* status register) before another interrupt can be delivered.
*/
oneshot = !timer_is_periodic(h, tn) || timer_level(h, tn);
+ diff_ns = hpet_tick_to_ns(h, diff);
+ if ( !oneshot )
+ period_ns = hpet_tick_to_ns(h, h->hpet.period[tn]);
+
TRACE_2_LONG_4D(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq,
- TRC_PAR_LONG(hpet_tick_to_ns(h, diff)),
- TRC_PAR_LONG(oneshot ? 0LL :
- hpet_tick_to_ns(h, h->hpet.period[tn])));
- create_periodic_time(vhpet_vcpu(h), &h->pt[tn],
- hpet_tick_to_ns(h, diff),
- oneshot ? 0 : hpet_tick_to_ns(h, h->hpet.period[tn]),
+ TRC_PAR_LONG(diff_ns),
+ TRC_PAR_LONG(period_ns));
+
+ create_periodic_time(vhpet_vcpu(h), &h->pt[tn], diff_ns, period_ns,
irq, timer_level(h, tn) ? hpet_timer_fired : NULL,
timer_level(h, tn) ? (void *)(unsigned long)tn : NULL,
timer_level(h, tn));
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 5e21fb4937d9..b8c84458ffdc 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1267,15 +1267,18 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
int vlapic_accept_pic_intr(struct vcpu *v)
{
+ bool target, accept = false;
+
if ( vlapic_hw_disabled(vcpu_vlapic(v)) || !has_vpic(v->domain) )
return 0;
- TRACE_2D(TRC_HVM_EMUL_LAPIC_PIC_INTR,
- (v == v->domain->arch.hvm.i8259_target),
- v ? __vlapic_accept_pic_intr(v) : -1);
+ target = v == v->domain->arch.hvm.i8259_target;
+ if ( target )
+ accept = __vlapic_accept_pic_intr(v);
+
+ TRACE_2D(TRC_HVM_EMUL_LAPIC_PIC_INTR, target, accept);
- return ((v == v->domain->arch.hvm.i8259_target) &&
- __vlapic_accept_pic_intr(v));
+ return target && accept;
}
void vlapic_adjust_i8259_target(struct domain *d)
@@ -1449,6 +1452,7 @@ static void lapic_rearm(struct vlapic *s)
{
unsigned long tmict;
uint64_t period, tdt_msr;
+ bool is_periodic;
s->pt.irq = vlapic_get_reg(s, APIC_LVTT) & APIC_VECTOR_MASK;
@@ -1464,12 +1468,15 @@ static void lapic_rearm(struct vlapic *s)
period = ((uint64_t)APIC_BUS_CYCLE_NS *
(uint32_t)tmict * s->hw.timer_divisor);
+ is_periodic = vlapic_lvtt_period(s);
+
TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period),
- TRC_PAR_LONG(vlapic_lvtt_period(s) ? period : 0LL), s->pt.irq);
+ TRC_PAR_LONG(is_periodic ? period : 0LL), s->pt.irq);
+
create_periodic_time(vlapic_vcpu(s), &s->pt, period,
- vlapic_lvtt_period(s) ? period : 0,
+ is_periodic ? period : 0,
s->pt.irq,
- vlapic_lvtt_period(s) ? vlapic_pt_cb : NULL,
+ is_periodic ? vlapic_pt_cb : NULL,
&s->timer_last_update, false);
s->timer_last_update = s->pt.last_plt_gtime;
}
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index af988a868c8a..91c2c6983393 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -512,14 +512,15 @@ void vpic_irq_negative_edge(struct domain *d, int irq)
int vpic_ack_pending_irq(struct vcpu *v)
{
- int irq;
+ int irq, accept;
struct hvm_hw_vpic *vpic = &v->domain->arch.hvm.vpic[0];
ASSERT(has_vpic(v->domain));
- TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, vlapic_accept_pic_intr(v),
- vpic->int_output);
- if ( !vlapic_accept_pic_intr(v) || !vpic->int_output )
+ accept = vlapic_accept_pic_intr(v);
+
+ TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, accept, vpic->int_output);
+ if ( !accept || !vpic->int_output )
return -1;
irq = vpic_intack(vpic);
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 05/12] x86/hvm: Remove duplicate calls caused by tracing
2021-09-20 17:25 ` [PATCH v2 05/12] x86/hvm: Remove duplicate calls caused by tracing Andrew Cooper
@ 2021-09-21 12:18 ` Jan Beulich
2021-09-21 18:04 ` Andrew Cooper
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-09-21 12:18 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 20.09.2021 19:25, Andrew Cooper wrote:
> 1) vpic_ack_pending_irq() calls vlapic_accept_pic_intr() twice, once in the
> TRACE_2D() instantiation and once "for real". Make the call only once.
>
> 2) vlapic_accept_pic_intr() similarly calls __vlapic_accept_pic_intr() twice,
> although this is more complicated to disentangle.
>
> v cannot be NULL because it has already been dereferenced in the function,
> causing the ternary expression to always call __vlapic_accept_pic_intr().
> However, the return expression of the function takes care to skip the call
> if this vCPU isn't the PIC target. As __vlapic_accept_pic_intr() is far
> from trivial, make the TRACE_2D() semantics match the return semantics by
> only calling __vlapic_accept_pic_intr() when the vCPU is the PIC target.
>
> 3) hpet_set_timer() duplicates calls to hpet_tick_to_ns(). Pull the logic out
> which simplifies both the TRACE and create_periodic_time() calls.
>
> 4) lapic_rearm() makes multiple calls to vlapic_lvtt_period(). Pull it out
> into a local variable.
>
> vlapic_accept_pic_intr() is called on every VMEntry, so this is a reduction in
> VMEntry complexity across the board.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> --- a/xen/arch/x86/hvm/vpic.c
> +++ b/xen/arch/x86/hvm/vpic.c
> @@ -512,14 +512,15 @@ void vpic_irq_negative_edge(struct domain *d, int irq)
>
> int vpic_ack_pending_irq(struct vcpu *v)
> {
> - int irq;
> + int irq, accept;
Strictly speaking "accept" wants to be bool, and ...
> struct hvm_hw_vpic *vpic = &v->domain->arch.hvm.vpic[0];
>
> ASSERT(has_vpic(v->domain));
>
> - TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, vlapic_accept_pic_intr(v),
> - vpic->int_output);
> - if ( !vlapic_accept_pic_intr(v) || !vpic->int_output )
> + accept = vlapic_accept_pic_intr(v);
... vlapic_accept_pic_intr() would eventually also want to be
converted to return bool.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 05/12] x86/hvm: Remove duplicate calls caused by tracing
2021-09-21 12:18 ` Jan Beulich
@ 2021-09-21 18:04 ` Andrew Cooper
0 siblings, 0 replies; 52+ messages in thread
From: Andrew Cooper @ 2021-09-21 18:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 21/09/2021 13:18, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
>> 1) vpic_ack_pending_irq() calls vlapic_accept_pic_intr() twice, once in the
>> TRACE_2D() instantiation and once "for real". Make the call only once.
>>
>> 2) vlapic_accept_pic_intr() similarly calls __vlapic_accept_pic_intr() twice,
>> although this is more complicated to disentangle.
>>
>> v cannot be NULL because it has already been dereferenced in the function,
>> causing the ternary expression to always call __vlapic_accept_pic_intr().
>> However, the return expression of the function takes care to skip the call
>> if this vCPU isn't the PIC target. As __vlapic_accept_pic_intr() is far
>> from trivial, make the TRACE_2D() semantics match the return semantics by
>> only calling __vlapic_accept_pic_intr() when the vCPU is the PIC target.
>>
>> 3) hpet_set_timer() duplicates calls to hpet_tick_to_ns(). Pull the logic out
>> which simplifies both the TRACE and create_periodic_time() calls.
>>
>> 4) lapic_rearm() makes multiple calls to vlapic_lvtt_period(). Pull it out
>> into a local variable.
>>
>> vlapic_accept_pic_intr() is called on every VMEntry, so this is a reduction in
>> VMEntry complexity across the board.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
>
>> --- a/xen/arch/x86/hvm/vpic.c
>> +++ b/xen/arch/x86/hvm/vpic.c
>> @@ -512,14 +512,15 @@ void vpic_irq_negative_edge(struct domain *d, int irq)
>>
>> int vpic_ack_pending_irq(struct vcpu *v)
>> {
>> - int irq;
>> + int irq, accept;
> Strictly speaking "accept" wants to be bool, and ...
>
>> struct hvm_hw_vpic *vpic = &v->domain->arch.hvm.vpic[0];
>>
>> ASSERT(has_vpic(v->domain));
>>
>> - TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, vlapic_accept_pic_intr(v),
>> - vpic->int_output);
>> - if ( !vlapic_accept_pic_intr(v) || !vpic->int_output )
>> + accept = vlapic_accept_pic_intr(v);
> ... vlapic_accept_pic_intr() would eventually also want to be
> converted to return bool.
Yeah, but given the potential for backport here, I specifically avoided
unrelated changed. We've got loads of functions wanting to change to bool.
~Andrew
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 06/12] xen/credit2: Clean up trace handling
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (4 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 05/12] x86/hvm: Remove duplicate calls caused by tracing Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-24 16:55 ` Dario Faggioli
2021-09-20 17:25 ` [PATCH v2 07/12] xen/rt: " Andrew Cooper
` (10 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall, Dario Faggioli
There is no need for bitfields anywhere - use more sensible types. There is
also no need to cast 'd' to (unsigned char *) before passing it to a function
taking void *.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.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: Dario Faggioli <dfaggioli@suse.com>
v2:
* Fix whitespace.
---
xen/common/sched/credit2.c | 315 ++++++++++++++++++++++-----------------------
1 file changed, 153 insertions(+), 162 deletions(-)
diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index 41312158ec63..9b943e5168f0 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -1080,13 +1080,13 @@ static void update_max_weight(struct csched2_runqueue_data *rqd, int new_weight,
if ( unlikely(tb_init_done) )
{
struct {
- unsigned rqi:16, max_weight:16;
- } d;
- d.rqi = rqd->id;
- d.max_weight = rqd->max_weight;
- __trace_var(TRC_CSCHED2_RUNQ_MAX_WEIGHT, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t rqi, max_weight;
+ } d = {
+ .rqi = rqd->id,
+ .max_weight = rqd->max_weight,
+ };
+
+ __trace_var(TRC_CSCHED2_RUNQ_MAX_WEIGHT, 1, sizeof(d), &d);
}
}
@@ -1114,9 +1114,7 @@ _runq_assign(struct csched2_unit *svc, struct csched2_runqueue_data *rqd)
.rqi = rqd->id,
};
- __trace_var(TRC_CSCHED2_RUNQ_ASSIGN, 1,
- sizeof(d),
- (unsigned char *)&d);
+ __trace_var(TRC_CSCHED2_RUNQ_ASSIGN, 1, sizeof(d), &d);
}
}
@@ -1342,16 +1340,14 @@ update_runq_load(const struct scheduler *ops,
uint8_t rq_id, shift;
uint32_t _pad;
} d = {
- .rq_avgload = rqd->avgload,
- .b_avgload = rqd->b_avgload,
- .rq_load = rqd->load,
- .rq_id = rqd->id,
- .shift = P,
+ .rq_avgload = rqd->avgload,
+ .b_avgload = rqd->b_avgload,
+ .rq_load = rqd->load,
+ .rq_id = rqd->id,
+ .shift = P,
};
- __trace_var(TRC_CSCHED2_UPDATE_RUNQ_LOAD, 1,
- sizeof(d),
- (unsigned char *)&d);
+ __trace_var(TRC_CSCHED2_UPDATE_RUNQ_LOAD, 1, sizeof(d), &d);
}
}
@@ -1401,16 +1397,16 @@ update_svc_load(const struct scheduler *ops,
{
struct {
uint64_t v_avgload;
- unsigned unit:16, dom:16;
- unsigned shift;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- d.v_avgload = svc->avgload;
- d.shift = P;
- __trace_var(TRC_CSCHED2_UPDATE_UNIT_LOAD, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t shift;
+ } d = {
+ .v_avgload = svc->avgload,
+ .unit = svc->unit->unit_id,
+ .dom = svc->unit->domain->domain_id,
+ .shift = P,
+ };
+
+ __trace_var(TRC_CSCHED2_UPDATE_UNIT_LOAD, 1, sizeof(d), &d);
}
}
@@ -1457,15 +1453,15 @@ static void runq_insert(struct csched2_unit *svc)
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- unsigned pos;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- d.pos = pos;
- __trace_var(TRC_CSCHED2_RUNQ_POS, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t pos;
+ } d = {
+ .unit = svc->unit->unit_id,
+ .dom = svc->unit->domain->domain_id,
+ .pos = pos,
+ };
+
+ __trace_var(TRC_CSCHED2_RUNQ_POS, 1, sizeof(d), &d);
}
}
@@ -1557,16 +1553,16 @@ static s_time_t tickle_score(const struct scheduler *ops, s_time_t now,
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- int credit, score;
- } d;
- d.dom = cur->unit->domain->domain_id;
- d.unit = cur->unit->unit_id;
- d.credit = cur->credit;
- d.score = score;
- __trace_var(TRC_CSCHED2_TICKLE_CHECK, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t credit, score;
+ } d = {
+ .unit = cur->unit->unit_id,
+ .dom = cur->unit->domain->domain_id,
+ .credit = cur->credit,
+ .score = score,
+ };
+
+ __trace_var(TRC_CSCHED2_TICKLE_CHECK, 1, sizeof(d), &d);
}
return score;
@@ -1604,17 +1600,16 @@ runq_tickle(const struct scheduler *ops, struct csched2_unit *new, s_time_t now)
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- unsigned processor;
- int credit;
- } d;
- d.dom = unit->domain->domain_id;
- d.unit = unit->unit_id;
- d.processor = cpu;
- d.credit = new->credit;
- __trace_var(TRC_CSCHED2_TICKLE_NEW, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t processor, credit;
+ } d = {
+ .dom = unit->domain->domain_id,
+ .unit = unit->unit_id,
+ .processor = cpu,
+ .credit = new->credit,
+ };
+
+ __trace_var(TRC_CSCHED2_TICKLE_NEW, 1, sizeof(d), &d);
}
/*
@@ -1753,12 +1748,12 @@ runq_tickle(const struct scheduler *ops, struct csched2_unit *new, s_time_t now)
if ( unlikely(tb_init_done) )
{
struct {
- unsigned cpu:16, pad:16;
- } d;
- d.cpu = ipid; d.pad = 0;
- __trace_var(TRC_CSCHED2_TICKLE, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t cpu, _pad;
+ } d = {
+ .cpu = ipid,
+ };
+
+ __trace_var(TRC_CSCHED2_TICKLE, 1, sizeof(d), &d);
}
tickle_cpu(ipid, rqd);
@@ -1834,16 +1829,16 @@ static void reset_credit(int cpu, s_time_t now, struct csched2_unit *snext)
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- int credit_start, credit_end;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- d.credit_start = start_credit;
- d.credit_end = svc->credit;
- __trace_var(TRC_CSCHED2_CREDIT_RESET, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t credit_start, credit_end;
+ } d = {
+ .unit = svc->unit->unit_id,
+ .dom = svc->unit->domain->domain_id,
+ .credit_start = start_credit,
+ .credit_end = svc->credit,
+ };
+
+ __trace_var(TRC_CSCHED2_CREDIT_RESET, 1, sizeof(d), &d);
}
}
@@ -1889,18 +1884,17 @@ void burn_credits(struct csched2_runqueue_data *rqd,
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- int credit, budget;
- int delta;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- d.credit = svc->credit;
- d.budget = has_cap(svc) ? svc->budget : INT_MIN;
- d.delta = delta;
- __trace_var(TRC_CSCHED2_CREDIT_BURN, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t credit, budget, delta;
+ } d = {
+ .unit = svc->unit->unit_id,
+ .dom = svc->unit->domain->domain_id,
+ .credit = svc->credit,
+ .budget = has_cap(svc) ? svc->budget : INT_MIN,
+ .delta = delta,
+ };
+
+ __trace_var(TRC_CSCHED2_CREDIT_BURN, 1, sizeof(d), &d);
}
}
@@ -2545,17 +2539,17 @@ csched2_res_pick(const struct scheduler *ops, const struct sched_unit *unit)
{
struct {
uint64_t b_avgload;
- unsigned unit:16, dom:16;
- unsigned rq_id:16, new_cpu:16;
- } d;
- d.dom = unit->domain->domain_id;
- d.unit = unit->unit_id;
- d.rq_id = min_rqd ? min_rqd->id : -1;
- d.b_avgload = min_avgload;
- d.new_cpu = new_cpu;
- __trace_var(TRC_CSCHED2_PICKED_CPU, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint16_t rq_id, new_cpu;
+ } d = {
+ .b_avgload = min_avgload,
+ .unit = unit->unit_id,
+ .dom = unit->domain->domain_id,
+ .rq_id = min_rqd ? min_rqd->id : -1,
+ .new_cpu = new_cpu,
+ };
+
+ __trace_var(TRC_CSCHED2_PICKED_CPU, 1, sizeof(d), &d);
}
return get_sched_res(new_cpu);
@@ -2616,16 +2610,16 @@ static void migrate(const struct scheduler *ops,
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- unsigned rqi:16, trqi:16;
- } d;
- d.dom = unit->domain->domain_id;
- d.unit = unit->unit_id;
- d.rqi = svc->rqd->id;
- d.trqi = trqd->id;
- __trace_var(TRC_CSCHED2_MIGRATE, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint16_t rqi, trqi;
+ } d = {
+ .unit = unit->unit_id,
+ .dom = unit->domain->domain_id,
+ .rqi = svc->rqd->id,
+ .trqi = trqd->id,
+ };
+
+ __trace_var(TRC_CSCHED2_MIGRATE, 1, sizeof(d), &d);
}
if ( svc->flags & CSFLAG_scheduled )
@@ -2762,15 +2756,15 @@ static void balance_load(const struct scheduler *ops, int cpu, s_time_t now)
if ( unlikely(tb_init_done) )
{
struct {
- unsigned lrq_id:16, orq_id:16;
- unsigned load_delta;
- } d;
- d.lrq_id = st.lrqd->id;
- d.orq_id = st.orqd->id;
- d.load_delta = st.load_delta;
- __trace_var(TRC_CSCHED2_LOAD_CHECK, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t lrq_id, orq_id;
+ uint32_t load_delta;
+ } d = {
+ .lrq_id = st.lrqd->id,
+ .orq_id = st.orqd->id,
+ .load_delta = st.load_delta,
+ };
+
+ __trace_var(TRC_CSCHED2_LOAD_CHECK, 1, sizeof(d), &d);
}
/*
@@ -2808,15 +2802,13 @@ static void balance_load(const struct scheduler *ops, int cpu, s_time_t now)
uint16_t lrq_id, orq_id;
uint32_t _pad;
} d = {
- .lb_avgload = st.lrqd->b_avgload,
- .ob_avgload = st.orqd->b_avgload,
- .lrq_id = st.lrqd->id,
- .orq_id = st.orqd->id,
+ .lb_avgload = st.lrqd->b_avgload,
+ .ob_avgload = st.orqd->b_avgload,
+ .lrq_id = st.lrqd->id,
+ .orq_id = st.orqd->id,
};
- __trace_var(TRC_CSCHED2_LOAD_BALANCE, 1,
- sizeof(d),
- (unsigned char *)&d);
+ __trace_var(TRC_CSCHED2_LOAD_BALANCE, 1, sizeof(d), &d);
}
SCHED_STAT_CRANK(acct_load_balance);
@@ -3402,15 +3394,15 @@ runq_candidate(struct csched2_runqueue_data *rqd,
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- unsigned runtime;
- } d;
- d.dom = scurr->unit->domain->domain_id;
- d.unit = scurr->unit->unit_id;
- d.runtime = now - scurr->unit->state_entry_time;
- __trace_var(TRC_CSCHED2_RATELIMIT, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t runtime;
+ } d = {
+ .unit = scurr->unit->unit_id,
+ .dom = scurr->unit->domain->domain_id,
+ .runtime = now - scurr->unit->state_entry_time,
+ };
+
+ __trace_var(TRC_CSCHED2_RATELIMIT, 1, sizeof(d), &d);
}
return scurr;
}
@@ -3463,13 +3455,13 @@ runq_candidate(struct csched2_runqueue_data *rqd,
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- __trace_var(TRC_CSCHED2_RUNQ_CAND_CHECK, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ } d = {
+ .unit = svc->unit->unit_id,
+ .dom = svc->unit->domain->domain_id,
+ };
+
+ __trace_var(TRC_CSCHED2_RUNQ_CAND_CHECK, 1, sizeof(d), &d);
}
/*
@@ -3537,17 +3529,16 @@ runq_candidate(struct csched2_runqueue_data *rqd,
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- unsigned tickled_cpu;
- int credit;
- } d;
- d.dom = snext->unit->domain->domain_id;
- d.unit = snext->unit->unit_id;
- d.credit = snext->credit;
- d.tickled_cpu = snext->tickled_cpu;
- __trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t tickled_cpu, credit;
+ } d = {
+ .unit = snext->unit->unit_id,
+ .dom = snext->unit->domain->domain_id,
+ .tickled_cpu = snext->tickled_cpu,
+ .credit = snext->credit,
+ };
+
+ __trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1, sizeof(d), &d);
}
if ( unlikely(snext->tickled_cpu != -1 && snext->tickled_cpu != cpu) )
@@ -3603,18 +3594,18 @@ static void csched2_schedule(
if ( unlikely(tb_init_done) )
{
struct {
- unsigned cpu:16, rq_id:16;
- unsigned tasklet:8, idle:8, smt_idle:8, tickled:8;
- } d;
- d.cpu = cur_cpu;
- d.rq_id = c2r(sched_cpu);
- d.tasklet = tasklet_work_scheduled;
- d.idle = is_idle_unit(currunit);
- d.smt_idle = cpumask_test_cpu(sched_cpu, &rqd->smt_idle);
- d.tickled = tickled;
- __trace_var(TRC_CSCHED2_SCHEDULE, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t cpu, rq_id;
+ uint8_t tasklet, idle, smt_idle, tickled;
+ } d = {
+ .cpu = cur_cpu,
+ .rq_id = c2r(sched_cpu),
+ .tasklet = tasklet_work_scheduled,
+ .idle = is_idle_unit(currunit),
+ .smt_idle = cpumask_test_cpu(sched_cpu, &rqd->smt_idle),
+ .tickled = tickled,
+ };
+
+ __trace_var(TRC_CSCHED2_SCHEDULE, 1, sizeof(d), &d);
}
/* Update credits (and budget, if necessary). */
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 06/12] xen/credit2: Clean up trace handling
2021-09-20 17:25 ` [PATCH v2 06/12] xen/credit2: Clean up trace handling Andrew Cooper
@ 2021-09-24 16:55 ` Dario Faggioli
0 siblings, 0 replies; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 16:55 UTC (permalink / raw)
To: andrew.cooper3, xen-devel
Cc: julien, George.Dunlap, sstabellini, Jan Beulich, iwj, wl
[-- Attachment #1: Type: text/plain, Size: 729 bytes --]
On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
> There is no need for bitfields anywhere - use more sensible types.
> There is
> also no need to cast 'd' to (unsigned char *) before passing it to a
> function
> taking void *.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
Thanks and Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 07/12] xen/rt: Clean up trace handling
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (5 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 06/12] xen/credit2: Clean up trace handling Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-24 16:56 ` Dario Faggioli
2021-09-20 17:25 ` [PATCH v2 08/12] xen/sched: " Andrew Cooper
` (9 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall, Dario Faggioli,
Meng Xu
Most uses of bitfields and __packed are unnecessary. There is also no need to
cast 'd' to (unsigned char *) before passing it to a function taking void *.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@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: Dario Faggioli <dfaggioli@suse.com>
CC: Meng Xu <mengxu@cis.upenn.edu>
v2:
* New
---
xen/common/sched/rt.c | 97 ++++++++++++++++++++++++---------------------------
1 file changed, 46 insertions(+), 51 deletions(-)
diff --git a/xen/common/sched/rt.c b/xen/common/sched/rt.c
index c58edca0de84..d5da35cac75e 100644
--- a/xen/common/sched/rt.c
+++ b/xen/common/sched/rt.c
@@ -455,21 +455,21 @@ rt_update_deadline(s_time_t now, struct rt_unit *svc)
svc->cur_budget = svc->budget;
svc->priority_level = 0;
- /* TRACE */
+ if ( unlikely(tb_init_done) )
{
- struct __packed {
- unsigned unit:16, dom:16;
- unsigned priority_level;
+ struct {
+ uint16_t unit, dom;
+ uint32_t priority_level;
uint64_t cur_deadline, cur_budget;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- d.priority_level = svc->priority_level;
- d.cur_deadline = (uint64_t) svc->cur_deadline;
- d.cur_budget = (uint64_t) svc->cur_budget;
- trace_var(TRC_RTDS_BUDGET_REPLENISH, 1,
- sizeof(d),
- (unsigned char *) &d);
+ } d = {
+ .dom = svc->unit->domain->domain_id,
+ .unit = svc->unit->unit_id,
+ .priority_level = svc->priority_level,
+ .cur_deadline = svc->cur_deadline,
+ .cur_budget = svc->cur_budget,
+ };
+
+ __trace_var(TRC_RTDS_BUDGET_REPLENISH, 1, sizeof(d), &d);
}
return;
@@ -965,7 +965,7 @@ burn_budget(const struct scheduler *ops, struct rt_unit *svc, s_time_t now)
}
}
- /* TRACE */
+ if ( unlikely(tb_init_done) )
{
struct __packed {
uint16_t unit, dom;
@@ -982,9 +982,7 @@ burn_budget(const struct scheduler *ops, struct rt_unit *svc, s_time_t now)
.has_extratime = !!(svc->flags & RTDS_extratime),
};
- trace_var(TRC_RTDS_BUDGET_BURN, 1,
- sizeof(d),
- (unsigned char *) &d);
+ __trace_var(TRC_RTDS_BUDGET_BURN, 1, sizeof(d), &d);
}
}
@@ -1019,22 +1017,19 @@ runq_pick(const struct scheduler *ops, const cpumask_t *mask, unsigned int cpu)
break;
}
- /* TRACE */
+ if ( unlikely(tb_init_done) && svc )
{
- if( svc != NULL )
- {
- struct __packed {
- unsigned unit:16, dom:16;
- uint64_t cur_deadline, cur_budget;
- } d;
- d.dom = svc->unit->domain->domain_id;
- d.unit = svc->unit->unit_id;
- d.cur_deadline = (uint64_t) svc->cur_deadline;
- d.cur_budget = (uint64_t) svc->cur_budget;
- trace_var(TRC_RTDS_RUNQ_PICK, 1,
- sizeof(d),
- (unsigned char *) &d);
- }
+ struct __packed {
+ uint16_t unit, dom;
+ uint64_t cur_deadline, cur_budget;
+ } d = {
+ .unit = svc->unit->unit_id,
+ .dom = svc->unit->domain->domain_id,
+ .cur_deadline = svc->cur_deadline,
+ .cur_budget = svc->cur_budget,
+ };
+
+ __trace_var(TRC_RTDS_RUNQ_PICK, 1, sizeof(d), &d);
}
return svc;
@@ -1055,18 +1050,19 @@ rt_schedule(const struct scheduler *ops, struct sched_unit *currunit,
struct rt_unit *snext = NULL;
bool migrated = false;
- /* TRACE */
+ if ( unlikely(tb_init_done) )
{
- struct __packed {
- unsigned cpu:16, tasklet:8, tickled:4, idle:4;
- } d;
- d.cpu = cur_cpu;
- d.tasklet = tasklet_work_scheduled;
- d.tickled = cpumask_test_cpu(sched_cpu, &prv->tickled);
- d.idle = is_idle_unit(currunit);
- trace_var(TRC_RTDS_SCHEDULE, 1,
- sizeof(d),
- (unsigned char *)&d);
+ struct {
+ uint16_t cpu;
+ uint8_t tasklet, tickled:4, idle:4;
+ } d = {
+ .cpu = cur_cpu,
+ .tasklet = tasklet_work_scheduled,
+ .tickled = cpumask_test_cpu(sched_cpu, &prv->tickled),
+ .idle = is_idle_unit(currunit),
+ };
+
+ __trace_var(TRC_RTDS_SCHEDULE, 1, sizeof(d), &d);
}
/* clear ticked bit now that we've been scheduled */
@@ -1223,16 +1219,15 @@ runq_tickle(const struct scheduler *ops, const struct rt_unit *new)
SCHED_STAT_CRANK(tickled_no_cpu);
return;
out:
- /* TRACE */
+ if ( unlikely(tb_init_done) )
{
struct {
- unsigned cpu:16, pad:16;
- } d;
- d.cpu = cpu_to_tickle;
- d.pad = 0;
- trace_var(TRC_RTDS_TICKLE, 1,
- sizeof(d),
- (unsigned char *)&d);
+ uint16_t cpu, _pad;
+ } d = {
+ .cpu = cpu_to_tickle,
+ };
+
+ __trace_var(TRC_RTDS_TICKLE, 1, sizeof(d), &d);
}
cpumask_set_cpu(cpu_to_tickle, &prv->tickled);
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 07/12] xen/rt: Clean up trace handling
2021-09-20 17:25 ` [PATCH v2 07/12] xen/rt: " Andrew Cooper
@ 2021-09-24 16:56 ` Dario Faggioli
0 siblings, 0 replies; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 16:56 UTC (permalink / raw)
To: andrew.cooper3, xen-devel
Cc: julien, George.Dunlap, sstabellini, Jan Beulich, iwj, wl, mengxu
[-- Attachment #1: Type: text/plain, Size: 664 bytes --]
On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
> Most uses of bitfields and __packed are unnecessary. There is also
> no need to
> cast 'd' to (unsigned char *) before passing it to a function taking
> void *.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
Thanks and Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 08/12] xen/sched: Clean up trace handling
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (6 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 07/12] xen/rt: " Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-24 16:57 ` Dario Faggioli
2021-09-20 17:25 ` [PATCH v2 09/12] xen/trace: Minor code cleanup Andrew Cooper
` (8 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall, Juergen Gross,
Dario Faggioli
There is no need for bitfields anywhere - use more sensible types. There is
also no need to cast 'd' to (unsigned char *) before passing it to a function
taking void *.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@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: Juergen Gross <jgross@suse.com>
CC: Dario Faggioli <dfaggioli@suse.com>
v2:
* New
---
xen/common/sched/core.c | 4 ++--
xen/common/sched/credit.c | 38 ++++++++++++++++++--------------------
xen/common/sched/null.c | 42 +++++++++++++++++++++++++-----------------
3 files changed, 45 insertions(+), 39 deletions(-)
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 8f4b1ca10d1c..fe133cbf117c 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -205,7 +205,7 @@ static inline struct scheduler *vcpu_scheduler(const struct vcpu *v)
static inline void trace_runstate_change(const struct vcpu *v, int new_state)
{
- struct { uint32_t vcpu:16, domain:16; } d;
+ struct { uint16_t vcpu, domain; } d;
uint32_t event;
if ( likely(!tb_init_done) )
@@ -223,7 +223,7 @@ static inline void trace_runstate_change(const struct vcpu *v, int new_state)
static inline void trace_continue_running(const struct vcpu *v)
{
- struct { uint32_t vcpu:16, domain:16; } d;
+ struct { uint16_t vcpu, domain; } d;
if ( likely(!tb_init_done) )
return;
diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c
index d0aa017c643e..f277fa37a8b1 100644
--- a/xen/common/sched/credit.c
+++ b/xen/common/sched/credit.c
@@ -1828,21 +1828,18 @@ static void csched_schedule(
SCHED_STAT_CRANK(schedule);
CSCHED_UNIT_CHECK(unit);
- /*
- * Here in Credit1 code, we usually just call TRACE_nD() helpers, and
- * don't care about packing. But scheduling happens very often, so it
- * actually is important that the record is as small as possible.
- */
if ( unlikely(tb_init_done) )
{
struct {
- unsigned cpu:16, tasklet:8, idle:8;
- } d;
- d.cpu = cur_cpu;
- d.tasklet = tasklet_work_scheduled;
- d.idle = is_idle_unit(unit);
- __trace_var(TRC_CSCHED_SCHEDULE, 1, sizeof(d),
- (unsigned char *)&d);
+ uint16_t cpu;
+ uint8_t tasklet, idle;
+ } d = {
+ .cpu = cur_cpu,
+ .tasklet = tasklet_work_scheduled,
+ .idle = is_idle_unit(unit),
+ };
+
+ __trace_var(TRC_CSCHED_SCHEDULE, 1, sizeof(d), &d);
}
runtime = now - unit->state_entry_time;
@@ -1904,14 +1901,15 @@ static void csched_schedule(
if ( unlikely(tb_init_done) )
{
struct {
- unsigned unit:16, dom:16;
- unsigned runtime;
- } d;
- d.dom = unit->domain->domain_id;
- d.unit = unit->unit_id;
- d.runtime = runtime;
- __trace_var(TRC_CSCHED_RATELIMIT, 1, sizeof(d),
- (unsigned char *)&d);
+ uint16_t unit, dom;
+ uint32_t runtime;
+ } d = {
+ .dom = unit->domain->domain_id,
+ .unit = unit->unit_id,
+ .runtime = runtime,
+ };
+
+ __trace_var(TRC_CSCHED_RATELIMIT, 1, sizeof(d), &d);
}
goto out;
diff --git a/xen/common/sched/null.c b/xen/common/sched/null.c
index 82d5d1baab85..deb59747fbe8 100644
--- a/xen/common/sched/null.c
+++ b/xen/common/sched/null.c
@@ -329,10 +329,12 @@ pick_res(const struct null_private *prv, const struct sched_unit *unit)
struct {
uint16_t unit, dom;
uint32_t new_cpu;
- } d;
- d.dom = unit->domain->domain_id;
- d.unit = unit->unit_id;
- d.new_cpu = new_cpu;
+ } d = {
+ .unit = unit->unit_id,
+ .dom = unit->domain->domain_id,
+ .new_cpu = new_cpu,
+ };
+
__trace_var(TRC_SNULL_PICKED_CPU, 1, sizeof(d), &d);
}
@@ -357,10 +359,12 @@ static void unit_assign(struct null_private *prv, struct sched_unit *unit,
struct {
uint16_t unit, dom;
uint32_t cpu;
- } d;
- d.dom = unit->domain->domain_id;
- d.unit = unit->unit_id;
- d.cpu = cpu;
+ } d = {
+ .unit = unit->unit_id,
+ .dom = unit->domain->domain_id,
+ .cpu = cpu,
+ };
+
__trace_var(TRC_SNULL_UNIT_ASSIGN, 1, sizeof(d), &d);
}
}
@@ -388,10 +392,12 @@ static bool unit_deassign(struct null_private *prv, const struct sched_unit *uni
struct {
uint16_t unit, dom;
uint32_t cpu;
- } d;
- d.dom = unit->domain->domain_id;
- d.unit = unit->unit_id;
- d.cpu = cpu;
+ } d = {
+ .unit = unit->unit_id,
+ .dom = unit->domain->domain_id,
+ .cpu = cpu,
+ };
+
__trace_var(TRC_SNULL_UNIT_DEASSIGN, 1, sizeof(d), &d);
}
@@ -691,11 +697,13 @@ static void null_unit_migrate(const struct scheduler *ops,
struct {
uint16_t unit, dom;
uint16_t cpu, new_cpu;
- } d;
- d.dom = unit->domain->domain_id;
- d.unit = unit->unit_id;
- d.cpu = sched_unit_master(unit);
- d.new_cpu = new_cpu;
+ } d = {
+ .unit = unit->unit_id,
+ .dom = unit->domain->domain_id,
+ .cpu = sched_unit_master(unit),
+ .new_cpu = new_cpu,
+ };
+
__trace_var(TRC_SNULL_MIGRATE, 1, sizeof(d), &d);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 08/12] xen/sched: Clean up trace handling
2021-09-20 17:25 ` [PATCH v2 08/12] xen/sched: " Andrew Cooper
@ 2021-09-24 16:57 ` Dario Faggioli
0 siblings, 0 replies; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 16:57 UTC (permalink / raw)
To: andrew.cooper3, xen-devel
Cc: julien, George.Dunlap, sstabellini, Juergen Gross, Jan Beulich, iwj, wl
[-- Attachment #1: Type: text/plain, Size: 681 bytes --]
On Mon, 2021-09-20 at 18:25 +0100, Andrew Cooper wrote:
> There is no need for bitfields anywhere - use more sensible types.
> There is
> also no need to cast 'd' to (unsigned char *) before passing it to a
> function
> taking void *.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
Thanks and Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 09/12] xen/trace: Minor code cleanup
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (7 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 08/12] xen/sched: " Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-21 11:03 ` Jan Beulich
2021-09-20 17:25 ` [PATCH v2 10/12] x86/pv: Move x86/trace.c to x86/pv/trace.c Andrew Cooper
` (7 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall, Dario Faggioli
* Delete trailing whitespace
* Replace an opencoded DIV_ROUND_UP()
* Drop bogus smp_rmb() - spin_lock_irqsave() has full smp_mb() semantics.
Signed-off-by: Andrew Cooper <andrew.cooper3@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: Dario Faggioli <dfaggioli@suse.com>
---
xen/common/trace.c | 37 +++++++++++++++----------------------
xen/include/asm-x86/hvm/trace.h | 2 +-
2 files changed, 16 insertions(+), 23 deletions(-)
diff --git a/xen/common/trace.c b/xen/common/trace.c
index 4297ff505fb9..41a3c170446b 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -75,10 +75,6 @@ static cpumask_t tb_cpu_mask;
/* which tracing events are enabled */
static u32 tb_event_mask = TRC_ALL;
-/* Return the number of elements _type necessary to store at least _x bytes of data
- * i.e., sizeof(_type) * ans >= _x. */
-#define fit_to_type(_type, _x) (((_x)+sizeof(_type)-1) / sizeof(_type))
-
static int cpu_callback(
struct notifier_block *nfb, unsigned long action, void *hcpu)
{
@@ -96,8 +92,8 @@ static struct notifier_block cpu_nfb = {
static uint32_t calc_tinfo_first_offset(void)
{
- int offset_in_bytes = offsetof(struct t_info, mfn_offset[NR_CPUS]);
- return fit_to_type(uint32_t, offset_in_bytes);
+ return DIV_ROUND_UP(offsetof(struct t_info, mfn_offset[NR_CPUS]),
+ sizeof(uint32_t));
}
/**
@@ -148,7 +144,7 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
pages = max_pages;
}
- /*
+ /*
* NB this calculation is correct, because t_info_first_offset is
* in words, not bytes
*/
@@ -167,7 +163,7 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
* trace buffers. The trace buffers are then available for debugging use, via
* the %TRACE_xD macros exported in <xen/trace.h>.
*
- * This function may also be called later when enabling trace buffers
+ * This function may also be called later when enabling trace buffers
* via the SET_SIZE hypercall.
*/
static int alloc_trace_bufs(unsigned int pages)
@@ -401,7 +397,7 @@ int tb_control(struct xen_sysctl_tbuf_op *tbc)
break;
case XEN_SYSCTL_TBUFOP_enable:
/* Enable trace buffers. Check buffers are already allocated. */
- if ( opt_tbuf_size == 0 )
+ if ( opt_tbuf_size == 0 )
rc = -EINVAL;
else
tb_init_done = 1;
@@ -438,7 +434,7 @@ int tb_control(struct xen_sysctl_tbuf_op *tbc)
return rc;
}
-static inline unsigned int calc_rec_size(bool_t cycles, unsigned int extra)
+static inline unsigned int calc_rec_size(bool_t cycles, unsigned int extra)
{
unsigned int rec_size = 4;
@@ -597,7 +593,7 @@ static inline void __insert_record(struct t_buf *buf,
rec->u.cycles.cycles_lo = (uint32_t)tsc;
rec->u.cycles.cycles_hi = (uint32_t)(tsc >> 32);
dst = rec->u.cycles.extra_u32;
- }
+ }
if ( extra_data && extra )
memcpy(dst, extra_data, extra);
@@ -717,9 +713,6 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
if ( !cpumask_test_cpu(smp_processor_id(), &tb_cpu_mask) )
return;
- /* Read tb_init_done /before/ t_bufs. */
- smp_rmb();
-
spin_lock_irqsave(&this_cpu(t_lock), flags);
buf = this_cpu(t_bufs);
@@ -735,14 +728,14 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
/* Calculate the record size */
rec_size = calc_rec_size(cycles, extra);
-
+
/* How many bytes are available in the buffer? */
bytes_to_tail = calc_bytes_avail(buf);
-
+
/* How many bytes until the next wrap-around? */
bytes_to_wrap = calc_bytes_to_wrap(buf);
-
- /*
+
+ /*
* Calculate expected total size to commit this record by
* doing a dry-run.
*/
@@ -756,7 +749,7 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
{
total_size += bytes_to_wrap;
bytes_to_wrap = data_size;
- }
+ }
total_size += LOST_REC_SIZE;
bytes_to_wrap -= LOST_REC_SIZE;
@@ -768,7 +761,7 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
if ( rec_size > bytes_to_wrap )
{
total_size += bytes_to_wrap;
- }
+ }
total_size += rec_size;
/* Do we have enough space for everything? */
@@ -781,7 +774,7 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
}
/*
- * Now, actually write information
+ * Now, actually write information
*/
bytes_to_wrap = calc_bytes_to_wrap(buf);
@@ -791,7 +784,7 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
{
insert_wrap_record(buf, LOST_REC_SIZE);
bytes_to_wrap = data_size;
- }
+ }
insert_lost_records(buf);
bytes_to_wrap -= LOST_REC_SIZE;
diff --git a/xen/include/asm-x86/hvm/trace.h b/xen/include/asm-x86/hvm/trace.h
index 145b59f6ac65..696e42eb9499 100644
--- a/xen/include/asm-x86/hvm/trace.h
+++ b/xen/include/asm-x86/hvm/trace.h
@@ -52,7 +52,7 @@
#define DO_TRC_HVM_CLTS DEFAULT_HVM_MISC
#define DO_TRC_HVM_LMSW DEFAULT_HVM_MISC
#define DO_TRC_HVM_LMSW64 DEFAULT_HVM_MISC
-#define DO_TRC_HVM_REALMODE_EMULATE DEFAULT_HVM_MISC
+#define DO_TRC_HVM_REALMODE_EMULATE DEFAULT_HVM_MISC
#define DO_TRC_HVM_TRAP DEFAULT_HVM_MISC
#define DO_TRC_HVM_TRAP_DEBUG DEFAULT_HVM_MISC
#define DO_TRC_HVM_VLAPIC DEFAULT_HVM_MISC
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 09/12] xen/trace: Minor code cleanup
2021-09-20 17:25 ` [PATCH v2 09/12] xen/trace: Minor code cleanup Andrew Cooper
@ 2021-09-21 11:03 ` Jan Beulich
2021-09-24 15:16 ` Dario Faggioli
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-09-21 11:03 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Dario Faggioli, Xen-devel
On 20.09.2021 19:25, Andrew Cooper wrote:
> * Delete trailing whitespace
> * Replace an opencoded DIV_ROUND_UP()
> * Drop bogus smp_rmb() - spin_lock_irqsave() has full smp_mb() semantics.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Like for v1: Largely
Reviewed-by: Jan Beulich <jbeulich@suse.com>
One remark:
> @@ -717,9 +713,6 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
> if ( !cpumask_test_cpu(smp_processor_id(), &tb_cpu_mask) )
> return;
>
> - /* Read tb_init_done /before/ t_bufs. */
> - smp_rmb();
> -
> spin_lock_irqsave(&this_cpu(t_lock), flags);
>
> buf = this_cpu(t_bufs);
I wonder whether the comment wouldn't be helpful to move down here,
in of course a slightly edited form (going from /before/ to /after/).
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 09/12] xen/trace: Minor code cleanup
2021-09-21 11:03 ` Jan Beulich
@ 2021-09-24 15:16 ` Dario Faggioli
0 siblings, 0 replies; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 15:16 UTC (permalink / raw)
To: Jan Beulich, andrew.cooper3
Cc: julien, George.Dunlap, sstabellini, iwj, wl, xen-devel
[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]
On Tue, 2021-09-21 at 13:03 +0200, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Like for v1: Largely
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
> One remark:
>
> > @@ -717,9 +713,6 @@ void __trace_var(u32 event, bool_t cycles,
> > unsigned int extra,
> > if ( !cpumask_test_cpu(smp_processor_id(), &tb_cpu_mask) )
> > return;
> >
> > - /* Read tb_init_done /before/ t_bufs. */
> > - smp_rmb();
> > -
> > spin_lock_irqsave(&this_cpu(t_lock), flags);
> >
> > buf = this_cpu(t_bufs);
>
> I wonder whether the comment wouldn't be helpful to move down here,
> in of course a slightly edited form (going from /before/ to /after/).
>
FWIW, I agree with this (but the R-o-b: stands no matter whether it's
done or not).
Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 10/12] x86/pv: Move x86/trace.c to x86/pv/trace.c
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (8 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 09/12] xen/trace: Minor code cleanup Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-21 15:58 ` Jan Beulich
2021-09-20 17:25 ` [PATCH v2 11/12] xen/arch: Drop asm-*/trace.h Andrew Cooper
` (6 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu
This entire file is pv-only, and not excluded from the build by
CONFIG_TRACEBUFFER. Move it into the pv/ directory, build it conditionally,
and drop unused includes.
Also move the contents of asm/trace.h to asm/pv/trace.h to avoid the functions
being declared across the entire hypervisor.
One caller in fixup_page_fault() is effectively PV only, but is not subject to
dead code elimination. Add an additional IS_ENABLED(CONFIG_PV) to keep the
build happy.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
v2:
* New
---
xen/arch/x86/Makefile | 1 -
xen/arch/x86/pv/Makefile | 1 +
xen/arch/x86/pv/emul-inv-op.c | 2 +-
xen/arch/x86/pv/emul-priv-op.c | 1 +
xen/arch/x86/pv/ro-page-fault.c | 2 +-
xen/arch/x86/{ => pv}/trace.c | 13 +++++------
xen/arch/x86/pv/traps.c | 2 +-
xen/arch/x86/traps.c | 3 ++-
xen/include/asm-x86/{ => pv}/trace.h | 8 ++++---
xen/include/asm-x86/trace.h | 42 ------------------------------------
10 files changed, 17 insertions(+), 58 deletions(-)
rename xen/arch/x86/{ => pv}/trace.c (95%)
copy xen/include/asm-x86/{ => pv}/trace.h (92%)
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 202d4d27823d..82dd4e18bd36 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -61,7 +61,6 @@ obj-y += spec_ctrl.o
obj-y += srat.o
obj-y += string.o
obj-y += time.o
-obj-y += trace.o
obj-y += traps.o
obj-y += tsx.o
obj-y += usercopy.o
diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
index 75b01b0062d8..6cda354cc41f 100644
--- a/xen/arch/x86/pv/Makefile
+++ b/xen/arch/x86/pv/Makefile
@@ -12,6 +12,7 @@ obj-y += misc-hypercalls.o
obj-y += mm.o
obj-y += ro-page-fault.o
obj-$(CONFIG_PV_SHIM) += shim.o
+obj-$(CONFIG_TRACEBUFFER) += trace.o
obj-y += traps.o
obj-bin-y += dom0_build.init.o
diff --git a/xen/arch/x86/pv/emul-inv-op.c b/xen/arch/x86/pv/emul-inv-op.c
index b15f302bcaad..2c07eed9a092 100644
--- a/xen/arch/x86/pv/emul-inv-op.c
+++ b/xen/arch/x86/pv/emul-inv-op.c
@@ -19,7 +19,7 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/trace.h>
+#include <asm/pv/trace.h>
#include "emulate.h"
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 11467a1e3a90..d0df5bc616c0 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -30,6 +30,7 @@
#include <asm/hypercall.h>
#include <asm/mc146818rtc.h>
#include <asm/pv/domain.h>
+#include <asm/pv/trace.h>
#include <asm/shared.h>
#include <xsm/xsm.h>
diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c
index 335aa8dc5dc0..ac5b66870c8c 100644
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -20,7 +20,7 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/trace.h>
+#include <asm/pv/trace.h>
#include <asm/shadow.h>
#include "emulate.h"
diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/pv/trace.c
similarity index 95%
rename from xen/arch/x86/trace.c
rename to xen/arch/x86/pv/trace.c
index 4a953c5b2f1e..550c22765bae 100644
--- a/xen/arch/x86/trace.c
+++ b/xen/arch/x86/pv/trace.c
@@ -1,9 +1,6 @@
-#include <xen/init.h>
-#include <xen/kernel.h>
-#include <xen/lib.h>
-#include <xen/domain.h>
#include <xen/sched.h>
-#include <xen/trace.h>
+
+#include <asm/pv/trace.h>
void __trace_pv_trap(int trapnr, unsigned long eip,
int use_error_code, unsigned error_code)
@@ -21,7 +18,7 @@ void __trace_pv_trap(int trapnr, unsigned long eip,
d.trapnr = trapnr;
d.error_code = error_code;
d.use_error_code=!!use_error_code;
-
+
__trace_var(TRC_PV_TRAP, 1, sizeof(d), &d);
}
else
@@ -38,7 +35,7 @@ void __trace_pv_trap(int trapnr, unsigned long eip,
d.trapnr = trapnr;
d.error_code = error_code;
d.use_error_code=!!use_error_code;
-
+
event = TRC_PV_TRAP;
event |= TRC_64_FLAG;
__trace_var(event, 1, sizeof(d), &d);
@@ -58,7 +55,7 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
d.eip = eip;
d.addr = addr;
d.error_code = error_code;
-
+
__trace_var(TRC_PV_PAGE_FAULT, 1, sizeof(d), &d);
}
else
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 7439b76df88d..764773c02104 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -22,10 +22,10 @@
#include <xen/event.h>
#include <xen/hypercall.h>
#include <xen/lib.h>
-#include <xen/trace.h>
#include <xen/softirq.h>
#include <asm/apic.h>
+#include <asm/pv/trace.h>
#include <asm/shared.h>
#include <asm/traps.h>
#include <irq_vectors.h>
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4a0e498b4c21..0cc1ee95cb5b 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -82,6 +82,7 @@
#include <xsm/xsm.h>
#include <asm/mach-default/irq_vectors.h>
#include <asm/pv/traps.h>
+#include <asm/pv/trace.h>
#include <asm/pv/mm.h>
/*
@@ -1480,7 +1481,7 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
{
int ret = paging_fault(addr, regs);
- if ( ret == EXCRET_fault_fixed )
+ if ( IS_ENABLED(CONFIG_PV) && ret == EXCRET_fault_fixed )
trace_trap_two_addr(TRC_PV_PAGING_FIXUP, regs->rip, addr);
return ret;
}
diff --git a/xen/include/asm-x86/trace.h b/xen/include/asm-x86/pv/trace.h
similarity index 92%
copy from xen/include/asm-x86/trace.h
copy to xen/include/asm-x86/pv/trace.h
index e65b5de6eec4..c616206eeb15 100644
--- a/xen/include/asm-x86/trace.h
+++ b/xen/include/asm-x86/pv/trace.h
@@ -1,5 +1,7 @@
-#ifndef __ASM_TRACE_H__
-#define __ASM_TRACE_H__
+#ifndef XEN_X86_PV_TRACE_H
+#define XEN_X86_PV_TRACE_H
+
+#include <xen/trace.h>
#include <asm/page.h>
@@ -43,4 +45,4 @@ static inline void trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
__trace_ptwr_emulation(addr, npte);
}
-#endif /* __ASM_TRACE_H__ */
+#endif /* XEN_X86_PV_TRACE_H */
diff --git a/xen/include/asm-x86/trace.h b/xen/include/asm-x86/trace.h
index e65b5de6eec4..edef1bb099d4 100644
--- a/xen/include/asm-x86/trace.h
+++ b/xen/include/asm-x86/trace.h
@@ -1,46 +1,4 @@
#ifndef __ASM_TRACE_H__
#define __ASM_TRACE_H__
-#include <asm/page.h>
-
-void __trace_pv_trap(int trapnr, unsigned long eip,
- int use_error_code, unsigned error_code);
-static inline void trace_pv_trap(int trapnr, unsigned long eip,
- int use_error_code, unsigned error_code)
-{
- if ( unlikely(tb_init_done) )
- __trace_pv_trap(trapnr, eip, use_error_code, error_code);
-}
-
-void __trace_pv_page_fault(unsigned long addr, unsigned error_code);
-static inline void trace_pv_page_fault(unsigned long addr,
- unsigned error_code)
-{
- if ( unlikely(tb_init_done) )
- __trace_pv_page_fault(addr, error_code);
-}
-
-void __trace_trap_one_addr(unsigned event, unsigned long va);
-static inline void trace_trap_one_addr(unsigned event, unsigned long va)
-{
- if ( unlikely(tb_init_done) )
- __trace_trap_one_addr(event, va);
-}
-
-void __trace_trap_two_addr(unsigned event, unsigned long va1,
- unsigned long va2);
-static inline void trace_trap_two_addr(unsigned event, unsigned long va1,
- unsigned long va2)
-{
- if ( unlikely(tb_init_done) )
- __trace_trap_two_addr(event, va1, va2);
-}
-
-void __trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte);
-static inline void trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
-{
- if ( unlikely(tb_init_done) )
- __trace_ptwr_emulation(addr, npte);
-}
-
#endif /* __ASM_TRACE_H__ */
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 10/12] x86/pv: Move x86/trace.c to x86/pv/trace.c
2021-09-20 17:25 ` [PATCH v2 10/12] x86/pv: Move x86/trace.c to x86/pv/trace.c Andrew Cooper
@ 2021-09-21 15:58 ` Jan Beulich
0 siblings, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-09-21 15:58 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 20.09.2021 19:25, Andrew Cooper wrote:
> This entire file is pv-only, and not excluded from the build by
> CONFIG_TRACEBUFFER. Move it into the pv/ directory, build it conditionally,
> and drop unused includes.
>
> Also move the contents of asm/trace.h to asm/pv/trace.h to avoid the functions
> being declared across the entire hypervisor.
>
> One caller in fixup_page_fault() is effectively PV only, but is not subject to
> dead code elimination. Add an additional IS_ENABLED(CONFIG_PV) to keep the
> build happy.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 11/12] xen/arch: Drop asm-*/trace.h
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (9 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 10/12] x86/pv: Move x86/trace.c to x86/pv/trace.c Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-21 16:01 ` Jan Beulich
2021-10-12 12:31 ` Julien Grall
2021-09-20 17:25 ` [PATCH v2 12/12] x86/trace: Clean up trace handling Andrew Cooper
` (5 subsequent siblings)
16 siblings, 2 replies; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk
The feature is x86 and pv-dom0 specific, and each architecture's main trace.h
files are empty. Don't force all new architectures to create an empty file.
While moving the declaration of tb_init_done, change from int to bool.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
v2:
* New
---
xen/common/trace.c | 2 +-
xen/include/asm-arm/trace.h | 12 ------------
xen/include/asm-x86/trace.h | 4 ----
xen/include/xen/trace.h | 13 +++++--------
4 files changed, 6 insertions(+), 25 deletions(-)
delete mode 100644 xen/include/asm-arm/trace.h
delete mode 100644 xen/include/asm-x86/trace.h
diff --git a/xen/common/trace.c b/xen/common/trace.c
index 41a3c170446b..a5dc468296d6 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -67,7 +67,7 @@ static DEFINE_PER_CPU(unsigned long, lost_records_first_tsc);
/* a flag recording whether initialization has been done */
/* or more properly, if the tbuf subsystem is enabled right now */
-int tb_init_done __read_mostly;
+bool __read_mostly tb_init_done;
/* which CPUs tracing is enabled on */
static cpumask_t tb_cpu_mask;
diff --git a/xen/include/asm-arm/trace.h b/xen/include/asm-arm/trace.h
deleted file mode 100644
index e06def61f6f3..000000000000
--- a/xen/include/asm-arm/trace.h
+++ /dev/null
@@ -1,12 +0,0 @@
-#ifndef __ASM_TRACE_H__
-#define __ASM_TRACE_H__
-
-#endif /* __ASM_TRACE_H__ */
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/include/asm-x86/trace.h b/xen/include/asm-x86/trace.h
deleted file mode 100644
index edef1bb099d4..000000000000
--- a/xen/include/asm-x86/trace.h
+++ /dev/null
@@ -1,4 +0,0 @@
-#ifndef __ASM_TRACE_H__
-#define __ASM_TRACE_H__
-
-#endif /* __ASM_TRACE_H__ */
diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
index 3cbb542af888..055883287e8c 100644
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -21,19 +21,14 @@
#ifndef __XEN_TRACE_H__
#define __XEN_TRACE_H__
-/* Put 'tb_init_done' here because 'asm/trace.h' may use it */
-#ifdef CONFIG_TRACEBUFFER
-extern int tb_init_done;
-#else
-#define tb_init_done false
-#endif
-
#include <xen/types.h>
#include <public/sysctl.h>
#include <public/trace.h>
-#include <asm/trace.h>
#ifdef CONFIG_TRACEBUFFER
+
+extern bool tb_init_done;
+
/* Used to initialise trace buffer functionality */
void init_trace_bufs(void);
@@ -58,6 +53,8 @@ void __trace_hypercall(uint32_t event, unsigned long op,
#include <xen/errno.h>
+#define tb_init_done false
+
static inline void init_trace_bufs(void) {}
static inline int tb_control(struct xen_sysctl_tbuf_op *tbc)
{
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 11/12] xen/arch: Drop asm-*/trace.h
2021-09-20 17:25 ` [PATCH v2 11/12] xen/arch: Drop asm-*/trace.h Andrew Cooper
@ 2021-09-21 16:01 ` Jan Beulich
2021-10-12 12:31 ` Julien Grall
1 sibling, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-09-21 16:01 UTC (permalink / raw)
To: Andrew Cooper
Cc: Roger Pau Monné,
Wei Liu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Xen-devel
On 20.09.2021 19:25, Andrew Cooper wrote:
> The feature is x86 and pv-dom0 specific, and each architecture's main trace.h
> files are empty. Don't force all new architectures to create an empty file.
The first half sentence looks to be wrong according to Julien's
reply to patch 16. But the change here doesn't seem to depend on
that aspect in any way.
> While moving the declaration of tb_init_done, change from int to bool.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
With a suitable adjustment to the text:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 11/12] xen/arch: Drop asm-*/trace.h
2021-09-20 17:25 ` [PATCH v2 11/12] xen/arch: Drop asm-*/trace.h Andrew Cooper
2021-09-21 16:01 ` Jan Beulich
@ 2021-10-12 12:31 ` Julien Grall
1 sibling, 0 replies; 52+ messages in thread
From: Julien Grall @ 2021-10-12 12:31 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Jan Beulich, Roger Pau Monné,
Wei Liu, Stefano Stabellini, Volodymyr Babchuk
Hi Andrew,
On 20/09/2021 18:25, Andrew Cooper wrote:
> The feature is x86 and pv-dom0 specific, and each architecture's main trace.h
> files are empty. Don't force all new architectures to create an empty file.
>
> While moving the declaration of tb_init_done, change from int to bool.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
For the Arm bits:
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 12/12] x86/trace: Clean up trace handling
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (10 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 11/12] xen/arch: Drop asm-*/trace.h Andrew Cooper
@ 2021-09-20 17:25 ` Andrew Cooper
2021-09-21 16:08 ` Jan Beulich
2021-09-20 19:29 ` [PATCH v2.1 13/12] xen/trace: Introduce new API Andrew Cooper
` (4 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 17:25 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu
Use more appropriate types.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
v2:
* New
---
xen/arch/x86/irq.c | 4 +-
xen/arch/x86/mm/p2m-pt.c | 6 +-
xen/arch/x86/mm/shadow/multi.c | 2 +-
xen/arch/x86/pv/trace.c | 159 +++++++++++++++++++----------------------
4 files changed, 78 insertions(+), 93 deletions(-)
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index a1693f92dd92..67cbf6b979dc 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -130,8 +130,8 @@ static void _trace_irq_mask(uint32_t event, int irq, int vector,
const cpumask_t *mask)
{
struct {
- unsigned int irq:16, vec:16;
- unsigned int mask[6];
+ uint16_t irq, vec;
+ uint32_t mask[6];
} d = {
.irq = irq,
.vec = vector,
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 5a0c0f5aceff..09c99d78aa40 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -609,9 +609,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
if ( tb_init_done )
{
struct {
- u64 gfn, mfn;
- int p2mt;
- int d:16,order:16;
+ uint64_t gfn, mfn;
+ uint32_t p2mt;
+ uint16_t d, order;
} t;
t.gfn = gfn;
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 8bb028c2e2fa..15265fc81dca 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2118,7 +2118,7 @@ static inline void trace_shadow_emulate(guest_l1e_t gl1e, unsigned long va)
so put it first for alignment sake. */
guest_l1e_t gl1e, write_val;
guest_va_t va;
- unsigned flags:29, emulation_count:3;
+ uint32_t flags:29, emulation_count:3;
} d;
u32 event;
diff --git a/xen/arch/x86/pv/trace.c b/xen/arch/x86/pv/trace.c
index 550c22765bae..a952fbc1eb0f 100644
--- a/xen/arch/x86/pv/trace.c
+++ b/xen/arch/x86/pv/trace.c
@@ -7,38 +7,35 @@ void __trace_pv_trap(int trapnr, unsigned long eip,
{
if ( is_pv_32bit_vcpu(current) )
{
- struct __packed {
- unsigned eip:32,
- trapnr:15,
- use_error_code:1,
- error_code:16;
- } d;
-
- d.eip = eip;
- d.trapnr = trapnr;
- d.error_code = error_code;
- d.use_error_code=!!use_error_code;
+ struct {
+ uint32_t eip;
+ uint16_t trapnr:15;
+ bool use_error_code:1;
+ uint16_t error_code;
+ } d = {
+ .eip = eip,
+ .trapnr = trapnr,
+ .use_error_code = use_error_code,
+ .error_code = error_code,
+ };
__trace_var(TRC_PV_TRAP, 1, sizeof(d), &d);
}
else
{
struct __packed {
- unsigned long eip;
- unsigned trapnr:15,
- use_error_code:1,
- error_code:16;
- } d;
- unsigned event;
-
- d.eip = eip;
- d.trapnr = trapnr;
- d.error_code = error_code;
- d.use_error_code=!!use_error_code;
-
- event = TRC_PV_TRAP;
- event |= TRC_64_FLAG;
- __trace_var(event, 1, sizeof(d), &d);
+ uint64_t rip;
+ uint16_t trapnr:15;
+ bool use_error_code:1;
+ uint16_t error_code;
+ } d = {
+ .rip = eip,
+ .trapnr = trapnr,
+ .use_error_code = use_error_code,
+ .error_code = error_code,
+ };
+
+ __trace_var(TRC_PV_TRAP | TRC_64_FLAG, 1, sizeof(d), &d);
}
}
@@ -48,30 +45,28 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
if ( is_pv_32bit_vcpu(current) )
{
- struct __packed {
- u32 eip, addr, error_code;
- } d;
-
- d.eip = eip;
- d.addr = addr;
- d.error_code = error_code;
+ struct {
+ uint32_t eip, addr, error_code;
+ } d = {
+ .eip = eip,
+ .addr = addr,
+ .error_code = error_code,
+ };
__trace_var(TRC_PV_PAGE_FAULT, 1, sizeof(d), &d);
}
else
{
struct __packed {
- unsigned long eip, addr;
- u32 error_code;
- } d;
- unsigned event;
-
- d.eip = eip;
- d.addr = addr;
- d.error_code = error_code;
- event = TRC_PV_PAGE_FAULT;
- event |= TRC_64_FLAG;
- __trace_var(event, 1, sizeof(d), &d);
+ uint64_t eip, addr;
+ uint32_t error_code;
+ } d = {
+ .eip = eip,
+ .addr = addr,
+ .error_code = error_code,
+ };
+
+ __trace_var(TRC_PV_PAGE_FAULT | TRC_64_FLAG, 1, sizeof(d), &d);
}
}
@@ -83,10 +78,7 @@ void __trace_trap_one_addr(unsigned event, unsigned long va)
__trace_var(event, 1, sizeof(d), &d);
}
else
- {
- event |= TRC_64_FLAG;
- __trace_var(event, 1, sizeof(va), &va);
- }
+ __trace_var(event | TRC_64_FLAG, 1, sizeof(va), &va);
}
void __trace_trap_two_addr(unsigned event, unsigned long va1,
@@ -94,22 +86,25 @@ void __trace_trap_two_addr(unsigned event, unsigned long va1,
{
if ( is_pv_32bit_vcpu(current) )
{
- struct __packed {
- u32 va1, va2;
- } d;
- d.va1=va1;
- d.va2=va2;
+ struct {
+ uint32_t va1, va2;
+ } d = {
+ .va1 = va1,
+ .va2 = va2,
+ };
+
__trace_var(event, 1, sizeof(d), &d);
}
else
{
- struct __packed {
- unsigned long va1, va2;
- } d;
- d.va1=va1;
- d.va2=va2;
- event |= TRC_64_FLAG;
- __trace_var(event, 1, sizeof(d), &d);
+ struct {
+ uint64_t va1, va2;
+ } d = {
+ .va1 = va1,
+ .va2 = va2,
+ };
+
+ __trace_var(event | TRC_64_FLAG, 1, sizeof(d), &d);
}
}
@@ -117,40 +112,30 @@ void __trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
{
unsigned long eip = guest_cpu_user_regs()->rip;
- /* We have a couple of different modes to worry about:
- * - 32-on-32: 32-bit pte, 32-bit virtual addresses
- * - pae-on-pae, pae-on-64: 64-bit pte, 32-bit virtual addresses
- * - 64-on-64: 64-bit pte, 64-bit virtual addresses
- * pae-on-64 is the only one that requires extra code; in all other
- * cases, "unsigned long" is the size of a guest virtual address.
- */
-
if ( is_pv_32bit_vcpu(current) )
{
- struct __packed {
- l1_pgentry_t pte;
- u32 addr, eip;
- } d;
- d.addr = addr;
- d.eip = eip;
- d.pte = npte;
+ struct {
+ uint64_t pte;
+ uint32_t addr, eip;
+ } d = {
+ .pte = l1e_get_intpte(npte),
+ .addr = addr,
+ .eip = eip,
+ };
__trace_var(TRC_PV_PTWR_EMULATION_PAE, 1, sizeof(d), &d);
}
else
{
struct {
- l1_pgentry_t pte;
- unsigned long addr, eip;
- } d;
- unsigned event;
-
- d.addr = addr;
- d.eip = eip;
- d.pte = npte;
-
- event = TRC_PV_PTWR_EMULATION;
- event |= TRC_64_FLAG;
- __trace_var(event, 1/*tsc*/, sizeof(d), &d);
+ uint64_t pte;
+ uint64_t addr, rip;
+ } d = {
+ .pte = l1e_get_intpte(npte),
+ .addr = addr,
+ .rip = eip,
+ };
+
+ __trace_var(TRC_PV_PTWR_EMULATION | TRC_64_FLAG, 1, sizeof(d), &d);
}
}
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 12/12] x86/trace: Clean up trace handling
2021-09-20 17:25 ` [PATCH v2 12/12] x86/trace: Clean up trace handling Andrew Cooper
@ 2021-09-21 16:08 ` Jan Beulich
2021-09-21 18:34 ` Andrew Cooper
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-09-21 16:08 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 20.09.2021 19:25, Andrew Cooper wrote:
> Use more appropriate types.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a suggestion:
> @@ -48,30 +45,28 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
>
> if ( is_pv_32bit_vcpu(current) )
> {
> - struct __packed {
> - u32 eip, addr, error_code;
> - } d;
> -
> - d.eip = eip;
> - d.addr = addr;
> - d.error_code = error_code;
> + struct {
> + uint32_t eip, addr, error_code;
> + } d = {
> + .eip = eip,
> + .addr = addr,
> + .error_code = error_code,
> + };
>
> __trace_var(TRC_PV_PAGE_FAULT, 1, sizeof(d), &d);
> }
> else
> {
> struct __packed {
> - unsigned long eip, addr;
> - u32 error_code;
> - } d;
> - unsigned event;
> -
> - d.eip = eip;
> - d.addr = addr;
> - d.error_code = error_code;
> - event = TRC_PV_PAGE_FAULT;
> - event |= TRC_64_FLAG;
> - __trace_var(event, 1, sizeof(d), &d);
> + uint64_t eip, addr;
Like you've done in __trace_pv_trap() and __trace_ptwr_emulation(),
perhaps name the field "rip" here as well?
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 12/12] x86/trace: Clean up trace handling
2021-09-21 16:08 ` Jan Beulich
@ 2021-09-21 18:34 ` Andrew Cooper
0 siblings, 0 replies; 52+ messages in thread
From: Andrew Cooper @ 2021-09-21 18:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 21/09/2021 17:08, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
>> Use more appropriate types.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
> with a suggestion:
>
>> @@ -48,30 +45,28 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
>>
>> if ( is_pv_32bit_vcpu(current) )
>> {
>> - struct __packed {
>> - u32 eip, addr, error_code;
>> - } d;
>> -
>> - d.eip = eip;
>> - d.addr = addr;
>> - d.error_code = error_code;
>> + struct {
>> + uint32_t eip, addr, error_code;
>> + } d = {
>> + .eip = eip,
>> + .addr = addr,
>> + .error_code = error_code,
>> + };
>>
>> __trace_var(TRC_PV_PAGE_FAULT, 1, sizeof(d), &d);
>> }
>> else
>> {
>> struct __packed {
>> - unsigned long eip, addr;
>> - u32 error_code;
>> - } d;
>> - unsigned event;
>> -
>> - d.eip = eip;
>> - d.addr = addr;
>> - d.error_code = error_code;
>> - event = TRC_PV_PAGE_FAULT;
>> - event |= TRC_64_FLAG;
>> - __trace_var(event, 1, sizeof(d), &d);
>> + uint64_t eip, addr;
> Like you've done in __trace_pv_trap() and __trace_ptwr_emulation(),
> perhaps name the field "rip" here as well?
Fixed. (Honestly - these all start to blur together given how large the
series is.)
~Andrew
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2.1 13/12] xen/trace: Introduce new API
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (11 preceding siblings ...)
2021-09-20 17:25 ` [PATCH v2 12/12] x86/trace: Clean up trace handling Andrew Cooper
@ 2021-09-20 19:29 ` Andrew Cooper
2021-09-24 13:21 ` Jan Beulich
2021-09-20 19:32 ` [PATCH v2.1 14/12] xen: Switch to new TRACE() API Andrew Cooper
` (3 subsequent siblings)
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 19:29 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall
This will be used to clean up mess of macros which exists throughout the
codebase.
Signed-off-by: Andrew Cooper <andrew.cooper3@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>
v2.1:
* New
---
xen/include/xen/trace.h | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
index 055883287e8c..72c20550f6a6 100644
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -74,6 +74,30 @@ static inline void __trace_hypercall(uint32_t event, unsigned long op,
const xen_ulong_t *args) {}
#endif /* CONFIG_TRACEBUFFER */
+/*
+ * Create a trace record, packaging up to 7 additional parameters into a
+ * uint32_t array.
+ */
+#define TRACE_INTERNAL(_e, _c, ...) \
+ do { \
+ if ( unlikely(tb_init_done) ) \
+ { \
+ uint32_t _d[] = { __VA_ARGS__ }; \
+ BUILD_BUG_ON(ARRAY_SIZE(_d) > TRACE_EXTRA_MAX); \
+ __trace_var(_e, _c, sizeof(_d), sizeof(_d) ? _d : NULL); \
+ } \
+ } while ( 0 )
+
+/* Split a uint64_t into two adjacent uint32_t's for a trace record. */
+#define TRACE_PARAM64(p) (uint32_t)(p), ((p) >> 32)
+
+/* Create a trace record with time included. */
+#define TRACE_TIME(_e, ...) TRACE_INTERNAL(_e, true, ##__VA_ARGS__)
+
+/* Create a trace record with no time included. */
+#define TRACE(_e, ...) TRACE_INTERNAL(_e, false, ##__VA_ARGS__)
+
+
/* Convenience macros for calling the trace function. */
#define TRACE_0D(_e) \
do { \
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2.1 13/12] xen/trace: Introduce new API
2021-09-20 19:29 ` [PATCH v2.1 13/12] xen/trace: Introduce new API Andrew Cooper
@ 2021-09-24 13:21 ` Jan Beulich
0 siblings, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-09-24 13:21 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Xen-devel
On 20.09.2021 21:29, Andrew Cooper wrote:
> --- a/xen/include/xen/trace.h
> +++ b/xen/include/xen/trace.h
> @@ -74,6 +74,30 @@ static inline void __trace_hypercall(uint32_t event, unsigned long op,
> const xen_ulong_t *args) {}
> #endif /* CONFIG_TRACEBUFFER */
>
> +/*
> + * Create a trace record, packaging up to 7 additional parameters into a
> + * uint32_t array.
> + */
> +#define TRACE_INTERNAL(_e, _c, ...) \
> + do { \
> + if ( unlikely(tb_init_done) ) \
> + { \
> + uint32_t _d[] = { __VA_ARGS__ }; \
> + BUILD_BUG_ON(ARRAY_SIZE(_d) > TRACE_EXTRA_MAX); \
> + __trace_var(_e, _c, sizeof(_d), sizeof(_d) ? _d : NULL); \
> + } \
> + } while ( 0 )
I know we sort of disagree on this aspect, but I would really like
to understand what you (and others) think the leading underscores
are good for in macro parameter names. And if those went away, I'd
like to ask that the local variable also become e.g. d_, like we
have started doing elsewhere.
> +/* Split a uint64_t into two adjacent uint32_t's for a trace record. */
> +#define TRACE_PARAM64(p) (uint32_t)(p), ((p) >> 32)
You don't have a leading underscore here, for example.
> +/* Create a trace record with time included. */
> +#define TRACE_TIME(_e, ...) TRACE_INTERNAL(_e, true, ##__VA_ARGS__)
> +
> +/* Create a trace record with no time included. */
> +#define TRACE(_e, ...) TRACE_INTERNAL(_e, false, ##__VA_ARGS__)
Is , ## __VA_ARGS__ really doing what you expect? So far it has been
my understanding that the special behavior concatenating with a
comma only applies to the GNU form of variable macro arguments, e.g.
#define TRACE(_e, args...) TRACE_INTERNAL(_e, false, ## args)
As a minor aspect (nit) - iirc it was you who had been asking me in a
few cases to treat ## like a normal binary operator when considering
style, requesting me to have a blank on each side of it.
> +
> +
Nit: Please can you avoid introducing double blank lines?
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2.1 14/12] xen: Switch to new TRACE() API
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (12 preceding siblings ...)
2021-09-20 19:29 ` [PATCH v2.1 13/12] xen/trace: Introduce new API Andrew Cooper
@ 2021-09-20 19:32 ` Andrew Cooper
2021-09-24 13:34 ` Jan Beulich
2021-09-24 15:30 ` Dario Faggioli
2021-09-20 19:33 ` [PATCH v2.1 15/12] xen/trace: Drop old trace macros Andrew Cooper
` (2 subsequent siblings)
16 siblings, 2 replies; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 19:32 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall
(Almost) no functional change.
irq_move_cleanup_interrupt() changes two smp_processor_id() calls to the 'me'
local variable which manifests as a minor code improvement. All other
differences in the compiled binary are to do with line numbers changing.
Some conversion notes:
* HVMTRACE_LONG_[234]D() and TRACE_2_LONG_[234]D() were latently buggy. They
blindly discard extra parameters, but luckily no users are impacted. They
are also obfuscated wrappers, depending on exactly one or two parameters
being TRC_PAR_LONG() to compile successfully.
* HVMTRACE_LONG_1D() behaves unlike its named companions, and takes exactly
one 64bit parameter which it splits manually. It's one user,
vmx_cr_access()'s LMSW path, is gets adjusted to use TRACE_PARAM64().
* TRACE_?D() and TRACE_2_LONG_*() change to TRACE_TIME() as cycles is always.
* HVMTRACE_ND() is opencoded for VMENTRY/VMEXIT records to include cycles.
These are converted to TRACE_TIME(), with the old modifier parameter
expressed as an OR at the callsite. One callsite, svm_vmenter_helper() had
a nested tb_init_done check, which is dropped. (The optimiser also spotted
this, which is why it doesn't manifest as a binary difference.)
* All HVMTRACE_?D() change to TRACE() as cycles is explicitly skipped.
Signed-off-by: Andrew Cooper <andrew.cooper3@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>
v2.1:
* New
I'm in two minds as to whether to split this up by subsystem or not. It is
95% x86, and isn't a massive patch.
---
xen/arch/x86/acpi/cpu_idle.c | 12 +++++-----
xen/arch/x86/compat.c | 4 ++--
xen/arch/x86/cpu/mwait-idle.c | 6 ++---
xen/arch/x86/emul-i8254.c | 14 +++++------
xen/arch/x86/hvm/emulate.c | 8 +++----
xen/arch/x86/hvm/hpet.c | 6 ++---
xen/arch/x86/hvm/hvm.c | 16 ++++++-------
xen/arch/x86/hvm/rtc.c | 12 +++++-----
xen/arch/x86/hvm/svm/intr.c | 6 ++---
xen/arch/x86/hvm/svm/svm.c | 40 +++++++++++++++-----------------
xen/arch/x86/hvm/vlapic.c | 29 +++++++++++------------
xen/arch/x86/hvm/vmx/intr.c | 6 ++---
xen/arch/x86/hvm/vmx/vmx.c | 54 +++++++++++++++++++++----------------------
xen/arch/x86/hvm/vpic.c | 13 +++++------
xen/arch/x86/irq.c | 14 +++++------
xen/arch/x86/traps.c | 2 +-
xen/common/domain.c | 4 ++--
xen/common/grant_table.c | 6 ++---
xen/common/sched/core.c | 48 ++++++++++++++++++--------------------
xen/common/sched/credit.c | 30 +++++++++++-------------
xen/drivers/cpufreq/utility.c | 2 +-
21 files changed, 159 insertions(+), 173 deletions(-)
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index d788c8bffc84..98cbb8dd8316 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -752,7 +752,7 @@ static void acpi_processor_idle(void)
/* Get start time (ticks) */
t1 = alternative_call(cpuidle_get_tick);
/* Trace cpu idle entry */
- TRACE_4D(TRC_PM_IDLE_ENTRY, cx->idx, t1, exp, pred);
+ TRACE_TIME(TRC_PM_IDLE_ENTRY, cx->idx, t1, exp, pred);
update_last_cx_stat(power, cx, t1);
@@ -762,8 +762,8 @@ static void acpi_processor_idle(void)
t2 = alternative_call(cpuidle_get_tick);
trace_exit_reason(irq_traced);
/* Trace cpu idle exit */
- TRACE_6D(TRC_PM_IDLE_EXIT, cx->idx, t2,
- irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
+ TRACE_TIME(TRC_PM_IDLE_EXIT, cx->idx, t2,
+ irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
/* Update statistics */
update_idle_stats(power, cx, t1, t2);
/* Re-enable interrupts */
@@ -783,7 +783,7 @@ static void acpi_processor_idle(void)
/* Get start time (ticks) */
t1 = alternative_call(cpuidle_get_tick);
/* Trace cpu idle entry */
- TRACE_4D(TRC_PM_IDLE_ENTRY, cx->idx, t1, exp, pred);
+ TRACE_TIME(TRC_PM_IDLE_ENTRY, cx->idx, t1, exp, pred);
update_last_cx_stat(power, cx, t1);
@@ -838,8 +838,8 @@ static void acpi_processor_idle(void)
cstate_restore_tsc();
trace_exit_reason(irq_traced);
/* Trace cpu idle exit */
- TRACE_6D(TRC_PM_IDLE_EXIT, cx->idx, t2,
- irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
+ TRACE_TIME(TRC_PM_IDLE_EXIT, cx->idx, t2,
+ irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
/* Update statistics */
update_idle_stats(power, cx, t1, t2);
diff --git a/xen/arch/x86/compat.c b/xen/arch/x86/compat.c
index 58b202f701d5..e4e09f34b87e 100644
--- a/xen/arch/x86/compat.c
+++ b/xen/arch/x86/compat.c
@@ -42,8 +42,8 @@ long do_sched_op_compat(int cmd, unsigned long arg)
return fn(cmd, guest_handle_from_ptr(NULL, void));
case SCHEDOP_shutdown:
- TRACE_3D(TRC_SCHED_SHUTDOWN,
- current->domain->domain_id, current->vcpu_id, arg);
+ TRACE_TIME(TRC_SCHED_SHUTDOWN,
+ current->domain->domain_id, current->vcpu_id, arg);
domain_shutdown(current->domain, (u8)arg);
break;
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index f0c6ff9d5229..43567865764b 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -808,7 +808,7 @@ static void mwait_idle(void)
lapic_timer_off();
before = alternative_call(cpuidle_get_tick);
- TRACE_4D(TRC_PM_IDLE_ENTRY, cx->type, before, exp, pred);
+ TRACE_TIME(TRC_PM_IDLE_ENTRY, cx->type, before, exp, pred);
update_last_cx_stat(power, cx, before);
@@ -819,8 +819,8 @@ static void mwait_idle(void)
cstate_restore_tsc();
trace_exit_reason(irq_traced);
- TRACE_6D(TRC_PM_IDLE_EXIT, cx->type, after,
- irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
+ TRACE_TIME(TRC_PM_IDLE_EXIT, cx->type, after,
+ irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
/* Now back in C0. */
update_idle_stats(power, cx, before, after);
diff --git a/xen/arch/x86/emul-i8254.c b/xen/arch/x86/emul-i8254.c
index 050c784702af..19b5c95af216 100644
--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -159,7 +159,7 @@ static int pit_get_gate(PITState *pit, int channel)
static void pit_time_fired(struct vcpu *v, void *priv)
{
uint64_t *count_load_time = priv;
- TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB);
+ TRACE_TIME(TRC_HVM_EMUL_PIT_TIMER_CB);
*count_load_time = get_guest_time(v);
}
@@ -189,19 +189,19 @@ static void pit_load_count(PITState *pit, int channel, int val)
case 2:
case 3:
/* Periodic timer. */
- TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period);
+ TRACE_TIME(TRC_HVM_EMUL_PIT_START_TIMER, period, period);
create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired,
&pit->count_load_time[channel], false);
break;
case 1:
case 4:
/* One-shot timer. */
- TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0);
+ TRACE_TIME(TRC_HVM_EMUL_PIT_START_TIMER, period, 0);
create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired,
&pit->count_load_time[channel], false);
break;
default:
- TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_PIT_STOP_TIMER);
destroy_periodic_time(&pit->pt0);
break;
}
@@ -385,7 +385,7 @@ void pit_stop_channel0_irq(PITState *pit)
if ( !has_vpit(current->domain) )
return;
- TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_PIT_STOP_TIMER);
spin_lock(&pit->lock);
destroy_periodic_time(&pit->pt0);
spin_unlock(&pit->lock);
@@ -454,7 +454,7 @@ void pit_reset(struct domain *d)
if ( is_hvm_domain(d) )
{
- TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_PIT_STOP_TIMER);
destroy_periodic_time(&pit->pt0);
pit->pt0.source = PTSRC_isa;
}
@@ -499,7 +499,7 @@ void pit_deinit(struct domain *d)
if ( is_hvm_domain(d) )
{
- TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_PIT_STOP_TIMER);
destroy_periodic_time(&pit->pt0);
}
}
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 425c8ddd9779..f863a19df311 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2183,7 +2183,7 @@ static int hvmemul_read_cr(
case 3:
case 4:
*val = current->arch.hvm.guest_cr[reg];
- HVMTRACE_LONG_2D(CR_READ, reg, TRC_PAR_LONG(*val));
+ TRACE(TRC_HVM_CR_READ64, reg, TRACE_PARAM64(*val));
return X86EMUL_OKAY;
default:
break;
@@ -2199,7 +2199,7 @@ static int hvmemul_write_cr(
{
int rc;
- HVMTRACE_LONG_2D(CR_WRITE, reg, TRC_PAR_LONG(val));
+ TRACE(TRC_HVM_CR_WRITE64, reg, TRACE_PARAM64(val));
switch ( reg )
{
case 0:
@@ -2244,7 +2244,7 @@ static int hvmemul_read_xcr(
int rc = x86emul_read_xcr(reg, val, ctxt);
if ( rc == X86EMUL_OKAY )
- HVMTRACE_LONG_2D(XCR_READ, reg, TRC_PAR_LONG(*val));
+ TRACE(TRC_HVM_XCR_READ64, reg, TRACE_PARAM64(*val));
return rc;
}
@@ -2254,7 +2254,7 @@ static int hvmemul_write_xcr(
uint64_t val,
struct x86_emulate_ctxt *ctxt)
{
- HVMTRACE_LONG_2D(XCR_WRITE, reg, TRC_PAR_LONG(val));
+ TRACE(TRC_HVM_XCR_WRITE64, reg, TRACE_PARAM64(val));
return x86emul_write_xcr(reg, val, ctxt);
}
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 8267f0b8a278..5873bb8a506d 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -212,7 +212,7 @@ static void hpet_stop_timer(HPETState *h, unsigned int tn,
{
ASSERT(tn < HPET_TIMER_NUM);
ASSERT(rw_is_write_locked(&h->lock));
- TRACE_1D(TRC_HVM_EMUL_HPET_STOP_TIMER, tn);
+ TRACE_TIME(TRC_HVM_EMUL_HPET_STOP_TIMER, tn);
destroy_periodic_time(&h->pt[tn]);
/* read the comparator to get it updated so a read while stopped will
* return the expected value. */
@@ -316,8 +316,8 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
if ( !oneshot )
period_ns = hpet_tick_to_ns(h, h->hpet.period[tn]);
- TRACE_2_LONG_4D(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq,
- TRC_PAR_LONG(diff_ns), TRC_PAR_LONG(period_ns));
+ TRACE_TIME(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq,
+ TRACE_PARAM64(diff_ns), TRACE_PARAM64(period_ns));
create_periodic_time(vhpet_vcpu(h), &h->pt[tn], diff_ns, period_ns,
irq, timer_level(h, tn) ? hpet_timer_fired : NULL,
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 09cf6330ad26..fa211d14afd8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1679,7 +1679,7 @@ void hvm_hlt(unsigned int eflags)
do_sched_op(SCHEDOP_block, guest_handle_from_ptr(NULL, void));
- HVMTRACE_1D(HLT, /* pending = */ vcpu_runnable(curr));
+ TRACE(TRC_HVM_HLT, /* pending = */ vcpu_runnable(curr));
}
void hvm_triple_fault(void)
@@ -2136,7 +2136,7 @@ int hvm_mov_to_cr(unsigned int cr, unsigned int gpr)
unsigned long val = *decode_gpr(guest_cpu_user_regs(), gpr);
int rc;
- HVMTRACE_LONG_2D(CR_WRITE, cr, TRC_PAR_LONG(val));
+ TRACE(TRC_HVM_CR_WRITE64, cr, TRACE_PARAM64(val));
HVM_DBG_LOG(DBG_LEVEL_1, "CR%u, value = %lx", cr, val);
switch ( cr )
@@ -2201,7 +2201,7 @@ int hvm_mov_from_cr(unsigned int cr, unsigned int gpr)
}
*reg = val;
- HVMTRACE_LONG_2D(CR_READ, cr, TRC_PAR_LONG(val));
+ TRACE(TRC_HVM_CR_READ64, cr, TRACE_PARAM64(val));
HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR%u, value = %lx", cr, val);
return X86EMUL_OKAY;
@@ -3463,7 +3463,7 @@ int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len)
}
guest_cpuid(curr, leaf, subleaf, &res);
- HVMTRACE_6D(CPUID, leaf, subleaf, res.a, res.b, res.c, res.d);
+ TRACE(TRC_HVM_CPUID, leaf, subleaf, res.a, res.b, res.c, res.d);
regs->rax = res.a;
regs->rbx = res.b;
@@ -3477,7 +3477,7 @@ void hvm_rdtsc_intercept(struct cpu_user_regs *regs)
{
msr_split(regs, hvm_get_guest_tsc(current));
- HVMTRACE_2D(RDTSC, regs->eax, regs->edx);
+ TRACE(TRC_HVM_RDTSC, regs->eax, regs->edx);
}
int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
@@ -3584,8 +3584,7 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
}
out:
- HVMTRACE_3D(MSR_READ, msr,
- (uint32_t)*msr_content, (uint32_t)(*msr_content >> 32));
+ TRACE(TRC_HVM_MSR_READ, msr, TRACE_PARAM64(*msr_content));
return ret;
gp_fault:
@@ -3601,8 +3600,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
struct domain *d = v->domain;
int ret;
- HVMTRACE_3D(MSR_WRITE, msr,
- (uint32_t)msr_content, (uint32_t)(msr_content >> 32));
+ TRACE(TRC_HVM_MSR_WRITE, msr, TRACE_PARAM64(msr_content));
if ( may_defer && unlikely(monitored_msr(v->domain, msr)) )
{
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 3150f5f1479b..6c133eabf5d4 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -92,7 +92,7 @@ static void rtc_pf_callback(struct vcpu *v, void *opaque)
&& ++(s->pt_dead_ticks) >= 10 )
{
/* VM is ignoring its RTC; no point in running the timer */
- TRACE_0D(TRC_HVM_EMUL_RTC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_RTC_STOP_TIMER);
destroy_periodic_time(&s->pt);
s->period = 0;
}
@@ -155,7 +155,7 @@ static void rtc_timer_update(RTCState *s)
delta = period - ((now - s->start_time) % period);
if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
{
- TRACE_2D(TRC_HVM_EMUL_RTC_START_TIMER, delta, period);
+ TRACE_TIME(TRC_HVM_EMUL_RTC_START_TIMER, delta, period);
create_periodic_time(v, &s->pt, delta, period,
RTC_IRQ, rtc_pf_callback, s, false);
}
@@ -166,7 +166,7 @@ static void rtc_timer_update(RTCState *s)
}
/* fall through */
default:
- TRACE_0D(TRC_HVM_EMUL_RTC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_RTC_STOP_TIMER);
destroy_periodic_time(&s->pt);
s->period = 0;
break;
@@ -519,7 +519,7 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
rtc_update_irq(s);
if ( (data ^ orig) & RTC_PIE )
{
- TRACE_0D(TRC_HVM_EMUL_RTC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_RTC_STOP_TIMER);
destroy_periodic_time(&s->pt);
s->period = 0;
rtc_timer_update(s);
@@ -802,7 +802,7 @@ void rtc_reset(struct domain *d)
if ( !has_vrtc(d) )
return;
- TRACE_0D(TRC_HVM_EMUL_RTC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_RTC_STOP_TIMER);
destroy_periodic_time(&s->pt);
s->period = 0;
s->pt.source = PTSRC_isa;
@@ -877,7 +877,7 @@ void rtc_deinit(struct domain *d)
spin_barrier(&s->lock);
- TRACE_0D(TRC_HVM_EMUL_RTC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_RTC_STOP_TIMER);
destroy_periodic_time(&s->pt);
kill_timer(&s->update_timer);
kill_timer(&s->update_timer2);
diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 7f815d230785..122c9d7fbf14 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -101,8 +101,8 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
}
}
- HVMTRACE_3D(INTR_WINDOW, intack.vector, intack.source,
- vmcb->event_inj.v ? vmcb->event_inj.vector : -1);
+ TRACE(TRC_HVM_INTR_WINDOW, intack.vector, intack.source,
+ vmcb->event_inj.v ? vmcb->event_inj.vector : -1);
/*
* Create a dummy virtual interrupt to intercept as soon as the
@@ -217,7 +217,7 @@ void svm_intr_assist(void)
}
else
{
- HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
+ TRACE(TRC_HVM_INJ_VIRQ, intack.vector, /*fake=*/ 0);
svm_inject_extint(v, intack.vector);
pt_intr_post(v, intack);
}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index f0e10dec046e..b98a2dd71d1e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1049,10 +1049,8 @@ void svm_vmenter_helper(const struct cpu_user_regs *regs)
svm_asid_handle_vmrun();
- if ( unlikely(tb_init_done) )
- HVMTRACE_ND(VMENTRY,
- nestedhvm_vcpu_in_guestmode(curr) ? TRC_HVM_NESTEDFLAG : 0,
- 1/*cycles*/);
+ TRACE_TIME(TRC_HVM_VMENTRY |
+ (nestedhvm_vcpu_in_guestmode(curr) ? TRC_HVM_NESTEDFLAG : 0));
svm_sync_vmcb(curr, vmcb_needs_vmsave);
@@ -1424,10 +1422,10 @@ static void svm_inject_event(const struct x86_event *event)
if ( _event.vector == TRAP_page_fault &&
_event.type == X86_EVENTTYPE_HW_EXCEPTION )
- HVMTRACE_LONG_2D(PF_INJECT, _event.error_code,
- TRC_PAR_LONG(_event.cr2));
+ TRACE(TRC_HVM_PF_INJECT64, _event.error_code,
+ TRACE_PARAM64(_event.cr2));
else
- HVMTRACE_2D(INJ_EXC, _event.vector, _event.error_code);
+ TRACE(TRC_HVM_INJ_EXC, _event.vector, _event.error_code);
}
static bool svm_event_pending(const struct vcpu *v)
@@ -1787,7 +1785,7 @@ static void svm_dr_access(struct vcpu *v, struct cpu_user_regs *regs)
{
struct vmcb_struct *vmcb = vcpu_nestedhvm(v).nv_n1vmcx;
- HVMTRACE_0D(DR_WRITE);
+ TRACE(TRC_HVM_DR_WRITE);
__restore_debug_registers(vmcb, v);
}
@@ -2436,7 +2434,7 @@ static void svm_invlpga_intercept(
static void svm_invlpg_intercept(unsigned long linear)
{
- HVMTRACE_LONG_2D(INVLPG, 0, TRC_PAR_LONG(linear));
+ TRACE(TRC_HVM_INVLPG64, 0, TRACE_PARAM64(linear));
paging_invlpg(current, linear);
}
@@ -2564,11 +2562,11 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
exit_reason = vmcb->exitcode;
if ( hvm_long_mode_active(v) )
- HVMTRACE_ND(VMEXIT64, vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0,
- 1/*cycles*/, exit_reason, TRC_PAR_LONG(regs->rip));
+ TRACE_TIME(TRC_HVM_VMEXIT64 | (vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0),
+ exit_reason, TRACE_PARAM64(regs->rip));
else
- HVMTRACE_ND(VMEXIT, vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0,
- 1/*cycles*/, exit_reason, regs->eip);
+ TRACE_TIME(TRC_HVM_VMEXIT | (vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0),
+ exit_reason, regs->eip);
if ( vcpu_guestmode )
{
@@ -2660,17 +2658,17 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
{
case VMEXIT_INTR:
/* Asynchronous event, handled when we STGI'd after the VMEXIT. */
- HVMTRACE_0D(INTR);
+ TRACE(TRC_HVM_INTR);
break;
case VMEXIT_NMI:
/* Asynchronous event, handled when we STGI'd after the VMEXIT. */
- HVMTRACE_0D(NMI);
+ TRACE(TRC_HVM_NMI);
break;
case VMEXIT_SMI:
/* Asynchronous event, handled when we STGI'd after the VMEXIT. */
- HVMTRACE_0D(SMI);
+ TRACE(TRC_HVM_SMI);
break;
case VMEXIT_ICEBP:
@@ -2758,9 +2756,9 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
if ( trace_will_trace_event(TRC_SHADOW) )
break;
if ( hvm_long_mode_active(v) )
- HVMTRACE_LONG_2D(PF_XEN, regs->error_code, TRC_PAR_LONG(va));
+ TRACE(TRC_HVM_PF_XEN64, regs->error_code, TRACE_PARAM64(va));
else
- HVMTRACE_2D(PF_XEN, regs->error_code, va);
+ TRACE(TRC_HVM_PF_XEN, regs->error_code, va);
break;
}
@@ -2769,7 +2767,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
}
case VMEXIT_EXCEPTION_AC:
- HVMTRACE_1D(TRAP, TRAP_alignment_check);
+ TRACE(TRC_HVM_TRAP, TRAP_alignment_check);
hvm_inject_hw_exception(TRAP_alignment_check, vmcb->exitinfo1);
break;
@@ -2779,7 +2777,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
/* Asynchronous event, handled when we STGI'd after the VMEXIT. */
case VMEXIT_EXCEPTION_MC:
- HVMTRACE_0D(MCE);
+ TRACE(TRC_HVM_MCE);
svm_vmexit_mce_intercept(v, regs);
break;
@@ -2917,7 +2915,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
if ( (insn_len = svm_get_insn_len(v, INSTR_VMCALL)) == 0 )
break;
BUG_ON(vcpu_guestmode);
- HVMTRACE_1D(VMMCALL, regs->eax);
+ TRACE(TRC_HVM_VMMCALL, regs->eax);
if ( hvm_hypercall(regs) == HVM_HCALL_completed )
__update_guest_eip(regs, insn_len);
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index b8c84458ffdc..30fd3b46ec4d 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -692,7 +692,7 @@ int guest_rdmsr_x2apic(const struct vcpu *v, uint32_t msr, uint64_t *val)
static void vlapic_pt_cb(struct vcpu *v, void *data)
{
- TRACE_0D(TRC_HVM_EMUL_LAPIC_TIMER_CB);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_TIMER_CB);
*(s_time_t *)data = hvm_get_guest_time(v);
}
@@ -746,9 +746,8 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
delta = delta * vlapic->hw.timer_divisor / old_divisor;
}
- TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
- TRC_PAR_LONG(is_periodic ? period : 0),
- vlapic->pt.irq);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, TRACE_PARAM64(delta),
+ TRACE_PARAM64(is_periodic ? period : 0), vlapic->pt.irq);
create_periodic_time(current, &vlapic->pt, delta,
is_periodic ? period : 0, vlapic->pt.irq,
@@ -768,7 +767,7 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
}
else
{
- TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
destroy_periodic_time(&vlapic->pt);
/*
* From now, TMCCT should return 0 until TMICT is set again.
@@ -1202,8 +1201,8 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value)
vlapic->hw.tdt_msr = value;
/* .... reprogram tdt timer */
- TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
- TRC_PAR_LONG(0LL), vlapic->pt.irq);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, TRACE_PARAM64(delta),
+ TRACE_PARAM64(0LL), vlapic->pt.irq);
create_periodic_time(v, &vlapic->pt, delta, 0,
vlapic->pt.irq, vlapic_tdt_pt_cb,
&vlapic->timer_last_update, false);
@@ -1216,8 +1215,8 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value)
/* trigger a timer event if needed */
if ( value > 0 )
{
- TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(0LL),
- TRC_PAR_LONG(0LL), vlapic->pt.irq);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, TRACE_PARAM64(0LL),
+ TRACE_PARAM64(0LL), vlapic->pt.irq);
create_periodic_time(v, &vlapic->pt, 0, 0,
vlapic->pt.irq, vlapic_tdt_pt_cb,
&vlapic->timer_last_update, false);
@@ -1226,7 +1225,7 @@ void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value)
else
{
/* .... stop tdt timer */
- TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
destroy_periodic_time(&vlapic->pt);
}
@@ -1276,7 +1275,7 @@ int vlapic_accept_pic_intr(struct vcpu *v)
if ( target )
accept = __vlapic_accept_pic_intr(v);
- TRACE_2D(TRC_HVM_EMUL_LAPIC_PIC_INTR, target, accept);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_PIC_INTR, target, accept);
return target && accept;
}
@@ -1427,7 +1426,7 @@ static void vlapic_do_init(struct vlapic *vlapic)
vlapic_set_reg(vlapic, APIC_SPIV, 0xff);
vlapic->hw.disabled |= VLAPIC_SW_DISABLED;
- TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
destroy_periodic_time(&vlapic->pt);
}
@@ -1470,8 +1469,8 @@ static void lapic_rearm(struct vlapic *s)
(uint32_t)tmict * s->hw.timer_divisor);
is_periodic = vlapic_lvtt_period(s);
- TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(period),
- TRC_PAR_LONG(is_periodic ? period : 0LL), s->pt.irq);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, TRACE_PARAM64(period),
+ TRACE_PARAM64(is_periodic ? period : 0LL), s->pt.irq);
create_periodic_time(vlapic_vcpu(s), &s->pt, period,
is_periodic ? period : 0,
@@ -1650,7 +1649,7 @@ void vlapic_destroy(struct vcpu *v)
return;
tasklet_kill(&vlapic->init_sipi.tasklet);
- TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
+ TRACE_TIME(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
destroy_periodic_time(&vlapic->pt);
unmap_domain_page_global(vlapic->regs);
free_domheap_page(vlapic->regs_page);
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 80bfbb478782..303d07dc0a3a 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -79,8 +79,8 @@ static void vmx_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
unsigned long intr;
__vmread(VM_ENTRY_INTR_INFO, &intr);
- HVMTRACE_3D(INTR_WINDOW, intack.vector, intack.source,
- (intr & INTR_INFO_VALID_MASK) ? intr & 0xff : -1);
+ TRACE(TRC_HVM_INTR_WINDOW, intack.vector, intack.source,
+ (intr & INTR_INFO_VALID_MASK) ? intr & 0xff : -1);
}
if ( (intack.source == hvm_intsrc_nmi) && cpu_has_vmx_vnmi )
@@ -402,7 +402,7 @@ void vmx_intr_assist(void)
}
else
{
- HVMTRACE_2D(INJ_VIRQ, intack.vector, /*fake=*/ 0);
+ TRACE(TRC_HVM_INJ_VIRQ, intack.vector, /*fake=*/ 0);
vmx_inject_extint(intack.vector, intack.source);
pt_intr_post(v, intack);
}
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 974a25b34c41..47a5df20ebc1 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1875,10 +1875,10 @@ static void vmx_inject_event(const struct x86_event *event)
if ( (_event.vector == TRAP_page_fault) &&
(_event.type == X86_EVENTTYPE_HW_EXCEPTION) )
- HVMTRACE_LONG_2D(PF_INJECT, _event.error_code,
- TRC_PAR_LONG(curr->arch.hvm.guest_cr[2]));
+ TRACE(TRC_HVM_PF_INJECT64, _event.error_code,
+ TRACE_PARAM64(curr->arch.hvm.guest_cr[2]));
else
- HVMTRACE_2D(INJ_EXC, _event.vector, _event.error_code);
+ TRACE(TRC_HVM_INJ_EXC, _event.vector, _event.error_code);
}
static bool vmx_event_pending(const struct vcpu *v)
@@ -2783,7 +2783,7 @@ static void noinline vmx_dr_access(unsigned long exit_qualification,
{
struct vcpu *v = current;
- HVMTRACE_0D(DR_WRITE);
+ TRACE(TRC_HVM_DR_WRITE);
if ( !v->arch.hvm.flag_dr_dirty )
__restore_debug_registers(v);
@@ -2795,7 +2795,7 @@ static void noinline vmx_dr_access(unsigned long exit_qualification,
static void noinline vmx_invlpg_intercept(unsigned long linear)
{
- HVMTRACE_LONG_2D(INVLPG, /*invlpga=*/ 0, TRC_PAR_LONG(linear));
+ TRACE(TRC_HVM_INVLPG64, /*invlpga=*/ 0, TRACE_PARAM64(linear));
paging_invlpg(current, linear);
}
@@ -2843,7 +2843,7 @@ static int vmx_cr_access(cr_access_qual_t qual)
hvm_monitor_crX(CR0, value, old);
curr->arch.hvm.guest_cr[0] = value;
vmx_update_guest_cr(curr, 0, 0);
- HVMTRACE_0D(CLTS);
+ TRACE(TRC_HVM_CLTS);
break;
}
@@ -2856,7 +2856,7 @@ static int vmx_cr_access(cr_access_qual_t qual)
value = (value & ~(X86_CR0_MP|X86_CR0_EM|X86_CR0_TS)) |
(qual.lmsw_data &
(X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS));
- HVMTRACE_LONG_1D(LMSW, value);
+ TRACE(TRC_HVM_LMSW64, TRACE_PARAM64(value));
if ( (rc = hvm_set_cr0(value, true)) == X86EMUL_EXCEPTION )
hvm_inject_hw_exception(TRAP_gp_fault, 0);
@@ -3522,7 +3522,7 @@ static void vmx_do_extint(struct cpu_user_regs *regs)
BUG_ON(!(vector & INTR_INFO_VALID_MASK));
vector &= INTR_INFO_VECTOR_MASK;
- HVMTRACE_1D(INTR, vector);
+ TRACE(TRC_HVM_INTR, vector);
regs->entry_vector = vector;
do_IRQ(regs);
@@ -3662,7 +3662,7 @@ static void vmx_failed_vmentry(unsigned int exit_reason,
case EXIT_REASON_MCE_DURING_VMENTRY:
printk("MCE\n");
- HVMTRACE_0D(MCE);
+ TRACE(TRC_HVM_MCE);
/* Already handled. */
break;
@@ -3703,7 +3703,7 @@ static int vmx_handle_eoi_write(void)
{
update_guest_eip(); /* Safe: APIC data write */
vlapic_EOI_set(vcpu_vlapic(current));
- HVMTRACE_0D(VLAPIC);
+ TRACE(TRC_HVM_VLAPIC);
return 1;
}
@@ -3864,10 +3864,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
__vmread(VM_EXIT_REASON, &exit_reason);
if ( hvm_long_mode_active(v) )
- HVMTRACE_ND(VMEXIT64, 0, 1/*cycles*/, exit_reason,
- TRC_PAR_LONG(regs->rip));
+ TRACE_TIME(TRC_HVM_VMEXIT64, exit_reason, TRACE_PARAM64(regs->rip));
else
- HVMTRACE_ND(VMEXIT, 0, 1/*cycles*/, exit_reason, regs->eip);
+ TRACE_TIME(TRC_HVM_VMEXIT, exit_reason, regs->eip);
perfc_incra(vmexits, exit_reason);
@@ -3969,7 +3968,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
default:
perfc_incr(realmode_exits);
v->arch.hvm.vmx.vmx_emulate = 1;
- HVMTRACE_0D(REALMODE_EMULATE);
+ TRACE(TRC_HVM_REALMODE_EMULATE);
return;
}
case EXIT_REASON_EXTERNAL_INTERRUPT:
@@ -4032,7 +4031,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
* Table 23-1, "Exit Qualification for Debug Exceptions").
*/
__vmread(EXIT_QUALIFICATION, &exit_qualification);
- HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
+ TRACE(TRC_HVM_TRAP_DEBUG, exit_qualification);
__restore_debug_registers(v);
write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
@@ -4094,7 +4093,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
domain_pause_for_debugger();
break;
case TRAP_int3:
- HVMTRACE_1D(TRAP, vector);
+ TRACE(TRC_HVM_TRAP, vector);
if ( !v->domain->debugger_attached )
{
unsigned long insn_len;
@@ -4119,7 +4118,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
}
break;
case TRAP_no_device:
- HVMTRACE_1D(TRAP, vector);
+ TRACE(TRC_HVM_TRAP, vector);
vmx_fpu_dirty_intercept();
break;
case TRAP_page_fault:
@@ -4137,37 +4136,36 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
if ( trace_will_trace_event(TRC_SHADOW) )
break;
if ( hvm_long_mode_active(v) )
- HVMTRACE_LONG_2D(PF_XEN, regs->error_code,
- TRC_PAR_LONG(exit_qualification) );
+ TRACE(TRC_HVM_PF_XEN64, regs->error_code,
+ TRACE_PARAM64(exit_qualification));
else
- HVMTRACE_2D(PF_XEN,
- regs->error_code, exit_qualification );
+ TRACE(TRC_HVM_PF_XEN, regs->error_code, exit_qualification);
break;
}
hvm_inject_page_fault(regs->error_code, exit_qualification);
break;
case TRAP_alignment_check:
- HVMTRACE_1D(TRAP, vector);
+ TRACE(TRC_HVM_TRAP, vector);
vmx_propagate_intr(intr_info);
break;
case TRAP_nmi:
if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=
X86_EVENTTYPE_NMI )
goto exit_and_crash;
- HVMTRACE_0D(NMI);
+ TRACE(TRC_HVM_NMI);
/* Already handled above. */
break;
case TRAP_machine_check:
- HVMTRACE_0D(MCE);
+ TRACE(TRC_HVM_MCE);
/* Already handled above. */
break;
case TRAP_invalid_op:
- HVMTRACE_1D(TRAP, vector);
+ TRACE(TRC_HVM_TRAP, vector);
hvm_ud_intercept(regs);
break;
default:
- HVMTRACE_1D(TRAP, vector);
+ TRACE(TRC_HVM_TRAP, vector);
goto exit_and_crash;
}
break;
@@ -4255,7 +4253,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
break;
case EXIT_REASON_VMCALL:
- HVMTRACE_1D(VMMCALL, regs->eax);
+ TRACE(TRC_HVM_VMMCALL, regs->eax);
if ( hvm_hypercall(regs) == HVM_HCALL_completed )
update_guest_eip(); /* Safe: VMCALL */
@@ -4644,7 +4642,7 @@ bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
if ( unlikely(curr->arch.hvm.vmx.lbr_flags & LBR_FIXUP_MASK) )
lbr_fixup();
- HVMTRACE_ND(VMENTRY, 0, 1/*cycles*/);
+ TRACE_TIME(TRC_HVM_VMENTRY);
__vmwrite(GUEST_RIP, regs->rip);
__vmwrite(GUEST_RSP, regs->rsp);
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 91c2c6983393..1360f40b1044 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -99,8 +99,7 @@ static void vpic_update_int_output(struct hvm_hw_vpic *vpic)
ASSERT(vpic_is_locked(vpic));
irq = vpic_get_highest_priority_irq(vpic);
- TRACE_3D(TRC_HVM_EMUL_PIC_INT_OUTPUT, vpic->int_output, vpic->is_master,
- irq);
+ TRACE_TIME(TRC_HVM_EMUL_PIC_INT_OUTPUT, vpic->int_output, vpic->is_master, irq);
if ( vpic->int_output == (!vpic->init_state && irq >= 0) )
return;
@@ -119,7 +118,7 @@ static void vpic_update_int_output(struct hvm_hw_vpic *vpic)
if ( v != NULL )
{
- TRACE_1D(TRC_HVM_EMUL_PIC_KICK, irq);
+ TRACE_TIME(TRC_HVM_EMUL_PIC_KICK, irq);
vcpu_kick(v);
}
}
@@ -144,7 +143,7 @@ static void __vpic_intack(struct hvm_hw_vpic *vpic, int irq)
ASSERT(vpic_is_locked(vpic));
- TRACE_2D(TRC_HVM_EMUL_PIC_INTACK, vpic->is_master, irq);
+ TRACE_TIME(TRC_HVM_EMUL_PIC_INTACK, vpic->is_master, irq);
/* Edge-triggered: clear the IRR (forget the edge). */
if ( !(vpic->elcr & mask) )
vpic->irr &= ~mask;
@@ -483,7 +482,7 @@ void vpic_irq_positive_edge(struct domain *d, int irq)
ASSERT(irq <= 15);
ASSERT(vpic_is_locked(vpic));
- TRACE_1D(TRC_HVM_EMUL_PIC_POSEDGE, irq);
+ TRACE_TIME(TRC_HVM_EMUL_PIC_POSEDGE, irq);
if ( irq == 2 )
return;
@@ -501,7 +500,7 @@ void vpic_irq_negative_edge(struct domain *d, int irq)
ASSERT(irq <= 15);
ASSERT(vpic_is_locked(vpic));
- TRACE_1D(TRC_HVM_EMUL_PIC_NEGEDGE, irq);
+ TRACE_TIME(TRC_HVM_EMUL_PIC_NEGEDGE, irq);
if ( irq == 2 )
return;
@@ -519,7 +518,7 @@ int vpic_ack_pending_irq(struct vcpu *v)
accept = vlapic_accept_pic_intr(v);
- TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, accept, vpic->int_output);
+ TRACE_TIME(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, accept, vpic->int_output);
if ( !accept || !vpic->int_output )
return -1;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 67cbf6b979dc..df85932f44b4 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -236,7 +236,7 @@ static void _clear_irq_vector(struct irq_desc *desc)
for_each_cpu(cpu, tmp_mask)
{
ASSERT(per_cpu(vector_irq, cpu)[old_vector] == irq);
- TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, irq, old_vector, cpu);
+ TRACE_TIME(TRC_HW_IRQ_MOVE_FINISH, irq, old_vector, cpu);
per_cpu(vector_irq, cpu)[old_vector] = ~irq;
}
@@ -799,13 +799,11 @@ void irq_move_cleanup_interrupt(struct cpu_user_regs *regs)
goto unlock;
}
send_IPI_self(IRQ_MOVE_CLEANUP_VECTOR);
- TRACE_3D(TRC_HW_IRQ_MOVE_CLEANUP_DELAY,
- irq, vector, smp_processor_id());
+ TRACE_TIME(TRC_HW_IRQ_MOVE_CLEANUP_DELAY, irq, vector, me);
goto unlock;
}
- TRACE_3D(TRC_HW_IRQ_MOVE_CLEANUP,
- irq, vector, smp_processor_id());
+ TRACE_TIME(TRC_HW_IRQ_MOVE_CLEANUP, irq, vector, me);
per_cpu(vector_irq, me)[vector] = ~irq;
desc->arch.move_cleanup_count--;
@@ -1939,7 +1937,7 @@ void do_IRQ(struct cpu_user_regs *regs)
spin_unlock(&desc->lock);
}
}
- TRACE_1D(TRC_HW_IRQ_UNMAPPED_VECTOR, vector);
+ TRACE_TIME(TRC_HW_IRQ_UNMAPPED_VECTOR, vector);
}
goto out_no_unlock;
}
@@ -1980,7 +1978,7 @@ void do_IRQ(struct cpu_user_regs *regs)
tsc_in = tb_init_done ? get_cycles() : 0;
do_IRQ_guest(desc, vector);
- TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
+ TRACE_TIME(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
goto out_no_end;
}
@@ -2004,7 +2002,7 @@ void do_IRQ(struct cpu_user_regs *regs)
tsc_in = tb_init_done ? get_cycles() : 0;
action->handler(irq, action->dev_id, regs);
- TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
+ TRACE_TIME(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
spin_lock_irq(&desc->lock);
}
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 0cc1ee95cb5b..ca8374eadf9e 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1881,7 +1881,7 @@ void do_device_not_available(struct cpu_user_regs *regs)
curr->arch.pv.ctrlreg[0] &= ~X86_CR0_TS;
}
else
- TRACE_0D(TRC_PV_MATH_STATE_RESTORE);
+ TRACE_TIME(TRC_PV_MATH_STATE_RESTORE);
#else
ASSERT_UNREACHABLE();
#endif
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6ee5d033b0c2..e8672f86cc82 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -575,7 +575,7 @@ struct domain *domain_create(domid_t domid,
hardware_domain = d;
}
- TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
+ TRACE_TIME(TRC_DOM0_DOM_ADD, d->domain_id);
lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
@@ -1141,7 +1141,7 @@ void domain_destroy(struct domain *d)
if ( atomic_cmpxchg(&d->refcnt, 0, DOMAIN_DESTROYED) != 0 )
return;
- TRACE_1D(TRC_DOM0_DOM_REM, d->domain_id);
+ TRACE_TIME(TRC_DOM0_DOM_REM, d->domain_id);
/* Delete from task list and task hashtable. */
spin_lock(&domlist_update_lock);
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fe1fc11b228e..7f1b2b010bc1 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1271,7 +1271,7 @@ map_grant_ref(
goto undo_out;
}
- TRACE_1D(TRC_MEM_PAGE_GRANT_MAP, op->dom);
+ TRACE_TIME(TRC_MEM_PAGE_GRANT_MAP, op->dom);
/*
* All maptrack entry users check mt->flags first before using the
@@ -1398,7 +1398,7 @@ unmap_common(
return;
}
- TRACE_1D(TRC_MEM_PAGE_GRANT_UNMAP, dom);
+ TRACE_TIME(TRC_MEM_PAGE_GRANT_UNMAP, dom);
rgt = rd->grant_table;
@@ -2384,7 +2384,7 @@ gnttab_transfer(
put_gfn(d, gop.mfn);
#endif
- TRACE_1D(TRC_MEM_PAGE_GRANT_TRANSFER, e->domain_id);
+ TRACE_TIME(TRC_MEM_PAGE_GRANT_TRANSFER, e->domain_id);
/* Tell the guest about its new page frame. */
grant_read_lock(e->grant_table);
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index fe133cbf117c..37e1a17a888b 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -827,7 +827,7 @@ int sched_init_domain(struct domain *d, unsigned int poolid)
return ret;
SCHED_STAT_CRANK(dom_init);
- TRACE_1D(TRC_SCHED_DOM_ADD, d->domain_id);
+ TRACE_TIME(TRC_SCHED_DOM_ADD, d->domain_id);
rcu_read_lock(&sched_res_rculock);
@@ -850,7 +850,7 @@ void sched_destroy_domain(struct domain *d)
if ( d->cpupool )
{
SCHED_STAT_CRANK(dom_destroy);
- TRACE_1D(TRC_SCHED_DOM_REM, d->domain_id);
+ TRACE_TIME(TRC_SCHED_DOM_REM, d->domain_id);
rcu_read_lock(&sched_res_rculock);
@@ -891,7 +891,7 @@ void vcpu_sleep_nosync(struct vcpu *v)
unsigned long flags;
spinlock_t *lock;
- TRACE_2D(TRC_SCHED_SLEEP, v->domain->domain_id, v->vcpu_id);
+ TRACE_TIME(TRC_SCHED_SLEEP, v->domain->domain_id, v->vcpu_id);
rcu_read_lock(&sched_res_rculock);
@@ -920,7 +920,7 @@ void vcpu_wake(struct vcpu *v)
spinlock_t *lock;
struct sched_unit *unit = v->sched_unit;
- TRACE_2D(TRC_SCHED_WAKE, v->domain->domain_id, v->vcpu_id);
+ TRACE_TIME(TRC_SCHED_WAKE, v->domain->domain_id, v->vcpu_id);
rcu_read_lock(&sched_res_rculock);
@@ -1429,7 +1429,7 @@ void vcpu_block(void)
}
else
{
- TRACE_2D(TRC_SCHED_BLOCK, v->domain->domain_id, v->vcpu_id);
+ TRACE_TIME(TRC_SCHED_BLOCK, v->domain->domain_id, v->vcpu_id);
raise_softirq(SCHEDULE_SOFTIRQ);
}
}
@@ -1502,7 +1502,7 @@ static long do_poll(struct sched_poll *sched_poll)
if ( sched_poll->timeout != 0 )
set_timer(&v->poll_timer, sched_poll->timeout);
- TRACE_2D(TRC_SCHED_BLOCK, d->domain_id, v->vcpu_id);
+ TRACE_TIME(TRC_SCHED_BLOCK, d->domain_id, v->vcpu_id);
raise_softirq(SCHEDULE_SOFTIRQ);
return 0;
@@ -1530,7 +1530,7 @@ long vcpu_yield(void)
SCHED_STAT_CRANK(vcpu_yield);
- TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
+ TRACE_TIME(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
raise_softirq(SCHEDULE_SOFTIRQ);
return 0;
}
@@ -1888,9 +1888,8 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( copy_from_guest(&sched_shutdown, arg, 1) )
break;
- TRACE_3D(TRC_SCHED_SHUTDOWN,
- current->domain->domain_id, current->vcpu_id,
- sched_shutdown.reason);
+ TRACE_TIME(TRC_SCHED_SHUTDOWN, current->domain->domain_id,
+ current->vcpu_id, sched_shutdown.reason);
ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason);
break;
@@ -1905,8 +1904,8 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( copy_from_guest(&sched_shutdown, arg, 1) )
break;
- TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
- d->domain_id, current->vcpu_id, sched_shutdown.reason);
+ TRACE_TIME(TRC_SCHED_SHUTDOWN_CODE, d->domain_id, current->vcpu_id,
+ sched_shutdown.reason);
spin_lock(&d->shutdown_lock);
if ( d->shutdown_code == SHUTDOWN_CODE_INVALID )
@@ -2069,7 +2068,7 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
rcu_read_lock(&sched_res_rculock);
if ( (ret = sched_adjust_dom(dom_scheduler(d), d, op)) == 0 )
- TRACE_1D(TRC_SCHED_ADJDOM, d->domain_id);
+ TRACE_TIME(TRC_SCHED_ADJDOM, d->domain_id);
rcu_read_unlock(&sched_res_rculock);
@@ -2164,14 +2163,13 @@ static void sched_switch_units(struct sched_resource *sr,
sr->curr = next;
sr->prev = prev;
- TRACE_3D(TRC_SCHED_SWITCH_INFPREV, prev->domain->domain_id,
- prev->unit_id, now - prev->state_entry_time);
- TRACE_4D(TRC_SCHED_SWITCH_INFNEXT, next->domain->domain_id,
- next->unit_id,
- (next->vcpu_list->runstate.state == RUNSTATE_runnable) ?
- (now - next->state_entry_time) : 0, prev->next_time);
- TRACE_4D(TRC_SCHED_SWITCH, prev->domain->domain_id, prev->unit_id,
- next->domain->domain_id, next->unit_id);
+ TRACE_TIME(TRC_SCHED_SWITCH_INFPREV, prev->domain->domain_id,
+ prev->unit_id, now - prev->state_entry_time);
+ TRACE_TIME(TRC_SCHED_SWITCH_INFNEXT, next->domain->domain_id, next->unit_id,
+ (next->vcpu_list->runstate.state == RUNSTATE_runnable) ?
+ (now - next->state_entry_time) : 0, prev->next_time);
+ TRACE_TIME(TRC_SCHED_SWITCH, prev->domain->domain_id, prev->unit_id,
+ next->domain->domain_id, next->unit_id);
ASSERT(!unit_running(next));
@@ -2363,10 +2361,10 @@ static void sched_context_switch(struct vcpu *vprev, struct vcpu *vnext,
{
if ( unlikely(vprev == vnext) )
{
- TRACE_4D(TRC_SCHED_SWITCH_INFCONT,
- vnext->domain->domain_id, vnext->sched_unit->unit_id,
- now - vprev->runstate.state_entry_time,
- vprev->sched_unit->next_time);
+ TRACE_TIME(TRC_SCHED_SWITCH_INFCONT,
+ vnext->domain->domain_id, vnext->sched_unit->unit_id,
+ now - vprev->runstate.state_entry_time,
+ vprev->sched_unit->next_time);
sched_context_switched(vprev, vnext);
/*
diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c
index f277fa37a8b1..0ea7145172ee 100644
--- a/xen/common/sched/credit.c
+++ b/xen/common/sched/credit.c
@@ -842,8 +842,7 @@ _csched_cpu_pick(const struct scheduler *ops, const struct sched_unit *unit,
if ( commit && spc )
spc->idle_bias = cpu;
- TRACE_3D(TRC_CSCHED_PICKED_CPU, unit->domain->domain_id, unit->unit_id,
- cpu);
+ TRACE_TIME(TRC_CSCHED_PICKED_CPU, unit->domain->domain_id, unit->unit_id, cpu);
return cpu;
}
@@ -887,8 +886,8 @@ __csched_unit_acct_start(struct csched_private *prv, struct csched_unit *svc)
}
}
- TRACE_3D(TRC_CSCHED_ACCOUNT_START, sdom->dom->domain_id,
- svc->unit->unit_id, sdom->active_unit_count);
+ TRACE_TIME(TRC_CSCHED_ACCOUNT_START, sdom->dom->domain_id,
+ svc->unit->unit_id, sdom->active_unit_count);
spin_unlock_irqrestore(&prv->lock, flags);
}
@@ -913,8 +912,8 @@ __csched_unit_acct_stop_locked(struct csched_private *prv,
list_del_init(&sdom->active_sdom_elem);
}
- TRACE_3D(TRC_CSCHED_ACCOUNT_STOP, sdom->dom->domain_id,
- svc->unit->unit_id, sdom->active_unit_count);
+ TRACE_TIME(TRC_CSCHED_ACCOUNT_STOP, sdom->dom->domain_id,
+ svc->unit->unit_id, sdom->active_unit_count);
}
static void
@@ -937,8 +936,8 @@ csched_unit_acct(struct csched_private *prv, unsigned int cpu)
if ( svc->pri == CSCHED_PRI_TS_BOOST )
{
svc->pri = CSCHED_PRI_TS_UNDER;
- TRACE_2D(TRC_CSCHED_BOOST_END, svc->sdom->dom->domain_id,
- svc->unit->unit_id);
+ TRACE_TIME(TRC_CSCHED_BOOST_END, svc->sdom->dom->domain_id,
+ svc->unit->unit_id);
}
/*
@@ -1145,8 +1144,7 @@ csched_unit_wake(const struct scheduler *ops, struct sched_unit *unit)
if ( !migrating && svc->pri == CSCHED_PRI_TS_UNDER &&
!test_bit(CSCHED_FLAG_UNIT_PARKED, &svc->flags) )
{
- TRACE_2D(TRC_CSCHED_BOOST_START, unit->domain->domain_id,
- unit->unit_id);
+ TRACE_TIME(TRC_CSCHED_BOOST_START, unit->domain->domain_id, unit->unit_id);
SCHED_STAT_CRANK(unit_boost);
svc->pri = CSCHED_PRI_TS_BOOST;
}
@@ -1647,8 +1645,8 @@ csched_runq_steal(int peer_cpu, int cpu, int pri, int balance_step)
if ( __csched_unit_is_migrateable(prv, unit, cpu, cpumask_scratch) )
{
/* We got a candidate. Grab it! */
- TRACE_3D(TRC_CSCHED_STOLEN_UNIT, peer_cpu,
- unit->domain->domain_id, unit->unit_id);
+ TRACE_TIME(TRC_CSCHED_STOLEN_UNIT, peer_cpu,
+ unit->domain->domain_id, unit->unit_id);
SCHED_UNIT_STAT_CRANK(speer, migrate_q);
SCHED_STAT_CRANK(migrate_queued);
runq_remove(speer);
@@ -1751,7 +1749,7 @@ csched_load_balance(struct csched_private *prv, int cpu,
*/
if ( CSCHED_PCPU(peer_cpu)->nr_runnable <= 1 )
{
- TRACE_2D(TRC_CSCHED_STEAL_CHECK, peer_cpu, /* skipp'n */ 0);
+ TRACE_TIME(TRC_CSCHED_STEAL_CHECK, peer_cpu, /* skipp'n */ 0);
goto next_cpu;
}
@@ -1767,11 +1765,11 @@ csched_load_balance(struct csched_private *prv, int cpu,
if ( !lock )
{
SCHED_STAT_CRANK(steal_trylock_failed);
- TRACE_2D(TRC_CSCHED_STEAL_CHECK, peer_cpu, /* skip */ 0);
+ TRACE_TIME(TRC_CSCHED_STEAL_CHECK, peer_cpu, /* skip */ 0);
goto next_cpu;
}
- TRACE_2D(TRC_CSCHED_STEAL_CHECK, peer_cpu, /* checked */ 1);
+ TRACE_TIME(TRC_CSCHED_STEAL_CHECK, peer_cpu, /* checked */ 1);
/* Any work over there to steal? */
speer = cpumask_test_cpu(peer_cpu, online) ?
@@ -1939,7 +1937,7 @@ static void csched_schedule(
/* Tasklet work (which runs in idle UNIT context) overrides all else. */
if ( tasklet_work_scheduled )
{
- TRACE_0D(TRC_CSCHED_SCHED_TASKLET);
+ TRACE_TIME(TRC_CSCHED_SCHED_TASKLET);
snext = CSCHED_UNIT(sched_idle_unit(sched_cpu));
snext->pri = CSCHED_PRI_TS_BOOST;
}
diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index b93895d4dddc..bc3e62bdeadd 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -367,7 +367,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
retval = alternative_call(cpufreq_driver.target,
policy, target_freq, relation);
if ( retval == 0 )
- TRACE_2D(TRC_PM_FREQ_CHANGE, prev_freq/1000, policy->cur/1000);
+ TRACE_TIME(TRC_PM_FREQ_CHANGE, prev_freq / 1000, policy->cur / 1000);
}
return retval;
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2.1 14/12] xen: Switch to new TRACE() API
2021-09-20 19:32 ` [PATCH v2.1 14/12] xen: Switch to new TRACE() API Andrew Cooper
@ 2021-09-24 13:34 ` Jan Beulich
2021-09-24 15:30 ` Dario Faggioli
1 sibling, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-09-24 13:34 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Xen-devel
On 20.09.2021 21:32, Andrew Cooper wrote:
> (Almost) no functional change.
>
> irq_move_cleanup_interrupt() changes two smp_processor_id() calls to the 'me'
> local variable which manifests as a minor code improvement. All other
> differences in the compiled binary are to do with line numbers changing.
>
> Some conversion notes:
> * HVMTRACE_LONG_[234]D() and TRACE_2_LONG_[234]D() were latently buggy. They
> blindly discard extra parameters, but luckily no users are impacted. They
> are also obfuscated wrappers, depending on exactly one or two parameters
> being TRC_PAR_LONG() to compile successfully.
> * HVMTRACE_LONG_1D() behaves unlike its named companions, and takes exactly
> one 64bit parameter which it splits manually. It's one user,
> vmx_cr_access()'s LMSW path, is gets adjusted to use TRACE_PARAM64().
> * TRACE_?D() and TRACE_2_LONG_*() change to TRACE_TIME() as cycles is always.
> * HVMTRACE_ND() is opencoded for VMENTRY/VMEXIT records to include cycles.
> These are converted to TRACE_TIME(), with the old modifier parameter
> expressed as an OR at the callsite. One callsite, svm_vmenter_helper() had
> a nested tb_init_done check, which is dropped. (The optimiser also spotted
> this, which is why it doesn't manifest as a binary difference.)
> * All HVMTRACE_?D() change to TRACE() as cycles is explicitly skipped.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> I'm in two minds as to whether to split this up by subsystem or not. It is
> 95% x86, and isn't a massive patch.
Either way looks fine to me in this case; splitting might allow parts
to go in before you've managed to get acks from all relevant people.
If anything I might have preferred seeing e.g. all the HVM*() macros
getting replaced and dropped at the same time, rather than the
dropping (combined with others) getting split off.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2.1 14/12] xen: Switch to new TRACE() API
2021-09-20 19:32 ` [PATCH v2.1 14/12] xen: Switch to new TRACE() API Andrew Cooper
2021-09-24 13:34 ` Jan Beulich
@ 2021-09-24 15:30 ` Dario Faggioli
1 sibling, 0 replies; 52+ messages in thread
From: Dario Faggioli @ 2021-09-24 15:30 UTC (permalink / raw)
To: andrew.cooper3, xen-devel
Cc: julien, George.Dunlap, sstabellini, Jan Beulich, iwj, wl
[-- Attachment #1: Type: text/plain, Size: 2058 bytes --]
On Mon, 2021-09-20 at 20:32 +0100, Andrew Cooper wrote:
> (Almost) no functional change.
>
> irq_move_cleanup_interrupt() changes two smp_processor_id() calls to
> the 'me'
> local variable which manifests as a minor code improvement. All
> other
> differences in the compiled binary are to do with line numbers
> changing.
>
> Some conversion notes:
> * HVMTRACE_LONG_[234]D() and TRACE_2_LONG_[234]D() were latently
> buggy. They
> blindly discard extra parameters, but luckily no users are
> impacted. They
> are also obfuscated wrappers, depending on exactly one or two
> parameters
> being TRC_PAR_LONG() to compile successfully.
> * HVMTRACE_LONG_1D() behaves unlike its named companions, and takes
> exactly
> one 64bit parameter which it splits manually. It's one user,
> vmx_cr_access()'s LMSW path, is gets adjusted to use
> TRACE_PARAM64().
> * TRACE_?D() and TRACE_2_LONG_*() change to TRACE_TIME() as cycles
> is always.
>
Was this supposed to be "as cycles is always 1", or something like
that? (But maybe it's fine and it is me. I'm no native speaker after
all...)
In any case...
> * HVMTRACE_ND() is opencoded for VMENTRY/VMEXIT records to include
> cycles.
> These are converted to TRACE_TIME(), with the old modifier
> parameter
> expressed as an OR at the callsite. One callsite,
> svm_vmenter_helper() had
> a nested tb_init_done check, which is dropped. (The optimiser
> also spotted
> this, which is why it doesn't manifest as a binary difference.)
> * All HVMTRACE_?D() change to TRACE() as cycles is explicitly
> skipped.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2.1 15/12] xen/trace: Drop old trace macros
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (13 preceding siblings ...)
2021-09-20 19:32 ` [PATCH v2.1 14/12] xen: Switch to new TRACE() API Andrew Cooper
@ 2021-09-20 19:33 ` Andrew Cooper
2021-09-24 13:31 ` Jan Beulich
2021-09-20 19:40 ` [PATCH v2.1 16/12] xen/trace: Restrict CONFIG_TRACEBUFFER to x86 PV Andrew Cooper
2021-09-21 20:08 ` [PATCH v2.1 RFC 17/12] xen/trace: Drop cycles parameter Andrew Cooper
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 19:33 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich,
Stefano Stabellini, Wei Liu, Julien Grall
With all users updated to the new API, drop the old API. This includes all of
asm/hvm/trace.h, which allows us to drop some includes.
Signed-off-by: Andrew Cooper <andrew.cooper3@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>
v2.1:
* New
---
xen/arch/x86/hvm/emulate.c | 1 -
xen/arch/x86/hvm/hpet.c | 1 -
xen/arch/x86/hvm/hvm.c | 1 -
xen/arch/x86/hvm/io.c | 1 -
xen/arch/x86/hvm/svm/intr.c | 1 -
xen/arch/x86/hvm/svm/svm.c | 1 -
xen/arch/x86/hvm/vmx/intr.c | 1 -
xen/arch/x86/hvm/vmx/vmx.c | 1 -
xen/include/asm-x86/hvm/trace.h | 114 --------------------------------------
xen/include/asm-x86/hvm/vmx/vmx.h | 1 -
xen/include/xen/trace.h | 35 ------------
11 files changed, 158 deletions(-)
delete mode 100644 xen/include/asm-x86/hvm/trace.h
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index f863a19df311..a651dd395b56 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -22,7 +22,6 @@
#include <asm/hvm/emulate.h>
#include <asm/hvm/hvm.h>
#include <asm/hvm/monitor.h>
-#include <asm/hvm/trace.h>
#include <asm/hvm/support.h>
#include <asm/hvm/svm/svm.h>
#include <asm/iocap.h>
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 5873bb8a506d..1736b8f12583 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -19,7 +19,6 @@
#include <asm/hvm/vpt.h>
#include <asm/hvm/io.h>
#include <asm/hvm/support.h>
-#include <asm/hvm/trace.h>
#include <asm/current.h>
#include <asm/hpet.h>
#include <asm/mc146818rtc.h>
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index fa211d14afd8..5a81ffe9dc41 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -62,7 +62,6 @@
#include <asm/hvm/vpt.h>
#include <asm/hvm/support.h>
#include <asm/hvm/cacheattr.h>
-#include <asm/hvm/trace.h>
#include <asm/hvm/nestedhvm.h>
#include <asm/hvm/monitor.h>
#include <asm/hvm/viridian.h>
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 046a8eb4ed1b..9be206488043 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -40,7 +40,6 @@
#include <asm/hvm/vpt.h>
#include <asm/hvm/vpic.h>
#include <asm/hvm/vlapic.h>
-#include <asm/hvm/trace.h>
#include <asm/hvm/emulate.h>
#include <public/sched.h>
#include <xen/iocap.h>
diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 122c9d7fbf14..154258c8b31f 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -37,7 +37,6 @@
#include <xen/kernel.h>
#include <public/hvm/ioreq.h>
#include <xen/domain_page.h>
-#include <asm/hvm/trace.h>
static void svm_inject_nmi(struct vcpu *v)
{
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b98a2dd71d1e..1f9dadaa173a 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -55,7 +55,6 @@
#include <asm/x86_emulate.h>
#include <public/sched.h>
#include <asm/hvm/vpt.h>
-#include <asm/hvm/trace.h>
#include <asm/hap.h>
#include <asm/apic.h>
#include <asm/debugger.h>
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 303d07dc0a3a..ed17c79996c5 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -36,7 +36,6 @@
#include <asm/hvm/vlapic.h>
#include <asm/hvm/nestedhvm.h>
#include <public/hvm/ioreq.h>
-#include <asm/hvm/trace.h>
#include <asm/vm_event.h>
/*
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 47a5df20ebc1..4f99fda8d662 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -48,7 +48,6 @@
#include <asm/x86_emulate.h>
#include <asm/hvm/vpt.h>
#include <public/hvm/save.h>
-#include <asm/hvm/trace.h>
#include <asm/hvm/monitor.h>
#include <asm/xenoprof.h>
#include <asm/debugger.h>
diff --git a/xen/include/asm-x86/hvm/trace.h b/xen/include/asm-x86/hvm/trace.h
deleted file mode 100644
index 696e42eb9499..000000000000
--- a/xen/include/asm-x86/hvm/trace.h
+++ /dev/null
@@ -1,114 +0,0 @@
-#ifndef __ASM_X86_HVM_TRACE_H__
-#define __ASM_X86_HVM_TRACE_H__
-
-#include <xen/trace.h>
-
-#define DEFAULT_HVM_TRACE_ON 1
-#define DEFAULT_HVM_TRACE_OFF 0
-
-#define DEFAULT_HVM_VMSWITCH DEFAULT_HVM_TRACE_ON
-#define DEFAULT_HVM_PF DEFAULT_HVM_TRACE_ON
-#define DEFAULT_HVM_INJECT DEFAULT_HVM_TRACE_ON
-#define DEFAULT_HVM_IO DEFAULT_HVM_TRACE_ON
-#define DEFAULT_HVM_REGACCESS DEFAULT_HVM_TRACE_ON
-#define DEFAULT_HVM_MISC DEFAULT_HVM_TRACE_ON
-#define DEFAULT_HVM_INTR DEFAULT_HVM_TRACE_ON
-
-#define DO_TRC_HVM_VMENTRY DEFAULT_HVM_VMSWITCH
-#define DO_TRC_HVM_VMEXIT DEFAULT_HVM_VMSWITCH
-#define DO_TRC_HVM_VMEXIT64 DEFAULT_HVM_VMSWITCH
-#define DO_TRC_HVM_PF_XEN DEFAULT_HVM_PF
-#define DO_TRC_HVM_PF_XEN64 DEFAULT_HVM_PF
-#define DO_TRC_HVM_PF_INJECT DEFAULT_HVM_PF
-#define DO_TRC_HVM_PF_INJECT64 DEFAULT_HVM_PF
-#define DO_TRC_HVM_INJ_EXC DEFAULT_HVM_INJECT
-#define DO_TRC_HVM_INJ_VIRQ DEFAULT_HVM_INJECT
-#define DO_TRC_HVM_REINJ_VIRQ DEFAULT_HVM_INJECT
-#define DO_TRC_HVM_INTR_WINDOW DEFAULT_HVM_INJECT
-#define DO_TRC_HVM_IO_READ DEFAULT_HVM_IO
-#define DO_TRC_HVM_IO_WRITE DEFAULT_HVM_IO
-#define DO_TRC_HVM_CR_READ DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_CR_READ64 DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_CR_WRITE DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_CR_WRITE64 DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_DR_READ DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_DR_WRITE DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_XCR_READ64 DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_XCR_WRITE64 DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_MSR_READ DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_MSR_WRITE DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_RDTSC DEFAULT_HVM_REGACCESS
-#define DO_TRC_HVM_CPUID DEFAULT_HVM_MISC
-#define DO_TRC_HVM_INTR DEFAULT_HVM_INTR
-#define DO_TRC_HVM_NMI DEFAULT_HVM_INTR
-#define DO_TRC_HVM_MCE DEFAULT_HVM_INTR
-#define DO_TRC_HVM_SMI DEFAULT_HVM_INTR
-#define DO_TRC_HVM_VMMCALL DEFAULT_HVM_MISC
-#define DO_TRC_HVM_HLT DEFAULT_HVM_MISC
-#define DO_TRC_HVM_INVLPG DEFAULT_HVM_MISC
-#define DO_TRC_HVM_INVLPG64 DEFAULT_HVM_MISC
-#define DO_TRC_HVM_IO_ASSIST DEFAULT_HVM_MISC
-#define DO_TRC_HVM_MMIO_ASSIST DEFAULT_HVM_MISC
-#define DO_TRC_HVM_CLTS DEFAULT_HVM_MISC
-#define DO_TRC_HVM_LMSW DEFAULT_HVM_MISC
-#define DO_TRC_HVM_LMSW64 DEFAULT_HVM_MISC
-#define DO_TRC_HVM_REALMODE_EMULATE DEFAULT_HVM_MISC
-#define DO_TRC_HVM_TRAP DEFAULT_HVM_MISC
-#define DO_TRC_HVM_TRAP_DEBUG DEFAULT_HVM_MISC
-#define DO_TRC_HVM_VLAPIC DEFAULT_HVM_MISC
-
-
-#define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)
-
-#define TRACE_2_LONG_2D(_e, d1, d2, ...) \
- TRACE_4D(_e, d1, d2)
-#define TRACE_2_LONG_3D(_e, d1, d2, d3, ...) \
- TRACE_5D(_e, d1, d2, d3)
-#define TRACE_2_LONG_4D(_e, d1, d2, d3, d4, ...) \
- TRACE_6D(_e, d1, d2, d3, d4)
-
-#define HVMTRACE_ND(evt, modifier, cycles, ...) \
- do { \
- if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt ) \
- { \
- uint32_t _d[] = { __VA_ARGS__ }; \
- __trace_var(TRC_HVM_ ## evt | (modifier), cycles, \
- sizeof(_d), sizeof(_d) ? _d : NULL); \
- } \
- } while(0)
-
-#define HVMTRACE_6D(evt, d1, d2, d3, d4, d5, d6) \
- HVMTRACE_ND(evt, 0, 0, d1, d2, d3, d4, d5, d6)
-#define HVMTRACE_5D(evt, d1, d2, d3, d4, d5) \
- HVMTRACE_ND(evt, 0, 0, d1, d2, d3, d4, d5)
-#define HVMTRACE_4D(evt, d1, d2, d3, d4) \
- HVMTRACE_ND(evt, 0, 0, d1, d2, d3, d4)
-#define HVMTRACE_3D(evt, d1, d2, d3) \
- HVMTRACE_ND(evt, 0, 0, d1, d2, d3)
-#define HVMTRACE_2D(evt, d1, d2) \
- HVMTRACE_ND(evt, 0, 0, d1, d2)
-#define HVMTRACE_1D(evt, d1) \
- HVMTRACE_ND(evt, 0, 0, d1)
-#define HVMTRACE_0D(evt) \
- HVMTRACE_ND(evt, 0, 0)
-
-#define HVMTRACE_LONG_1D(evt, d1) \
- HVMTRACE_2D(evt ## 64, (d1) & 0xFFFFFFFF, (d1) >> 32)
-#define HVMTRACE_LONG_2D(evt, d1, d2, ...) \
- HVMTRACE_3D(evt ## 64, d1, d2)
-#define HVMTRACE_LONG_3D(evt, d1, d2, d3, ...) \
- HVMTRACE_4D(evt ## 64, d1, d2, d3)
-#define HVMTRACE_LONG_4D(evt, d1, d2, d3, d4, ...) \
- HVMTRACE_5D(evt ## 64, d1, d2, d3, d4)
-
-#endif /* __ASM_X86_HVM_TRACE_H__ */
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 85530d2e0e26..2987d5870606 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -25,7 +25,6 @@
#include <asm/processor.h>
#include <asm/i387.h>
#include <asm/hvm/support.h>
-#include <asm/hvm/trace.h>
#include <asm/hvm/vmx/vmcs.h>
extern int8_t opt_ept_exec_sp;
diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
index 72c20550f6a6..940be79d7b8e 100644
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -97,39 +97,4 @@ static inline void __trace_hypercall(uint32_t event, unsigned long op,
/* Create a trace record with no time included. */
#define TRACE(_e, ...) TRACE_INTERNAL(_e, false, ##__VA_ARGS__)
-
-/* Convenience macros for calling the trace function. */
-#define TRACE_0D(_e) \
- do { \
- trace_var(_e, 1, 0, NULL); \
- } while ( 0 )
-
-/* Common helper for TRACE_{1..6}D() below. */
-#define TRACE_varD(_e, ...) \
- do { \
- if ( unlikely(tb_init_done) ) \
- { \
- uint32_t _d[] = { __VA_ARGS__ }; \
- __trace_var(_e, true, sizeof(_d), _d); \
- } \
- } while ( 0 )
-
-#define TRACE_1D(_e, d1) \
- TRACE_varD(_e, d1)
-
-#define TRACE_2D(_e, d1, d2) \
- TRACE_varD(_e, d1, d2)
-
-#define TRACE_3D(_e, d1, d2, d3) \
- TRACE_varD(_e, d1, d2, d3)
-
-#define TRACE_4D(_e, d1, d2, d3, d4) \
- TRACE_varD(_e, d1, d2, d3, d4)
-
-#define TRACE_5D(_e, d1, d2, d3, d4, d5) \
- TRACE_varD(_e, d1, d2, d3, d4, d5)
-
-#define TRACE_6D(_e, d1, d2, d3, d4, d5, d6) \
- TRACE_varD(_e, d1, d2, d3, d4, d5, d6)
-
#endif /* __XEN_TRACE_H__ */
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2.1 15/12] xen/trace: Drop old trace macros
2021-09-20 19:33 ` [PATCH v2.1 15/12] xen/trace: Drop old trace macros Andrew Cooper
@ 2021-09-24 13:31 ` Jan Beulich
0 siblings, 0 replies; 52+ messages in thread
From: Jan Beulich @ 2021-09-24 13:31 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Xen-devel
On 20.09.2021 21:33, Andrew Cooper wrote:
> With all users updated to the new API, drop the old API. This includes all of
> asm/hvm/trace.h, which allows us to drop some includes.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
albeit I'd like to note that ...
> --- a/xen/include/asm-x86/hvm/trace.h
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -#ifndef __ASM_X86_HVM_TRACE_H__
> -#define __ASM_X86_HVM_TRACE_H__
> -
> -#include <xen/trace.h>
> -
> -#define DEFAULT_HVM_TRACE_ON 1
> -#define DEFAULT_HVM_TRACE_OFF 0
> -
> -#define DEFAULT_HVM_VMSWITCH DEFAULT_HVM_TRACE_ON
> -#define DEFAULT_HVM_PF DEFAULT_HVM_TRACE_ON
> -#define DEFAULT_HVM_INJECT DEFAULT_HVM_TRACE_ON
> -#define DEFAULT_HVM_IO DEFAULT_HVM_TRACE_ON
> -#define DEFAULT_HVM_REGACCESS DEFAULT_HVM_TRACE_ON
> -#define DEFAULT_HVM_MISC DEFAULT_HVM_TRACE_ON
> -#define DEFAULT_HVM_INTR DEFAULT_HVM_TRACE_ON
... least the part up to here as potentially useful to limit trace
output. Afaics there's no replacement in the new model, as you
invoke the base tracing macros now directly.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2.1 16/12] xen/trace: Restrict CONFIG_TRACEBUFFER to x86 PV
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (14 preceding siblings ...)
2021-09-20 19:33 ` [PATCH v2.1 15/12] xen/trace: Drop old trace macros Andrew Cooper
@ 2021-09-20 19:40 ` Andrew Cooper
2021-09-21 1:10 ` Julien Grall
2021-09-21 20:08 ` [PATCH v2.1 RFC 17/12] xen/trace: Drop cycles parameter Andrew Cooper
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-20 19:40 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk
The mapping interface can only be used by x86 PV guests.
This can and should be fixed by changing to an acquire_resource() based
interface, which is compatbile with x86 PVH and ARM dom0's, but until this
happens, don't give the impression of this feature being useable elsewhere.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
xen/common/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index db687b1785e7..6b6f7139e6f0 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -429,6 +429,7 @@ config DTB_FILE
config TRACEBUFFER
bool "Enable tracing infrastructure" if EXPERT
+ depends on PV
default y
---help---
Enable tracing infrastructure and pre-defined tracepoints within Xen.
--
2.11.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2.1 16/12] xen/trace: Restrict CONFIG_TRACEBUFFER to x86 PV
2021-09-20 19:40 ` [PATCH v2.1 16/12] xen/trace: Restrict CONFIG_TRACEBUFFER to x86 PV Andrew Cooper
@ 2021-09-21 1:10 ` Julien Grall
0 siblings, 0 replies; 52+ messages in thread
From: Julien Grall @ 2021-09-21 1:10 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné,
Wei Liu, Stefano Stabellini, Volodymyr Babchuk
[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]
Hi Andrew,
On Tue, 21 Sep 2021, 00:41 Andrew Cooper, <andrew.cooper3@citrix.com> wrote:
> The mapping interface can only be used by x86 PV guests.
>
Tracebuffer works on Arm... The support was added a couple of years ago
using the foreign mapping interface.
> This can and should be fixed by changing to an acquire_resource() based
> interface, which is compatbile with x86 PVH and ARM dom0's, but until this
> happens, don't give the impression of this feature being useable elsewhere.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> ---
> xen/common/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index db687b1785e7..6b6f7139e6f0 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -429,6 +429,7 @@ config DTB_FILE
>
> config TRACEBUFFER
> bool "Enable tracing infrastructure" if EXPERT
> + depends on PV
> default y
> ---help---
> Enable tracing infrastructure and pre-defined tracepoints within
> Xen.
> --
> 2.11.0
>
>
[-- Attachment #2: Type: text/html, Size: 2540 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2.1 RFC 17/12] xen/trace: Drop cycles parameter
2021-09-20 17:25 [PATCH v2 00/12] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
` (15 preceding siblings ...)
2021-09-20 19:40 ` [PATCH v2.1 16/12] xen/trace: Restrict CONFIG_TRACEBUFFER to x86 PV Andrew Cooper
@ 2021-09-21 20:08 ` Andrew Cooper
2021-09-22 7:03 ` Jan Beulich
16 siblings, 1 reply; 52+ messages in thread
From: Andrew Cooper @ 2021-09-21 20:08 UTC (permalink / raw)
To: Xen-devel
Cc: George Dunlap, Ian Jackson, Jan Beulich, Stefano Stabellini,
Wei Liu, Julien Grall, Dario Faggioli, Juergen Gross,
Volodymyr Babchuk, Meng Xu
Not a patch, but an RFC idea for one...
It occurred to me that the cycles parameter from __trace_var() and
friends is pointless, as the cycles bit is encoded in the top bit of the
event field anyway, and passed in the adjacent parameter.
Dropping the cycles parameter results in +85/-1029 (-944) net change.
The common change in callers is e.g. from:
lea 0x3c(%rsp),%rcx
mov $0x4,%edx
mov $0x1,%esi
mov $0x10f002,%edi
mov %ebp,0x3c(%rsp)
callq ffff82d04022ea20 <__trace_var>
to this:
lea 0x3c(%rsp),%rdx
mov $0x4,%esi
mov $0x8010f002,%edi
mov %ebp,0x3c(%rsp)
callq ffff82d04022ea20 <__trace_var>
Just sprinkling "| TRC_HD_CYCLE_FLAG" over the place makes things a
little bit unwieldy.
Instead, I was thinking of implementing trace() (and a thin trace_time()
wrapper) mirroring the "new API" in patch 14. Half of the trace_var()
users should be __trace_var() already because of living inside a
tb_init_done conditional, and the rest are actually opencoded TRACE()
taking no extra data.
Thoughts?
~Andrew
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2.1 RFC 17/12] xen/trace: Drop cycles parameter
2021-09-21 20:08 ` [PATCH v2.1 RFC 17/12] xen/trace: Drop cycles parameter Andrew Cooper
@ 2021-09-22 7:03 ` Jan Beulich
2021-09-22 9:38 ` Andrew Cooper
0 siblings, 1 reply; 52+ messages in thread
From: Jan Beulich @ 2021-09-22 7:03 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Dario Faggioli, Juergen Gross, Volodymyr Babchuk,
Meng Xu, Xen-devel
On 21.09.2021 22:08, Andrew Cooper wrote:
> Not a patch, but an RFC idea for one...
>
> It occurred to me that the cycles parameter from __trace_var() and
> friends is pointless, as the cycles bit is encoded in the top bit of the
> event field anyway, and passed in the adjacent parameter.
>
> Dropping the cycles parameter results in +85/-1029 (-944) net change.
>
> The common change in callers is e.g. from:
>
> lea 0x3c(%rsp),%rcx
> mov $0x4,%edx
> mov $0x1,%esi
> mov $0x10f002,%edi
> mov %ebp,0x3c(%rsp)
> callq ffff82d04022ea20 <__trace_var>
>
> to this:
>
> lea 0x3c(%rsp),%rdx
> mov $0x4,%esi
> mov $0x8010f002,%edi
> mov %ebp,0x3c(%rsp)
> callq ffff82d04022ea20 <__trace_var>
>
>
> Just sprinkling "| TRC_HD_CYCLE_FLAG" over the place makes things a
> little bit unwieldy.
>
> Instead, I was thinking of implementing trace() (and a thin trace_time()
> wrapper) mirroring the "new API" in patch 14. Half of the trace_var()
> users should be __trace_var() already because of living inside a
> tb_init_done conditional, and the rest are actually opencoded TRACE()
> taking no extra data.
>
> Thoughts?
Sounds quite reasonable to me.
Jan
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2.1 RFC 17/12] xen/trace: Drop cycles parameter
2021-09-22 7:03 ` Jan Beulich
@ 2021-09-22 9:38 ` Andrew Cooper
0 siblings, 0 replies; 52+ messages in thread
From: Andrew Cooper @ 2021-09-22 9:38 UTC (permalink / raw)
To: Jan Beulich
Cc: George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Julien Grall, Dario Faggioli, Juergen Gross, Volodymyr Babchuk,
Meng Xu, Xen-devel
On 22/09/2021 08:03, Jan Beulich wrote:
> On 21.09.2021 22:08, Andrew Cooper wrote:
>> Not a patch, but an RFC idea for one...
>>
>> It occurred to me that the cycles parameter from __trace_var() and
>> friends is pointless, as the cycles bit is encoded in the top bit of the
>> event field anyway, and passed in the adjacent parameter.
>>
>> Dropping the cycles parameter results in +85/-1029 (-944) net change.
>>
>> The common change in callers is e.g. from:
>>
>> lea 0x3c(%rsp),%rcx
>> mov $0x4,%edx
>> mov $0x1,%esi
>> mov $0x10f002,%edi
>> mov %ebp,0x3c(%rsp)
>> callq ffff82d04022ea20 <__trace_var>
>>
>> to this:
>>
>> lea 0x3c(%rsp),%rdx
>> mov $0x4,%esi
>> mov $0x8010f002,%edi
>> mov %ebp,0x3c(%rsp)
>> callq ffff82d04022ea20 <__trace_var>
>>
>>
>> Just sprinkling "| TRC_HD_CYCLE_FLAG" over the place makes things a
>> little bit unwieldy.
>>
>> Instead, I was thinking of implementing trace() (and a thin trace_time()
>> wrapper) mirroring the "new API" in patch 14. Half of the trace_var()
>> users should be __trace_var() already because of living inside a
>> tb_init_done conditional, and the rest are actually opencoded TRACE()
>> taking no extra data.
>>
>> Thoughts?
> Sounds quite reasonable to me.
In which case, for v3 I'll reposition "new API" towards the beginning of
the series, extend it with these two new APIs, then adjust all the
cleanup patches to avoid double-patching most of the call-sites.
~Andrew
^ permalink raw reply [flat|nested] 52+ messages in thread