| Message ID | 20230719183734.21681-14-larysa.zaremba@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | XDP metadata via kfuncs for ice | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next, async |
| netdev/apply | fail | Patch does not apply to bpf-next |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for ShellCheck |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_progs_no_alu32 on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-18 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-19 | success | Logs for test_progs_no_alu32_parallel on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-20 | success | Logs for test_progs_no_alu32_parallel on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-21 | success | Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-22 | success | Logs for test_progs_parallel on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-23 | success | Logs for test_progs_parallel on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-24 | success | Logs for test_progs_parallel on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-25 | success | Logs for test_verifier on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-26 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-27 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-28 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-29 | success | Logs for veristat |
On Wed, Jul 19, 2023 at 08:37:26PM +0200, Larysa Zaremba wrote: > Implement .xmo_rx_csum callback to allow XDP code to determine, > whether HW has validated any checksums. > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > index 54685d0747aa..6647a7e55ac8 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci, > return 0; > } > > +/** > + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum > + * @ctx: XDP buff pointer > + * @csum_status: destination address > + * @csum_info: destination address > + * > + * Copy HW checksum level (if was checked) to the destination address. > + */ > +static int ice_xdp_rx_csum(const struct xdp_md *ctx, > + enum xdp_csum_status *csum_status, > + union xdp_csum_info *csum_info) > +{ > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > + const union ice_32b_rx_flex_desc *eop_desc; > + enum ice_rx_csum_status status; > + u16 ptype; > + > + eop_desc = xdp_ext->pkt_ctx.eop_desc; > + ptype = ice_get_ptype(eop_desc); > + > + status = ice_get_rx_csum_status(eop_desc, ptype); > + if (status & ICE_RX_CSUM_NONE) > + return -ENODATA; > + > + *csum_status = ice_rx_csum_lvl(status) + 1; > + return 0; > +} and xdp_csum_info from previous patch left uninitialized? What was the point adding it then?
Alexei Starovoitov wrote: > On Wed, Jul 19, 2023 at 08:37:26PM +0200, Larysa Zaremba wrote: > > Implement .xmo_rx_csum callback to allow XDP code to determine, > > whether HW has validated any checksums. > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > index 54685d0747aa..6647a7e55ac8 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci, > > return 0; > > } > > > > +/** > > + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum > > + * @ctx: XDP buff pointer > > + * @csum_status: destination address > > + * @csum_info: destination address > > + * > > + * Copy HW checksum level (if was checked) to the destination address. > > + */ > > +static int ice_xdp_rx_csum(const struct xdp_md *ctx, > > + enum xdp_csum_status *csum_status, > > + union xdp_csum_info *csum_info) > > +{ > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > > + const union ice_32b_rx_flex_desc *eop_desc; > > + enum ice_rx_csum_status status; > > + u16 ptype; > > + > > + eop_desc = xdp_ext->pkt_ctx.eop_desc; > > + ptype = ice_get_ptype(eop_desc); > > + > > + status = ice_get_rx_csum_status(eop_desc, ptype); > > + if (status & ICE_RX_CSUM_NONE) > > + return -ENODATA; > > + > > + *csum_status = ice_rx_csum_lvl(status) + 1; > > + return 0; > > +} > > and xdp_csum_info from previous patch left uninitialized? > What was the point adding it then? I suppose this driver only returns CHECKSUM_NONE or CHECKSUM_UNNECESSARY? Also based on a grep of the driver dir. In general the layout makes sense to me, and +1 on supporting other drivers that do return CHECKSUM_COMPLETE or CHECKSUM_PARTIAL rigth from the start.
On Wed, Jul 19, 2023 at 05:51:17PM -0400, Willem de Bruijn wrote: > Alexei Starovoitov wrote: > > On Wed, Jul 19, 2023 at 08:37:26PM +0200, Larysa Zaremba wrote: > > > Implement .xmo_rx_csum callback to allow XDP code to determine, > > > whether HW has validated any checksums. > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > --- > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++ > > > 1 file changed, 29 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > index 54685d0747aa..6647a7e55ac8 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci, > > > return 0; > > > } > > > > > > +/** > > > + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum > > > + * @ctx: XDP buff pointer > > > + * @csum_status: destination address > > > + * @csum_info: destination address > > > + * > > > + * Copy HW checksum level (if was checked) to the destination address. > > > + */ > > > +static int ice_xdp_rx_csum(const struct xdp_md *ctx, > > > + enum xdp_csum_status *csum_status, > > > + union xdp_csum_info *csum_info) > > > +{ > > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > > > + const union ice_32b_rx_flex_desc *eop_desc; > > > + enum ice_rx_csum_status status; > > > + u16 ptype; > > > + > > > + eop_desc = xdp_ext->pkt_ctx.eop_desc; > > > + ptype = ice_get_ptype(eop_desc); > > > + > > > + status = ice_get_rx_csum_status(eop_desc, ptype); > > > + if (status & ICE_RX_CSUM_NONE) > > > + return -ENODATA; > > > + > > > + *csum_status = ice_rx_csum_lvl(status) + 1; > > > + return 0; > > > +} > > > > and xdp_csum_info from previous patch left uninitialized? > > What was the point adding it then? > > I suppose this driver only returns CHECKSUM_NONE or > CHECKSUM_UNNECESSARY? Also based on a grep of the driver dir. > Yes, correct, current ice HW cannot produce complete checksum, so only CHECKSUM_UNNECESSARY for known protocols, CHECKSUM_NONE otherwise, nothing to initialize csum_info with in either case. xdp_csum_info is initialized in veth implementation though, but only csum_start/offset, so complete XDP checksum has no users in this patchset. Is this a problem? In previous version I had CHECKSUM_UNNECESSARY-only kfunc, but I think everyone has agreed, csum hint kfunc should give more comprehensive output. > In general the layout makes sense to me, and +1 on supporting > other drivers that do return CHECKSUM_COMPLETE or CHECKSUM_PARTIAL > rigth from the start. >
On Thu, Jul 20, 2023 at 2:47 AM Zaremba, Larysa <larysa.zaremba@intel.com> wrote: > > On Wed, Jul 19, 2023 at 05:51:17PM -0400, Willem de Bruijn wrote: > > Alexei Starovoitov wrote: > > > On Wed, Jul 19, 2023 at 08:37:26PM +0200, Larysa Zaremba wrote: > > > > Implement .xmo_rx_csum callback to allow XDP code to determine, > > > > whether HW has validated any checksums. > > > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > > --- > > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++ > > > > 1 file changed, 29 insertions(+) > > > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > index 54685d0747aa..6647a7e55ac8 100644 > > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci, > > > > return 0; > > > > } > > > > > > > > +/** > > > > + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum > > > > + * @ctx: XDP buff pointer > > > > + * @csum_status: destination address > > > > + * @csum_info: destination address > > > > + * > > > > + * Copy HW checksum level (if was checked) to the destination address. > > > > + */ > > > > +static int ice_xdp_rx_csum(const struct xdp_md *ctx, > > > > + enum xdp_csum_status *csum_status, > > > > + union xdp_csum_info *csum_info) > > > > +{ > > > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > > > > + const union ice_32b_rx_flex_desc *eop_desc; > > > > + enum ice_rx_csum_status status; > > > > + u16 ptype; > > > > + > > > > + eop_desc = xdp_ext->pkt_ctx.eop_desc; > > > > + ptype = ice_get_ptype(eop_desc); > > > > + > > > > + status = ice_get_rx_csum_status(eop_desc, ptype); > > > > + if (status & ICE_RX_CSUM_NONE) > > > > + return -ENODATA; > > > > + > > > > + *csum_status = ice_rx_csum_lvl(status) + 1; > > > > + return 0; > > > > +} > > > > > > and xdp_csum_info from previous patch left uninitialized? > > > What was the point adding it then? > > > > I suppose this driver only returns CHECKSUM_NONE or > > CHECKSUM_UNNECESSARY? Also based on a grep of the driver dir. > > > > Yes, correct, current ice HW cannot produce complete checksum, > so only CHECKSUM_UNNECESSARY for known protocols, CHECKSUM_NONE otherwise, > nothing to initialize csum_info with in either case. > > xdp_csum_info is initialized in veth implementation though, but only > csum_start/offset, so complete XDP checksum has no users in this patchset. > Is this a problem? > > In previous version I had CHECKSUM_UNNECESSARY-only kfunc, but I think everyone > has agreed, csum hint kfunc should give more comprehensive output. csum kfunc supposed to be generic. If for ICE it fills in one argument and for veth another then the whole idea of generic api is not working.
On Thu, Jul 20, 2023 at 08:14:52AM -0700, Alexei Starovoitov wrote: > On Thu, Jul 20, 2023 at 2:47 AM Zaremba, Larysa > <larysa.zaremba@intel.com> wrote: > > > > On Wed, Jul 19, 2023 at 05:51:17PM -0400, Willem de Bruijn wrote: > > > Alexei Starovoitov wrote: > > > > On Wed, Jul 19, 2023 at 08:37:26PM +0200, Larysa Zaremba wrote: > > > > > Implement .xmo_rx_csum callback to allow XDP code to determine, > > > > > whether HW has validated any checksums. > > > > > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > > > --- > > > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++ > > > > > 1 file changed, 29 insertions(+) > > > > > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > index 54685d0747aa..6647a7e55ac8 100644 > > > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci, > > > > > return 0; > > > > > } > > > > > > > > > > +/** > > > > > + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum > > > > > + * @ctx: XDP buff pointer > > > > > + * @csum_status: destination address > > > > > + * @csum_info: destination address > > > > > + * > > > > > + * Copy HW checksum level (if was checked) to the destination address. > > > > > + */ > > > > > +static int ice_xdp_rx_csum(const struct xdp_md *ctx, > > > > > + enum xdp_csum_status *csum_status, > > > > > + union xdp_csum_info *csum_info) > > > > > +{ > > > > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > > > > > + const union ice_32b_rx_flex_desc *eop_desc; > > > > > + enum ice_rx_csum_status status; > > > > > + u16 ptype; > > > > > + > > > > > + eop_desc = xdp_ext->pkt_ctx.eop_desc; > > > > > + ptype = ice_get_ptype(eop_desc); > > > > > + > > > > > + status = ice_get_rx_csum_status(eop_desc, ptype); > > > > > + if (status & ICE_RX_CSUM_NONE) > > > > > + return -ENODATA; > > > > > + > > > > > + *csum_status = ice_rx_csum_lvl(status) + 1; I'll duplicate an improved version of this line from another thread in case it could help with the comprehension during review: *csum_status = XDP_CHECKSUM_VALID_LVL0 + ice_rx_csum_lvl(status); > > > > > + return 0; > > > > > +} > > > > > > > > and xdp_csum_info from previous patch left uninitialized? > > > > What was the point adding it then? > > > > > > I suppose this driver only returns CHECKSUM_NONE or > > > CHECKSUM_UNNECESSARY? Also based on a grep of the driver dir. > > > > > > > Yes, correct, current ice HW cannot produce complete checksum, > > so only CHECKSUM_UNNECESSARY for known protocols, CHECKSUM_NONE otherwise, > > nothing to initialize csum_info with in either case. > > > > xdp_csum_info is initialized in veth implementation though, but only > > csum_start/offset, so complete XDP checksum has no users in this patchset. > > Is this a problem? > > > > In previous version I had CHECKSUM_UNNECESSARY-only kfunc, but I think everyone > > has agreed, csum hint kfunc should give more comprehensive output. > > csum kfunc supposed to be generic. > If for ICE it fills in one argument and for veth another then the whole > idea of generic api is not working. Both ice and veth fill in the csum_status, the need to fill in the csum_info is determined by the status. I don not see a problem with that. Maybe you have an issue with putting a valid checksum number into a status instead of info? Please clarify.
On 07/20, Zaremba, Larysa wrote: > On Thu, Jul 20, 2023 at 08:14:52AM -0700, Alexei Starovoitov wrote: > > On Thu, Jul 20, 2023 at 2:47 AM Zaremba, Larysa > > <larysa.zaremba@intel.com> wrote: > > > > > > On Wed, Jul 19, 2023 at 05:51:17PM -0400, Willem de Bruijn wrote: > > > > Alexei Starovoitov wrote: > > > > > On Wed, Jul 19, 2023 at 08:37:26PM +0200, Larysa Zaremba wrote: > > > > > > Implement .xmo_rx_csum callback to allow XDP code to determine, > > > > > > whether HW has validated any checksums. > > > > > > > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > > > > --- > > > > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++ > > > > > > 1 file changed, 29 insertions(+) > > > > > > > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > index 54685d0747aa..6647a7e55ac8 100644 > > > > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci, > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum > > > > > > + * @ctx: XDP buff pointer > > > > > > + * @csum_status: destination address > > > > > > + * @csum_info: destination address > > > > > > + * > > > > > > + * Copy HW checksum level (if was checked) to the destination address. > > > > > > + */ > > > > > > +static int ice_xdp_rx_csum(const struct xdp_md *ctx, > > > > > > + enum xdp_csum_status *csum_status, > > > > > > + union xdp_csum_info *csum_info) > > > > > > +{ > > > > > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > > > > > > + const union ice_32b_rx_flex_desc *eop_desc; > > > > > > + enum ice_rx_csum_status status; > > > > > > + u16 ptype; > > > > > > + > > > > > > + eop_desc = xdp_ext->pkt_ctx.eop_desc; > > > > > > + ptype = ice_get_ptype(eop_desc); > > > > > > + > > > > > > + status = ice_get_rx_csum_status(eop_desc, ptype); > > > > > > + if (status & ICE_RX_CSUM_NONE) > > > > > > + return -ENODATA; > > > > > > + > > > > > > + *csum_status = ice_rx_csum_lvl(status) + 1; > > I'll duplicate an improved version of this line from another thread in case it > could help with the comprehension during review: > > *csum_status = XDP_CHECKSUM_VALID_LVL0 + ice_rx_csum_lvl(status); > > > > > > > + return 0; > > > > > > +} > > > > > > > > > > and xdp_csum_info from previous patch left uninitialized? > > > > > What was the point adding it then? > > > > > > > > I suppose this driver only returns CHECKSUM_NONE or > > > > CHECKSUM_UNNECESSARY? Also based on a grep of the driver dir. > > > > > > > > > > Yes, correct, current ice HW cannot produce complete checksum, > > > so only CHECKSUM_UNNECESSARY for known protocols, CHECKSUM_NONE otherwise, > > > nothing to initialize csum_info with in either case. > > > > > > xdp_csum_info is initialized in veth implementation though, but only > > > csum_start/offset, so complete XDP checksum has no users in this patchset. > > > Is this a problem? > > > > > > In previous version I had CHECKSUM_UNNECESSARY-only kfunc, but I think everyone > > > has agreed, csum hint kfunc should give more comprehensive output. > > > > csum kfunc supposed to be generic. > > If for ICE it fills in one argument and for veth another then the whole > > idea of generic api is not working. > > Both ice and veth fill in the csum_status, the need to fill in the csum_info is > determined by the status. I don not see a problem with that. > > Maybe you have an issue with putting a valid checksum number into a status > instead of info? Please clarify. +1, that seems to match skb interface Regarding 'generic api not working' in general: I think we've discussed that with this 'flexible' kfunc format we can allow non-generic kfuncs for specific devices if we think that it makes sense to differentiate/experiment/etc. Do you think it makes sense to go non-generic route here?
Stanislav Fomichev wrote: > On 07/20, Zaremba, Larysa wrote: > > On Thu, Jul 20, 2023 at 08:14:52AM -0700, Alexei Starovoitov wrote: > > > On Thu, Jul 20, 2023 at 2:47 AM Zaremba, Larysa > > > <larysa.zaremba@intel.com> wrote: > > > > > > > > On Wed, Jul 19, 2023 at 05:51:17PM -0400, Willem de Bruijn wrote: > > > > > Alexei Starovoitov wrote: > > > > > > On Wed, Jul 19, 2023 at 08:37:26PM +0200, Larysa Zaremba wrote: > > > > > > > Implement .xmo_rx_csum callback to allow XDP code to determine, > > > > > > > whether HW has validated any checksums. > > > > > > > > > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > > > > > --- > > > > > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++ > > > > > > > 1 file changed, 29 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > > index 54685d0747aa..6647a7e55ac8 100644 > > > > > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > > @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci, > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > +/** > > > > > > > + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum > > > > > > > + * @ctx: XDP buff pointer > > > > > > > + * @csum_status: destination address > > > > > > > + * @csum_info: destination address > > > > > > > + * > > > > > > > + * Copy HW checksum level (if was checked) to the destination address. > > > > > > > + */ > > > > > > > +static int ice_xdp_rx_csum(const struct xdp_md *ctx, > > > > > > > + enum xdp_csum_status *csum_status, > > > > > > > + union xdp_csum_info *csum_info) > > > > > > > +{ > > > > > > > + const struct ice_xdp_buff *xdp_ext = (void *)ctx; > > > > > > > + const union ice_32b_rx_flex_desc *eop_desc; > > > > > > > + enum ice_rx_csum_status status; > > > > > > > + u16 ptype; > > > > > > > + > > > > > > > + eop_desc = xdp_ext->pkt_ctx.eop_desc; > > > > > > > + ptype = ice_get_ptype(eop_desc); > > > > > > > + > > > > > > > + status = ice_get_rx_csum_status(eop_desc, ptype); > > > > > > > + if (status & ICE_RX_CSUM_NONE) > > > > > > > + return -ENODATA; > > > > > > > + > > > > > > > + *csum_status = ice_rx_csum_lvl(status) + 1; > > > > I'll duplicate an improved version of this line from another thread in case it > > could help with the comprehension during review: > > > > *csum_status = XDP_CHECKSUM_VALID_LVL0 + ice_rx_csum_lvl(status); > > > > > > > > > + return 0; > > > > > > > +} > > > > > > > > > > > > and xdp_csum_info from previous patch left uninitialized? > > > > > > What was the point adding it then? > > > > > > > > > > I suppose this driver only returns CHECKSUM_NONE or > > > > > CHECKSUM_UNNECESSARY? Also based on a grep of the driver dir. > > > > > > > > > > > > > Yes, correct, current ice HW cannot produce complete checksum, > > > > so only CHECKSUM_UNNECESSARY for known protocols, CHECKSUM_NONE otherwise, > > > > nothing to initialize csum_info with in either case. > > > > > > > > xdp_csum_info is initialized in veth implementation though, but only > > > > csum_start/offset, so complete XDP checksum has no users in this patchset. > > > > Is this a problem? > > > > > > > > In previous version I had CHECKSUM_UNNECESSARY-only kfunc, but I think everyone > > > > has agreed, csum hint kfunc should give more comprehensive output. > > > > > > csum kfunc supposed to be generic. > > > If for ICE it fills in one argument and for veth another then the whole > > > idea of generic api is not working. > > > > Both ice and veth fill in the csum_status, the need to fill in the csum_info is > > determined by the status. I don not see a problem with that. > > > > Maybe you have an issue with putting a valid checksum number into a status > > instead of info? Please clarify. > > +1, that seems to match skb interface > > Regarding 'generic api not working' in general: I think we've discussed > that with this 'flexible' kfunc format we can allow non-generic kfuncs for > specific devices if we think that it makes sense to > differentiate/experiment/etc. Do you think it makes sense to go > non-generic route here? I think we should expose the standard CHECKSUM_* behavior. The current encoding captures all four types. Which type is returned is not necessarily defined only by the device. Some devices can return CHECKSUM_NONE for some packets, CHECKSUM_PARTIAL for others and CHECKSUM_UNNECESSARY for a third set (e.g., mlx4). Specializing the return type to the device would not simplify the struct.
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c index 54685d0747aa..6647a7e55ac8 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c @@ -660,8 +660,37 @@ static int ice_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tci, return 0; } +/** + * ice_xdp_rx_csum_lvl - Get level, at which HW has checked the checksum + * @ctx: XDP buff pointer + * @csum_status: destination address + * @csum_info: destination address + * + * Copy HW checksum level (if was checked) to the destination address. + */ +static int ice_xdp_rx_csum(const struct xdp_md *ctx, + enum xdp_csum_status *csum_status, + union xdp_csum_info *csum_info) +{ + const struct ice_xdp_buff *xdp_ext = (void *)ctx; + const union ice_32b_rx_flex_desc *eop_desc; + enum ice_rx_csum_status status; + u16 ptype; + + eop_desc = xdp_ext->pkt_ctx.eop_desc; + ptype = ice_get_ptype(eop_desc); + + status = ice_get_rx_csum_status(eop_desc, ptype); + if (status & ICE_RX_CSUM_NONE) + return -ENODATA; + + *csum_status = ice_rx_csum_lvl(status) + 1; + return 0; +} + const struct xdp_metadata_ops ice_xdp_md_ops = { .xmo_rx_timestamp = ice_xdp_rx_hw_ts, .xmo_rx_hash = ice_xdp_rx_hash, .xmo_rx_vlan_tag = ice_xdp_rx_vlan_tag, + .xmo_rx_csum = ice_xdp_rx_csum, };
Implement .xmo_rx_csum callback to allow XDP code to determine, whether HW has validated any checksums. Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> --- drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+)