linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 for-next 0/2] RDMA/hns: Add supports for stash
@ 2020-11-20 10:17 Weihang Li
  2020-11-20 10:17 ` [PATCH v3 for-next 1/2] RDMA/hns: Add support for CQ stash Weihang Li
  2020-11-20 10:17 ` [PATCH v3 for-next 2/2] RDMA/hns: Add support for QP stash Weihang Li
  0 siblings, 2 replies; 5+ messages in thread
From: Weihang Li @ 2020-11-20 10:17 UTC (permalink / raw)
  To: dledford, jgg; +Cc: leon, linux-rdma, linuxarm

Stash is a mechanism that uses the core information carried by the ARM AXI
bus to access the L3 cache. The CPU and I/O subsystems can access the L3
cache consistently by enabling stash, so the performance can be improved.

Changes since v2:
- Replace hr_reg_set() with hr_reg_enable(), which supports to set a single
  bit.
link: https://patchwork.kernel.org/project/linux-rdma/cover/1605527919-48769-1-git-send-email-liweihang@huawei.com/

Changes since v1:
- Fix comments from Jason about the unused macros.
- Rewrite hr_reg_set() to make it easier for driver to define and set a
  field.
link: https://patchwork.kernel.org/project/linux-rdma/cover/1601458452-55263-1-git-send-email-liweihang@huawei.com/

Lang Cheng (2):
  RDMA/hns: Add support for CQ stash
  RDMA/hns: Add support for QP stash

 drivers/infiniband/hw/hns/hns_roce_common.h | 10 +++++++
 drivers/infiniband/hw/hns/hns_roce_device.h |  1 +
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  |  9 +++++++
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h  | 41 ++++++++++++++++++-----------
 4 files changed, 45 insertions(+), 16 deletions(-)

-- 
2.8.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 for-next 1/2] RDMA/hns: Add support for CQ stash
  2020-11-20 10:17 [PATCH v3 for-next 0/2] RDMA/hns: Add supports for stash Weihang Li
@ 2020-11-20 10:17 ` Weihang Li
  2020-11-20 20:13   ` Jason Gunthorpe
  2020-11-20 10:17 ` [PATCH v3 for-next 2/2] RDMA/hns: Add support for QP stash Weihang Li
  1 sibling, 1 reply; 5+ messages in thread
From: Weihang Li @ 2020-11-20 10:17 UTC (permalink / raw)
  To: dledford, jgg; +Cc: leon, linux-rdma, linuxarm

From: Lang Cheng <chenglang@huawei.com>

Stash is a mechanism that uses the core information carried by the ARM AXI
bus to access the L3 cache. It can be used to improve the performance by
increasing the hit ratio of L3 cache. CQs need to enable stash by default.

Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_common.h | 10 ++++++++
 drivers/infiniband/hw/hns/hns_roce_device.h |  1 +
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  |  3 +++
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h  | 39 +++++++++++++++++------------
 4 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h b/drivers/infiniband/hw/hns/hns_roce_common.h
index f5669ff..41a2252 100644
--- a/drivers/infiniband/hw/hns/hns_roce_common.h
+++ b/drivers/infiniband/hw/hns/hns_roce_common.h
@@ -53,6 +53,16 @@
 #define roce_set_bit(origin, shift, val) \
 	roce_set_field((origin), (1ul << (shift)), (shift), (val))
 
+#define FIELD_LOC(field_h, field_l) field_h, field_l
+
+#define _hr_reg_enable(arr, field_h, field_l)                                  \
+	(arr)[(field_l) / 32] |=                                               \
+		(BIT((field_l) % 32)) +                                        \
+		BUILD_BUG_ON_ZERO((field_h) != (field_l)) +                    \
+		BUILD_BUG_ON_ZERO((field_l) / 32 >= ARRAY_SIZE(arr))
+
+#define hr_reg_enable(arr, field) _hr_reg_enable(arr, field)
+
 #define ROCEE_GLB_CFG_ROCEE_DB_SQ_MODE_S 3
 #define ROCEE_GLB_CFG_ROCEE_DB_OTH_MODE_S 4
 
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index 1d99022..ab7df8e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -223,6 +223,7 @@ enum {
 	HNS_ROCE_CAP_FLAG_QP_FLOW_CTRL		= BIT(9),
 	HNS_ROCE_CAP_FLAG_ATOMIC		= BIT(10),
 	HNS_ROCE_CAP_FLAG_SDI_MODE		= BIT(14),
+	HNS_ROCE_CAP_FLAG_STASH			= BIT(17),
 };
 
 #define HNS_ROCE_DB_TYPE_COUNT			2
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 4b82912..da7f909 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -3177,6 +3177,9 @@ static void hns_roce_v2_write_cqc(struct hns_roce_dev *hr_dev,
 		       V2_CQC_BYTE_8_CQE_SIZE_S, hr_cq->cqe_size ==
 		       HNS_ROCE_V3_CQE_SIZE ? 1 : 0);
 
+	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_STASH)
+		hr_reg_enable(cq_context->raw, CQC_STASH);
+
 	cq_context->cqe_cur_blk_addr = cpu_to_le32(to_hr_hw_page_addr(mtts[0]));
 
 	roce_set_field(cq_context->byte_16_hop_addr,
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
index 1409d05..50a5187 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
@@ -267,22 +267,27 @@ enum hns_roce_sgid_type {
 };
 
 struct hns_roce_v2_cq_context {
-	__le32	byte_4_pg_ceqn;
-	__le32	byte_8_cqn;
-	__le32	cqe_cur_blk_addr;
-	__le32	byte_16_hop_addr;
-	__le32	cqe_nxt_blk_addr;
-	__le32	byte_24_pgsz_addr;
-	__le32	byte_28_cq_pi;
-	__le32	byte_32_cq_ci;
-	__le32	cqe_ba;
-	__le32	byte_40_cqe_ba;
-	__le32	byte_44_db_record;
-	__le32	db_record_addr;
-	__le32	byte_52_cqe_cnt;
-	__le32	byte_56_cqe_period_maxcnt;
-	__le32	cqe_report_timer;
-	__le32	byte_64_se_cqe_idx;
+	union {
+		struct {
+			__le32 byte_4_pg_ceqn;
+			__le32 byte_8_cqn;
+			__le32 cqe_cur_blk_addr;
+			__le32 byte_16_hop_addr;
+			__le32 cqe_nxt_blk_addr;
+			__le32 byte_24_pgsz_addr;
+			__le32 byte_28_cq_pi;
+			__le32 byte_32_cq_ci;
+			__le32 cqe_ba;
+			__le32 byte_40_cqe_ba;
+			__le32 byte_44_db_record;
+			__le32 db_record_addr;
+			__le32 byte_52_cqe_cnt;
+			__le32 byte_56_cqe_period_maxcnt;
+			__le32 cqe_report_timer;
+			__le32 byte_64_se_cqe_idx;
+		};
+		__le32 raw[16];
+	};
 };
 #define HNS_ROCE_V2_CQ_DEFAULT_BURST_NUM 0x0
 #define HNS_ROCE_V2_CQ_DEFAULT_INTERVAL	0x0
@@ -360,6 +365,8 @@ struct hns_roce_v2_cq_context {
 #define	V2_CQC_BYTE_64_SE_CQE_IDX_S 0
 #define	V2_CQC_BYTE_64_SE_CQE_IDX_M GENMASK(23, 0)
 
+#define CQC_STASH FIELD_LOC(63, 63)
+
 struct hns_roce_srq_context {
 	__le32	byte_4_srqn_srqst;
 	__le32	byte_8_limit_wl;
-- 
2.8.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v3 for-next 2/2] RDMA/hns: Add support for QP stash
  2020-11-20 10:17 [PATCH v3 for-next 0/2] RDMA/hns: Add supports for stash Weihang Li
  2020-11-20 10:17 ` [PATCH v3 for-next 1/2] RDMA/hns: Add support for CQ stash Weihang Li
@ 2020-11-20 10:17 ` Weihang Li
  1 sibling, 0 replies; 5+ messages in thread
From: Weihang Li @ 2020-11-20 10:17 UTC (permalink / raw)
  To: dledford, jgg; +Cc: leon, linux-rdma, linuxarm

From: Lang Cheng <chenglang@huawei.com>

Stash is a mechanism that uses the core information carried by the ARM AXI
bus to access the L3 cache. It can be used to improve the performance by
increasing the hit ratio of L3 cache. QPs need to enable stash by default.

Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 6 ++++++
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index da7f909..00c67d6 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -3986,6 +3986,12 @@ static void modify_qp_reset_to_init(struct ib_qp *ibqp,
 	hr_qp->access_flags = attr->qp_access_flags;
 	roce_set_field(context->byte_252_err_txcqn, V2_QPC_BYTE_252_TX_CQN_M,
 		       V2_QPC_BYTE_252_TX_CQN_S, to_hr_cq(ibqp->send_cq)->cqn);
+
+	if (hr_dev->caps.qpc_sz < HNS_ROCE_V3_QPC_SZ)
+		return;
+
+	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_STASH)
+		hr_reg_enable(context->ext, QPCEX_STASH);
 }
 
 static void modify_qp_init_to_init(struct ib_qp *ibqp,
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
index 50a5187..4679afb 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
@@ -898,6 +898,8 @@ struct hns_roce_v2_qp_context {
 #define	V2_QPC_BYTE_256_SQ_FLUSH_IDX_S 16
 #define V2_QPC_BYTE_256_SQ_FLUSH_IDX_M GENMASK(31, 16)
 
+#define QPCEX_STASH FIELD_LOC(82, 82)
+
 #define	V2_QP_RWE_S 1 /* rdma write enable */
 #define	V2_QP_RRE_S 2 /* rdma read enable */
 #define	V2_QP_ATE_S 3 /* rdma atomic enable */
-- 
2.8.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 for-next 1/2] RDMA/hns: Add support for CQ stash
  2020-11-20 10:17 ` [PATCH v3 for-next 1/2] RDMA/hns: Add support for CQ stash Weihang Li
@ 2020-11-20 20:13   ` Jason Gunthorpe
  2020-11-26  6:56     ` liweihang
  0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2020-11-20 20:13 UTC (permalink / raw)
  To: Weihang Li; +Cc: dledford, leon, linux-rdma, linuxarm

On Fri, Nov 20, 2020 at 06:17:19PM +0800, Weihang Li wrote:
> From: Lang Cheng <chenglang@huawei.com>
> 
> Stash is a mechanism that uses the core information carried by the ARM AXI
> bus to access the L3 cache. It can be used to improve the performance by
> increasing the hit ratio of L3 cache. CQs need to enable stash by default.
> 
> Signed-off-by: Lang Cheng <chenglang@huawei.com>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
>  drivers/infiniband/hw/hns/hns_roce_common.h | 10 ++++++++
>  drivers/infiniband/hw/hns/hns_roce_device.h |  1 +
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c  |  3 +++
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.h  | 39 +++++++++++++++++------------
>  4 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h b/drivers/infiniband/hw/hns/hns_roce_common.h
> index f5669ff..41a2252 100644
> +++ b/drivers/infiniband/hw/hns/hns_roce_common.h
> @@ -53,6 +53,16 @@
>  #define roce_set_bit(origin, shift, val) \
>  	roce_set_field((origin), (1ul << (shift)), (shift), (val))
>  
> +#define FIELD_LOC(field_h, field_l) field_h, field_l
> +
> +#define _hr_reg_enable(arr, field_h, field_l)                                  \
> +	(arr)[(field_l) / 32] |=                                               \
> +		(BIT((field_l) % 32)) +                                        \
> +		BUILD_BUG_ON_ZERO((field_h) != (field_l)) +                    \
> +		BUILD_BUG_ON_ZERO((field_l) / 32 >= ARRAY_SIZE(arr))
> +
> +#define hr_reg_enable(arr, field) _hr_reg_enable(arr, field)
> +
>  #define ROCEE_GLB_CFG_ROCEE_DB_SQ_MODE_S 3
>  #define ROCEE_GLB_CFG_ROCEE_DB_OTH_MODE_S 4
>  
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
> index 1d99022..ab7df8e 100644
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -223,6 +223,7 @@ enum {
>  	HNS_ROCE_CAP_FLAG_QP_FLOW_CTRL		= BIT(9),
>  	HNS_ROCE_CAP_FLAG_ATOMIC		= BIT(10),
>  	HNS_ROCE_CAP_FLAG_SDI_MODE		= BIT(14),
> +	HNS_ROCE_CAP_FLAG_STASH			= BIT(17),
>  };
>  
>  #define HNS_ROCE_DB_TYPE_COUNT			2
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> index 4b82912..da7f909 100644
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> @@ -3177,6 +3177,9 @@ static void hns_roce_v2_write_cqc(struct hns_roce_dev *hr_dev,
>  		       V2_CQC_BYTE_8_CQE_SIZE_S, hr_cq->cqe_size ==
>  		       HNS_ROCE_V3_CQE_SIZE ? 1 : 0);
>  
> +	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_STASH)
> +		hr_reg_enable(cq_context->raw, CQC_STASH);
> +
>  	cq_context->cqe_cur_blk_addr = cpu_to_le32(to_hr_hw_page_addr(mtts[0]));
>  
>  	roce_set_field(cq_context->byte_16_hop_addr,
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
> index 1409d05..50a5187 100644
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
> @@ -267,22 +267,27 @@ enum hns_roce_sgid_type {
>  };
>  
>  struct hns_roce_v2_cq_context {
> -	__le32	byte_4_pg_ceqn;
> -	__le32	byte_8_cqn;
> -	__le32	cqe_cur_blk_addr;
> -	__le32	byte_16_hop_addr;
> -	__le32	cqe_nxt_blk_addr;
> -	__le32	byte_24_pgsz_addr;
> -	__le32	byte_28_cq_pi;
> -	__le32	byte_32_cq_ci;
> -	__le32	cqe_ba;
> -	__le32	byte_40_cqe_ba;
> -	__le32	byte_44_db_record;
> -	__le32	db_record_addr;
> -	__le32	byte_52_cqe_cnt;
> -	__le32	byte_56_cqe_period_maxcnt;
> -	__le32	cqe_report_timer;
> -	__le32	byte_64_se_cqe_idx;
> +	union {
> +		struct {
> +			__le32 byte_4_pg_ceqn;
> +			__le32 byte_8_cqn;
> +			__le32 cqe_cur_blk_addr;
> +			__le32 byte_16_hop_addr;
> +			__le32 cqe_nxt_blk_addr;
> +			__le32 byte_24_pgsz_addr;
> +			__le32 byte_28_cq_pi;
> +			__le32 byte_32_cq_ci;
> +			__le32 cqe_ba;
> +			__le32 byte_40_cqe_ba;
> +			__le32 byte_44_db_record;
> +			__le32 db_record_addr;
> +			__le32 byte_52_cqe_cnt;
> +			__le32 byte_56_cqe_period_maxcnt;
> +			__le32 cqe_report_timer;
> +			__le32 byte_64_se_cqe_idx;
> +		};
> +		__le32 raw[16];
> +	};

It has missed the point of how the FIELD_LOC worked in the iba macros,
you want to specify the type

  FIELD_LOC(struct hns_roce_v2_cq_context, 63, 63)

And not introduce a raw array in a union, just validate the type and
cast it to a __le32 *

And again, if you are going to be building macros like this then
setting fields to all ones must be the corner case.

Write it more clearly:

hr_reg_set(cq_context, CQC_STASH, FIELD_ALL_ONES(CQC_STASH));

Now you can replace some of the other macros with the safer/simpler
scheme.

Jason

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 for-next 1/2] RDMA/hns: Add support for CQ stash
  2020-11-20 20:13   ` Jason Gunthorpe
@ 2020-11-26  6:56     ` liweihang
  0 siblings, 0 replies; 5+ messages in thread
From: liweihang @ 2020-11-26  6:56 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: dledford, leon, linux-rdma, Linuxarm

On 2020/11/21 4:13, Jason Gunthorpe wrote:
> On Fri, Nov 20, 2020 at 06:17:19PM +0800, Weihang Li wrote:
>> From: Lang Cheng <chenglang@huawei.com>
>>
>> Stash is a mechanism that uses the core information carried by the ARM AXI
>> bus to access the L3 cache. It can be used to improve the performance by
>> increasing the hit ratio of L3 cache. CQs need to enable stash by default.
>>
>> Signed-off-by: Lang Cheng <chenglang@huawei.com>
>> Signed-off-by: Weihang Li <liweihang@huawei.com>
>>  drivers/infiniband/hw/hns/hns_roce_common.h | 10 ++++++++
>>  drivers/infiniband/hw/hns/hns_roce_device.h |  1 +
>>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c  |  3 +++
>>  drivers/infiniband/hw/hns/hns_roce_hw_v2.h  | 39 +++++++++++++++++------------
>>  4 files changed, 37 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h b/drivers/infiniband/hw/hns/hns_roce_common.h
>> index f5669ff..41a2252 100644
>> +++ b/drivers/infiniband/hw/hns/hns_roce_common.h
>> @@ -53,6 +53,16 @@
>>  #define roce_set_bit(origin, shift, val) \
>>  	roce_set_field((origin), (1ul << (shift)), (shift), (val))
>>  
>> +#define FIELD_LOC(field_h, field_l) field_h, field_l
>> +
>> +#define _hr_reg_enable(arr, field_h, field_l)                                  \
>> +	(arr)[(field_l) / 32] |=                                               \
>> +		(BIT((field_l) % 32)) +                                        \
>> +		BUILD_BUG_ON_ZERO((field_h) != (field_l)) +                    \
>> +		BUILD_BUG_ON_ZERO((field_l) / 32 >= ARRAY_SIZE(arr))
>> +
>> +#define hr_reg_enable(arr, field) _hr_reg_enable(arr, field)
>> +
>>  #define ROCEE_GLB_CFG_ROCEE_DB_SQ_MODE_S 3
>>  #define ROCEE_GLB_CFG_ROCEE_DB_OTH_MODE_S 4
>>  
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
>> index 1d99022..ab7df8e 100644
>> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
>> @@ -223,6 +223,7 @@ enum {
>>  	HNS_ROCE_CAP_FLAG_QP_FLOW_CTRL		= BIT(9),
>>  	HNS_ROCE_CAP_FLAG_ATOMIC		= BIT(10),
>>  	HNS_ROCE_CAP_FLAG_SDI_MODE		= BIT(14),
>> +	HNS_ROCE_CAP_FLAG_STASH			= BIT(17),
>>  };
>>  
>>  #define HNS_ROCE_DB_TYPE_COUNT			2
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> index 4b82912..da7f909 100644
>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> @@ -3177,6 +3177,9 @@ static void hns_roce_v2_write_cqc(struct hns_roce_dev *hr_dev,
>>  		       V2_CQC_BYTE_8_CQE_SIZE_S, hr_cq->cqe_size ==
>>  		       HNS_ROCE_V3_CQE_SIZE ? 1 : 0);
>>  
>> +	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_STASH)
>> +		hr_reg_enable(cq_context->raw, CQC_STASH);
>> +
>>  	cq_context->cqe_cur_blk_addr = cpu_to_le32(to_hr_hw_page_addr(mtts[0]));
>>  
>>  	roce_set_field(cq_context->byte_16_hop_addr,
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
>> index 1409d05..50a5187 100644
>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
>> @@ -267,22 +267,27 @@ enum hns_roce_sgid_type {
>>  };
>>  
>>  struct hns_roce_v2_cq_context {
>> -	__le32	byte_4_pg_ceqn;
>> -	__le32	byte_8_cqn;
>> -	__le32	cqe_cur_blk_addr;
>> -	__le32	byte_16_hop_addr;
>> -	__le32	cqe_nxt_blk_addr;
>> -	__le32	byte_24_pgsz_addr;
>> -	__le32	byte_28_cq_pi;
>> -	__le32	byte_32_cq_ci;
>> -	__le32	cqe_ba;
>> -	__le32	byte_40_cqe_ba;
>> -	__le32	byte_44_db_record;
>> -	__le32	db_record_addr;
>> -	__le32	byte_52_cqe_cnt;
>> -	__le32	byte_56_cqe_period_maxcnt;
>> -	__le32	cqe_report_timer;
>> -	__le32	byte_64_se_cqe_idx;
>> +	union {
>> +		struct {
>> +			__le32 byte_4_pg_ceqn;
>> +			__le32 byte_8_cqn;
>> +			__le32 cqe_cur_blk_addr;
>> +			__le32 byte_16_hop_addr;
>> +			__le32 cqe_nxt_blk_addr;
>> +			__le32 byte_24_pgsz_addr;
>> +			__le32 byte_28_cq_pi;
>> +			__le32 byte_32_cq_ci;
>> +			__le32 cqe_ba;
>> +			__le32 byte_40_cqe_ba;
>> +			__le32 byte_44_db_record;
>> +			__le32 db_record_addr;
>> +			__le32 byte_52_cqe_cnt;
>> +			__le32 byte_56_cqe_period_maxcnt;
>> +			__le32 cqe_report_timer;
>> +			__le32 byte_64_se_cqe_idx;
>> +		};
>> +		__le32 raw[16];
>> +	};
> 
> It has missed the point of how the FIELD_LOC worked in the iba macros,
> you want to specify the type
> 
>   FIELD_LOC(struct hns_roce_v2_cq_context, 63, 63)
> 
> And not introduce a raw array in a union, just validate the type and
> cast it to a __le32 *
> 

Thank you, we will modify it as you suggest.

> And again, if you are going to be building macros like this then
> setting fields to all ones must be the corner case.
> 
> Write it more clearly:
> 
> hr_reg_set(cq_context, CQC_STASH, FIELD_ALL_ONES(CQC_STASH));
> 

OK, we will use hr_reg_enable(ptr, field) to set a single bit to one. And
use hr_reg_write(ptr, field, val) to fill a field with a value, which can be
also used like hr_reg_write(ptr, field, FIELD_ALL_ONES(field)).

Weihang

> Now you can replace some of the other macros with the safer/simpler
> scheme.
> 
> Jason
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-26  6:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 10:17 [PATCH v3 for-next 0/2] RDMA/hns: Add supports for stash Weihang Li
2020-11-20 10:17 ` [PATCH v3 for-next 1/2] RDMA/hns: Add support for CQ stash Weihang Li
2020-11-20 20:13   ` Jason Gunthorpe
2020-11-26  6:56     ` liweihang
2020-11-20 10:17 ` [PATCH v3 for-next 2/2] RDMA/hns: Add support for QP stash Weihang Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).