Patches 1-3 fix bugs causing uninitialised stack to leak into the trace
buffers.  Xentrace is a developer/debugging activity restricted to fully
privileged entities, so the leaking of uninitialised stack contents is not a
security concern here.

Patches 4 and 5 are cleanup worthy of backporting, because their knock-on
effects in release builds.

Patches 6 and later are cleanup and probably not for backporting.  They
convert all trace records to using fixed types, and move some PV-specifics to
only be built for PV && TRACEBUFFER, and removing stub files from all
architectures.

I have yet more cleanup in progress making most of the macros disappear, but
this series is getting long enough already, (and taking time I don't really
have).

Andrew Cooper (12):
  xen/trace: Don't over-read trace objects
  xen/memory: Remove tail padding from TRC_MEM_* records
  xen/credit2: Remove tail padding from TRC_CSCHED2_* records
  x86/hvm: Reduce stack usage from HVMTRACE_ND()
  x86/hvm: Remove duplicate calls caused by tracing
  xen/credit2: Clean up trace handling
  xen/rt: Clean up trace handling
  xen/sched: Clean up trace handling
  xen/trace: Minor code cleanup
  x86/pv: Move x86/trace.c to x86/pv/trace.c
  xen/arch: Drop asm-*/trace.h
  x86/trace: Clean up trace handling

 tools/xentrace/formats               |   4 +
 tools/xentrace/xenalyze.c            |  12 +-
 xen/arch/x86/Makefile                |   1 -
 xen/arch/x86/hvm/hpet.c              |  15 +-
 xen/arch/x86/hvm/hvm.c               |   5 +-
 xen/arch/x86/hvm/svm/svm.c           |   8 +-
 xen/arch/x86/hvm/vlapic.c            |  23 ++-
 xen/arch/x86/hvm/vmx/vmx.c           |   9 +-
 xen/arch/x86/hvm/vpic.c              |   9 +-
 xen/arch/x86/irq.c                   |   4 +-
 xen/arch/x86/mm/p2m-pod.c            |  17 +-
 xen/arch/x86/mm/p2m-pt.c             |   6 +-
 xen/arch/x86/mm/shadow/multi.c       |   2 +-
 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              | 141 ++++++++++++++
 xen/arch/x86/pv/traps.c              |   2 +-
 xen/arch/x86/trace.c                 | 159 ----------------
 xen/arch/x86/traps.c                 |   3 +-
 xen/common/memory.c                  |   4 +-
 xen/common/sched/core.c              |   4 +-
 xen/common/sched/credit.c            |  38 ++--
 xen/common/sched/credit2.c           | 344 +++++++++++++++++------------------
 xen/common/sched/null.c              |  42 +++--
 xen/common/sched/rt.c                | 121 ++++++------
 xen/common/trace.c                   |  60 +++---
 xen/include/asm-arm/trace.h          |  12 --
 xen/include/asm-x86/hvm/trace.h      |  30 ++-
 xen/include/asm-x86/{ => pv}/trace.h |   8 +-
 xen/include/xen/trace.h              |  13 +-
 32 files changed, 537 insertions(+), 565 deletions(-)
 create mode 100644 xen/arch/x86/pv/trace.c
 delete mode 100644 xen/arch/x86/trace.c
 delete mode 100644 xen/include/asm-arm/trace.h
 rename xen/include/asm-x86/{ => pv}/trace.h (92%)

-- 
2.11.0

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

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

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