sonic-net / DASH

Disaggregated APIs for SONiC Hosts
Apache License 2.0
77 stars 88 forks source link

Invalid match type: 'list' error when try to run bmv2 switch for sirius_pipeline project #110

Open madhudhavala opened 2 years ago

madhudhavala commented 2 years ago

1) I followed the same steps as mentioned in README in https://github.com/Azure/DASH/tree/main/sirius-pipeline

2) when i debugged i see below one is the main issue $ p4c -b bmv2 bmv2/sirius_pipeline.p4 -o bmv2/sirius_pipeline.bmv2 >>>> worked well $ simple_switch --log-console --interface 0@veth0 --interface 1@veth2 /bmv2/sirius_pipeline.bmv2/sirius_pipeline.json >>>> did't work well Calling target program-options parser Invalid match type: 'list'

3) Here iam attaching compiled program output sirius_pipeline.json as sirius_pipeline.txt becasue .json is not allowed to attach in this issue tracker. sirius_pipeline.txt

4) Could some one suggest possible solution or work around

KrisNey-MSFT commented 2 years ago

I would suggest sending to @marian-pritsak or tagging him to look at this request..?

madhudhavala commented 2 years ago

Thank you for looking into this. i see this issue is labelled with bug.

olha-omelianenko commented 2 years ago

Hi @madhudhavala, I've started to look through DASH and faced the same issue as you did.

According to my investigation, the root cause of the issue - is that there is an additional 'match_kind' type defined under 'sirius_acl.p4' script (which contains 'list' match type definition - about which the environment complains). https://github.com/Azure/DASH/blob/d0807c8a16fdad113f0dae27f4cd2fdfbf943639/sirius-pipeline/bmv2/sirius_acl.p4#L6-L14

From what I've read about the match kind types, they're defined by target architecture. P4 core and several other common architectures (such as PSA, PNA and v1model) contain neither 'list' nor 'range_list' match kinds (see some references below). If I understand correctly, we are trying to compile p4 script to json using 'bmv2' as the target device and 'v1model' as the target architecture. https://p4.org/p4-spec/docs/P4-16-v1.2.1.html#sec-match-kind-type https://p4.org/p4-spec/docs/PSA.html#sec-match-kinds https://p4.org/p4-spec/docs/PNA.html#sec-match-kinds https://github.com/p4lang/p4c/blob/e2ca9cc9daf538140b6f60a328df6bd37934723c/p4include/v1model.p4#L49L55

If I understand correctly, the community is currently working on the fix under the following issue: https://github.com/Azure/DASH/issues/91

As a temporary workaround, I've tried to exclude ACL-related logic from the code - and the 'simple_switch' container at least started after that. To achieve that, I've commented out all the references to 'sirius_acl.p4' functionality in the following places. https://github.com/Azure/DASH/blob/d0807c8a16fdad113f0dae27f4cd2fdfbf943639/sirius-pipeline/bmv2/sirius_inbound.p4#L7 https://github.com/Azure/DASH/blob/d0807c8a16fdad113f0dae27f4cd2fdfbf943639/sirius-pipeline/bmv2/sirius_inbound.p4#L60-L63 https://github.com/Azure/DASH/blob/d0807c8a16fdad113f0dae27f4cd2fdfbf943639/sirius-pipeline/bmv2/sirius_outbound.p4#L5 https://github.com/Azure/DASH/blob/d0807c8a16fdad113f0dae27f4cd2fdfbf943639/sirius-pipeline/bmv2/sirius_outbound.p4#L88-L91

marian-pritsak commented 2 years ago

Fixed with the latest commit Unsupported match types are excluded until they are not supported in simulator

madhudhavala commented 2 years ago

Thank you for the fix @marian-pritsak. Thank you @olha-omelianenko for details Thank you @KrisNey-MSFT for follow up

KrisNey-MSFT commented 2 years ago

.

KrisNey-MSFT commented 1 year ago

We are missing list-match-type support in the Compiler - request volunteer in Comm Meeting

KrisNey-MSFT commented 1 year ago

For now this is a workaround for now and ideally it should first be supported in compiler and then behaviour model.
Request a volunteer in the Community meeting for Compiler work.

yusefMS06 commented 1 year ago

@madhudhavala Does this work item still require an additional resource?

chrispsommers commented 4 months ago

This is a compiler and backend (bmv2) project of probably fairly large scope. Consensus in WG seems to be the solution is for the controller (SONiC dashorch) to expand list entries into the cross-product of individual entries.

ashutosh-agrawal commented 4 months ago

Also, in the P4 code for ACL [https://github.com/sonic-net/DASH/blob/main/dash-pipeline/bmv2/dash_acl.p4], we have replaced list-match entries with optional in case of bmv2 and ternary in case of p4dpdk. Since, neither of the simulators support list-match, should we change bmv2 to ternary as well? That will make sure that src/dst ip and L4 ports fields will be included in generated apis.

KrisNey-MSFT commented 3 months ago

We can fix this issue by taking the 1st item of the list to bmv2. It is done in DASH SAI, and the SAI API takes the list, and this will not fail any longer.
Closing

KrisNey-MSFT commented 3 months ago

Bandaid provided, closing

Replace optional in bmv2 with dash_acl.p4 @ashutosh-agrawal to provide name of key, show to @r12f

albertovillarreal-keys commented 2 months ago

Hello,

Using the example below, how can i modify this config to work in bmv2. The comment in this issue

{
  "name": "dash_acl_1000_rule_0",
  "op": "create",
  "type": "SAI_OBJECT_TYPE_DASH_ACL_RULE",
  "attributes": [
    "SAI_DASH_ACL_RULE_ATTR_DASH_ACL_GROUP_ID",
    "$in_acl_group_#1000",
    "SAI_DASH_ACL_RULE_ATTR_PRIORITY",
    "0",
    "SAI_DASH_ACL_RULE_ATTR_ACTION",
    "SAI_DASH_ACL_RULE_ACTION_PERMIT",
    "SAI_DASH_ACL_RULE_ATTR_SIP",
    "1.4.0.1/32",
    "SAI_DASH_ACL_RULE_ATTR_DIP",
    "1.1.0.1/32"
  ]
}

Currently if i try to configure bmv2 with this kind of definition i get this message when try to create and apply config. When we create the dpu config with one prefix we get the following error:

E NotImplementedError: ipprefixlist, 1.4.0.1/32

KrisNey-MSFT commented 1 month ago

Prefix list was supposed to be implemented as a single; this value s/be consumed. Perhaps it did not become implemented. @r12f and @marian-pritsak - Kristina to check

KrisNey-MSFT commented 1 month ago

It looks like we can try to match against it (dash_acl.p4), however it is not defined/implemented by bmv2 or PNA. We could manually implement a libsai, rather than try to generate it, for the ACL APIs.

KrisNey-MSFT commented 1 month ago

ACL API design is not P4 friendly. Requires quite a lot of work to execute. Not use option match-kind. Since this is a prefix-list, we need to break it down. How can we establish mappings if we want to update? Underneath, we need to save old list, do a diff, perform update. Would passing a single prefix work? P4 supports only 1 maximum in the MAT.
Support only 1 list with 1 element (for bmv2) and apply it? If there are 2, we error out?