ebpf/PSA: Support for wide fields in Register and Meter #3854
Conversation
This commit allows to use Registers and Meters with fields wider than 64 bits.
|
We could promote some of you to committers if you can review each other's code. |
| unsigned EBPFInitializerUtils::ebpfTypeWidth(P4::TypeMap *typeMap, const IR::Expression *expr) { | ||
| auto type = typeMap->getType(expr); | ||
| if (type == nullptr) type = expr->type; | ||
| if (type->is<IR::Type_InfInt>()) return 32; // let's assume 32 bit for int type |
There was a problem hiding this comment.
the only place where this can happen as far I know is in rhs of shifts.
There was a problem hiding this comment.
I can't recall where exactly hit the problem with this type.
For now I am able to make this statement true with slice, like field[127:64] (which is translated using bit shifts).
backends/ebpf/ebpfParser.cpp
Outdated
| // 0x112233445566778890 | ||
| // To correctly insert that padding, the length of field must be known, but tools like | ||
| // nikss-ctl (and the nikss library) don't consume P4info.txt to have such knowledge. | ||
| // There is also a bug in (de)parser causing that such fields aren't deparsed correctly. |
There was a problem hiding this comment.
causing such fields to be deparsed incorrectly.
backends/ebpf/ebpfParser.cpp
Outdated
| // nikss-ctl (and the nikss library) don't consume P4info.txt to have such knowledge. | ||
| // There is also a bug in (de)parser causing that such fields aren't deparsed correctly. | ||
| ::error(ErrorType::ERR_UNSUPPORTED_ON_TARGET, | ||
| "%1%: fields wider than 64 bits must have size in multiply of 8 bits (1 byte) " |
There was a problem hiding this comment.
a size multiple of 8 bits
| @@ -293,6 +295,23 @@ void StateTranslationVisitor::compileExtractField(const IR::Expression *expr, cs | |||
| builder->append(")"); | |||
| builder->endOfStatement(true); | |||
| } else { | |||
| if (program->options.arch == "psa" && widthToExtract % 8 != 0) { | |||
There was a problem hiding this comment.
yes, in my implementation I never figured out how all this stuff should be handled.
backends/ebpf/psa/README.md
Outdated
| @@ -619,7 +619,8 @@ table caching pass `--table-caching` to the compiler. | |||
|
|
|||
| We list the known bugs/limitations below. Refer to the Roadmap section for features planned in the near future. | |||
|
|
|||
| - Larger bit fields (e.g. IPv6 addresses) may not work properly. | |||
| - Fields wider than 64 bits must have size multiply of 8 bits, otherwise they may have unexpected value in the LSB byte. | |||
| initialValue != nullptr ? initialValue : new IR::Constant(leftExpression->type, 0); | ||
| const auto *assigment = new IR::AssignmentStatement(leftExpression, right); | ||
| ControlBodyTranslatorPSA cg(*translator); | ||
| assigment->apply(cg); |
There was a problem hiding this comment.
does this work?
For example, the typemap won't always have a type for the right.
In general, it would be nice to do this in a separate pass before code generation.
There was a problem hiding this comment.
I think it should works (PTF tests are passing, especially register.RegisterDefaultPSATest).
If the initial value is defined for register, then type map contains type for right - because of expression generated by frontend.
If the initial value is not defined, type map does not contain type for right, but type is copied from left. For this case exist this line:
Line 449 in 7554cb9
Maybe it is not the best way, but is there any reason to not to use expression->type?
There was a problem hiding this comment.
Expression->type is only populated if you run the type inference with a flag indicating that you want this.
Some simple expressions like constants may have the type correct, though.
If it works, let's leave it like this.
|
Approved, but you may want to make a few changes. |
hey @mbudiu-vmw, I think it'd make sense to promote me and @tatry, so that we can review the code - especially related to eBPF backend. |
|
@jnfoster I have tried to add them to the list of committers, but I don't seem to be able to. |
This commit allows to use Registers and Meters with fields wider than 64 bits.
CC: @osinstom