| Message ID | 20241115021618.20565-1-emil.s.tantilov@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net] idpf: add read memory barrier when checking descriptor done bit | expand |
From: Emil Tantilov <emil.s.tantilov@intel.com> Date: Thu, 14 Nov 2024 18:16:18 -0800 > Add read memory barrier to ensure the order of operations when accessing > control queue descriptors. Specifically, we want to avoid cases where loads > can be reordered: > > 1. Load #1 is dispatched to read descriptor flags. > 2. Load #2 is dispatched to read some other field from the descriptor. > 3. Load #2 completes, accessing memory/cache at a point in time when the DD > flag is zero. > 4. NIC DMA overwrites the descriptor, now the DD flag is one. > 5. Any fields loaded before step 4 are now inconsistent with the actual > descriptor state. > > Add read memory barrier between steps 1 and 2, so that load #2 is not > executed until load has completed. > > Fixes: 8077c727561a ("idpf: add controlq init and reset checks") > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com> > Suggested-by: Lance Richardson <rlance@google.com> > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> > --- > drivers/net/ethernet/intel/idpf/idpf_controlq.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_controlq.c b/drivers/net/ethernet/intel/idpf/idpf_controlq.c > index 4849590a5591..61c7fafa54a1 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_controlq.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_controlq.c > @@ -375,6 +375,11 @@ int idpf_ctlq_clean_sq(struct idpf_ctlq_info *cq, u16 *clean_count, > desc = IDPF_CTLQ_DESC(cq, ntc); > if (!(le16_to_cpu(desc->flags) & IDPF_CTLQ_FLAG_DD)) > break; I'd put an empty newline here. > + /* > + * This barrier is needed to ensure that no other fields > + * are read until we check the DD flag. > + */ Are you sure you need to copy this comment all over the place? If so (I don't remember whether checkpatch complains on barriers with no comment), maybe we could make it more compact to not waste space? Like /* Make sure no other fields are read until DD is set */ 4x less lines, the same meaning. > + dma_rmb(); > > /* strip off FW internal code */ > desc_err = le16_to_cpu(desc->ret_val) & 0xff; > @@ -562,6 +567,11 @@ int idpf_ctlq_recv(struct idpf_ctlq_info *cq, u16 *num_q_msg, > > if (!(flags & IDPF_CTLQ_FLAG_DD)) > break; Same. > + /* > + * This barrier is needed to ensure that no other fields > + * are read until we check the DD flag. > + */ > + dma_rmb(); > > q_msg[i].vmvf_type = (flags & > (IDPF_CTLQ_FLAG_FTYPE_VM | Thanks, Olek
On 11/20/2024 8:29 AM, Alexander Lobakin wrote: > From: Emil Tantilov <emil.s.tantilov@intel.com> > Date: Thu, 14 Nov 2024 18:16:18 -0800 > >> Add read memory barrier to ensure the order of operations when accessing >> control queue descriptors. Specifically, we want to avoid cases where loads >> can be reordered: >> >> 1. Load #1 is dispatched to read descriptor flags. >> 2. Load #2 is dispatched to read some other field from the descriptor. >> 3. Load #2 completes, accessing memory/cache at a point in time when the DD >> flag is zero. >> 4. NIC DMA overwrites the descriptor, now the DD flag is one. >> 5. Any fields loaded before step 4 are now inconsistent with the actual >> descriptor state. >> >> Add read memory barrier between steps 1 and 2, so that load #2 is not >> executed until load has completed. >> >> Fixes: 8077c727561a ("idpf: add controlq init and reset checks") >> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com> >> Suggested-by: Lance Richardson <rlance@google.com> >> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> >> --- >> drivers/net/ethernet/intel/idpf/idpf_controlq.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/idpf/idpf_controlq.c b/drivers/net/ethernet/intel/idpf/idpf_controlq.c >> index 4849590a5591..61c7fafa54a1 100644 >> --- a/drivers/net/ethernet/intel/idpf/idpf_controlq.c >> +++ b/drivers/net/ethernet/intel/idpf/idpf_controlq.c >> @@ -375,6 +375,11 @@ int idpf_ctlq_clean_sq(struct idpf_ctlq_info *cq, u16 *clean_count, >> desc = IDPF_CTLQ_DESC(cq, ntc); >> if (!(le16_to_cpu(desc->flags) & IDPF_CTLQ_FLAG_DD)) >> break; > > I'd put an empty newline here. OK > >> + /* >> + * This barrier is needed to ensure that no other fields >> + * are read until we check the DD flag. >> + */ > > Are you sure you need to copy this comment all over the place? > If so (I don't remember whether checkpatch complains on barriers with no It does, though it does not seem to check specifically for the dma_ versions (bug?) I think its good practice to include it. > comment), maybe we could make it more compact to not waste space? > Like > > /* Make sure no other fields are read until DD is set */ > > 4x less lines, the same meaning. Not quite, since the barrier does not guarantee DD being set, but I can change it to: /* Make sure no other fields are read until DD is checked */ > >> + dma_rmb(); >> >> /* strip off FW internal code */ >> desc_err = le16_to_cpu(desc->ret_val) & 0xff; >> @@ -562,6 +567,11 @@ int idpf_ctlq_recv(struct idpf_ctlq_info *cq, u16 *num_q_msg, >> >> if (!(flags & IDPF_CTLQ_FLAG_DD)) >> break; > > Same. OK, will update in v2. Thanks, Emil > >> + /* >> + * This barrier is needed to ensure that no other fields >> + * are read until we check the DD flag. >> + */ >> + dma_rmb(); >> >> q_msg[i].vmvf_type = (flags & >> (IDPF_CTLQ_FLAG_FTYPE_VM | > > Thanks, > Olek
diff --git a/drivers/net/ethernet/intel/idpf/idpf_controlq.c b/drivers/net/ethernet/intel/idpf/idpf_controlq.c index 4849590a5591..61c7fafa54a1 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_controlq.c +++ b/drivers/net/ethernet/intel/idpf/idpf_controlq.c @@ -375,6 +375,11 @@ int idpf_ctlq_clean_sq(struct idpf_ctlq_info *cq, u16 *clean_count, desc = IDPF_CTLQ_DESC(cq, ntc); if (!(le16_to_cpu(desc->flags) & IDPF_CTLQ_FLAG_DD)) break; + /* + * This barrier is needed to ensure that no other fields + * are read until we check the DD flag. + */ + dma_rmb(); /* strip off FW internal code */ desc_err = le16_to_cpu(desc->ret_val) & 0xff; @@ -562,6 +567,11 @@ int idpf_ctlq_recv(struct idpf_ctlq_info *cq, u16 *num_q_msg, if (!(flags & IDPF_CTLQ_FLAG_DD)) break; + /* + * This barrier is needed to ensure that no other fields + * are read until we check the DD flag. + */ + dma_rmb(); q_msg[i].vmvf_type = (flags & (IDPF_CTLQ_FLAG_FTYPE_VM |
Add read memory barrier to ensure the order of operations when accessing control queue descriptors. Specifically, we want to avoid cases where loads can be reordered: 1. Load #1 is dispatched to read descriptor flags. 2. Load #2 is dispatched to read some other field from the descriptor. 3. Load #2 completes, accessing memory/cache at a point in time when the DD flag is zero. 4. NIC DMA overwrites the descriptor, now the DD flag is one. 5. Any fields loaded before step 4 are now inconsistent with the actual descriptor state. Add read memory barrier between steps 1 and 2, so that load #2 is not executed until load has completed. Fixes: 8077c727561a ("idpf: add controlq init and reset checks") Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com> Suggested-by: Lance Richardson <rlance@google.com> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> --- drivers/net/ethernet/intel/idpf/idpf_controlq.c | 10 ++++++++++ 1 file changed, 10 insertions(+)