| Message ID | 1470672872-19450-11-git-send-email-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
"dev" <dev-bounces@openvswitch.org> wrote on 08/08/2016 11:14:21 AM: > From: Ben Pfaff <blp@ovn.org> > To: dev@openvswitch.org > Cc: Ben Pfaff <blp@ovn.org> > Date: 08/08/2016 11:16 AM > Subject: [ovs-dev] [PATCH v2 10/21] lflow: Correct register > definitions to use subfields for overlaps. > Sent by: "dev" <dev-bounces@openvswitch.org> > > OVN expressions need to know what fields overlap or alias one another. > This is supposed to be done via subfields: if two fields overlap, then the > smaller one should be defined as a subfield of the larger one. For > example, reg0 should be defined as xxreg0[96..127]. The symbol table in > lflow didn't do this, so it's possible for confusion to result. (I don't > have evidence of this actually happening, because it would only occur > in a case where the same bits of a field were referred to with different > names.) > > This commit fixes the problem. It deserves a test, but that's somewhat > difficult at this point, so it will actually happen in a future commit. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- Yes, we do really need a test for this - add it to the pile of debt... Acked-by: Ryan Moats <rmoats@us.ibm.com>
On Mon, Aug 08, 2016 at 12:12:53PM -0500, Ryan Moats wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 08/08/2016 11:14:21 AM: > > > From: Ben Pfaff <blp@ovn.org> > > To: dev@openvswitch.org > > Cc: Ben Pfaff <blp@ovn.org> > > Date: 08/08/2016 11:16 AM > > Subject: [ovs-dev] [PATCH v2 10/21] lflow: Correct register > > definitions to use subfields for overlaps. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > OVN expressions need to know what fields overlap or alias one another. > > This is supposed to be done via subfields: if two fields overlap, then > the > > smaller one should be defined as a subfield of the larger one. For > > example, reg0 should be defined as xxreg0[96..127]. The symbol table in > > lflow didn't do this, so it's possible for confusion to result. (I don't > > have evidence of this actually happening, because it would only occur > > in a case where the same bits of a field were referred to with different > > names.) > > > > This commit fixes the problem. It deserves a test, but that's somewhat > > difficult at this point, so it will actually happen in a future commit. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > Yes, we do really need a test for this - add it to the pile of debt... > > Acked-by: Ryan Moats <rmoats@us.ibm.com> Oh, the test is added one or two commits later, so it's only a very short-term debt.
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index 9eb92c8..71d42a7 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -52,6 +52,20 @@ lflow_reset_processing(void) physical_reset_processing(); } +static void +add_subregister(const char *name, + const char *parent_name, int parent_idx, + int width, int idx, + struct shash *symtab) +{ + int lsb = width * idx; + int msb = lsb + (width - 1); + char *expansion = xasprintf("%s%d[%d..%d]", + parent_name, parent_idx, lsb, msb); + expr_symtab_add_subfield(symtab, name, NULL, expansion); + free(expansion); +} + void lflow_init(void) { @@ -64,16 +78,42 @@ lflow_init(void) expr_symtab_add_string(&symtab, "inport", MFF_LOG_INPORT, NULL); expr_symtab_add_string(&symtab, "outport", MFF_LOG_OUTPORT, NULL); - /* Logical registers. */ + /* Logical registers: + * 128-bit xxregs + * 64-bit xregs + * 32-bit regs + * + * The expression language doesn't handle overlapping fields properly + * unless they're formally defined as subfields. It's a little awkward. */ + for (int xxi = 0; xxi < MFF_N_LOG_REGS / 4; xxi++) { + char *xxname = xasprintf("xxreg%d", xxi); + expr_symtab_add_field(&symtab, xxname, MFF_XXREG0 + xxi, NULL, false); + free(xxname); + } + for (int xi = 0; xi < MFF_N_LOG_REGS / 2; xi++) { + char *xname = xasprintf("xreg%d", xi); + int xxi = xi / 2; + if (xxi < MFF_N_LOG_REGS / 4) { + add_subregister(xname, "xxreg", xxi, 64, 1 - xi % 2, &symtab); + } else { + expr_symtab_add_field(&symtab, xname, MFF_XREG0 + xi, NULL, false); + } + free(xname); + } for (int i = 0; i < MFF_N_LOG_REGS; i++) { char *name = xasprintf("reg%d", i); - expr_symtab_add_field(&symtab, name, MFF_LOG_REG0 + i, NULL, false); + int xxi = i / 4; + int xi = i / 2; + if (xxi < MFF_N_LOG_REGS / 4) { + add_subregister(name, "xxreg", xxi, 32, 3 - i % 4, &symtab); + } else if (xi < MFF_N_LOG_REGS / 2) { + add_subregister(name, "xreg", xi, 32, 1 - i % 2, &symtab); + } else { + expr_symtab_add_field(&symtab, name, MFF_REG0 + i, NULL, false); + } free(name); } - expr_symtab_add_field(&symtab, "xxreg0", MFF_XXREG0, NULL, false); - expr_symtab_add_field(&symtab, "xxreg1", MFF_XXREG1, NULL, false); - /* Flags used in logical to physical transformation. */ expr_symtab_add_field(&symtab, "flags", MFF_LOG_FLAGS, NULL, false); char flags_str[16];
OVN expressions need to know what fields overlap or alias one another. This is supposed to be done via subfields: if two fields overlap, then the smaller one should be defined as a subfield of the larger one. For example, reg0 should be defined as xxreg0[96..127]. The symbol table in lflow didn't do this, so it's possible for confusion to result. (I don't have evidence of this actually happening, because it would only occur in a case where the same bits of a field were referred to with different names.) This commit fixes the problem. It deserves a test, but that's somewhat difficult at this point, so it will actually happen in a future commit. Signed-off-by: Ben Pfaff <blp@ovn.org> --- ovn/controller/lflow.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-)