Re: [PATCH v7 02/10] xen/domain: Add vmtrace_frames domain creation parameter

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.01.2021 22:27, Andrew Cooper wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -132,6 +132,48 @@ static void vcpu_info_reset(struct vcpu *v)
>      v->vcpu_info_mfn = INVALID_MFN;
>  }
>  
> +static void vmtrace_free_buffer(struct vcpu *v)
> +{
> +    const struct domain *d = v->domain;
> +    struct page_info *pg = v->vmtrace.buf;
> +    unsigned int i;
> +
> +    if ( !pg )
> +        return;
> +
> +    for ( i = 0; i < d->vmtrace_frames; i++ )
> +    {
> +        put_page_alloc_ref(&pg[i]);
> +        put_page_and_type(&pg[i]);
> +    }
> +
> +    v->vmtrace.buf = NULL;

To set a good precedent, maybe this wants moving up ahead of
the loop and ...

> +}
> +
> +static int vmtrace_alloc_buffer(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    struct page_info *pg;
> +    unsigned int i;
> +
> +    if ( !d->vmtrace_frames )
> +        return 0;
> +
> +    pg = alloc_domheap_pages(d, get_order_from_pages(d->vmtrace_frames),
> +                             MEMF_no_refcount);
> +    if ( !pg )
> +        return -ENOMEM;
> +
> +    v->vmtrace.buf = pg;

... this wants moving down past the loop, to avoid
globally announcing something that isn't fully initialized
yet / anymore?

> +    for ( i = 0; i < d->vmtrace_frames; i++ )
> +        /* Domain can't know about this page yet - something fishy going on. 
> */
> +        if ( !get_page_and_type(&pg[i], d, PGT_writable_page) )
> +            BUG();

Whatever the final verdict to the other similar places
that one of your patch changes should be applied here,
too.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -94,6 +94,7 @@ struct xen_domctl_createdomain {
>      uint32_t max_evtchn_port;
>      int32_t max_grant_frames;
>      int32_t max_maptrack_frames;
> +    uint32_t vmtrace_frames;

Considering page size related irritations elsewhere in the
public interface, could you have a comment clarify the unit
of this value (Xen's page size according to the rest of the
patch), and that space will be allocated once per-vCPU
rather than per-domain (to stand a chance of recognizing
the ultimate memory footprint resulting from this)?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -257,6 +257,10 @@ struct vcpu
>      /* vPCI per-vCPU area, used to store data for long running operations. */
>      struct vpci_vcpu vpci;
>  
> +    struct {
> +        struct page_info *buf;
> +    } vmtrace;

While perhaps minor, I'm unconvinced "buf" is a good name
for a field of this type.

> @@ -470,6 +474,9 @@ struct domain
>      unsigned    pbuf_idx;
>      spinlock_t  pbuf_lock;
>  
> +    /* Used by vmtrace features */
> +    uint32_t    vmtrace_frames;

unsigned int? Also could you move this to an existing 32-bit
hole, like immediately after "monitor"?

Jan