Some questions for clarification below, Jarno
Acked-by: Jarno Rajahalme <[email protected]> On Jul 25, 2014, at 10:25 PM, Ben Pfaff <[email protected]> wrote: > These 64-bit registers are intended to conform with the OpenFlow 1.5 > draft specification. > > EXT-244. > Signed-off-by: Ben Pfaff <[email protected]> > --- > include/openflow/openflow-1.2.h | 5 ++++- > lib/flow.c | 8 +++++++ > lib/flow.h | 22 +++++++++++++++++++ > lib/match.c | 17 ++++++++++++++- > lib/match.h | 5 ++++- > lib/meta-flow.c | 47 +++++++++++++++++++++++++++++++++++++++++ > lib/meta-flow.h | 20 +++++++++++++++++- > utilities/ovs-ofctl.8.in | 17 ++++++++++++++- > 8 files changed, 136 insertions(+), 5 deletions(-) > > diff --git a/include/openflow/openflow-1.2.h b/include/openflow/openflow-1.2.h > index d25f2dc..903847b 100644 > --- a/include/openflow/openflow-1.2.h > +++ b/include/openflow/openflow-1.2.h > @@ -68,6 +68,7 @@ enum ofp12_oxm_class { > OFPXMC12_NXM_0 = 0x0000, /* Backward compatibility with NXM */ > OFPXMC12_NXM_1 = 0x0001, /* Backward compatibility with NXM */ > OFPXMC12_OPENFLOW_BASIC = 0x8000, /* Basic class for OpenFlow */ > + OFPXMC12_PACKET_REGS = 0x8001, /* Packet registers (pipeline fields). > */ > OFPXMC12_EXPERIMENTER = 0xffff, /* Experimenter class */ > }; > > @@ -195,7 +196,9 @@ enum oxm12_ofb_match_fields { > #define OXM_OF_IPV6_EXTHDR_W OXM_HEADER_W (OFPXMT13_OFB_IPV6_EXTHDR, 2) > #define OXM_OF_PBB_UCA OXM_HEADER (OFPXMT14_OFB_PBB_UCA, 1) > #define OXM_OF_TCP_FLAGS OXM_HEADER (OFPXMT15_OFB_TCP_FLAGS, 2) > -#define OXM_OF_TCP_FLAGS_W OXM_HEADER_W (OFPXMT15_OFB_TCP_FLAGS, 2) This seems an unrelated change. > + > +#define OXM_OF_PKT_REG(N) (NXM_HEADER (OFPXMC12_PACKET_REGS, N, 8)) > +#define OXM_OF_PKT_REG_W(N) (NXM_HEADER_W(OFPXMC12_PACKET_REGS, N, 8)) > > /* The VLAN id is 12-bits, so we can use the entire 16 bits to indicate > * special conditions. > diff --git a/lib/flow.c b/lib/flow.c > index 330a458..5e04015 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -898,6 +898,14 @@ flow_wildcards_set_reg_mask(struct flow_wildcards *wc, > int idx, uint32_t mask) > wc->masks.regs[idx] = mask; > } > > +/* Sets the wildcard mask for register 'idx' in 'wc' to 'mask'. > + * (A 0-bit indicates a wildcard bit.) */ > +void > +flow_wildcards_set_xreg_mask(struct flow_wildcards *wc, int idx, uint64_t > mask) > +{ > + flow_set_xreg(&wc->masks, idx, mask); > +} > + > /* Calculates the 5-tuple hash from the given miniflow. > * This returns the same value as flow_hash_5tuple for the corresponding > * flow. */ > diff --git a/lib/flow.h b/lib/flow.h > index 1bce46c..3b8d24d 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -40,9 +40,16 @@ struct pkt_metadata; > * failures in places which likely need to be updated. */ > #define FLOW_WC_SEQ 27 > > +/* Number of Open vSwitch extension 32-bit registers. */ > #define FLOW_N_REGS 8 > BUILD_ASSERT_DECL(FLOW_N_REGS <= NXM_NX_MAX_REGS); > > +/* Number of OpenFlow 1.5+ 64-bit registers. > + * > + * Each of these overlays a pair of Open vSwitch 32-bit registers, so there > + * are half as many of them.*/ > +#define FLOW_N_XREGS (FLOW_N_REGS / 2) > + > /* Used for struct flow's dl_type member for frames that have no Ethernet > * type, that is, pure 802.2 frames. */ > #define FLOW_DL_TYPE_NONE 0x5ff > @@ -208,6 +215,19 @@ void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 > lse); > > void flow_compose(struct ofpbuf *, const struct flow *); > > +static inline uint64_t > +flow_get_xreg(const struct flow *flow, int idx) > +{ > + return ((uint64_t) flow->regs[idx * 2] << 32) | flow->regs[idx * 2 + 1]; > +} > + > +static inline void > +flow_set_xreg(struct flow *flow, int idx, uint64_t value) > +{ > + flow->regs[idx * 2] = value >> 32; > + flow->regs[idx * 2 + 1] = value; > +} > + Did you consider modifying the struct flow instead, e.g.: struct flow { /* L1 */ struct flow_tnl tunnel; /* Encapsulating tunnel parameters. */ ovs_be64 metadata; /* OpenFlow Metadata. */ union { uint32_t regs[FLOW_N_REGS]; /* Registers. */ uint64_t xregs[FLOW_N_REGS / 2]; /* 64-bit registers overlaid with 32-bit registers. */ }; uint32_t skb_priority; /* Packet priority for QoS. */ > static inline int > flow_compare_3way(const struct flow *a, const struct flow *b) > { > @@ -291,6 +311,8 @@ bool flow_wildcards_is_catchall(const struct > flow_wildcards *); > > void flow_wildcards_set_reg_mask(struct flow_wildcards *, > int idx, uint32_t mask); > +void flow_wildcards_set_xreg_mask(struct flow_wildcards *, > + int idx, uint64_t mask); > > void flow_wildcards_and(struct flow_wildcards *dst, > const struct flow_wildcards *src1, > diff --git a/lib/match.c b/lib/match.c > index 58fa0e4..6161556 100644 > --- a/lib/match.c > +++ b/lib/match.c > @@ -201,6 +201,21 @@ match_set_reg_masked(struct match *match, unsigned int > reg_idx, > } > > void > +match_set_xreg(struct match *match, unsigned int xreg_idx, uint64_t value) > +{ > + match_set_xreg_masked(match, xreg_idx, value, UINT64_MAX); > +} > + > +void > +match_set_xreg_masked(struct match *match, unsigned int xreg_idx, > + uint64_t value, uint64_t mask) > +{ > + ovs_assert(xreg_idx < FLOW_N_XREGS); > + flow_wildcards_set_xreg_mask(&match->wc, xreg_idx, mask); > + flow_set_xreg(&match->flow, xreg_idx, value & mask); > +} > + > +void > match_set_metadata(struct match *match, ovs_be64 metadata) > { > match_set_metadata_masked(match, metadata, OVS_BE64_MAX); > @@ -1020,7 +1035,7 @@ match_format(const struct match *match, struct ds *s, > unsigned int priority) > } > for (i = 0; i < FLOW_N_REGS; i++) { > #define REGNAME_LEN 20 > - char regname[REGNAME_LEN]; > + char regname[20]; Why this change? > if (snprintf(regname, REGNAME_LEN, "reg%d", i) >= REGNAME_LEN) { > strcpy(regname, "reg?"); > } > diff --git a/lib/match.h b/lib/match.h > index 1246b20..ce9fb28 100644 > --- a/lib/match.h > +++ b/lib/match.h > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -50,6 +50,9 @@ void match_set_recirc_id_masked(struct match *, uint32_t > value, uint32_t mask); > void match_set_reg(struct match *, unsigned int reg_idx, uint32_t value); > void match_set_reg_masked(struct match *, unsigned int reg_idx, > uint32_t value, uint32_t mask); > +void match_set_xreg(struct match *, unsigned int xreg_idx, uint64_t value); > +void match_set_xreg_masked(struct match *, unsigned int xreg_idx, > + uint64_t value, uint64_t mask); > void match_set_metadata(struct match *, ovs_be64 metadata); > void match_set_metadata_masked(struct match *, > ovs_be64 metadata, ovs_be64 mask); > diff --git a/lib/meta-flow.c b/lib/meta-flow.c > index e980a2a..b76c11d 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -236,6 +236,29 @@ const struct mf_field mf_fields[MFF_N_IDS] = { > #error "Need to update mf_fields[] to match FLOW_N_REGS" > #endif > > +#define XREGISTER(IDX) \ > + { \ > + MFF_XREG##IDX, "xreg" #IDX, NULL, \ > + MF_FIELD_SIZES(be64), \ > + MFM_FULLY, \ > + MFS_HEXADECIMAL, \ > + MFP_NONE, \ > + true, \ > + OXM_OF_PKT_REG(IDX), "OXM_OF_PKT_REG" #IDX, \ > + OXM_OF_PKT_REG(IDX), "OXM_OF_PKT_REG" #IDX, OFP15_VERSION, \ > + OFPUTIL_P_NXM_OXM_ANY, \ > + OFPUTIL_P_NXM_OXM_ANY, \ > + -1, \ > + } > +#if FLOW_N_XREGS == 4 > + XREGISTER(0), > + XREGISTER(1), > + XREGISTER(2), > + XREGISTER(3), > +#else > +#error "Need to update mf_fields[] to match FLOW_N_XREGS" > +#endif > + > /* ## -- ## */ > /* ## L2 ## */ > /* ## -- ## */ > @@ -922,6 +945,8 @@ mf_is_all_wild(const struct mf_field *mf, const struct > flow_wildcards *wc) > return !wc->masks.pkt_mark; > CASE_MFF_REGS: > return !wc->masks.regs[mf->id - MFF_REG0]; > + CASE_MFF_XREGS: > + return !flow_get_xreg(&wc->masks, mf->id - MFF_XREG0); > > case MFF_ETH_SRC: > return eth_addr_is_zero(wc->masks.dl_src); > @@ -1160,6 +1185,7 @@ mf_is_value_valid(const struct mf_field *mf, const > union mf_value *value) > case MFF_SKB_PRIORITY: > case MFF_PKT_MARK: > CASE_MFF_REGS: > + CASE_MFF_XREGS: > case MFF_ETH_SRC: > case MFF_ETH_DST: > case MFF_ETH_TYPE: > @@ -1290,6 +1316,10 @@ mf_get_value(const struct mf_field *mf, const struct > flow *flow, > value->be32 = htonl(flow->regs[mf->id - MFF_REG0]); > break; > > + CASE_MFF_XREGS: > + value->be64 = htonll(flow_get_xreg(flow, mf->id - MFF_XREG0)); > + break; > + > case MFF_ETH_SRC: > memcpy(value->mac, flow->dl_src, ETH_ADDR_LEN); > break; > @@ -1492,6 +1522,10 @@ mf_set_value(const struct mf_field *mf, > match_set_reg(match, mf->id - MFF_REG0, ntohl(value->be32)); > break; > > + CASE_MFF_XREGS: > + match_set_xreg(match, mf->id - MFF_XREG0, ntohll(value->be64)); > + break; > + > case MFF_ETH_SRC: > match_set_dl_src(match, value->mac); > break; > @@ -1711,6 +1745,10 @@ mf_set_flow_value(const struct mf_field *mf, > flow->regs[mf->id - MFF_REG0] = ntohl(value->be32); > break; > > + CASE_MFF_XREGS: > + flow_set_xreg(flow, mf->id - MFF_XREG0, ntohll(value->be64)); > + break; > + > case MFF_ETH_SRC: > memcpy(flow->dl_src, value->mac, ETH_ADDR_LEN); > break; > @@ -1928,6 +1966,10 @@ mf_set_wild(const struct mf_field *mf, struct match > *match) > match_set_reg_masked(match, mf->id - MFF_REG0, 0, 0); > break; > > + CASE_MFF_XREGS: > + match_set_xreg_masked(match, mf->id - MFF_XREG0, 0, 0); > + break; > + > case MFF_ETH_SRC: > memset(match->flow.dl_src, 0, ETH_ADDR_LEN); > memset(match->wc.masks.dl_src, 0, ETH_ADDR_LEN); > @@ -2151,6 +2193,11 @@ mf_set(const struct mf_field *mf, > ntohl(value->be32), ntohl(mask->be32)); > break; > > + CASE_MFF_XREGS: > + match_set_xreg_masked(match, mf->id - MFF_XREG0, > + ntohll(value->be64), ntohll(mask->be64)); > + break; > + > case MFF_PKT_MARK: > match_set_pkt_mark_masked(match, ntohl(value->be32), > ntohl(mask->be32)); > diff --git a/lib/meta-flow.h b/lib/meta-flow.h > index f65ed20..7c0c43b 100644 > --- a/lib/meta-flow.h > +++ b/lib/meta-flow.h > @@ -60,6 +60,15 @@ enum OVS_PACKED_ENUM mf_field_id { > #error "Need to update MFF_REG* to match FLOW_N_REGS" > #endif > > +#if FLOW_N_XREGS == 4 > + MFF_XREG0, /* be32 */ > + MFF_XREG1, /* be32 */ > + MFF_XREG2, /* be32 */ > + MFF_XREG3, /* be32 */ > +#else > +#error "Need to update MFF_REG* to match FLOW_N_XREGS" > +#endif > + > /* L2. */ > MFF_ETH_SRC, /* mac */ > MFF_ETH_DST, /* mac */ > @@ -141,7 +150,7 @@ struct mf_bitmap { > #define MF_BITMAP_INITIALIZER { { [0] = 0 } } > > /* Use this macro as CASE_MFF_REGS: in a switch statement to choose all of the > - * MFF_REGx cases. */ > + * MFF_REGn cases. */ > #if FLOW_N_REGS == 8 > #define CASE_MFF_REGS \ > case MFF_REG0: case MFF_REG1: case MFF_REG2: case MFF_REG3: \ > @@ -150,6 +159,15 @@ struct mf_bitmap { > #error "Need to update CASE_MFF_REGS to match FLOW_N_REGS" > #endif > > +/* Use this macro as CASE_MFF_XREGS: in a switch statement to choose all of > the > + * MFF_REGn cases. */ > +#if FLOW_N_XREGS == 4 > +#define CASE_MFF_XREGS \ > + case MFF_XREG0: case MFF_XREG1: case MFF_XREG2: case MFF_XREG3 > +#else > +#error "Need to update CASE_MFF_XREGS to match FLOW_N_XREGS" > +#endif > + > /* Prerequisites for matching a field. > * > * A field may only be matched if the correct lower-level protocols are also > diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in > index 63f4fee..6198186 100644 > --- a/utilities/ovs-ofctl.8.in > +++ b/utilities/ovs-ofctl.8.in > @@ -1084,7 +1084,22 @@ indicates that the corresponding bit in \fIvalue\fR > must match > exactly, and a 0-bit wildcards that bit. > .IP > When a packet enters an OpenFlow switch, all of the registers are set > -to 0. Only explicit Nicira extension actions change register values. > +to 0. Only explicit actions change register values. > +. > +.IP "\fBregx\fIidx\fB=\fIvalue\fR[\fB/\fImask\fR]” “regx” -> “xreg” ? > +Matches \fIvalue\fR either exactly or with optional \fImask\fR in > +64-bit ``extended register'' number \fIidx\fR. Each of the 64-bit > +extended registers overlays two of the 32-bit registers: \fBxreg0\fR > +overlays \fBreg0\fR and \fBreg1\fR, with \fBreg0\fR supplying the > +most-significant bits of \fBxreg0\fR and \fBreg1\fR the > +least-significant. f\fBxreg1\fR similarly overlays \fBreg2\fR and > +\fBreg3\fR, and so on. > +.IP > +These fields were added in Open vSwitch 2.3 to conform with the > +OpenFlow 1.5 (draft) specification. OpenFlow 1.5 calls these fields > +just the ``packet registers,'' but Open vSwitch already had 32-bit > +registers by that name, which is why Open vSwitch refers to the > +standard registers as ``extended registers''. > . > .IP \fBpkt_mark=\fIvalue\fR[\fB/\fImask\fR] > Matches packet metadata mark \fIvalue\fR either exactly or with optional > -- > 1.9.1 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
