| Message ID | 20241122044059.20019-1-emil.s.tantilov@intel.com |
|---|---|
| State | Under Review |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [iwl-net,v2] idpf: add read memory barrier when checking descriptor done bit | expand |
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Emil Tantilov > Sent: Thursday, November 21, 2024 8:41 PM > To: intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Samudrala, Sridhar > <sridhar.samudrala@intel.com>; rlance@google.com; decot@google.com; > willemb@google.com; Hay, Joshua A <joshua.a.hay@intel.com>; Nguyen, > Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Lobakin, > Aleksander <aleksander.lobakin@intel.com> > Subject: [Intel-wired-lan] [PATCH iwl-net v2] idpf: add read memory barrier > when checking descriptor done bit > > 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 #1 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> > --- > Changelog > v2: > - Rewrote comment to fit on a single line > - Added new line as separator > - Updated last sentence in commit message to include load # > v1: > https://lore.kernel.org/intel-wired-lan/20241115021618.20565-1- > emil.s.tantilov@intel.com/ > --- > drivers/net/ethernet/intel/idpf/idpf_controlq.c | 6 ++++++ > 1 file changed, 6 insertions(+) > Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>
diff --git a/drivers/net/ethernet/intel/idpf/idpf_controlq.c b/drivers/net/ethernet/intel/idpf/idpf_controlq.c index 4849590a5591..b28991dd1870 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_controlq.c +++ b/drivers/net/ethernet/intel/idpf/idpf_controlq.c @@ -376,6 +376,9 @@ int idpf_ctlq_clean_sq(struct idpf_ctlq_info *cq, u16 *clean_count, if (!(le16_to_cpu(desc->flags) & IDPF_CTLQ_FLAG_DD)) break; + /* Ensure no other fields are read until DD flag is checked */ + dma_rmb(); + /* strip off FW internal code */ desc_err = le16_to_cpu(desc->ret_val) & 0xff; @@ -563,6 +566,9 @@ int idpf_ctlq_recv(struct idpf_ctlq_info *cq, u16 *num_q_msg, if (!(flags & IDPF_CTLQ_FLAG_DD)) break; + /* Ensure no other fields are read until DD flag is checked */ + dma_rmb(); + q_msg[i].vmvf_type = (flags & (IDPF_CTLQ_FLAG_FTYPE_VM | IDPF_CTLQ_FLAG_FTYPE_PF)) >>
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 #1 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> --- Changelog v2: - Rewrote comment to fit on a single line - Added new line as separator - Updated last sentence in commit message to include load # v1: https://lore.kernel.org/intel-wired-lan/20241115021618.20565-1-emil.s.tantilov@intel.com/ --- drivers/net/ethernet/intel/idpf/idpf_controlq.c | 6 ++++++ 1 file changed, 6 insertions(+)