| Message ID | 20200520070415.3392210-1-jeffrey.t.kirsher@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Intel RDMA Driver Updates 2020-05-19 | expand |
On Wed, May 20, 2020 at 12:03:59AM -0700, Jeff Kirsher wrote: > This patch set adds a unified Intel Ethernet Protocol Driver for RDMA that > supports a new network device E810 (iWARP and RoCEv2 capable) and the > existing X722 iWARP device. The driver architecture provides the extensibility > for future generations of Intel HW supporting RDMA. > > This driver replaces the legacy X722 driver i40iw and extends the ABI already > defined for i40iw. It is backward compatible with legacy X722 rdma-core > provider (libi40iw). > > This series was built against the rdma for-next branch. This series is > dependant upon the v4 100GbE Intel Wired LAN Driver Updates 2020-05-19 > 12 patch series, which adds virtual_bus interface and ice/i40e LAN > driver changes. > > v5-->v6: > *Convert irdma destroy QP to a synchronous API > *Drop HMC obj macros for use counts like IRDMA_INC_SD_REFCNT et al. > *cleanup unneccesary 'mem' variable in irdma_create_qp > *cleanup unused headers such as linux/moduleparam.h et. al > *set kernel_ver in irdma_ualloc_resp struct to current ABI ver. Placeholder to > support user-space compatbility checks in future > *GENMASK/FIELD_PREP scheme to set WQE descriptor fields considered for irdma > driver but decision to drop. The FIELD_PREP macro cannot be used on the device > bitfield mask array maintained for common WQE descriptors and initialized > based on HW generation. The macro expects compile time constants > only. The request was to use GENMASK for the #define constants. If you move to a code environment then the spot the constant appears in the C code should be FIELD_PREP'd into the something dynamic code can use. Jason
> Subject: Re: [RDMA RFC v6 00/16] Intel RDMA Driver Updates 2020-05-19 > > On Wed, May 20, 2020 at 12:03:59AM -0700, Jeff Kirsher wrote: > > This patch set adds a unified Intel Ethernet Protocol Driver for RDMA > > that supports a new network device E810 (iWARP and RoCEv2 capable) and > > the existing X722 iWARP device. The driver architecture provides the > > extensibility for future generations of Intel HW supporting RDMA. > > > > This driver replaces the legacy X722 driver i40iw and extends the ABI > > already defined for i40iw. It is backward compatible with legacy X722 > > rdma-core provider (libi40iw). > > > > This series was built against the rdma for-next branch. This series > > is dependant upon the v4 100GbE Intel Wired LAN Driver Updates > > 2020-05-19 > > 12 patch series, which adds virtual_bus interface and ice/i40e LAN > > driver changes. > > > > v5-->v6: > > *Convert irdma destroy QP to a synchronous API *Drop HMC obj macros > > for use counts like IRDMA_INC_SD_REFCNT et al. > > *cleanup unneccesary 'mem' variable in irdma_create_qp *cleanup unused > > headers such as linux/moduleparam.h et. al *set kernel_ver in > > irdma_ualloc_resp struct to current ABI ver. Placeholder to support > > user-space compatbility checks in future *GENMASK/FIELD_PREP scheme to > > set WQE descriptor fields considered for irdma driver but decision to > > drop. The FIELD_PREP macro cannot be used on the device bitfield mask > > array maintained for common WQE descriptors and initialized based on > > HW generation. The macro expects compile time constants only. > > The request was to use GENMASK for the #define constants. If you move to a > code environment then the spot the constant appears in the C code should be > FIELD_PREP'd into the something dynamic code can use. > Maybe I am missing something here, but from what I understood, the vantage point of using GENMASK for the masks was so that we could get rid of open coding the shift constants and use the FIELD_PREP macro to place the value in the field of a descriptor. This should work for the static masks. So something like -- -#define IRDMA_UDA_QPSQ_INLINEDATALEN_S 48 -#define IRDMA_UDA_QPSQ_INLINEDATALEN_M \ - ((u64)0xff << IRDMA_UDA_QPSQ_INLINEDATALEN_S) +#define IRDMA_UDA_QPSQ_INLINEDATALEN_M GENMASK_ULL(55, 48) -#define LS_64(val, field) (((u64)(val) << field ## _S) & (field ## _M)) +#define LS_64(val, field) (FIELD_PREP(val,(field ## _M))) However we have device's dynamically computed bitfield mask array and shifts for some WQE descriptor fields -- see icrdma_init_hw.c https://lore.kernel.org/linux-rdma/20200520070415.3392210-3-jeffrey.t.kirsher@intel.com/#Z30drivers:infiniband:hw:irdma:icrdma_hw.c we still need to use the custom macro FLD_LS_64 without FIELD_PREP in this case as FIELD_PREP expects compile time constants. +#define FLD_LS_64(dev, val, field) \ + (((u64)(val) << (dev)->hw_shifts[field ## _S]) & (dev)->hw_masks[field +## _M]) And the shifts are still required for these fields which causes a bit of inconsistency
On Wed, May 27, 2020 at 01:58:01AM +0000, Saleem, Shiraz wrote: > > Subject: Re: [RDMA RFC v6 00/16] Intel RDMA Driver Updates 2020-05-19 > > > > On Wed, May 20, 2020 at 12:03:59AM -0700, Jeff Kirsher wrote: > > > This patch set adds a unified Intel Ethernet Protocol Driver for RDMA > > > that supports a new network device E810 (iWARP and RoCEv2 capable) and > > > the existing X722 iWARP device. The driver architecture provides the > > > extensibility for future generations of Intel HW supporting RDMA. > > > > > > This driver replaces the legacy X722 driver i40iw and extends the ABI > > > already defined for i40iw. It is backward compatible with legacy X722 > > > rdma-core provider (libi40iw). > > > > > > This series was built against the rdma for-next branch. This series > > > is dependant upon the v4 100GbE Intel Wired LAN Driver Updates > > > 2020-05-19 > > > 12 patch series, which adds virtual_bus interface and ice/i40e LAN > > > driver changes. > > > > > > v5-->v6: > > > *Convert irdma destroy QP to a synchronous API *Drop HMC obj macros > > > for use counts like IRDMA_INC_SD_REFCNT et al. > > > *cleanup unneccesary 'mem' variable in irdma_create_qp *cleanup unused > > > headers such as linux/moduleparam.h et. al *set kernel_ver in > > > irdma_ualloc_resp struct to current ABI ver. Placeholder to support > > > user-space compatbility checks in future *GENMASK/FIELD_PREP scheme to > > > set WQE descriptor fields considered for irdma driver but decision to > > > drop. The FIELD_PREP macro cannot be used on the device bitfield mask > > > array maintained for common WQE descriptors and initialized based on > > > HW generation. The macro expects compile time constants only. > > > > The request was to use GENMASK for the #define constants. If you move to a > > code environment then the spot the constant appears in the C code should be > > FIELD_PREP'd into the something dynamic code can use. > > > > Maybe I am missing something here, but from what I understood, > the vantage point of using GENMASK for the masks > was so that we could get rid of open coding the shift constants and use the > FIELD_PREP macro to place the value in the field of a descriptor. > This should work for the static masks. So something like -- > > -#define IRDMA_UDA_QPSQ_INLINEDATALEN_S 48 > -#define IRDMA_UDA_QPSQ_INLINEDATALEN_M \ > - ((u64)0xff << IRDMA_UDA_QPSQ_INLINEDATALEN_S) > +#define IRDMA_UDA_QPSQ_INLINEDATALEN_M GENMASK_ULL(55, 48) > > -#define LS_64(val, field) (((u64)(val) << field ## _S) & (field ## _M)) > +#define LS_64(val, field) (FIELD_PREP(val,(field ## _M))) ^^^^ it is not needed anymore > > However we have device's dynamically computed bitfield mask array and shifts > for some WQE descriptor fields -- > see icrdma_init_hw.c > https://lore.kernel.org/linux-rdma/20200520070415.3392210-3-jeffrey.t.kirsher@intel.com/#Z30drivers:infiniband:hw:irdma:icrdma_hw.c I'm looking on it and see static assignments, to by dynamic you will need "to play" with hw_shifts/hw_masks later, but you don't. What am I missing? + for (i = 0; i < IRDMA_MAX_SHIFTS; ++i) + dev->hw_shifts[i] = i40iw_shifts[i]; + + for (i = 0; i < IRDMA_MAX_MASKS; ++i) + dev->hw_masks[i] = i40iw_masks[i]; > > we still need to use the custom macro FLD_LS_64 without FIELD_PREP in this case > as FIELD_PREP expects compile time constants. > +#define FLD_LS_64(dev, val, field) \ > + (((u64)(val) << (dev)->hw_shifts[field ## _S]) & (dev)->hw_masks[field > +## _M]) > And the shifts are still required for these fields which causes a bit of > inconsistency > > >
> Subject: Re: [RDMA RFC v6 00/16] Intel RDMA Driver Updates 2020-05-19 > [......] > > I'm looking on it and see static assignments, to by dynamic you will need "to play" > with hw_shifts/hw_masks later, but you don't. What am I missing? > > + for (i = 0; i < IRDMA_MAX_SHIFTS; ++i) > + dev->hw_shifts[i] = i40iw_shifts[i]; > + > + for (i = 0; i < IRDMA_MAX_MASKS; ++i) > + dev->hw_masks[i] = i40iw_masks[i]; > > > > > we still need to use the custom macro FLD_LS_64 without FIELD_PREP in > > this case as FIELD_PREP expects compile time constants. > > +#define FLD_LS_64(dev, val, field) \ > > + (((u64)(val) << (dev)->hw_shifts[field ## _S]) & > > +(dev)->hw_masks[field ## _M]) > > And the shifts are still required for these fields which causes a bit > > of inconsistency > > The device hw_masks/hw_shifts array store masks/shifts of those descriptor fields that have same name across HW generations but differ in some attribute such as field width. Yes they are statically assigned, initialized with values from i40iw_masks and icrdma_masks, depending on the HW generation. We can even use GENMASK for the values in i40iw_masks[] , icrdma_masks[] but FIELD_PREP cant be used on dev->hw_masks[]
On Fri, May 29, 2020 at 03:21:05PM +0000, Saleem, Shiraz wrote: > > Subject: Re: [RDMA RFC v6 00/16] Intel RDMA Driver Updates 2020-05-19 > > > > [......] > > > > > I'm looking on it and see static assignments, to by dynamic you will need "to play" > > with hw_shifts/hw_masks later, but you don't. What am I missing? > > > > + for (i = 0; i < IRDMA_MAX_SHIFTS; ++i) > > + dev->hw_shifts[i] = i40iw_shifts[i]; > > + > > + for (i = 0; i < IRDMA_MAX_MASKS; ++i) > > + dev->hw_masks[i] = i40iw_masks[i]; > > > > > > > > we still need to use the custom macro FLD_LS_64 without FIELD_PREP in > > > this case as FIELD_PREP expects compile time constants. > > > +#define FLD_LS_64(dev, val, field) \ > > > + (((u64)(val) << (dev)->hw_shifts[field ## _S]) & > > > +(dev)->hw_masks[field ## _M]) > > > And the shifts are still required for these fields which causes a bit > > > of inconsistency > > > > > > The device hw_masks/hw_shifts array store masks/shifts of those > descriptor fields that have same name across HW generations but > differ in some attribute such as field width. Yes they are statically assigned, > initialized with values from i40iw_masks and icrdma_masks, depending on > the HW generation. We can even use GENMASK for the values in > i40iw_masks[] , icrdma_masks[] but FIELD_PREP cant be used on > dev->hw_masks[] So compute the shift and mask when building i40iw_shifts array using the compile time constant? Jason
> Subject: Re: [RDMA RFC v6 00/16] Intel RDMA Driver Updates 2020-05-19 > > On Fri, May 29, 2020 at 03:21:05PM +0000, Saleem, Shiraz wrote: > > > Subject: Re: [RDMA RFC v6 00/16] Intel RDMA Driver Updates > > > 2020-05-19 > > > > > > > [......] > > > > > > > > I'm looking on it and see static assignments, to by dynamic you will need "to > play" > > > with hw_shifts/hw_masks later, but you don't. What am I missing? > > > > > > + for (i = 0; i < IRDMA_MAX_SHIFTS; ++i) > > > + dev->hw_shifts[i] = i40iw_shifts[i]; > > > + > > > + for (i = 0; i < IRDMA_MAX_MASKS; ++i) > > > + dev->hw_masks[i] = i40iw_masks[i]; > > > > > > > > > > > we still need to use the custom macro FLD_LS_64 without FIELD_PREP > > > > in this case as FIELD_PREP expects compile time constants. > > > > +#define FLD_LS_64(dev, val, field) \ > > > > + (((u64)(val) << (dev)->hw_shifts[field ## _S]) & > > > > +(dev)->hw_masks[field ## _M]) > > > > And the shifts are still required for these fields which causes a > > > > bit of inconsistency > > > > > > > > > > The device hw_masks/hw_shifts array store masks/shifts of those > > descriptor fields that have same name across HW generations but differ > > in some attribute such as field width. Yes they are statically > > assigned, initialized with values from i40iw_masks and icrdma_masks, > > depending on the HW generation. We can even use GENMASK for the values > > in i40iw_masks[] , icrdma_masks[] but FIELD_PREP cant be used on > > dev->hw_masks[] > > So compute the shift and mask when building i40iw_shifts array using the compile > time constant? > i40iw_shifts[] and i40iw_mask[] are setup as compile constants and used to initialize dev->hw_masks[], dev->hw_shifts[] if the device is gen1. I still don't see how FIELD_PREP can be used on a value and dev->hw_masks[i].
On Tue, Jun 02, 2020 at 10:59:46PM +0000, Saleem, Shiraz wrote: > > Subject: Re: [RDMA RFC v6 00/16] Intel RDMA Driver Updates 2020-05-19 > > > > On Fri, May 29, 2020 at 03:21:05PM +0000, Saleem, Shiraz wrote: > > > > Subject: Re: [RDMA RFC v6 00/16] Intel RDMA Driver Updates > > > > 2020-05-19 > > > > > > > > > > [......] > > > > > > > > > > > I'm looking on it and see static assignments, to by dynamic you will need "to > > play" > > > > with hw_shifts/hw_masks later, but you don't. What am I missing? > > > > > > > > + for (i = 0; i < IRDMA_MAX_SHIFTS; ++i) > > > > + dev->hw_shifts[i] = i40iw_shifts[i]; > > > > + > > > > + for (i = 0; i < IRDMA_MAX_MASKS; ++i) > > > > + dev->hw_masks[i] = i40iw_masks[i]; > > > > > > > > > > > > > > we still need to use the custom macro FLD_LS_64 without FIELD_PREP > > > > > in this case as FIELD_PREP expects compile time constants. > > > > > +#define FLD_LS_64(dev, val, field) \ > > > > > + (((u64)(val) << (dev)->hw_shifts[field ## _S]) & > > > > > +(dev)->hw_masks[field ## _M]) > > > > > And the shifts are still required for these fields which causes a > > > > > bit of inconsistency > > > > > > > > > > > > > > The device hw_masks/hw_shifts array store masks/shifts of those > > > descriptor fields that have same name across HW generations but differ > > > in some attribute such as field width. Yes they are statically > > > assigned, initialized with values from i40iw_masks and icrdma_masks, > > > depending on the HW generation. We can even use GENMASK for the values > > > in i40iw_masks[] , icrdma_masks[] but FIELD_PREP cant be used on > > > dev->hw_masks[] > > > > So compute the shift and mask when building i40iw_shifts array using the compile > > time constant? > > > > i40iw_shifts[] and i40iw_mask[] are setup as compile constants > and used to initialize dev->hw_masks[], dev->hw_shifts[] if the device is gen1. > I still don't see how FIELD_PREP can be used on a value and > dev->hw_masks[i]. Well, you can't, you'd still have to use this indirection, the point was to make the #define macros consistent Jason