review comments

Author: pbhide

examples/include/crypto-accelerator.p4

@@ -14,29 +14,28 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-/// Crypto accelerator Extern
-enum bit<8> crypto_algorithm_e {
-    AES_GCM = 1
-}
-enum bit<8> crypto_results_e {
-    SUCCESS = 0,
-    AUTH_FAILURE = 1,
-    HW_ERROR = 2
+/// Crypto accelerator Extern definition
+
+/// Crypto accelerator object is instantiated for each crypto algorithm
+enum crypto_algorithm_e {
+    AES_GCM
 }
 
-enum bit<8> crypto_error_action_e {
-    NO_ACTION = 0,
-    DROP_PACKET = 1,
-    SEND_TO_PORT = 2
+/// Results from crypto accelerator
+enum crypto_results_e {
+    SUCCESS,
+    AUTH_FAILURE,
+    HW_ERROR
 }
 
+/// special value to indicate that ICV is after the crypto payload
 #define ICV_AFTER_PAYLOAD ((int<32>)-1)
 
 /// The crypto_accelerator engine used in this example uses AES-GCM algorithm.
 /// It is assumed to be agnostic to wire protocols i.e. does not understand protocol 
 /// specific headers like ESP, AH etc
 ///
-/// Note that crypto accelerator does not modify the packet outside the payload area and ICV
+/// The crypto accelerator does not modify the packet outside the payload area and ICV
 ///     Any wire-protocol header, trailer add/remove is handled by P4 pipeline
 ///     The engine does not perform additional functions such as anti-replay protection, it
 ///     is done in P4 pipeline
@@ -56,12 +55,12 @@ enum bit<8> crypto_error_action_e {
 ///     Packet presented to the engine -
 ///     +------------------+--------------------------+-----------+
 ///     | Headers not to   | Encryption protocol      | payload   | 
-///     | Encrypted        | headers (E.g Esp, Esp-IV)|           |
+///     | be Encrypted     | headers (E.g Esp, Esp-IV)|           |
 ///     +------------------+-----------------------  -+-----------+
 ///     Packet after Encryption:
 ///     +------------------+--------------------------+-----------+-----------+
 ///     | Headers not to   | Encryption protocol      | Encrypted | ICV (opt) |
-///     | Encrypted        | headers (E.g Esp, Esp-IV)| Payload   |           |
+///     | be Encrypted     | headers (E.g Esp, Esp-IV)| Payload   |           |
 ///     +------------------+--------------------------+-----------+-----------+
 ///     ICV can be inserted either before or right after the encrypted payload 
 ///     as specified by icv_location/size
@@ -127,10 +126,6 @@ extern crypto_accelerator {
     // whichever method is called last overrides the previous calls
     void disable();
 
-    crypto_results_e get_results();       // get results of the previous operation
-
-    // set_error_action() indicates behavior on encoutering an error
-    // Default action is NO_ACTION i.e. report error via get_results()
-    // Certain actions may need action parameters, e.g send_to_port
-    void set_error_action<T>(crypto_error_action_e error_action, in T action_param);
+    // get results of the previous operation
+    crypto_results_e get_results();
 }

examples/ipsec-acc.p4

@@ -253,7 +253,7 @@ control ipsec_crypto( inout headers_t hdr,
                              bit<64> esn) {
 
         // IV(nonce) needed for AES-GCM algorithm
-        // if consists of iv that is sent on the wire plus an internally maintained salt value
+        // it consists of iv that is sent on the wire plus an internally maintained salt value
         bit<128> iv = (bit<128>)(salt ++ hdr.esp_iv.iv);
         ipsec_acc.set_iv(iv);
 
@@ -270,10 +270,9 @@ control ipsec_crypto( inout headers_t hdr,
         bit<16> aad_len = hdr.esp.minSizeInBytes();
 
         // Action parameter esn is expected to be stateful, i.e. the following operations
-        // are possible to update esn and check every packet against expected seq number
-        // to perform anti-replay checks.. this is not shown in this example
-        // esn = esn + 1;
-
+        // esn checking / anti-replay attack prevention is not shown in this example
+        // as it can be implemented in P4 pipeline and does not affect use of 
+        // the extern object shown in this example
         ipsec_acc.set_auth_data_offset(aad_offset);
         ipsec_acc.set_auth_data_len(aad_len);
 
@@ -300,7 +299,7 @@ control ipsec_crypto( inout headers_t hdr,
         hdr.recirc_header.ipsec_len = encr_pyld_len;
         hdr.recirc_header.setValid();
 
-        // TODO: recirc_packet() is hardware specific extern
+        // TODO: recirc_packet() is hardware specific extern, or it can be standardized
         recirc_packet();
     }
 
@@ -312,7 +311,9 @@ control ipsec_crypto( inout headers_t hdr,
                              bit<1> enable_auth,
                              bit<64> esn) {
 
-        // Initialize the ipsec accelerator
+        // update sequence number for each transmitted packet
+        // This can be done using stateful registers currently defined in P4
+        // it is not shown in this example
         // esn = esn + 1;
 
         // Set IV information needed for encryption
@@ -363,7 +364,7 @@ control ipsec_crypto( inout headers_t hdr,
 
         // instruct engine to add icv after encrypted payload
         ipsec_acc.set_icv_offset(ICV_AFTER_PAYLOAD);
-        ipsec_acc.set_icv_len((bit<32>)4); // Four bytes of ICV value.
+        ipsec_acc.set_icv_len(32w4); // Four bytes of ICV value.
 
         // run encryption w/ authentication
         ipsec_acc.enable_encrypt(enable_auth);
@@ -386,8 +387,10 @@ control ipsec_crypto( inout headers_t hdr,
             ipsec_esp_decrypt(spi, salt, key, key_size, ext_esn_en, auth_en, esn);
         }
     }
-    // setup crypto accelerator for decryption - get info from sa table
+    // setup crypto accelerator for encryption/decryption - get info from sa table
     // lookup sa_table using esp.spi
+    //  In this example same SA index is used for encrypt/decrypt, in real
+    // implementation it can be separated into two entries
     table ipsec_sa {
         key = {
             // For encrypt case get sa_idx from parser