On Mon Jan 27, 2020 at 1:26 PM, Daniel Borkmann wrote:
[...]
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index f1d74a2bd234..332aa433d045 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2892,6 +2892,25 @@ union bpf_attr {
> > * Obtain the 64bit jiffies
> > * Return
> > * The 64 bit jiffies
> > + *
> > + * int bpf_read_branch_records(struct bpf_perf_event_data *ctx, void *buf, u32 buf_size, u64 flags)
>
>
> Small nit: s/buf_size/size/, so that it matches with your BPF_CALL
> below.
Ok
>
> +BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *,
> ctx,
> + void *, buf, u32, size, u64, flags)
>
>
> > + * Description
> > + * For an eBPF program attached to a perf event, retrieve the
> > + * branch records (struct perf_branch_entry) associated to *ctx*
> > + * and store it in the buffer pointed by *buf* up to size
> > + * *buf_size* bytes.
> > + *
> > + * The *flags* can be set to **BPF_F_GET_BRANCH_RECORDS_SIZE** to
> > + * instead return the number of bytes required to store all the
> > + * branch entries. If this flag is set, *buf* may be NULL.
> > + * Return
> > + * On success, number of bytes written to *buf*. On error, a
> > + * negative value.
>
>
> Maybe pull the 2nd paragraph from above in here so that it reflects the
> description
> of the return value when flag is used also for this case in the 'Return'
> description.
Ok.
[...]
> >
> > +BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx,
> > + void *, buf, u32, size, u64, flags)
> > +{
> > +#ifndef CONFIG_X86
> > + return -ENOENT;
> > +#else
> > + struct perf_branch_stack *br_stack = ctx->data->br_stack;
> > + u32 br_entry_size = sizeof(struct perf_branch_entry);
>
>
> 'static const u32 br_entry_size' if we use it as such below.
Ok
>
>
> > + u32 to_copy;
> > +
> > + if (unlikely(flags & ~BPF_F_GET_BRANCH_RECORDS_SIZE))
> > + return -EINVAL;
> > +
> > + if (unlikely(!br_stack))
> > + return -EINVAL;
>
>
> Why the ifdef X86? In previous thread I meant to change it into since
> it's
> implicit:
>
>
> if (unlikely(!br_stack))
> return -ENOENT;
>
>
> Or is there any other additional rationale?
Yeah, so br_stack can be null if the perf_event is misconfigured (branch
record not enabled). So we need to differentiate that from arch not
supporting it.
[...]
Thanks,
Daniel
