[ovs-dev] [PATCH 2/5] lib/ofp-actions: make ofpact_reg_load.value mf_value from uint64_t

[Simon Horman]

On Tue, Sep 04, 2012 at 02:12:49PM -0700, Ben Pfaff wrote:
> On Tue, Sep 04, 2012 at 10:39:19AM +0900, Simon Horman wrote:
> > On Thu, Aug 30, 2012 at 09:29:27PM -0700, Ben Pfaff wrote:
> > > On Thu, Aug 30, 2012 at 10:40:24AM +0900, Simon Horman wrote:
> > > > From: Isaku Yamahata <yamahata at valinux.co.jp>
> > > > 
> > > > set_field action needs more than 64 bits value unlike nx_reg_load action.
> > > > This is preparation for set_field action. mf_value will be used later.
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>
> > > > Signed-off-by: Simon Horman <horms at verge.net.au>
> > > 
> > > Presumably the goal is to make ofpact_reg_load able to set any subset of
> > > a field, not just a 64-bit subset.  For this, it would seem more natural
> > > to use union mf_subvalue instead of union mf_value.  Then
> > > nxm_execute_reg_load() would become a single call to
> > > mf_write_subfield(), rather than (as seen in patch 3/5) a conditional.
> > 
> > I spoke with Yamahata-san and clarified the motivation for using mf_value.
> > 
> > It was:
> > * To allow the storage of IPv6 addresses which require more than 64bits and;
> > * To allow use of portions of meta-flow.c
> > 
> > Having looked over the code I'm not convinced that it aims to set
> > field subsets. Do you believe that should be an aim?
> 
> Certainly NXAST_REG_LOAD can modify part of a field rather than having
> to set an entire field (which is what I mean by a "subset", in case it
> wasn't clear).  That's what mf_subvalue represents: some part of a
> field.  (Naturally, it can represent a whole field, too, since that's
> just a special case of "part of a field".)

Hi Ben,

I took a look at using union mf_subvalue instead of union mf_value
but the result wasn't pretty and I don't think it resolves the
problem that Yamahata-san was trying to address. That is having
sufficient space for ipv6 addresses or in other words entities
wider than 64bits.

Instead I would like to propose leaving this patch as is and rolling the
following incremental change to the subsequent patch which adds the
conditional to mf_write_subfield() ("lib/ofp-actions: helper functions for
of12 set-field action").

It adds mf_write_subfield_flow() which differs from mf_write_subfield()
in that:
* It acts on a struct flow rather than a struct rule.
* It acts on a union mf_value rather than a union mf_subfield as input.

It is constructed mostly of code moved from nxm_reg_load().
And it is used in nxm_execute_reg_load() to:
* Avoid the conditional you noticed and;
* Thus allow the possibility for OF set-field to work with sub-fields

index 77ee83c..a9b9e79 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -2156,6 +2156,21 @@ mf_format(const struct mf_field *mf,
     }
 }
 
+/* Makes subfield 'sf' within 'flow' exactly match the 'sf->n_bits'
+ * least-significant bits in 'x'.
+ */
+void
+mf_write_subfield_flow(const struct mf_subfield *sf,
+                       const union mf_value *x, struct flow *flow)
+{
+    const struct mf_field *field = sf->field;
+    union mf_value value;
+
+    mf_get_value(field, flow, &value);
+    bitwise_copy(x, sizeof *x, 0, &value, field->n_bytes, sf->ofs, sf->n_bits);
+    mf_set_flow_value(sf->field, &value, flow);
+}
+
 /* Makes subfield 'sf' within 'rule' exactly match the 'sf->n_bits'
  * least-significant bits in 'x'.
  */
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index c5e31af..9d9150c 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -320,6 +320,8 @@ void mf_set_wild(const struct mf_field *, struct cls_rule *);
 void mf_random_value(const struct mf_field *, union mf_value *value);
 
 /* Subfields. */
+void mf_write_subfield_flow(const struct mf_subfield *sf,
+                            const union mf_value *x, struct flow *flow);
 void mf_write_subfield(const struct mf_subfield *, const union mf_subvalue *,
                        struct cls_rule *);
 
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 10fa712..2113365 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1218,24 +1218,15 @@ nxm_execute_reg_move(const struct ofpact_reg_move *move,
 void
 nxm_execute_reg_load(const struct ofpact_reg_load *load, struct flow *flow)
 {
-    if (load->ofpact.compat == OFPUTIL_OFPAT12_SET_FIELD) {
-        mf_set_flow_value(load->dst.field, &load->value, flow);
-        return;
-    }
-    nxm_reg_load(&load->dst, ntohll(load->value.be64), flow);
+    mf_write_subfield_flow(&load->dst, &load->value, flow);
 }
 
 void
 nxm_reg_load(const struct mf_subfield *dst, uint64_t src_data,
              struct flow *flow)
 {
-    union mf_value dst_value;
     union mf_value src_value;
 
-    mf_get_value(dst->field, flow, &dst_value);
     src_value.be64 = htonll(src_data);
-    bitwise_copy(&src_value, sizeof src_value.be64, 0,
-                 &dst_value, dst->field->n_bytes, dst->ofs,
-                 dst->n_bits);
-    mf_set_flow_value(dst->field, &dst_value, flow);
+    mf_write_subfield_flow(dst, &src_value, flow);
 }