-
Notifications
You must be signed in to change notification settings - Fork 444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dpdk Backend: Add IPSec support #3858
Conversation
2) Error if ipsec_accelerator is used with PSA Arch 3) Fix Logical Not handling
newHeaderName); | ||
return program; | ||
} | ||
ipsecHeader = new IR::Type_Header(IR::ID(newHeaderName), newHeaderFields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this header is always to going to present for all dpdk programs, perhaps it should be part of dpdk/pna.p4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be only added if ipsec_accelerators enable method is called.
ipsec_status status; | ||
|
||
state start { | ||
from_ipsec = ipsec.from_ipsec(status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show how 'status' might be used in the parser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be tested in "if" or "verify" statements.
The from_ipsec method can well be called outside the parser and the status can tested in if.
// containing headerinstances for all headers must be present | ||
BUG_CHECK(structure->header_type != "", | ||
"Encapsulating struct containing all headers used in the program is missing"); | ||
for (auto obj : program->objects) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use the visitor methods for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visitors for Type_Struct and P4Parser? This is for adding the new header and register declarations to the program. Only for placing these new declarations better in the program, I have added check for Type_Struct and Parser.
backends/dpdk/dpdkArch.cpp
Outdated
auto portOutInBound = new IR::AssignmentStatement( | ||
c->body->srcInfo, | ||
new IR::Member(new IR::PathExpression(IR::ID("m")), | ||
IR::ID("pna_main_output_metadata_output_port")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one of these two assignments must be wrong, since they write to the same field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assignments are placed in true and false blocks of "if-else"
corresponds to
if (direction == NET_TO_HOST)
m.pna_main_output_metadata_output_port = ipsec_inbound_port_out
else
m.pna_main_output_metadata_output_port = ipsec_outbound_port_out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be clearer to factor out the common subexpression. But it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comments and refactored common subexpression.
return stmt; | ||
}; | ||
|
||
for (auto kv : structure->deparsers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be also done in a visitor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we need to insert this set of statements (from lambda function) in deparser. We have the information about which control blocks is/are deparser(s) stored in the "structure". This information is collected in the initial passes while inspecting the program as per specified architecture. Hence, I am checking that while visiting P4Control. I am not able to think of any other way to achieve this here. Please suggest if I am missing something.
backends/dpdk/dpdkArch.h
Outdated
passes.push_back(new P4::ClearTypeMap(typeMap)); | ||
passes.push_back(new P4::ResolveReferences(refMap)); | ||
passes.push_back(new P4::TypeInference(refMap, typeMap, false)); | ||
passes.push_back(new P4::TypeChecking(refMap, typeMap)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually prefer to call typechecking before a pass if needed.
This way a pass will build whatever data structures it needs.
Here you assume that the next pass needs a fresh typemap.
This is perhaps more reasonable in a backend - but probably the backend should document the fact that every pass expects a correct typemap.
I wonder whether this can mess up with the structure you have built earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed TypeChecking from here as it is done later when needed. RefMap and TypeMap needs to be updated as we are adding new declarations and using those, hence rest of the passes are still needed.
Thanks for the review @hanw @mbudiu-vmw |
I have already approved |
This extern is to be included in dpdk specific pna.p4
This a pseudo header inserted by the compiler with one field for security association index. This header needs to be instantiated in the main encapsulating header struct which is passed as argument to MainParser and MainControl
Insert the pseudo header only if ipsec_accelerator is instantiated and enable is called.
3. IPSec needs 4 location registers. Size of these registers is 1 and these are set by the control plane.
Method “set_sa_index”: sets the ipsec_hdr.sa_id field.
Method “enable”:
a. Needs to validate the ipsec_instrisic header: ipsec_hdr.setValid()
b. Needs to set the output port to the IPsec output port. This is done in the beginning of deparser, so that if there are multiple calls to enable and disable, the setting of output port for ipsec is done just once.
a. Needs to invalidate the ipsec_hdr: ipsec_hdr.setInvalid()
a. Needs to test if the input port is equal to the IPsec accelerator port
b. Currently, for P4-DPDK, the status is always set to “success”.