Re: [RFC PATCH v1 05/10] pmu.h: introduce a stacktrace area

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.07.2025 17:06, Edwin Török wrote:
> The guest always allocates a full page that is mapped for 'struct
> xen_pmu_data' (there is no smaller mapping granularity).
> 
> The existing struct contains a flexible array member that is used to
> store architectural performance counters on Intel (their number is
> dynamically determined from CPUID).
> 
> AFAICT their number is currently limited to 32 due to the bitmap in the
> CPUID only having 32 bits defined for this, although the cpuid has 8
> bits reserved to specify the number of these counters, so this may grow.
> 
> For backwards compatibility and to leave as much room for future growth
> in these counters as possible: the newly introduced hypervisor
> stacktrace area will be stored at the end of the page.
> 
> Conceptually this would've been:
> ```
> struct xen_pmu_data_v2 {
>     union {
>         struct xen_pmu_data v1;
>         uint8_t pad[PAGE_SIZE - sizeof(struct xen_pmu_hv_stacktrace)];
>     };
>     struct xen_pmu_hv_stacktrace pmu_stack;
> };
> ```
> 
> But that is not valid C due to the flexible array member in v1.

And I fear it's not a good interface anyway. What's wrong with the caller
dedicating another page to holding the stack trace related data?

> There is code duplication in 'struct xen_pmu_arch_guest',
> but xlat.h checker fails if I try to reuse that in the definition of
> xen_pmu_arch_t.

What exactly is this referring to?

> --- a/xen/include/public/arch-x86/pmu.h
> +++ b/xen/include/public/arch-x86/pmu.h
> @@ -74,6 +74,23 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_regs_t);
>  #define PMU_SAMPLE_REAL    (1<<2) /* Sample is from realmode */
>  #define PMU_SAMPLE_PV      (1<<3) /* Sample from a PV guest */
>  
> +/*
> + * Architecture-specific information describing the state of the guest at
> + * the time of a PMU interrupt.
> + */
> +struct xen_pmu_arch_guest {
> +    union {
> +        /*
> +         * Processor's registers at the time of interrupt.
> +         * WO for hypervisor, RO for guests.
> +         */
> +        xen_pmu_regs_t regs;

How's this related to stack traces?

> @@ -129,67 +146,86 @@ struct xen_pmu_arch {
>  typedef struct xen_pmu_arch xen_pmu_arch_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_pmu_arch_t);
>  
> +

Stray double blank lines.

>  /* Memory layout:
> -*                ╭─────────────────────╮
> -*                │ struct xen_pmu_data │
> -* ╒══════════════╧═════════════════════╧═══════════════════════╕ ◁╮
> -* │ vcpu_id                                                    │  │
> -* ├────────────────────────────────────────────────────────────┤  │
> -* │ pcpu_id                                                    │  │
> -* ├────────────────────────────────────────────────────────────┤  │
> -* │ domain_id                                                  │  │
> -* ├────────────────────────────────────────────────────────────┤  │
> -* │██pad███████████████████████████████████████████████████████│  │
> -* ╞════╤═╤═══╤══════════════════╤══════════════════════════════╡  │
> -* │ pmu│ │ r │ regs             │██pad█████████████████████████│  │
> -* ├────╯ ├───╯ (xen or guest)   │██████████████████████████████│  │
> -* │      ╞══════════════════════╧══════════════════════════════╡  │
> -* │      │ pmu_flags                                           │  │
> -* │      ╞═══╤════════════════════╤════════════════════════════╡  │
> -* │      │ l │ lapic_lvtpc        │████████████████████████████│  │
> -* │      ├───╯ ███████████████████│██pad███████████████████████│  │
> -* │      │     ███████████████████│████████████████████████████│  │
> -* │      ╞═══╤═╤═══════╤═════╤════╪════╤═══════╤═══════════════╡  │
> -* │      │ c │ │       │ amd │    │    │ intel │         │█████│  │
> -* │      ├───┘ │       ╰─────╯    │    ╰───────╯         │█████│  │
> -* │      │     │ counter          │ fixed_counters       │█████│  │
> -* │      │     ├──────────────────┼──────────────────────┤█████│  │
> -* │      │     │ ctrls            │ arch_counters        │█████│  │
> -* │      │     ╞═════╤════════╤═══├──────────────────────┤█████│  │
> -* │      │     │     │ regs[] │  ┆│ global_ctrl          │█████│  │
> -* │      │     │     └────────╯  ┆├──────────────────────┤█████│  │
> -* │      │     │struct           ┆│ global_ovf_ctrl      │█████│  │
> -* │      │     │xen_pmu_cntr_pair┆├──────────────────────┤█████│  │
> -* │      │     │[counters]       ┆│ global_status        │█████│  │
> -* │      │     │                 ┆├──────────────────────┤█████│  │
> -* │      │     │                 ┆│ fixed_ctrl           │█████│  │
> -* │      │     │                 ┆├──────────────────────┤█████│  │
> -* │      │     │                 ┆│ ds_area              │█████│  │
> -* │      │     │                 ┆├──────────────────────┤█████│  │
> -* │      │     │                 ┆│ pebs_enable          │█pad█│  │
> -* │      │     │                 ┆├──────────────────────┤█████│  │
> -* │      │     │                 ▽│ debugctl             │█████│  │
> -* │      │     │██████████████████╞═══════╤════════╤═════╡█████│  │
> -* │      │     │██████████████████│       │ regs[] │ ┆[0]│█████│  │
> -* │      │     │██████████████████│       └────────╯ ┆   │█████│  │
> -* │      │     │██████████████████│ uint64_t         ┆   │█████│  │
> -* │      │     │██████████████████│ [fixed_counters] ┆   │█████│  │
> -* │      │     │██████████████████│                  ┆   │█████│  │
> -* │      │     │██████████████████│                  ┆   │█████│  │
> -* │      │     │██████████████████│ ─────────────────┆   │█████│  │
> -* │      │     │██████████████████│  struct          ┆   │█████│  │
> -* │      │     │██████████████████│ xen_pmu_cntr_pair┆   │█████│  │
> -* │      │     ╘══════════════════╡ [arch_counters]  ┆   ╞═════╡  │
> -* │      │                        │                  ┆   │     │  │
> -* │      │                        │                  ▽   │     │  │
> -* │      │                        ╘══════════════════════╛     │  │
> -* │      ╘═════════════════════════════════════════════════════╡  │
> -* ╞════════════════════════════════════════════════════════════╡  │
> -* │████████████████████████████████████████████████████████████│  │
> -* ┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆  ┆
> -* ┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆  ┆
> -* │████████████████████████████████████████████████████████████│  │ PAGE_SIZE
> -* ╘════════════════════════════════════════════════════════════╛ ◁╯
> + *                ╭─────────────────────╮
> + *                │ struct xen_pmu_data │
> + * ╒══════════════╧═════════════════════╧═══════════════════════╕ ◁╮
> + * │ vcpu_id                                                    │  │
> + * ├────────────────────────────────────────────────────────────┤  │
> + * │ pcpu_id                                                    │  │
> + * ├────────────────────────────────────────────────────────────┤  │
> + * │ domain_id                                                  │  │
> + * ├────────────────────────────────────────────────────────────┤  │
> + * │██pad███████████████████████████████████████████████████████│  │
> + * ╞════╤═╤═══╤══════════════════╤══════════════════════════════╡  │
> + * │ pmu│ │ r │ regs             │██pad█████████████████████████│  │
> + * ├────╯ ├───╯ (xen or guest)   │██████████████████████████████│  │
> + * │      ╞══════════════════════╧══════════════════════════════╡  │
> + * │      │ pmu_flags                                           │  │
> + * │      ╞═══╤════════════════════╤════════════════════════════╡  │
> + * │      │ l │ lapic_lvtpc        │████████████████████████████│  │
> + * │      ├───╯ ███████████████████│██pad███████████████████████│  │
> + * │      │     ███████████████████│████████████████████████████│  │
> + * │      ╞═══╤═╤═══════╤═════╤════╪════╤═══════╤═══════════════╡  │
> + * │      │ c │ │       │ amd │    │    │ intel │         │█████│  │
> + * │      ├───┘ │       ╰─────╯    │    ╰───────╯         │█████│  │
> + * │      │     │ counter          │ fixed_counters       │█████│  │
> + * │      │     ├──────────────────┼──────────────────────┤█████│  │
> + * │      │     │ ctrls            │ arch_counters        │█████│  │
> + * │      │     ╞═════╤════════╤═══├──────────────────────┤█████│  │
> + * │      │     │     │ regs[] │  ┆│ global_ctrl          │█████│  │
> + * │      │     │     └────────╯  ┆├──────────────────────┤█████│  │
> + * │      │     │struct           ┆│ global_ovf_ctrl      │█████│  │
> + * │      │     │xen_pmu_cntr_pair┆├──────────────────────┤█████│  │
> + * │      │     │[counters]       ┆│ global_status        │█████│  │
> + * │      │     │                 ┆├──────────────────────┤█████│  │
> + * │      │     │                 ┆│ fixed_ctrl           │█████│  │
> + * │      │     │                 ┆├──────────────────────┤█████│  │
> + * │      │     │                 ┆│ ds_area              │█████│  │
> + * │      │     │                 ┆├──────────────────────┤█████│  │
> + * │      │     │                 ┆│ pebs_enable          │█pad█│  │
> + * │      │     │                 ┆├──────────────────────┤█████│  │
> + * │      │     │                 ▽│ debugctl             │█████│  │
> + * │      │     │██████████████████╞═══════╤════════╤═════╡█████│  │
> + * │      │     │██████████████████│       │ regs[] │ ┆[0]│█████│  │
> + * │      │     │██████████████████│       └────────╯ ┆   │█████│  │
> + * │      │     │██████████████████│ uint64_t         ┆   │█████│  │
> + * │      │     │██████████████████│ [fixed_counters] ┆   │█████│  │
> + * │      │     │██████████████████│                  ┆   │█████│  │
> + * │      │     │██████████████████│                  ┆   │█████│  │
> + * │      │     │██████████████████│ ─────────────────┆   │█████│  │
> + * │      │     │██████████████████│  struct          ┆   │█████│  │
> + * │      │     │██████████████████│ xen_pmu_cntr_pair┆   │█████│  │
> + * │      │     ╘══════════════════╡ [arch_counters]  ┆   ╞═════╡  │
> + * │      │                        │                  ┆   │     │  │
> + * │      │                        │                  ▽   │     │  │
> + * │      │                        ╘══════════════════════╛     │  │
> + * │      ╘═════════════════════════════════════════════════════╡  │
> + * ╞════════════════════════════════════════════════════════════╡  │
> + * │████████████████████████████████████████████████████████████│  │
> + * ┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆  ┆
> + * ┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆┆  ┆
> + * │████████████████████████████████████████████████████████████│  │
> + * |############################################################|  |
> + * |##########.------------------------------.##################|  |
> + * |##########| struct xen_pmu_hv_stacktrace |##################|  |
> + * +==========+==============================+==================+  |
> + * |                              ^ [stacktrace_nr-1]           |  |
> + * |                              :                             |  |
> + * | stacktrace[stacktrace_nr]    : [0]                         |  |
> + * +------------------------------------------------------------+  |
> + * | stacktrace_nr                                              |  |
> + * +------------------------------------------------------------+  |
> + * | guest_domain_id                                            |  |
> + * +------------------------------------------------------------+  |
> + * |##pad#######################################################|  |
> + * +=======+=+===+==================+===========================+  |
> + * | guest | | r | regs             |##pad######################|  |
> + * +-------. +---. (xen or guest)   |###########################|  |
> + * |         +======================+===========================+  |
> + * |         |##pad2############################################|  | 
> PAGE_SIZE
> + * +=========+==================================================+ <.
>  */
>  
>  #endif /* __XEN_PUBLIC_ARCH_X86_PMU_H__ */
> @@ -202,4 +238,3 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_arch_t);
>   * indent-tabs-mode: nil
>   * End:
>   */
> -

Stray change?

> @@ -120,6 +120,40 @@ struct xen_pmu_data {
>      xen_pmu_arch_t pmu;
>  };
>  
> +/* stacktrace entry populated from the end,
> + * so stacktrace_nr == 1, means that stacktrace[PMU_MAX_STACKTRCE-1] is 
> valid.
> + * This is done, so that PMU_MAX_STACKTRACE can be changed in the future, 
> without breaking the ABI.
> + * The struct itself (and thus the stacktrace_nr field) will always be 
> placed at the end of a page.
> + *
> + * See arch-x86/pmu.h for an example memory layout on x86.
> + *
> + * */
> +#define PMU_MAX_STACKTRACE 127
> +
> +/* WO for hypervisor, RO for guest */
> +struct xen_pmu_hv_stacktrace {
> +    uint64_t stacktrace[PMU_MAX_STACKTRACE];
> +    uint64_t stacktrace_nr;
> +
> +    /* Like xen_pmu_data.domain_id, but instead of DOMID_XEN always contains 
> the
> +     * domain that was interrupted (DOMID_SELF if it matches the sampling
> +     * domain).
> +     */
> +    domid_t guest_domain_id;

How's this related to stack traces? And how would it help associating back
what vCPU the data belongs to?

> +    uint8_t pad[6];
> +
> +    /* When xen_pmu_data.domain_id == DOMID_XEN, this will contain
> +    * the registers of the guest that was interrupted.
> +    * This is useful for Dom0 kernel stacktraces, even if the interrupt
> +    * arrives while in Xen.
> +    * */

Nit: Indentation.

Jan