On Fri, 2020-11-13 at 14:59 -0800, Alexander Duyck wrote:
> On Fri, Nov 13, 2020 at 1:36 PM Tony Nguyen <
> anthony.l.ngu...@intel.com> wrote:
> >
> > From: Real Valiquette <real.valique...@intel.com>
> >
> > ACL filtering can be utilized to expand support of ntuple rules by
> > allowing
> > mask values to be specified for redirect to queue or drop.
> >
> > Implement support for specifying the 'm' value of ethtool ntuple
> > command
> > for currently supported fields (src-ip, dst-ip, src-port, and dst-
> > port).
> >
> > For example:
> >
> > ethtool -N eth0 flow-type tcp4 dst-port 8880 m 0x00ff action 10
> > or
> > ethtool -N eth0 flow-type tcp4 src-ip 192.168.0.55 m 0.0.0.255
> > action -1
> >
> > At this time the following flow-types support mask values: tcp4,
> > udp4,
> > sctp4, and ip4.
>
> So you spend all of the patch description describing how this might
> be
> used in the future. However there is nothing specific to the ethtool
> interface as far as I can tell anywhere in this patch. With this
> patch
> the actual command called out above cannot be performed, correct?
>
> > Begin implementation of ACL filters by setting up structures,
> > AdminQ
> > commands, and allocation of the ACL table in the hardware.
>
> This seems to be what this patch is actually doing. You may want to
> rewrite this patch description to focus on this and explain that you
> are enabling future support for ethtool ntuple masks. However save
> this feature description for the patch that actually enables the
> functionality.
I will do this. Thanks.
> > Co-developed-by: Chinh Cao <chinh.t....@intel.com>
> > Signed-off-by: Chinh Cao <chinh.t....@intel.com>
> > Signed-off-by: Real Valiquette <real.valique...@intel.com>
> > Co-developed-by: Tony Nguyen <anthony.l.ngu...@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.ngu...@intel.com>
> > Tested-by: Harikumar Bokkena <harikumarx.bokk...@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/Makefile | 2 +
> > drivers/net/ethernet/intel/ice/ice.h | 4 +
> > drivers/net/ethernet/intel/ice/ice_acl.c | 153 +++++++++
> > drivers/net/ethernet/intel/ice/ice_acl.h | 125 +++++++
> > drivers/net/ethernet/intel/ice/ice_acl_ctrl.c | 311
> > ++++++++++++++++++
> > .../net/ethernet/intel/ice/ice_adminq_cmd.h | 215 +++++++++++-
> > drivers/net/ethernet/intel/ice/ice_flow.h | 2 +
> > drivers/net/ethernet/intel/ice/ice_main.c | 50 +++
> > drivers/net/ethernet/intel/ice/ice_type.h | 3 +
> > 9 files changed, 863 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/net/ethernet/intel/ice/ice_acl.c
> > create mode 100644 drivers/net/ethernet/intel/ice/ice_acl.h
> > create mode 100644 drivers/net/ethernet/intel/ice/ice_acl_ctrl.c
> >
>
> <snip>
>
> > +/**
> > + * ice_acl_destroy_tbl - Destroy a previously created LEM table
> > for ACL
> > + * @hw: pointer to the HW struct
> > + */
> > +enum ice_status ice_acl_destroy_tbl(struct ice_hw *hw)
> > +{
> > + struct ice_aqc_acl_generic resp_buf;
> > + enum ice_status status;
> > +
> > + if (!hw->acl_tbl)
> > + return ICE_ERR_DOES_NOT_EXIST;
> > +
> > + /* call the AQ command to destroy the ACL table */
> > + status = ice_aq_dealloc_acl_tbl(hw, hw->acl_tbl->id,
> > &resp_buf, NULL);
> > + if (status) {
> > + ice_debug(hw, ICE_DBG_ACL, "AQ de-allocation of ACL
> > failed. status: %d\n",
> > + status);
> > + return status;
> > + }
> > +
> > + devm_kfree(ice_hw_to_dev(hw), hw->acl_tbl);
> > + hw->acl_tbl = NULL;
>
> What are the scenarios where you might see the dealloc_acl_tbl fail?
> I'm just wondering if it makes sense to keep the table just because
> the hardware is refusing to give it up.
The datasheet isn't clear on what would cause a failed allocation, but
we're trying to keep the SW structures in sync with HW.
<snip>
> > +/* This response structure is same in case of alloc/dealloc table,
> > + * alloc/dealloc action-pair
> > + */
> > +struct ice_aqc_acl_generic {
> > + /* if alloc_id is below 0x1000 then allocation failed due
> > to
> > + * unavailable resources, else this is set by FW to
> > identify
> > + * table allocation
> > + */
> > + __le16 alloc_id;
> > +
> > + union {
> > + /* to be used only in case of alloc/dealloc table
> > */
> > + struct {
> > + /* Index of the first TCAM block, otherwise
> > set to 0xFF
> > + * for a failed allocation
> > + */
> > + u8 first_tcam;
> > + /* Index of the last TCAM block. This index
> > shall be
> > + * set to the value of first_tcam for
> > single TCAM block
> > + * allocation, otherwise set to 0xFF for a
> > failed
> > + * allocation
> > + */
> > + u8 last_tcam;
> > + } table;
> > + /* reserved in case of alloc/dealloc action-pair */
> > + struct {
> > + __le16 reserved;
> > + } act_pair;
>
> Is there really any need to call out the reserved value? It seems
> like
> you could just leave the struct table in place and not bother with
> the
> union since you would likely just be memsetting the entire ops struct
> to 0 anyway.
>
Since this is used for table and action-pair calls, the reason we have
the union and reserved value is to make it explicit that for action-
pair calls, nothing is to be written and to clearly differentiate the
fields that should be set for table alloc/dealloc and those for
act_pair alloc/dealloc.
> > + } ops;
> > +
> > + /* index of first entry (in both TCAM and action memories),
> > + * otherwise set to 0xFF for a failed allocation
> > + */
> > + __le16 first_entry;
> > + /* index of last entry (in both TCAM and action memories),
> > + * otherwise set to 0xFF for a failed allocation
> > + */
> > + __le16 last_entry;
> > +
> > + /* Each act_mem element specifies the order of the memory
> > + * otherwise 0xFF
> > + */
> > + u8 act_mem[ICE_AQC_MAX_ACTION_MEMORIES];
> > +};
> > +
>
> <snip>
>
> > +/**
> > + * ice_deinit_acl - Unroll the initialization of the ACL block
> > + * @pf: ptr to PF device
> > + */
> > +static void ice_deinit_acl(struct ice_pf *pf)
> > +{
> > + ice_acl_destroy_tbl(&pf->hw);
>
> Why have the ice_acl_destroy_tbl function return a value if it is
> just
> going to be ignored?
>
> > +}
> > +
> > /**
> > * ice_init_fdir - Initialize flow director VSI and configuration
> > * @pf: pointer to the PF instance
> > @@ -4231,6 +4273,12 @@ ice_probe(struct pci_dev *pdev, const struct
> > pci_device_id __always_unused *ent)
> > /* Note: Flow director init failure is non-fatal to load */
> > if (ice_init_fdir(pf))
> > dev_err(dev, "could not initialize flow
> > director\n");
> > + if (test_bit(ICE_FLAG_FD_ENA, pf->flags)) {
> > + /* Note: ACL init failure is non-fatal to load */
> > + err = ice_init_acl(pf);
> > + if (err)
> > + dev_err(dev, "Failed to initialize ACL:
> > %d\n", err);
> > + }
> >
> > /* Note: DCB init failure is non-fatal to load */
> > if (ice_init_pf_dcb(pf, false)) {
> > @@ -4361,6 +4409,8 @@ static void ice_remove(struct pci_dev *pdev)
> >
> > ice_aq_cancel_waiting_tasks(pf);
> >
> > + if (test_bit(ICE_FLAG_FD_ENA, pf->flags))
> > + ice_deinit_acl(pf);
>
> Looking over the code is there any reason why you need to bother with
> checking the flag? It seems like if ACL is not enabled ice_deinit_acl
> won't do anything. So why bother checking the flag? Also is it really
> okay to just ignore if deallocating the table fails? What are the
> side
> effects?
>
I will remove the flag check. If the table deallocation fails, there
isn't much we can do to resolve it. I'll use the return value from
ice_acl_destroy_tbl() and add a warning to inform the user of an issue.
> > mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
> > if (!ice_is_safe_mode(pf))
> > ice_remove_arfs(pf);
> >
Thanks,
Tony
