Xen flamegraph (hypervisor stacktrace profile) support

[RFC PATCH v1 01/10] pmu.h: add a BUILD_BUG_ON to ensure it fits within one page

Posted by Edwin Török 4 months, 3 weeks ago

Linux already has a similar BUILD_BUG_ON.
Currently this struct is ~224 bytes on x86-64.

No functional change.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
---
 xen/arch/x86/cpu/vpmu.c  | 1 +
 xen/include/public/pmu.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index c28192ea26..7be79c2d00 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -401,6 +401,7 @@ static int vpmu_arch_initialise(struct vcpu *v)
     uint8_t vendor = current_cpu_data.x86_vendor;
     int ret;
 
+    BUILD_BUG_ON(sizeof(struct xen_pmu_data) > PAGE_SIZE);
     BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ);
     BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);
     BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ);
diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
index af8b7babdd..15decc024d 100644
--- a/xen/include/public/pmu.h
+++ b/xen/include/public/pmu.h
@@ -93,6 +93,9 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
  * Architecture-independent fields of xen_pmu_data are WO for the hypervisor
  * and RO for the guest but some fields in xen_pmu_arch can be writable
  * by both the hypervisor and the guest (see arch-$arch/pmu.h).
+ *
+ * PAGE_SIZE bytes of memory are allocated.
+ * This struct cannot be larger than PAGE_SIZE.
  */
 struct xen_pmu_data {
     /* Interrupted VCPU */
-- 
2.47.1

Re: [RFC PATCH v1 01/10] pmu.h: add a BUILD_BUG_ON to ensure it fits within one page

Posted by Andrew Cooper 4 months, 2 weeks ago

On 25/07/2025 4:06 pm, Edwin Török wrote:
> Linux already has a similar BUILD_BUG_ON.
> Currently this struct is ~224 bytes on x86-64.
>
> No functional change.
>
> Signed-off-by: Edwin Török <edwin.torok@cloud.com>
> ---
>  xen/arch/x86/cpu/vpmu.c  | 1 +
>  xen/include/public/pmu.h | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> index c28192ea26..7be79c2d00 100644
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -401,6 +401,7 @@ static int vpmu_arch_initialise(struct vcpu *v)
>      uint8_t vendor = current_cpu_data.x86_vendor;
>      int ret;
>  
> +    BUILD_BUG_ON(sizeof(struct xen_pmu_data) > PAGE_SIZE);
>      BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ);
>      BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);
>      BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ);

This is fine (even if it ought to be elsewhere, but don't worry about that).

> diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
> index af8b7babdd..15decc024d 100644
> --- a/xen/include/public/pmu.h
> +++ b/xen/include/public/pmu.h
> @@ -93,6 +93,9 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
>   * Architecture-independent fields of xen_pmu_data are WO for the hypervisor
>   * and RO for the guest but some fields in xen_pmu_arch can be writable
>   * by both the hypervisor and the guest (see arch-$arch/pmu.h).
> + *
> + * PAGE_SIZE bytes of memory are allocated.
> + * This struct cannot be larger than PAGE_SIZE.

This isn't.  Xen's PAGE_SIZE is not necessarily the same as PAGE_SIZE in
the guest consuming this header.

This highlights one of the problems that Xen's ABI entrenches.  Being
x86-only, it's 4k in practice, but there's no easy solution.

I'd just skip this comment.  Anything else is going to get tied up in
unrelated bigger problems.

~Andrew

Re: [RFC PATCH v1 01/10] pmu.h: add a BUILD_BUG_ON to ensure it fits within one page

Posted by Edwin Torok 4 months, 2 weeks ago

On Mon, Jul 28, 2025 at 11:21 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 25/07/2025 4:06 pm, Edwin Török wrote:
> > Linux already has a similar BUILD_BUG_ON.
> > Currently this struct is ~224 bytes on x86-64.
> >
> > No functional change.
> >
> > Signed-off-by: Edwin Török <edwin.torok@cloud.com>
> > ---
> >  xen/arch/x86/cpu/vpmu.c  | 1 +
> >  xen/include/public/pmu.h | 3 +++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> > index c28192ea26..7be79c2d00 100644
> > --- a/xen/arch/x86/cpu/vpmu.c
> > +++ b/xen/arch/x86/cpu/vpmu.c
> > @@ -401,6 +401,7 @@ static int vpmu_arch_initialise(struct vcpu *v)
> >      uint8_t vendor = current_cpu_data.x86_vendor;
> >      int ret;
> >
> > +    BUILD_BUG_ON(sizeof(struct xen_pmu_data) > PAGE_SIZE);
> >      BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ);
> >      BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);
> >      BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ);
>
> This is fine (even if it ought to be elsewhere, but don't worry about that).
>
> > diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
> > index af8b7babdd..15decc024d 100644
> > --- a/xen/include/public/pmu.h
> > +++ b/xen/include/public/pmu.h
> > @@ -93,6 +93,9 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
> >   * Architecture-independent fields of xen_pmu_data are WO for the hypervisor
> >   * and RO for the guest but some fields in xen_pmu_arch can be writable
> >   * by both the hypervisor and the guest (see arch-$arch/pmu.h).
> > + *
> > + * PAGE_SIZE bytes of memory are allocated.
> > + * This struct cannot be larger than PAGE_SIZE.
>
> This isn't.  Xen's PAGE_SIZE is not necessarily the same as PAGE_SIZE in
> the guest consuming this header.
>
> This highlights one of the problems that Xen's ABI entrenches.  Being
> x86-only, it's 4k in practice, but there's no easy solution.
>
> I'd just skip this comment.  Anything else is going to get tied up in
> unrelated bigger problems.

Thanks, I'll drop this comment in the next version of the series.

--Edwin

Re: [RFC PATCH v1 01/10] pmu.h: add a BUILD_BUG_ON to ensure it fits within one page

Posted by Jan Beulich 4 months, 2 weeks ago

On 28.07.2025 12:22, Edwin Torok wrote:
> On Mon, Jul 28, 2025 at 11:21 AM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>>
>> On 25/07/2025 4:06 pm, Edwin Török wrote:
>>> Linux already has a similar BUILD_BUG_ON.
>>> Currently this struct is ~224 bytes on x86-64.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Edwin Török <edwin.torok@cloud.com>
>>> ---
>>>  xen/arch/x86/cpu/vpmu.c  | 1 +
>>>  xen/include/public/pmu.h | 3 +++
>>>  2 files changed, 4 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
>>> index c28192ea26..7be79c2d00 100644
>>> --- a/xen/arch/x86/cpu/vpmu.c
>>> +++ b/xen/arch/x86/cpu/vpmu.c
>>> @@ -401,6 +401,7 @@ static int vpmu_arch_initialise(struct vcpu *v)
>>>      uint8_t vendor = current_cpu_data.x86_vendor;
>>>      int ret;
>>>
>>> +    BUILD_BUG_ON(sizeof(struct xen_pmu_data) > PAGE_SIZE);
>>>      BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ);
>>>      BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);
>>>      BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ);
>>
>> This is fine (even if it ought to be elsewhere, but don't worry about that).
>>
>>> diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
>>> index af8b7babdd..15decc024d 100644
>>> --- a/xen/include/public/pmu.h
>>> +++ b/xen/include/public/pmu.h
>>> @@ -93,6 +93,9 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
>>>   * Architecture-independent fields of xen_pmu_data are WO for the hypervisor
>>>   * and RO for the guest but some fields in xen_pmu_arch can be writable
>>>   * by both the hypervisor and the guest (see arch-$arch/pmu.h).
>>> + *
>>> + * PAGE_SIZE bytes of memory are allocated.
>>> + * This struct cannot be larger than PAGE_SIZE.
>>
>> This isn't.  Xen's PAGE_SIZE is not necessarily the same as PAGE_SIZE in
>> the guest consuming this header.
>>
>> This highlights one of the problems that Xen's ABI entrenches.  Being
>> x86-only, it's 4k in practice, but there's no easy solution.
>>
>> I'd just skip this comment.  Anything else is going to get tied up in
>> unrelated bigger problems.
> 
> Thanks, I'll drop this comment in the next version of the series.

As said, I'm happy to ack the change with the comment adjustment dropped.
That is, I could easily carry out what you say above while committing.

Jan

Re: [RFC PATCH v1 01/10] pmu.h: add a BUILD_BUG_ON to ensure it fits within one page

Posted by Jan Beulich 4 months, 2 weeks ago

On 25.07.2025 17:06, Edwin Török wrote:
> Linux already has a similar BUILD_BUG_ON.
> Currently this struct is ~224 bytes on x86-64.
> 
> No functional change.
> 
> Signed-off-by: Edwin Török <edwin.torok@cloud.com>
> ---
>  xen/arch/x86/cpu/vpmu.c  | 1 +
>  xen/include/public/pmu.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> index c28192ea26..7be79c2d00 100644
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -401,6 +401,7 @@ static int vpmu_arch_initialise(struct vcpu *v)
>      uint8_t vendor = current_cpu_data.x86_vendor;
>      int ret;
>  
> +    BUILD_BUG_ON(sizeof(struct xen_pmu_data) > PAGE_SIZE);
>      BUILD_BUG_ON(sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ);
>      BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);
>      BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ);

I'm fine with this change, and in isolation it can have my ack.

> --- a/xen/include/public/pmu.h
> +++ b/xen/include/public/pmu.h
> @@ -93,6 +93,9 @@ DEFINE_XEN_GUEST_HANDLE(xen_pmu_params_t);
>   * Architecture-independent fields of xen_pmu_data are WO for the hypervisor
>   * and RO for the guest but some fields in xen_pmu_arch can be writable
>   * by both the hypervisor and the guest (see arch-$arch/pmu.h).
> + *
> + * PAGE_SIZE bytes of memory are allocated.
> + * This struct cannot be larger than PAGE_SIZE.
>   */
>  struct xen_pmu_data {
>      /* Interrupted VCPU */

I'm not happy about this change: PAGE_SIZE is a Xen-internal entity, which
has no specific meaning in the public interface. (The fact that under io/
there are a number of similar references doesn't justify the use here.)

Jan