diff --git a/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java b/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java index fd3b504f57..828d623e44 100644 --- a/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java +++ b/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java @@ -16,9 +16,8 @@ package org.onosproject.p4tutorial.pipeconf; -import com.google.common.collect.BiMap; -import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import org.onlab.packet.DeserializationException; import org.onlab.packet.Ethernet; @@ -50,6 +49,7 @@ import org.onosproject.net.pi.runtime.PiPacketOperation; import java.nio.ByteBuffer; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Optional; import static java.lang.String.format; @@ -98,13 +98,13 @@ public final class PipelineInterpreterImpl private static final PiActionParamId ACT_PARAM_ID_PORT = PiActionParamId.of("port"); - private static final BiMap TABLE_MAP = - new ImmutableBiMap.Builder() + private static final Map TABLE_MAP = + new ImmutableMap.Builder() .put(0, TABLE_L2_FWD_ID) .build(); - private static final BiMap CRITERION_MAP = - new ImmutableBiMap.Builder() + private static final Map CRITERION_MAP = + ImmutableMap.builder() .put(Criterion.Type.IN_PORT, INGRESS_PORT_ID) .put(Criterion.Type.ETH_DST, ETH_DST_ID) .put(Criterion.Type.ETH_SRC, ETH_SRC_ID) @@ -116,21 +116,11 @@ public final class PipelineInterpreterImpl return Optional.ofNullable(CRITERION_MAP.get(type)); } - @Override - public Optional mapPiMatchFieldId(PiMatchFieldId headerFieldId) { - return Optional.ofNullable(CRITERION_MAP.inverse().get(headerFieldId)); - } - @Override public Optional mapFlowRuleTableId(int flowRuleTableId) { return Optional.ofNullable(TABLE_MAP.get(flowRuleTableId)); } - @Override - public Optional mapPiTableId(PiTableId piTableId) { - return Optional.ofNullable(TABLE_MAP.inverse().get(piTableId)); - } - @Override public PiAction mapTreatment(TrafficTreatment treatment, PiTableId piTableId) throws PiInterpreterException { diff --git a/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java b/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java index 1d1301e4fa..1ba85e0335 100644 --- a/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java +++ b/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java @@ -46,16 +46,6 @@ public interface PiPipelineInterpreter extends HandlerBehaviour { */ Optional mapCriterionType(Criterion.Type type); - /** - * Returns the criterion type that is equivalent to the given PI match field - * ID, if present. If not present, it means that the given match field is - * not supported by this interpreter. - * - * @param fieldId match field ID - * @return optional criterion type - */ - Optional mapPiMatchFieldId(PiMatchFieldId fieldId); - /** * Returns a PI table ID equivalent to the given numeric table ID (as in * {@link org.onosproject.net.flow.FlowRule#tableId()}). If not present, it @@ -71,18 +61,6 @@ public interface PiPipelineInterpreter extends HandlerBehaviour { // specific PiTableId even when mapping to a single table. Optional mapFlowRuleTableId(int flowRuleTableId); - /** - * Returns an integer table ID equivalent to the given PI table ID. If not - * present, it means that the given PI table ID cannot be mapped to any - * integer table ID, because such mapping would be meaningless or because - * such PI table ID is not defined by the pipeline model. - * - * @param piTableId PI table ID - * @return numeric table ID - */ - // FIXME: as above - Optional mapPiTableId(PiTableId piTableId); - /** * Returns an action of a PI pipeline that is functionally equivalent to the * given traffic treatment for the given table. diff --git a/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java b/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java index e910116a07..561d62a80b 100644 --- a/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java +++ b/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java @@ -54,7 +54,6 @@ import org.slf4j.LoggerFactory; import java.util.Collection; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.StringJoiner; @@ -80,7 +79,8 @@ final class PiFlowRuleTranslatorImpl { } /** - * Returns a PI table entry equivalent to the given flow rule, for the given pipeconf and device. + * Returns a PI table entry equivalent to the given flow rule, for the given + * pipeconf and device. * * @param rule flow rule * @param pipeconf pipeconf @@ -148,7 +148,7 @@ final class PiFlowRuleTranslatorImpl { tableEntryBuilder.withTimeout((double) rule.timeout()); } else { log.debug("Flow rule is temporary, but table '{}' doesn't support " + - "aging, translating to permanent.", tableModel.id()); + "aging, translating to permanent.", tableModel.id()); } } @@ -158,16 +158,19 @@ final class PiFlowRuleTranslatorImpl { /** - * Returns a PI action equivalent to the given treatment, optionally using the given interpreter. This method also - * checks that the produced PI table action is suitable for the given table ID and pipeline model. If suitable, the - * returned action instance will have parameters well-sized, according to the table model. + * Returns a PI action equivalent to the given treatment, optionally using + * the given interpreter. This method also checks that the produced PI table + * action is suitable for the given table ID and pipeline model. If + * suitable, the returned action instance will have parameters well-sized, + * according to the table model. * * @param treatment traffic treatment * @param interpreter interpreter * @param tableId PI table ID * @param pipelineModel pipeline model * @return PI table action - * @throws PiTranslationException if the treatment cannot be translated or if the PI action is not suitable for the + * @throws PiTranslationException if the treatment cannot be translated or + * if the PI action is not suitable for the * given pipeline model */ static PiTableAction translateTreatment(TrafficTreatment treatment, PiPipelineInterpreter interpreter, @@ -185,7 +188,8 @@ final class PiFlowRuleTranslatorImpl { } /** - * Builds a PI action out of the given treatment, optionally using the given interpreter. + * Builds a PI action out of the given treatment, optionally using the given + * interpreter. */ private static PiTableAction buildAction(TrafficTreatment treatment, PiPipelineInterpreter interpreter, PiTableId tableId) @@ -285,8 +289,9 @@ final class PiFlowRuleTranslatorImpl { } /** - * Builds a collection of PI field matches out of the given selector, optionally using the given interpreter. The - * field matches returned are guaranteed to be compatible for the given table model. + * Builds a collection of PI field matches out of the given selector, + * optionally using the given interpreter. The field matches returned are + * guaranteed to be compatible for the given table model. */ private static Collection translateFieldMatches(PiPipelineInterpreter interpreter, TrafficSelector selector, PiTableModel tableModel) @@ -312,18 +317,41 @@ final class PiFlowRuleTranslatorImpl { Set usedPiCriterionFields = Sets.newHashSet(); Set ignoredPiCriterionFields = Sets.newHashSet(); + + Map criterionMap = Maps.newHashMap(); + if (interpreter != null) { + // NOTE: if two criterion types map to the same match field ID, and + // those two criterion types are present in the selector, this won't + // work. This is unlikely to happen since those cases should be + // mutually exclusive: + // e.g. ICMPV6_TYPE -> metadata.my_normalized_icmp_type + // ICMPV4_TYPE -> metadata.my_normalized_icmp_type + // A packet can be either ICMPv6 or ICMPv4 but not both. + selector.criteria() + .stream() + .map(Criterion::type) + .filter(t -> t != PROTOCOL_INDEPENDENT) + .forEach(t -> { + PiMatchFieldId mfid = interpreter.mapCriterionType(t) + .orElse(null); + if (mfid != null) { + if (criterionMap.containsKey(mfid)) { + log.warn("Detected criterion mapping " + + "conflict for PiMatchFieldId {}", + mfid); + } + criterionMap.put(mfid, selector.getCriterion(t)); + } + }); + } + for (PiMatchFieldModel fieldModel : tableModel.matchFields()) { PiMatchFieldId fieldId = fieldModel.id(); int bitWidth = fieldModel.bitWidth(); - Optional criterionType = - interpreter == null - ? Optional.empty() - : interpreter.mapPiMatchFieldId(fieldId); - - Criterion criterion = criterionType.map(selector::getCriterion).orElse(null); + Criterion criterion = criterionMap.get(fieldId); if (!piCriterionFields.containsKey(fieldId) && criterion == null) { // Neither a field in PiCriterion is available nor a Criterion mapping is possible. diff --git a/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java b/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java index fe3c22fdea..6e05a5b53a 100644 --- a/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java +++ b/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java @@ -16,8 +16,8 @@ package org.onosproject.pipelines.basic; -import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import org.onlab.packet.DeserializationException; import org.onlab.packet.Ethernet; import org.onlab.util.ImmutableByteSequence; @@ -45,6 +45,7 @@ import org.onosproject.net.pi.runtime.PiPacketOperation; import java.nio.ByteBuffer; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Optional; import static java.lang.String.format; @@ -80,12 +81,12 @@ public class BasicInterpreterImpl extends AbstractHandlerBehaviour private static final int PORT_BITWIDTH = 9; - private static final ImmutableBiMap TABLE_MAP = - new ImmutableBiMap.Builder() + private static final Map TABLE_MAP = + new ImmutableMap.Builder() .put(0, BasicConstants.INGRESS_TABLE0_CONTROL_TABLE0) .build(); - private static final ImmutableBiMap CRITERION_MAP = - new ImmutableBiMap.Builder() + private static final Map CRITERION_MAP = + new ImmutableMap.Builder() .put(Criterion.Type.IN_PORT, HDR_STANDARD_METADATA_INGRESS_PORT) .put(Criterion.Type.ETH_DST, HDR_HDR_ETHERNET_DST_ADDR) .put(Criterion.Type.ETH_SRC, HDR_HDR_ETHERNET_SRC_ADDR) @@ -235,18 +236,8 @@ public class BasicInterpreterImpl extends AbstractHandlerBehaviour return Optional.ofNullable(CRITERION_MAP.get(type)); } - @Override - public Optional mapPiMatchFieldId(PiMatchFieldId headerFieldId) { - return Optional.ofNullable(CRITERION_MAP.inverse().get(headerFieldId)); - } - @Override public Optional mapFlowRuleTableId(int flowRuleTableId) { return Optional.ofNullable(TABLE_MAP.get(flowRuleTableId)); } - - @Override - public Optional mapPiTableId(PiTableId piTableId) { - return Optional.ofNullable(TABLE_MAP.inverse().get(piTableId)); - } } diff --git a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java index e5234729c7..dcd872bc1a 100644 --- a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java +++ b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java @@ -99,25 +99,6 @@ public class FabricInterpreter extends AbstractFabricHandlerBehavior .put(Criterion.Type.ICMPV6_CODE, FabricConstants.HDR_ICMP_CODE) .build(); - private static final ImmutableMap INVERSE_CRITERION_MAP = - ImmutableMap.builder() - .put(FabricConstants.HDR_IG_PORT, Criterion.Type.IN_PORT) - .put(FabricConstants.HDR_ETH_DST, Criterion.Type.ETH_DST_MASKED) - .put(FabricConstants.HDR_ETH_SRC, Criterion.Type.ETH_SRC_MASKED) - .put(FabricConstants.HDR_ETH_TYPE, Criterion.Type.ETH_TYPE) - .put(FabricConstants.HDR_MPLS_LABEL, Criterion.Type.MPLS_LABEL) - .put(FabricConstants.HDR_VLAN_ID, Criterion.Type.VLAN_VID) - .put(FabricConstants.HDR_IPV4_DST, Criterion.Type.IPV4_DST) - .put(FabricConstants.HDR_IPV4_SRC, Criterion.Type.IPV4_SRC) - .put(FabricConstants.HDR_IPV6_DST, Criterion.Type.IPV6_DST) - // FIXME: might be incorrect if we inverse the map.... - .put(FabricConstants.HDR_L4_SPORT, Criterion.Type.UDP_SRC) - .put(FabricConstants.HDR_L4_DPORT, Criterion.Type.UDP_DST) - .put(FabricConstants.HDR_IP_PROTO, Criterion.Type.IP_PROTO) - .put(FabricConstants.HDR_ICMP_TYPE, Criterion.Type.ICMPV6_TYPE) - .put(FabricConstants.HDR_ICMP_CODE, Criterion.Type.ICMPV6_CODE) - .build(); - private static final PiAction NOP = PiAction.builder() .withId(FabricConstants.NOP).build(); @@ -131,11 +112,6 @@ public class FabricInterpreter extends AbstractFabricHandlerBehavior return Optional.ofNullable(CRITERION_MAP.get(type)); } - @Override - public Optional mapPiMatchFieldId(PiMatchFieldId fieldId) { - return Optional.ofNullable(INVERSE_CRITERION_MAP.get(fieldId)); - } - @Override public Optional mapFlowRuleTableId(int flowRuleTableId) { // The only use case for Index ID->PiTableId is when using the single @@ -143,13 +119,6 @@ public class FabricInterpreter extends AbstractFabricHandlerBehavior return Optional.empty(); } - @Override - public Optional mapPiTableId(PiTableId piTableId) { - // The only use case for Index ID->PiTableId is when using the single - // table pipeliner. fabric.p4 is never used with such pipeliner. - return Optional.empty(); - } - @Override public PiAction mapTreatment(TrafficTreatment treatment, PiTableId piTableId) throws PiInterpreterException {