[ovs-dev] [nxm 40/42] ofproto: Implement support for registers in extended flow match.

Justin Pettit jpettit at nicira.com
Thu Nov 11 01:26:18 UTC 2010


On Oct 28, 2010, at 10:28 AM, Ben Pfaff wrote:

> +/* Action structure for NXAST_REG_LOAD.
> ...
> + * 'dst' must be one of the following:
> + *
> + *   - NXM_NX_REG(idx) for idx in the switch's accepted range.

Is there a reason not to allow writing directly to NXM_OF_VLAN_TCI or NXM_NX_TUN_ID?  Writing directly to the tunnel id seems like it would be very useful, if for no other reason than testing.

> +/* Metadata registers.
> + *
> + * Registers initially have value 0.  Not-yet-defined actions will allow
> + * register values to be manipulated.

Weren't they just defined above?

> @@ -540,6 +588,12 @@ nx_put_match(struct ofpbuf *b, const struct cls_rule *cr)
>         nxm_put_64(b, NXM_NX_TUN_ID, htonll(ntohl(flow->tun_id)));
>     }
> 
> +    /* Registers. */
> +    for (i = 0; i < 3; i++) {

I think you meant to put FLOW_N_REGS here instead of 3.

> +        nxm_put_32m(b, NXM_NX_REG(i),
> +                    htonl(flow->regs[i]), htonl(cr->wc.reg_masks[i]));
> +    }

Do you think it would make sense to only record registers that have a non-zero mask?

> +int
> +nxm_check_reg_load(const struct nx_action_reg_load *action,
> +                   const struct flow *flow)
> +{
> ...
> +    if (NXM_IS_NX_REG(dst->header)) {
> +        return BAD_ARGUMENT;
> +    }

Isn't this check backwards, since you want to make sure that this destination is a register?

> +void
> +nxm_execute_reg_move(const struct nx_action_reg_move *action,
> +                     struct flow *flow)
> +{
> ...
> +    /* Get the interesting bits of the source field. */
> +    const struct nxm_field *src = nxm_field_lookup(ntohl(action->src));
> +    int src_ofs = ntohs(action->src_ofs);
> +    uint64_t src_data = nxm_read_field(src, flow) & (mask << src_ofs);
> +
> +    /* Get the remaining bits of the destination field. */
> +    int dst_ofs = ntohs(action->dst_ofs);
> +    const struct nxm_field *dst = nxm_field_lookup(ntohl(action->dst));
> +    uint64_t dst_data = nxm_read_field(dst, flow) & ~(mask << dst_ofs);

I realize this is a lame request, but do you mind putting "src" and "src_ofs" in the same order as the "dst" ones?  To my eye, it looks like there's something fundamentally different about them by having the order be different.

> +void
> +nxm_execute_reg_load(const struct nx_action_reg_load *action,
> +                     struct flow *flow)
> +{
> +    /* Preparation. */
> +    int n_bits = (ntohs(action->ofs_nbits) & 0x3f) + 1;
> +    uint32_t mask = n_bits == 32 ? UINT32_MAX : (UINT32_C(1) << n_bits) - 1;
> +    uint32_t *reg = &flow->regs[NXM_NX_REG_IDX(ntohl(action->dst))];
> +
> +    /* Get source data. */
> +    uint32_t src_data = ntohll(action->value);
> +
> +    /* Get remaining bits of the destination field. */
> +    int dst_ofs = ntohs(action->ofs_nbits) >> 6;
> +    uint32_t dst_data = *reg & ~(mask << dst_ofs);
> +
> +    *reg = dst_data | (src_data << dst_ofs);
> +}

Is there a reason to limit this implementation to 32 bits right now?  I realize that the only supported destinations are 32-bit registers, but the value is 64 bits, so I don't see a reason to limit the implementation now.  Regardless, I think loading an immediate into a 64-bit tunnel will be something we will want in the near future for testing, if nothing else.

Otherwise, it looks good.

--Justin






More information about the dev mailing list