Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
Two small nits below,
Jarno
On Aug 4, 2014, at 9:21 AM, Ben Pfaff <b...@nicira.com> wrote:
> This will allow, later, to centralize all of the knowledge of instruction
> encoding inside ofp-actions.
>
> OFPIT11_ALL and OFPIT13_ALL are no longer used, so this commit removes
> them. Their definitions were wrong (they did not shift each bit into
> position correctly), so this commit is also a small bug fix.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> include/openflow/openflow-1.1.h | 4 ---
> include/openflow/openflow-1.3.h | 4 ---
> lib/ofp-actions.c | 66 +++++++++++++++++++++++++++++++++++++++++
> lib/ofp-actions.h | 4 +++
> lib/ofp-util.c | 6 ++--
> lib/ofp-util.h | 2 +-
> ofproto/ofproto-provider.h | 6 ++--
> ofproto/ofproto.c | 2 +-
> tests/ofproto.at | 2 +-
> 9 files changed, 80 insertions(+), 16 deletions(-)
>
> diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
> index bb6bcb0..5e618c9 100644
> --- a/include/openflow/openflow-1.1.h
> +++ b/include/openflow/openflow-1.1.h
> @@ -290,10 +290,6 @@ enum ofp11_instruction_type {
> OFPIT11_EXPERIMENTER = 0xFFFF /* Experimenter instruction */
> };
>
> -#define OFPIT11_ALL (OFPIT11_GOTO_TABLE | OFPIT11_WRITE_METADATA | \
> - OFPIT11_WRITE_ACTIONS | OFPIT11_APPLY_ACTIONS | \
> - OFPIT11_CLEAR_ACTIONS)
> -
> #define OFP11_INSTRUCTION_ALIGN 8
>
> /* Generic ofp_instruction structure. */
> diff --git a/include/openflow/openflow-1.3.h b/include/openflow/openflow-1.3.h
> index cc425f1..39de5b3 100644
> --- a/include/openflow/openflow-1.3.h
> +++ b/include/openflow/openflow-1.3.h
> @@ -91,10 +91,6 @@ enum ofp13_instruction_type {
> OFPIT13_METER = 6 /* Apply meter (rate limiter) */
> };
>
> -#define OFPIT13_ALL (OFPIT11_GOTO_TABLE | OFPIT11_WRITE_METADATA | \
> - OFPIT11_WRITE_ACTIONS | OFPIT11_APPLY_ACTIONS | \
> - OFPIT11_CLEAR_ACTIONS | OFPIT13_METER)
> -
> /* Instruction structure for OFPIT_METER */
> struct ofp13_instruction_meter {
> ovs_be16 type; /* OFPIT13_METER */
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 3818b2d..edcf25f 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -1646,6 +1646,72 @@ OVS_INSTRUCTIONS
> }
> }
>
> +struct ovsinst_xlate {
> + enum ovs_instruction_type ovsinst;
> + int ofpit;
> +};
It would be nice to have comment on the latter ref. OpenFlow specs. I’m not so
bothered about “xlate” this time :-)
> +
> +static const struct ovsinst_xlate *
> +get_ovsinst_xlate(enum ofp_version version)
> +{
> + /* OpenFlow 1.1 and 1.2 instructions. */
> + static const struct ovsinst_xlate of11[] = {
> + { OVSINST_OFPIT11_GOTO_TABLE, 1 },
> + { OVSINST_OFPIT11_WRITE_METADATA, 2 },
> + { OVSINST_OFPIT11_WRITE_ACTIONS, 3 },
> + { OVSINST_OFPIT11_APPLY_ACTIONS, 4 },
> + { OVSINST_OFPIT11_CLEAR_ACTIONS, 5 },
> + { 0, -1 },
> + };
> +
> + /* OpenFlow 1.3+ instructions. */
> + static const struct ovsinst_xlate of13[] = {
> + { OVSINST_OFPIT11_GOTO_TABLE, 1 },
> + { OVSINST_OFPIT11_WRITE_METADATA, 2 },
> + { OVSINST_OFPIT11_WRITE_ACTIONS, 3 },
> + { OVSINST_OFPIT11_APPLY_ACTIONS, 4 },
> + { OVSINST_OFPIT11_CLEAR_ACTIONS, 5 },
> + { OVSINST_OFPIT13_METER, 6 },
> + { 0, -1 },
> + };
> +
> + return version < OFP13_VERSION ? of11 : of13;
> +}
> +
> +/* Converts 'ovsinst_bitmap', a bitmap whose bits correspond to OVSINST_*
> + * values, into a bitmap of instructions suitable for OpenFlow 'version'
> + * (OFP11_VERSION or later), and returns the result. */
> +ovs_be32
> +ovsinst_bitmap_to_openflow(uint32_t ovsinst_bitmap, enum ofp_version version)
> +{
> + uint32_t ofpit_bitmap = 0;
> + const struct ovsinst_xlate *x;
> +
> + for (x = get_ovsinst_xlate(version); x->ofpit >= 0; x++) {
> + if (ovsinst_bitmap & (1u << x->ovsinst)) {
> + ofpit_bitmap |= 1u << x->ofpit;
> + }
> + }
> + return htonl(ofpit_bitmap);
> +}
> +
> +/* Converts 'ofpit_bitmap', a bitmap of instructions from an OpenFlow message
> + * with the given 'version' (OFP11_VERSION or later) into a bitmap whose bits
> + * correspond to OVSINST_* values, and returns the result. */
> +uint32_t
> +ovsinst_bitmap_from_openflow(ovs_be32 ofpit_bitmap, enum ofp_version version)
> +{
> + uint32_t ovsinst_bitmap = 0;
> + const struct ovsinst_xlate *x;
> +
> + for (x = get_ovsinst_xlate(version); x->ofpit >= 0; x++) {
> + if (ofpit_bitmap & htonl(1u << x->ofpit)) {
Maybe ntohl(ofpit_bitmap) once before the loop?
> + ovsinst_bitmap |= 1u << x->ovsinst;
> + }
> + }
> + return ovsinst_bitmap;
> +}
> +
> static inline struct ofp11_instruction *
> instruction_next(const struct ofp11_instruction *inst)
> {
> diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> index 5877151..c215ffc 100644
> --- a/lib/ofp-actions.h
> +++ b/lib/ofp-actions.h
> @@ -758,4 +758,8 @@ enum ovs_instruction_type
> ovs_instruction_type_from_ofpact_type(
> enum ofperr ovs_instruction_type_from_inst_type(
> enum ovs_instruction_type *instruction_type, const uint16_t inst_type);
>
> +ovs_be32 ovsinst_bitmap_to_openflow(uint32_t ovsinst_bitmap, enum
> ofp_version);
> +uint32_t ovsinst_bitmap_from_openflow(ovs_be32 ofpit_bitmap,
> + enum ofp_version);
> +
> #endif /* ofp-actions.h */
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 0ef4070..e643354 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -5083,7 +5083,8 @@ ofputil_put_ofp11_table_stats(const struct
> ofputil_table_stats *in,
> ovs_strlcpy(out->name, in->name, sizeof out->name);
> out->wildcards = fields_to_ofp11_flow_match_fields(&in->wildcards);
> out->match = fields_to_ofp11_flow_match_fields(&in->match);
> - out->instructions = htonl(in->instructions);
> + out->instructions = ovsinst_bitmap_to_openflow(in->ovsinsts,
> + OFP11_VERSION);
> out->write_actions = ofpact_bitmap_to_openflow(in->write_ofpacts,
> OFP11_VERSION);
> out->apply_actions = ofpact_bitmap_to_openflow(in->apply_ofpacts,
> @@ -5135,7 +5136,8 @@ ofputil_put_ofp12_table_stats(const struct
> ofputil_table_stats *in,
> OFP12_VERSION);
> out->metadata_match = in->metadata_match;
> out->metadata_write = in->metadata_write;
> - out->instructions = htonl(in->instructions & OFPIT11_ALL);
> + out->instructions = ovsinst_bitmap_to_openflow(in->ovsinsts,
> + OFP12_VERSION);
> out->config = htonl(in->config);
> out->max_entries = htonl(in->max_entries);
> out->active_count = htonl(in->active_count);
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index 38e7006..6c7559a 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -782,7 +782,7 @@ struct ofputil_table_stats {
> uint64_t apply_ofpacts; /* OFPACT_* supported on Apply-Actions. */
> struct mf_bitmap write_setfields; /* Fields that can be set in W-A. */
> struct mf_bitmap apply_setfields; /* Fields that can be set in A-A. */
> - uint32_t instructions; /* Bitmap of OFPIT_* values supported. */
> + uint32_t ovsinsts; /* Bitmap of OVSINST_* values supported. */
>
> uint32_t active_count; /* Number of active entries. */
> uint64_t lookup_count; /* Number of packets looked up in table. */
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 9c0c94e..ca17319 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -797,7 +797,7 @@ struct ofproto_class {
> *
> * - 'metadata_match' and 'metadata_write' to OVS_BE64_MAX.
> *
> - * - 'instructions' to all instructions.
> + * - 'ovsinsts' to all instructions.
> *
> * - 'config' to OFPTC11_TABLE_MISS_MASK.
> *
> @@ -815,8 +815,8 @@ struct ofproto_class {
> * - 'wildcards' to the set of wildcards actually supported by the table
> * (if it doesn't support all OpenFlow wildcards).
> *
> - * - 'instructions' to set the instructions actually supported by
> - * the table.
> + * - 'ovsinsts' to the set of instructions actually supported by the
> + * table.
> *
> * - 'write_actions' to set the write actions actually supported by
> * the table (if it doesn't support all OpenFlow actions).
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 78b6773..ac7cb13 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -3094,7 +3094,7 @@ handle_table_stats_request(struct ofconn *ofconn,
> stats[i].apply_setfields = rw_fields;
> stats[i].metadata_match = OVS_BE64_MAX;
> stats[i].metadata_write = OVS_BE64_MAX;
> - stats[i].instructions = OFPIT13_ALL;
> + stats[i].ovsinsts = (1u << N_OVS_INSTRUCTIONS) - 1;
> stats[i].config = OFPTC11_TABLE_MISS_MASK;
> stats[i].max_entries = 1000000; /* An arbitrary big number. */
> stats[i].active_count = classifier_count(&p->tables[i].cls);
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 06a7df4..5eef15b 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1067,7 +1067,7 @@ OVS_VSWITCHD_START
> (mid="wild=0x1ffffffffd, max=1000000,"
> tail="
> lookup=0, matched=0
> - match=0x1ffffffffd, instructions=0x00000007, config=0x00000003
> + match=0x1ffffffffd, instructions=0x0000003e, config=0x00000003
> write_actions=0x03ff8001, apply_actions=0x03ff8001
> write_setfields=0x0000000c0fe7fbdd
> apply_setfields=0x0000000c0fe7fbdd
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
