Re: [net-next v2 03/15] ice: initialize ACL table

["Nguyen, Anthony L" <anthony.l.nguyen@intel.com>]

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.nguyen@intel.com> wrote:
> > 
> > From: Real Valiquette <real.valiquette@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.cao@intel.com>
> > Signed-off-by: Chinh Cao <chinh.t.cao@intel.com>
> > Signed-off-by: Real Valiquette <real.valiquette@intel.com>
> > Co-developed-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > Tested-by: Harikumar Bokkena  <harikumarx.bokkena@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