| Message ID | 20210907075957.28848-3-hemant.agrawal@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: add raw vector support in DPAAx | expand |
> From: Gagandeep Singh <g.singh@nxp.com> > > The current crypto raw data vectors is extended to support > rte_security usecases, where we need total data length to know > how much additional memory space is available in buffer other > than data length so that driver/HW can write expanded size > data after encryption. > > Signed-off-by: Gagandeep Singh <g.singh@nxp.com> > Acked-by: Akhil Goyal <gakhil@marvell.com> > --- > lib/cryptodev/rte_crypto_sym.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/lib/cryptodev/rte_crypto_sym.h b/lib/cryptodev/rte_crypto_sym.h > index dcc0bd5933..e5cef1fb72 100644 > --- a/lib/cryptodev/rte_crypto_sym.h > +++ b/lib/cryptodev/rte_crypto_sym.h > @@ -37,6 +37,8 @@ struct rte_crypto_vec { > rte_iova_t iova; > /** length of the data buffer */ > uint32_t len; > + /** total buffer length*/ > + uint32_t tot_len; > }; > > /** > @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len, > seglen = mb->data_len - ofs; > if (len <= seglen) { > vec[0].len = len; > + vec[0].tot_len = mb->buf_len; That doesn't look right. We should take into a count mbuf headroom and input offset. Something like: vec[0].tot_len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; Same in other places below. > return 1; > } > > /* data spread across segments */ > vec[0].len = seglen; > left = len - seglen; > + vec[0].tot_len = mb->buf_len; > for (i = 1, nseg = mb->next; nseg != NULL; nseg = nseg->next, i++) { > > vec[i].base = rte_pktmbuf_mtod(nseg, void *); > @@ -995,6 +999,7 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len, > if (left <= seglen) { > /* whole requested data is completed */ > vec[i].len = left; > + vec[i].tot_len = mb->buf_len; > left = 0; > break; > } > @@ -1002,6 +1007,7 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len, > /* use whole segment */ > vec[i].len = seglen; > left -= seglen; > + vec[i].tot_len = mb->buf_len; > } > > RTE_ASSERT(left == 0); > -- > 2.17.1
> > > From: Gagandeep Singh <g.singh@nxp.com> > > > > The current crypto raw data vectors is extended to support > > rte_security usecases, where we need total data length to know > > how much additional memory space is available in buffer other > > than data length so that driver/HW can write expanded size > > data after encryption. > > > > Signed-off-by: Gagandeep Singh <g.singh@nxp.com> > > Acked-by: Akhil Goyal <gakhil@marvell.com> > > --- > > lib/cryptodev/rte_crypto_sym.h | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/lib/cryptodev/rte_crypto_sym.h > b/lib/cryptodev/rte_crypto_sym.h > > index dcc0bd5933..e5cef1fb72 100644 > > --- a/lib/cryptodev/rte_crypto_sym.h > > +++ b/lib/cryptodev/rte_crypto_sym.h > > @@ -37,6 +37,8 @@ struct rte_crypto_vec { > > rte_iova_t iova; > > /** length of the data buffer */ > > uint32_t len; > > + /** total buffer length*/ > > + uint32_t tot_len; > > }; > > > > /** > > @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf > *mb, uint32_t ofs, uint32_t len, > > seglen = mb->data_len - ofs; > > if (len <= seglen) { > > vec[0].len = len; > > + vec[0].tot_len = mb->buf_len; > > That doesn't look right. > We should take into a count mbuf headroom and input offset. > Something like: > vec[0].tot_len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; > Same in other places below. > I believe the packet can expand into headroom based on the protocol support.
Hi Konstantin On 9/21/2021 12:58 AM, Akhil Goyal wrote: >>> From: Gagandeep Singh <g.singh@nxp.com> >>> >>> The current crypto raw data vectors is extended to support >>> rte_security usecases, where we need total data length to know >>> how much additional memory space is available in buffer other >>> than data length so that driver/HW can write expanded size >>> data after encryption. >>> >>> Signed-off-by: Gagandeep Singh <g.singh@nxp.com> >>> Acked-by: Akhil Goyal <gakhil@marvell.com> >>> --- >>> lib/cryptodev/rte_crypto_sym.h | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/lib/cryptodev/rte_crypto_sym.h >> b/lib/cryptodev/rte_crypto_sym.h >>> index dcc0bd5933..e5cef1fb72 100644 >>> --- a/lib/cryptodev/rte_crypto_sym.h >>> +++ b/lib/cryptodev/rte_crypto_sym.h >>> @@ -37,6 +37,8 @@ struct rte_crypto_vec { >>> rte_iova_t iova; >>> /** length of the data buffer */ >>> uint32_t len; >>> + /** total buffer length*/ >>> + uint32_t tot_len; >>> }; >>> >>> /** >>> @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf >> *mb, uint32_t ofs, uint32_t len, >>> seglen = mb->data_len - ofs; >>> if (len <= seglen) { >>> vec[0].len = len; >>> + vec[0].tot_len = mb->buf_len; >> That doesn't look right. >> We should take into a count mbuf headroom and input offset. >> Something like: >> vec[0].tot_len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; >> Same in other places below. >> > I believe the packet can expand into headroom based on the protocol support. Yes, total length is representing the total buffer length available. The security protocol shall take care of the headroom and offsets.
> Hi Konstantin > > On 9/21/2021 12:58 AM, Akhil Goyal wrote: > >>> From: Gagandeep Singh <g.singh@nxp.com> > >>> > >>> The current crypto raw data vectors is extended to support > >>> rte_security usecases, where we need total data length to know > >>> how much additional memory space is available in buffer other > >>> than data length so that driver/HW can write expanded size > >>> data after encryption. > >>> > >>> Signed-off-by: Gagandeep Singh <g.singh@nxp.com> > >>> Acked-by: Akhil Goyal <gakhil@marvell.com> > >>> --- > >>> lib/cryptodev/rte_crypto_sym.h | 6 ++++++ > >>> 1 file changed, 6 insertions(+) > >>> > >>> diff --git a/lib/cryptodev/rte_crypto_sym.h > >> b/lib/cryptodev/rte_crypto_sym.h > >>> index dcc0bd5933..e5cef1fb72 100644 > >>> --- a/lib/cryptodev/rte_crypto_sym.h > >>> +++ b/lib/cryptodev/rte_crypto_sym.h > >>> @@ -37,6 +37,8 @@ struct rte_crypto_vec { > >>> rte_iova_t iova; > >>> /** length of the data buffer */ > >>> uint32_t len; > >>> + /** total buffer length*/ > >>> + uint32_t tot_len; > >>> }; > >>> > >>> /** > >>> @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf > >> *mb, uint32_t ofs, uint32_t len, > >>> seglen = mb->data_len - ofs; > >>> if (len <= seglen) { > >>> vec[0].len = len; > >>> + vec[0].tot_len = mb->buf_len; > >> That doesn't look right. > >> We should take into a count mbuf headroom and input offset. > >> Something like: > >> vec[0].tot_len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; > >> Same in other places below. > >> > > I believe the packet can expand into headroom based on the protocol support. > Yes, total length is representing the total buffer length available. The > security protocol shall take care of the headroom and offsets. Hmm, and how it will now how many bytes are in head-room, and how many are in tail-room? We either need to provide values for both, or assume that only tail-room is available for the driver.
> > Hi Konstantin > > > > On 9/21/2021 12:58 AM, Akhil Goyal wrote: > > >>> From: Gagandeep Singh <g.singh@nxp.com> > > >>> > > >>> The current crypto raw data vectors is extended to support > > >>> rte_security usecases, where we need total data length to know > > >>> how much additional memory space is available in buffer other > > >>> than data length so that driver/HW can write expanded size > > >>> data after encryption. > > >>> > > >>> Signed-off-by: Gagandeep Singh <g.singh@nxp.com> > > >>> Acked-by: Akhil Goyal <gakhil@marvell.com> > > >>> --- > > >>> lib/cryptodev/rte_crypto_sym.h | 6 ++++++ > > >>> 1 file changed, 6 insertions(+) > > >>> > > >>> diff --git a/lib/cryptodev/rte_crypto_sym.h > > >> b/lib/cryptodev/rte_crypto_sym.h > > >>> index dcc0bd5933..e5cef1fb72 100644 > > >>> --- a/lib/cryptodev/rte_crypto_sym.h > > >>> +++ b/lib/cryptodev/rte_crypto_sym.h > > >>> @@ -37,6 +37,8 @@ struct rte_crypto_vec { > > >>> rte_iova_t iova; > > >>> /** length of the data buffer */ > > >>> uint32_t len; > > >>> + /** total buffer length*/ > > >>> + uint32_t tot_len; > > >>> }; > > >>> > > >>> /** > > >>> @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct > rte_mbuf > > >> *mb, uint32_t ofs, uint32_t len, > > >>> seglen = mb->data_len - ofs; > > >>> if (len <= seglen) { > > >>> vec[0].len = len; > > >>> + vec[0].tot_len = mb->buf_len; > > >> That doesn't look right. > > >> We should take into a count mbuf headroom and input offset. > > >> Something like: > > >> vec[0].tot_len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; > > >> Same in other places below. > > >> > > > I believe the packet can expand into headroom based on the protocol > support. > > Yes, total length is representing the total buffer length available. The > > security protocol shall take care of the headroom and offsets. > > Hmm, and how it will now how many bytes are in head-room, and how > many are in tail-room? > We either need to provide values for both, or assume that only tail-room is > available for the driver. I believe it should be starting point where output can be written till the end of buffer. There should not be any headroom and tailroom for raw buffers. It should be mbuf->buf_len - ofs.
Hi Akhil, > > > On 9/21/2021 12:58 AM, Akhil Goyal wrote: > > > >>> From: Gagandeep Singh <g.singh@nxp.com> > > > >>> > > > >>> The current crypto raw data vectors is extended to support > > > >>> rte_security usecases, where we need total data length to know > > > >>> how much additional memory space is available in buffer other > > > >>> than data length so that driver/HW can write expanded size > > > >>> data after encryption. > > > >>> > > > >>> Signed-off-by: Gagandeep Singh <g.singh@nxp.com> > > > >>> Acked-by: Akhil Goyal <gakhil@marvell.com> > > > >>> --- > > > >>> lib/cryptodev/rte_crypto_sym.h | 6 ++++++ > > > >>> 1 file changed, 6 insertions(+) > > > >>> > > > >>> diff --git a/lib/cryptodev/rte_crypto_sym.h > > > >> b/lib/cryptodev/rte_crypto_sym.h > > > >>> index dcc0bd5933..e5cef1fb72 100644 > > > >>> --- a/lib/cryptodev/rte_crypto_sym.h > > > >>> +++ b/lib/cryptodev/rte_crypto_sym.h > > > >>> @@ -37,6 +37,8 @@ struct rte_crypto_vec { > > > >>> rte_iova_t iova; > > > >>> /** length of the data buffer */ > > > >>> uint32_t len; > > > >>> + /** total buffer length*/ > > > >>> + uint32_t tot_len; > > > >>> }; > > > >>> > > > >>> /** > > > >>> @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct > > rte_mbuf > > > >> *mb, uint32_t ofs, uint32_t len, > > > >>> seglen = mb->data_len - ofs; > > > >>> if (len <= seglen) { > > > >>> vec[0].len = len; > > > >>> + vec[0].tot_len = mb->buf_len; > > > >> That doesn't look right. > > > >> We should take into a count mbuf headroom and input offset. > > > >> Something like: > > > >> vec[0].tot_len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; > > > >> Same in other places below. > > > >> > > > > I believe the packet can expand into headroom based on the protocol > > support. > > > Yes, total length is representing the total buffer length available. The > > > security protocol shall take care of the headroom and offsets. > > > > Hmm, and how it will now how many bytes are in head-room, and how > > many are in tail-room? > > We either need to provide values for both, or assume that only tail-room is > > available for the driver. > I believe it should be starting point where output can be written till the end of buffer. Right, that's: base = rte_pktmbuf_mtod_offset(mb, void *, ofs); > There should not be any headroom and tailroom for raw buffers. I am not talking about raw buffers, what I am saying that some space in the mbuf might be already occupied, that's why we have data_off inside rte_mbuf, etc. > It should be mbuf->buf_len - ofs. No, it should be: len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; Otherwise PMD can overwrite memory beyond its buf_len.
> Hi Akhil, > > > > > On 9/21/2021 12:58 AM, Akhil Goyal wrote: > > > > >>> From: Gagandeep Singh <g.singh@nxp.com> > > > > >>> > > > > >>> The current crypto raw data vectors is extended to support > > > > >>> rte_security usecases, where we need total data length to know > > > > >>> how much additional memory space is available in buffer other > > > > >>> than data length so that driver/HW can write expanded size > > > > >>> data after encryption. > > > > >>> > > > > >>> Signed-off-by: Gagandeep Singh <g.singh@nxp.com> > > > > >>> Acked-by: Akhil Goyal <gakhil@marvell.com> > > > > >>> --- > > > > >>> lib/cryptodev/rte_crypto_sym.h | 6 ++++++ > > > > >>> 1 file changed, 6 insertions(+) > > > > >>> > > > > >>> diff --git a/lib/cryptodev/rte_crypto_sym.h > > > > >> b/lib/cryptodev/rte_crypto_sym.h > > > > >>> index dcc0bd5933..e5cef1fb72 100644 > > > > >>> --- a/lib/cryptodev/rte_crypto_sym.h > > > > >>> +++ b/lib/cryptodev/rte_crypto_sym.h > > > > >>> @@ -37,6 +37,8 @@ struct rte_crypto_vec { > > > > >>> rte_iova_t iova; > > > > >>> /** length of the data buffer */ > > > > >>> uint32_t len; > > > > >>> + /** total buffer length*/ > > > > >>> + uint32_t tot_len; > > > > >>> }; > > > > >>> > > > > >>> /** > > > > >>> @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct > > > rte_mbuf > > > > >> *mb, uint32_t ofs, uint32_t len, > > > > >>> seglen = mb->data_len - ofs; > > > > >>> if (len <= seglen) { > > > > >>> vec[0].len = len; > > > > >>> + vec[0].tot_len = mb->buf_len; > > > > >> That doesn't look right. > > > > >> We should take into a count mbuf headroom and input offset. > > > > >> Something like: > > > > >> vec[0].tot_len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; > > > > >> Same in other places below. > > > > >> > > > > > I believe the packet can expand into headroom based on the protocol > > > support. > > > > Yes, total length is representing the total buffer length available. The > > > > security protocol shall take care of the headroom and offsets. > > > > > > Hmm, and how it will now how many bytes are in head-room, and how > > > many are in tail-room? > > > We either need to provide values for both, or assume that only tail-room > is > > > available for the driver. > > I believe it should be starting point where output can be written till the end > of buffer. > > Right, that's: > base = rte_pktmbuf_mtod_offset(mb, void *, ofs); > > > There should not be any headroom and tailroom for raw buffers. > > I am not talking about raw buffers, what I am saying that some space in the > mbuf > might be already occupied, that's why we have data_off inside rte_mbuf, etc. > > > It should be mbuf->buf_len - ofs. > > No, it should be: > len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; > Otherwise PMD can overwrite memory beyond its buf_len. > @Hemant: Do you agree. Please send next version.
HI Akhil/Konstantin > > Hi Akhil, > > > > > > > On 9/21/2021 12:58 AM, Akhil Goyal wrote: > > > > > >>> From: Gagandeep Singh <g.singh@nxp.com> > > > > > >>> > > > > > >>> The current crypto raw data vectors is extended to support > > > > > >>> rte_security usecases, where we need total data length to > > > > > >>> know how much additional memory space is available in buffer > > > > > >>> other than data length so that driver/HW can write expanded > > > > > >>> size data after encryption. > > > > > >>> > > > > > >>> Signed-off-by: Gagandeep Singh <g.singh@nxp.com> > > > > > >>> Acked-by: Akhil Goyal <gakhil@marvell.com> > > > > > >>> --- > > > > > >>> lib/cryptodev/rte_crypto_sym.h | 6 ++++++ > > > > > >>> 1 file changed, 6 insertions(+) > > > > > >>> > > > > > >>> diff --git a/lib/cryptodev/rte_crypto_sym.h > > > > > >> b/lib/cryptodev/rte_crypto_sym.h > > > > > >>> index dcc0bd5933..e5cef1fb72 100644 > > > > > >>> --- a/lib/cryptodev/rte_crypto_sym.h > > > > > >>> +++ b/lib/cryptodev/rte_crypto_sym.h > > > > > >>> @@ -37,6 +37,8 @@ struct rte_crypto_vec { > > > > > >>> rte_iova_t iova; > > > > > >>> /** length of the data buffer */ > > > > > >>> uint32_t len; > > > > > >>> + /** total buffer length*/ > > > > > >>> + uint32_t tot_len; > > > > > >>> }; > > > > > >>> > > > > > >>> /** > > > > > >>> @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct > > > > rte_mbuf > > > > > >> *mb, uint32_t ofs, uint32_t len, > > > > > >>> seglen = mb->data_len - ofs; > > > > > >>> if (len <= seglen) { > > > > > >>> vec[0].len = len; > > > > > >>> + vec[0].tot_len = mb->buf_len; > > > > > >> That doesn't look right. > > > > > >> We should take into a count mbuf headroom and input offset. > > > > > >> Something like: > > > > > >> vec[0].tot_len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; > > > > > >> Same in other places below. > > > > > >> > > > > > > I believe the packet can expand into headroom based on the > > > > > > protocol > > > > support. > > > > > Yes, total length is representing the total buffer length > > > > > available. The security protocol shall take care of the headroom and > offsets. > > > > > > > > Hmm, and how it will now how many bytes are in head-room, and how > > > > many are in tail-room? > > > > We either need to provide values for both, or assume that only > > > > tail-room > > is > > > > available for the driver. > > > I believe it should be starting point where output can be written > > > till the end > > of buffer. > > > > Right, that's: > > base = rte_pktmbuf_mtod_offset(mb, void *, ofs); > > > > > There should not be any headroom and tailroom for raw buffers. > > > > I am not talking about raw buffers, what I am saying that some space > > in the mbuf might be already occupied, that's why we have data_off > > inside rte_mbuf, etc. > > > > > It should be mbuf->buf_len - ofs. > > > > No, it should be: > > len = mb->buf_len - rte_pktmbuf_headroom(m) - ofs; Otherwise PMD can > > overwrite memory beyond its buf_len. > > > @Hemant: Do you agree. Please send next version. [Hemant] [Hemant] Yes, I will send the new version
diff --git a/lib/cryptodev/rte_crypto_sym.h b/lib/cryptodev/rte_crypto_sym.h index dcc0bd5933..e5cef1fb72 100644 --- a/lib/cryptodev/rte_crypto_sym.h +++ b/lib/cryptodev/rte_crypto_sym.h @@ -37,6 +37,8 @@ struct rte_crypto_vec { rte_iova_t iova; /** length of the data buffer */ uint32_t len; + /** total buffer length*/ + uint32_t tot_len; }; /** @@ -980,12 +982,14 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len, seglen = mb->data_len - ofs; if (len <= seglen) { vec[0].len = len; + vec[0].tot_len = mb->buf_len; return 1; } /* data spread across segments */ vec[0].len = seglen; left = len - seglen; + vec[0].tot_len = mb->buf_len; for (i = 1, nseg = mb->next; nseg != NULL; nseg = nseg->next, i++) { vec[i].base = rte_pktmbuf_mtod(nseg, void *); @@ -995,6 +999,7 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len, if (left <= seglen) { /* whole requested data is completed */ vec[i].len = left; + vec[i].tot_len = mb->buf_len; left = 0; break; } @@ -1002,6 +1007,7 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len, /* use whole segment */ vec[i].len = seglen; left -= seglen; + vec[i].tot_len = mb->buf_len; } RTE_ASSERT(left == 0);