[ovs-dev] [RFC: add openvswitch actions using BPF 1/2] BPF: add a new BPF program type BPF_PROG_TYPE_OPENVSWITCH

[Andy Zhou]

On Thu, Feb 5, 2015 at 6:48 AM, Thomas Graf <tgraf at noironetworks.com> wrote:
> On 02/04/15 at 02:48pm, Andy Zhou wrote:
>>  struct bpf_verifier_ops {
>>       /* return eBPF function prototype for verification */
>> -     const struct bpf_func_proto *(*get_func_proto)(enum bpf_func_id func_id);
>> +     const struct bpf_func_proto *(*get_func_proto)(int func_id);
>
> This change should maybe go in a separate commit.
Agreed.  Ideally, each program type should have its own func_id space. Current
registration does not allow this. Should this be added?

In the interest of reusing common helper functions, should we also consider
partition the func_id space into common ones and program type specific ones?
For example, the prink like function can be specified the common func_id space.

>
>> +static const struct bpf_func_proto *ovs_func_proto(int func_id)
>> +{
>> +     switch (func_id) {
>> +     case OVS_BPF_FUNC_output:
>> +             return &bpf_helper_output_proto;
>> +     default:
>> +             return NULL;
>> +     }
>> +}
>
> You'd still want to use the map helpers so it seems like we should
> change the bpf verified to verify against both a global and type
> specific list unless we want to add all the map helpers to
> ovs_func_proto as well.

It is not clear what OVS can benefit from the map helpers.  Map provides
a fixed sized array.  OVS data structure, such as flow, are more
dynamic and non-contiguous in memory.

Is extending BPF to allow for passing those dynamic objects a reasonable
direction?

>
>> +static bool test_is_valid_access(int off, int size, enum bpf_access_type type)
>> +{
>> +     const struct bpf_context_access *access;
>> +
>> +     if (off < 0 || off >= ARRAY_SIZE(bpf_ctx_access))
>> +             return false;
>> +
>> +     access = &bpf_ctx_access[off];
>> +     if (access->size == size && (access->type & type))
>> +             return true;
>> +
>> +     return false;
>> +}
>
> OK. I see why you kept ctxt simple at first ;-)