LGTM,
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
On Sep 30, 2014, at 5:47 PM, Ben Pfaff <b...@nicira.com> wrote:
> Some of the Nicira extension actions include fixed-size 32-bit members that
> designate NXM fields. These actions can't accommodate 64-bit experimenter
> OXMs, so we need to figure out some kind of solution. This commit does
> that, in different ways for different actions.
>
> For some actions, I did not think it was worthwhile to worry about
> experimenter OXM, so I just disabled use of them. This is what I did for
> bundle, learn, and multipath actions.
>
> Other actions could be gracefully reinterpreted to support experimenter
> OXM. This is true of reg_move, which use NXM headers only at the end of
> the action and such that using an experimenter OXM would make the action
> longer (which unambigously signals to older OVS that the action is an
> error, which is desired behavior since older OVS cannot interpret this
> action). The stack push and pop actions are also in this category.
>
> reg_load was the most frustrating case. In OpenFlow 1.5 we had already
> eliminated this action in favor of OF1.5+ set_field. In other OpenFlow
> versions, though, reg_load is more powerful than set_field because it
> can modify partial fields. This commit therefore adds a new variant of
> reg_load, called reg_load2, which is simply OF1.5+ set_field with a Nicira
> extension header on it.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/bundle.c | 5 +
> lib/learn.c | 8 ++
> lib/multipath.c | 4 +
> lib/ofp-actions.c | 263 ++++++++++++++++++++++++++++++++++++++++++++---------
> 4 files changed, 236 insertions(+), 44 deletions(-)
>
> diff --git a/lib/bundle.c b/lib/bundle.c
> index a514ebb..793eb82 100644
> --- a/lib/bundle.c
> +++ b/lib/bundle.c
> @@ -207,6 +207,11 @@ bundle_parse__(const char *s, char **save_ptr,
> if (error) {
> return error;
> }
> +
> + if (!mf_nxm_header(bundle->dst.field->id)) {
> + return xasprintf("%s: experimenter OXM field '%s' not supported",
> + s, dst);
> + }
> }
>
> return NULL;
> diff --git a/lib/learn.c b/lib/learn.c
> index c04e1f3..e93015c 100644
> --- a/lib/learn.c
> +++ b/lib/learn.c
> @@ -221,6 +221,10 @@ learn_parse_load_immediate(const char *s, struct
> ofpact_learn_spec *spec)
> if (error) {
> return error;
> }
> + if (!mf_nxm_header(dst.field->id)) {
> + return xasprintf("%s: experimenter OXM field '%s' not supported",
> + full_s, s);
> + }
>
> if (!bitwise_is_all_zeros(&imm, sizeof imm, dst.n_bits,
> (8 * sizeof imm) - dst.n_bits)) {
> @@ -269,6 +273,10 @@ learn_parse_spec(const char *orig, char *name, char
> *value,
> if (error) {
> return error;
> }
> + if (!mf_nxm_header(spec->dst.field->id)) {
> + return xasprintf("%s: experimenter OXM field '%s' not supported",
> + orig, name);
> + }
>
> /* Parse source and check prerequisites. */
> if (value[0] != '\0') {
> diff --git a/lib/multipath.c b/lib/multipath.c
> index 8983c6a..edd295f 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -189,6 +189,10 @@ multipath_parse__(struct ofpact_multipath *mp, const
> char *s_, char *s)
> if (error) {
> return error;
> }
> + if (!mf_nxm_header(mp->dst.field->id)) {
> + return xasprintf("%s: experimenter OXM field '%s' not supported",
> + s, dst);
> + }
> if (mp->dst.n_bits < 16 && n_links > (1u << mp->dst.n_bits)) {
> return xasprintf("%s: %d-bit destination field has %u possible "
> "values, less than specified n_links %d",
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index a02dd22..4424fe2 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -210,10 +210,15 @@ enum ofp_raw_action_type {
> * [In OpenFlow 1.5, set_field is a superset of reg_load functionality, so
> * we drop reg_load.] */
> NXAST_RAW_REG_LOAD,
> + /* NX1.0-1.4(33): struct nx_action_reg_load2, ...
> + *
> + * [In OpenFlow 1.5, set_field is a superset of reg_load2 functionality,
> so
> + * we drop reg_load2.] */
> + NXAST_RAW_REG_LOAD2,
>
> /* OF1.5+(28): struct ofp15_action_copy_field, ... */
> OFPAT_RAW15_COPY_FIELD,
> - /* NX1.0-1.4(6): struct nx_action_reg_move. */
> + /* NX1.0-1.4(6): struct nx_action_reg_move, ... */
> NXAST_RAW_REG_MOVE,
>
> /* ## ------------------------- ## */
> @@ -248,6 +253,8 @@ enum ofp_raw_action_type {
>
> /* NX1.0+(15): struct nx_action_output_reg. */
> NXAST_RAW_OUTPUT_REG,
> + /* NX1.0+(32): struct nx_action_output_reg2. */
> + NXAST_RAW_OUTPUT_REG2,
>
> /* NX1.0+(16): struct nx_action_learn, ... */
> NXAST_RAW_LEARN,
> @@ -708,6 +715,27 @@ struct nx_action_output_reg {
> };
> OFP_ASSERT(sizeof(struct nx_action_output_reg) == 24);
>
> +/* Action structure for NXAST_OUTPUT_REG2.
> + *
> + * Like the NXAST_OUTPUT_REG but organized so that there is room for a 64-bit
> + * experimenter OXM as 'src'.
> + */
> +struct nx_action_output_reg2 {
> + ovs_be16 type; /* OFPAT_VENDOR. */
> + ovs_be16 len; /* 24. */
> + ovs_be32 vendor; /* NX_VENDOR_ID. */
> + ovs_be16 subtype; /* NXAST_OUTPUT_REG2. */
> +
> + ovs_be16 ofs_nbits; /* (ofs << 6) | (n_bits - 1). */
> + ovs_be16 max_len; /* Max length to send to controller. */
> +
> + /* Followed by:
> + * - 'src', as an OXM/NXM header (either 4 or 8 bytes).
> + * - Enough 0-bytes to pad the action out to 24 bytes. */
> + uint8_t pad[10];
> +};
> +OFP_ASSERT(sizeof(struct nx_action_output_reg2) == 24);
> +
> static enum ofperr
> decode_NXAST_RAW_OUTPUT_REG(const struct nx_action_output_reg *naor,
> struct ofpbuf *out)
> @@ -719,6 +747,7 @@ decode_NXAST_RAW_OUTPUT_REG(const struct
> nx_action_output_reg *naor,
> }
>
> output_reg = ofpact_put_OUTPUT_REG(out);
> + output_reg->ofpact.raw = NXAST_RAW_OUTPUT_REG;
> output_reg->src.field = mf_from_nxm_header(ntohl(naor->src));
> output_reg->src.ofs = nxm_decode_ofs(naor->ofs_nbits);
> output_reg->src.n_bits = nxm_decode_n_bits(naor->ofs_nbits);
> @@ -727,17 +756,61 @@ decode_NXAST_RAW_OUTPUT_REG(const struct
> nx_action_output_reg *naor,
> return mf_check_src(&output_reg->src, NULL);
> }
>
> +static enum ofperr
> +decode_NXAST_RAW_OUTPUT_REG2(const struct nx_action_output_reg2 *naor,
> + struct ofpbuf *out)
> +{
> + struct ofpact_output_reg *output_reg;
> + enum ofperr error;
> + struct ofpbuf b;
> +
> + output_reg = ofpact_put_OUTPUT_REG(out);
> + output_reg->ofpact.raw = NXAST_RAW_OUTPUT_REG2;
> + output_reg->src.ofs = nxm_decode_ofs(naor->ofs_nbits);
> + output_reg->src.n_bits = nxm_decode_n_bits(naor->ofs_nbits);
> + output_reg->max_len = ntohs(naor->max_len);
> +
> + ofpbuf_use_const(&b, naor, ntohs(naor->len));
> + ofpbuf_pull(&b, OBJECT_OFFSETOF(naor, pad));
> + error = nx_pull_header(&b, &output_reg->src.field, NULL);
> + if (error) {
> + return error;
> + }
> + if (!is_all_zeros(ofpbuf_data(&b), ofpbuf_size(&b))) {
> + return OFPERR_NXBRC_MUST_BE_ZERO;
> + }
> +
> + return mf_check_src(&output_reg->src, NULL);
> +}
> +
> static void
> encode_OUTPUT_REG(const struct ofpact_output_reg *output_reg,
> enum ofp_version ofp_version OVS_UNUSED,
> struct ofpbuf *out)
> {
> - struct nx_action_output_reg *naor = put_NXAST_OUTPUT_REG(out);
> + /* If 'output_reg' came in as an NXAST_RAW_OUTPUT_REG2 action, or if it
> + * cannot be encoded in the older form, encode it as
> + * NXAST_RAW_OUTPUT_REG2. */
> + if (output_reg->ofpact.raw == NXAST_RAW_OUTPUT_REG2
> + || !mf_nxm_header(output_reg->src.field->id)) {
> + struct nx_action_output_reg2 *naor = put_NXAST_OUTPUT_REG2(out);
> + size_t size = ofpbuf_size(out);
> +
> + naor->ofs_nbits = nxm_encode_ofs_nbits(output_reg->src.ofs,
> + output_reg->src.n_bits);
> + naor->max_len = htons(output_reg->max_len);
> +
> + ofpbuf_set_size(out, size - sizeof naor->pad);
> + nx_put_header(out, output_reg->src.field->id, 0, false);
> + ofpbuf_set_size(out, size);
> + } else {
> + struct nx_action_output_reg *naor = put_NXAST_OUTPUT_REG(out);
>
> - naor->ofs_nbits = nxm_encode_ofs_nbits(output_reg->src.ofs,
> - output_reg->src.n_bits);
> - naor->src = htonl(mf_nxm_header(output_reg->src.field->id));
> - naor->max_len = htons(output_reg->max_len);
> + naor->ofs_nbits = nxm_encode_ofs_nbits(output_reg->src.ofs,
> + output_reg->src.n_bits);
> + naor->src = htonl(mf_nxm_header(output_reg->src.field->id));
> + naor->max_len = htons(output_reg->max_len);
> + }
> }
>
> static char * WARN_UNUSED_RESULT
> @@ -1747,10 +1820,12 @@ struct nx_action_reg_move {
> ovs_be16 n_bits; /* Number of bits. */
> ovs_be16 src_ofs; /* Starting bit offset in source. */
> ovs_be16 dst_ofs; /* Starting bit offset in destination. */
> - ovs_be32 src; /* Source register. */
> - ovs_be32 dst; /* Destination register. */
> + /* Followed by:
> + * - OXM/NXM header for source field (4 or 8 bytes).
> + * - OXM/NXM header for destination field (4 or 8 bytes).
> + * - Padding with 0-bytes to a multiple of 8 bytes, if necessary. */
> };
> -OFP_ASSERT(sizeof(struct nx_action_reg_move) == 24);
> +OFP_ASSERT(sizeof(struct nx_action_reg_move) == 16);
>
> static enum ofperr
> decode_OFPAT_RAW15_COPY_FIELD(const struct ofp15_action_copy_field *oacf,
> @@ -1794,15 +1869,29 @@ decode_NXAST_RAW_REG_MOVE(const struct
> nx_action_reg_move *narm,
> struct ofpbuf *ofpacts)
> {
> struct ofpact_reg_move *move;
> + enum ofperr error;
> + struct ofpbuf b;
>
> move = ofpact_put_REG_MOVE(ofpacts);
> - move->src.field = mf_from_nxm_header(ntohl(narm->src));
> move->src.ofs = ntohs(narm->src_ofs);
> move->src.n_bits = ntohs(narm->n_bits);
> - move->dst.field = mf_from_nxm_header(ntohl(narm->dst));
> move->dst.ofs = ntohs(narm->dst_ofs);
> move->dst.n_bits = ntohs(narm->n_bits);
>
> + ofpbuf_use_const(&b, narm, ntohs(narm->len));
> + ofpbuf_pull(&b, sizeof *narm);
> + error = nx_pull_header(&b, &move->src.field, NULL);
> + if (error) {
> + return error;
> + }
> + error = nx_pull_header(&b, &move->dst.field, NULL);
> + if (error) {
> + return error;
> + }
> + if (!is_all_zeros(ofpbuf_data(&b), ofpbuf_size(&b))) {
> + return OFPERR_NXBRC_MUST_BE_ZERO;
> + }
> +
> return nxm_reg_move_check(move, NULL);
> }
>
> @@ -1810,11 +1899,9 @@ static void
> encode_REG_MOVE(const struct ofpact_reg_move *move,
> enum ofp_version ofp_version, struct ofpbuf *out)
> {
> + size_t start_ofs = ofpbuf_size(out);
> if (ofp_version >= OFP15_VERSION) {
> - struct ofp15_action_copy_field *copy;
> - size_t start_ofs = ofpbuf_size(out);
> -
> - copy = put_OFPAT15_COPY_FIELD(out);
> + struct ofp15_action_copy_field *copy = put_OFPAT15_COPY_FIELD(out);
> copy->n_bits = htons(move->dst.n_bits);
> copy->src_offset = htons(move->src.ofs);
> copy->dst_offset = htons(move->dst.ofs);
> @@ -1822,17 +1909,15 @@ encode_REG_MOVE(const struct ofpact_reg_move *move,
> ofpbuf_set_size(out, ofpbuf_size(out) - sizeof copy->pad);
> nx_put_header(out, move->src.field->id, ofp_version, false);
> nx_put_header(out, move->dst.field->id, ofp_version, false);
> - pad_ofpat(out, start_ofs);
> } else {
> - struct nx_action_reg_move *narm;
> -
> - narm = put_NXAST_REG_MOVE(out);
> + struct nx_action_reg_move *narm = put_NXAST_REG_MOVE(out);
> narm->n_bits = htons(move->dst.n_bits);
> narm->src_ofs = htons(move->src.ofs);
> narm->dst_ofs = htons(move->dst.ofs);
> - narm->src = htonl(mf_nxm_header(move->src.field->id));
> - narm->dst = htonl(mf_nxm_header(move->dst.field->id));
> + nx_put_header(out, move->src.field->id, 0, false);
> + nx_put_header(out, move->dst.field->id, 0, false);
> }
> + pad_ofpat(out, start_ofs);
> }
>
> static char * WARN_UNUSED_RESULT
> @@ -1921,6 +2006,26 @@ struct nx_action_reg_load {
> };
> OFP_ASSERT(sizeof(struct nx_action_reg_load) == 24);
>
> +/* Action structure for NXAST_REG_LOAD2.
> + *
> + * Compared to OFPAT_SET_FIELD, we can use this to set whole or partial
> fields
> + * in any OpenFlow version. Compared to NXAST_REG_LOAD, we can use this to
> set
> + * OXM experimenter fields. */
> +struct nx_action_reg_load2 {
> + ovs_be16 type; /* OFPAT_VENDOR. */
> + ovs_be16 len; /* At least 16. */
> + ovs_be32 vendor; /* NX_VENDOR_ID. */
> + ovs_be16 subtype; /* NXAST_SET_FIELD. */
> +
> + /* Followed by:
> + * - An NXM/OXM header, value, and optionally a mask.
> + * - Enough 0-bytes to pad out to a multiple of 64 bits.
> + *
> + * The "pad" member is the beginning of the above. */
> + uint8_t pad[6];
> +};
> +OFP_ASSERT(sizeof(struct nx_action_reg_load2) == 16);
> +
> static enum ofperr
> decode_ofpat_set_field(const struct ofp12_action_set_field *oasf,
> bool may_mask, struct ofpbuf *ofpacts)
> @@ -2027,6 +2132,35 @@ decode_NXAST_RAW_REG_LOAD(const struct
> nx_action_reg_load *narl,
> return 0;
> }
>
> +static enum ofperr
> +decode_NXAST_RAW_REG_LOAD2(const struct nx_action_reg_load2 *narl,
> + struct ofpbuf *out)
> +{
> + struct ofpact_set_field *sf;
> + enum ofperr error;
> + struct ofpbuf b;
> +
> + sf = ofpact_put_SET_FIELD(out);
> + sf->ofpact.raw = NXAST_RAW_REG_LOAD2;
> +
> + ofpbuf_use_const(&b, narl, ntohs(narl->len));
> + ofpbuf_pull(&b, OBJECT_OFFSETOF(narl, pad));
> + error = nx_pull_entry(&b, &sf->field, &sf->value, &sf->mask);
> + if (error) {
> + return error;
> + }
> + if (!is_all_zeros(ofpbuf_data(&b), ofpbuf_size(&b))) {
> + return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> + }
> +
> + if (!sf->field->writable) {
> + VLOG_WARN_RL(&rl, "destination field %s is not writable",
> + sf->field->name);
> + return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> + }
> + return 0;
> +}
> +
> static void
> ofpact_put_set_field(struct ofpbuf *openflow, enum ofp_version ofp_version,
> enum mf_field_id field, uint64_t value_)
> @@ -2068,15 +2202,30 @@ next_load_segment(const struct ofpact_set_field *sf,
> static void
> set_field_to_nxast(const struct ofpact_set_field *sf, struct ofpbuf *openflow)
> {
> - struct mf_subfield dst;
> - uint64_t value;
> + /* If 'sf' cannot be encoded as NXAST_REG_LOAD because it requires an
> + * experimenter OXM (or if it came in as NXAST_REG_LOAD2), encode as
> + * NXAST_REG_LOAD2. Otherwise use NXAST_REG_LOAD, which is backward
> + * compatible. */
> + if (sf->ofpact.raw == NXAST_RAW_REG_LOAD2
> + || !mf_nxm_header(sf->field->id)) {
> + struct nx_action_reg_load2 *narl OVS_UNUSED;
> + size_t start_ofs = ofpbuf_size(openflow);
> +
> + narl = put_NXAST_REG_LOAD2(openflow);
> + ofpbuf_set_size(openflow, ofpbuf_size(openflow) - sizeof narl->pad);
> + nx_put_entry(openflow, sf->field->id, 0, &sf->value, &sf->mask);
> + pad_ofpat(openflow, start_ofs);
> + } else {
> + struct mf_subfield dst;
> + uint64_t value;
>
> - dst.ofs = dst.n_bits = 0;
> - while (next_load_segment(sf, &dst, &value)) {
> - struct nx_action_reg_load *narl = put_NXAST_REG_LOAD(openflow);
> - narl->ofs_nbits = nxm_encode_ofs_nbits(dst.ofs, dst.n_bits);
> - narl->dst = htonl(mf_nxm_header(dst.field->id));
> - narl->value = htonll(value);
> + dst.ofs = dst.n_bits = 0;
> + while (next_load_segment(sf, &dst, &value)) {
> + struct nx_action_reg_load *narl = put_NXAST_REG_LOAD(openflow);
> + narl->ofs_nbits = nxm_encode_ofs_nbits(dst.ofs, dst.n_bits);
> + narl->dst = htonl(mf_nxm_header(dst.field->id));
> + narl->value = htonll(value);
> + }
> }
> }
>
> @@ -2219,10 +2368,12 @@ encode_SET_FIELD(const struct ofpact_set_field *sf,
> enum ofp_version ofp_version, struct ofpbuf *out)
> {
> if (ofp_version >= OFP15_VERSION) {
> - /* OF1.5+ only has Set-Field (we drop NXAST_REG_LOAD entirely). */
> + /* OF1.5+ only has Set-Field (reg_load is redundant so we drop it
> + * entirely). */
> set_field_to_set_field(sf, ofp_version, out);
> - } else if (sf->ofpact.raw == NXAST_RAW_REG_LOAD) {
> - /* It came in as NXAST_REG_LOAD, send it out the same way. */
> + } else if (sf->ofpact.raw == NXAST_RAW_REG_LOAD ||
> + sf->ofpact.raw == NXAST_RAW_REG_LOAD2) {
> + /* It came in as reg_load, send it out the same way. */
> set_field_to_nxast(sf, out);
> } else if (ofp_version < OFP12_VERSION) {
> /* OpenFlow 1.0 and 1.1 don't have Set-Field. */
> @@ -2378,19 +2529,36 @@ struct nx_action_stack {
> ovs_be32 vendor; /* NX_VENDOR_ID. */
> ovs_be16 subtype; /* NXAST_STACK_PUSH or NXAST_STACK_POP. */
> ovs_be16 offset; /* Bit offset into the field. */
> - ovs_be32 field; /* The field used for push or pop. */
> - ovs_be16 n_bits; /* (n_bits + 1) bits of the field. */
> - uint8_t zero[6]; /* Reserved, must be zero. */
> + /* Followed by:
> + * - OXM/NXM header for field to push or pop (4 or 8 bytes).
> + * - ovs_be16 'n_bits', the number of bits to extract from the field.
> + * - Enough 0-bytes to pad out the action to 24 bytes. */
> + uint8_t pad[12]; /* See above. */
> };
> OFP_ASSERT(sizeof(struct nx_action_stack) == 24);
>
> -static void
> +static enum ofperr
> decode_stack_action(const struct nx_action_stack *nasp,
> struct ofpact_stack *stack_action)
> {
> - stack_action->subfield.field = mf_from_nxm_header(ntohl(nasp->field));
> + enum ofperr error;
> + struct ofpbuf b;
> +
> stack_action->subfield.ofs = ntohs(nasp->offset);
> - stack_action->subfield.n_bits = ntohs(nasp->n_bits);
> +
> + ofpbuf_use_const(&b, nasp, sizeof *nasp);
> + ofpbuf_pull(&b, OBJECT_OFFSETOF(nasp, pad));
> + error = nx_pull_header(&b, &stack_action->subfield.field, NULL);
> + if (error) {
> + return error;
> + }
> + stack_action->subfield.n_bits = ntohs(*(const ovs_be16 *)
> ofpbuf_data(&b));
> + ofpbuf_pull(&b, 2);
> + if (!is_all_zeros(ofpbuf_data(&b), ofpbuf_size(&b))) {
> + return OFPERR_NXBRC_MUST_BE_ZERO;
> + }
> +
> + return 0;
> }
>
> static enum ofperr
> @@ -2398,8 +2566,8 @@ decode_NXAST_RAW_STACK_PUSH(const struct
> nx_action_stack *nasp,
> struct ofpbuf *ofpacts)
> {
> struct ofpact_stack *push = ofpact_put_STACK_PUSH(ofpacts);
> - decode_stack_action(nasp, push);
> - return nxm_stack_push_check(push, NULL);
> + enum ofperr error = decode_stack_action(nasp, push);
> + return error ? error : nxm_stack_push_check(push, NULL);
> }
>
> static enum ofperr
> @@ -2407,17 +2575,24 @@ decode_NXAST_RAW_STACK_POP(const struct
> nx_action_stack *nasp,
> struct ofpbuf *ofpacts)
> {
> struct ofpact_stack *pop = ofpact_put_STACK_POP(ofpacts);
> - decode_stack_action(nasp, pop);
> - return nxm_stack_pop_check(pop, NULL);
> + enum ofperr error = decode_stack_action(nasp, pop);
> + return error ? error : nxm_stack_pop_check(pop, NULL);
> }
>
> static void
> encode_STACK_op(const struct ofpact_stack *stack_action,
> struct nx_action_stack *nasp)
> {
> + struct ofpbuf b;
> + ovs_be16 n_bits;
> +
> nasp->offset = htons(stack_action->subfield.ofs);
> - nasp->n_bits = htons(stack_action->subfield.n_bits);
> - nasp->field = htonl(mf_nxm_header(stack_action->subfield.field->id));
> +
> + ofpbuf_use_stack(&b, nasp, ntohs(nasp->len));
> + ofpbuf_put_uninit(&b, OBJECT_OFFSETOF(nasp, pad));
> + nx_put_header(&b, stack_action->subfield.field->id, 0, false);
> + n_bits = htons(stack_action->subfield.n_bits);
> + ofpbuf_put(&b, &n_bits, sizeof n_bits);
> }
>
> static void
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
