On 1/17/23 09:11, Jason Gunthorpe wrote:
> On Mon, Jan 16, 2023 at 05:56:01PM -0600, Bob Pearson wrote:
>
>> +/* only implemented for 64 bit architectures */
>> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
>> +{
>> +#if defined CONFIG_64BIT
>
> #ifdef
>
> It is a little more typical style to provide an alternate version of
> the function when #ifdefing
I will do that.
>
>> + u64 *va;
>> +
>> + /* See IBA oA19-28 */
>> + if (unlikely(mr->state != RXE_MR_STATE_VALID)) {
>> + rxe_dbg_mr(mr, "mr not in valid state");
>> + return -EINVAL;
>> + }
>> +
>> + va = iova_to_vaddr(mr, iova, sizeof(value));
>> + if (unlikely(!va)) {
>> + rxe_dbg_mr(mr, "iova out of range");
>> + return -ERANGE;
>> + }
>> +
>> + /* See IBA A19.4.2 */
>> + if (unlikely((uintptr_t)va & 0x7 || iova & 0x7)) {
>> + rxe_dbg_mr(mr, "misaligned address");
>> + return -RXE_ERR_NOT_ALIGNED;
>> + }
>> +
>> + /* Do atomic write after all prior operations have completed */
>> + smp_store_release(va, value);
>> +
>> + return 0;
>> +#else
>> + WARN_ON(1);
>> + return -EINVAL;
>> +#endif
>> +}
>> +
>> int advance_dma_data(struct rxe_dma_info *dma, unsigned int length)
>> {
>> struct rxe_sge *sge = &dma->sge[dma->cur_sge];
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index 1e38e5da1f4c..49298ff88d25 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -764,30 +764,40 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
>> return RESPST_ACKNOWLEDGE;
>> }
>>
>> -#ifdef CONFIG_64BIT
>> -static enum resp_states do_atomic_write(struct rxe_qp *qp,
>> - struct rxe_pkt_info *pkt)
>> +static enum resp_states atomic_write_reply(struct rxe_qp *qp,
>> + struct rxe_pkt_info *pkt)
>> {
>> - struct rxe_mr *mr = qp->resp.mr;
>> - int payload = payload_size(pkt);
>> - u64 src, *dst;
>> -
>> - if (mr->state != RXE_MR_STATE_VALID)
>> - return RESPST_ERR_RKEY_VIOLATION;
>> + struct resp_res *res = qp->resp.res;
>> + struct rxe_mr *mr;
>> + u64 value;
>> + u64 iova;
>> + int err;
>>
>> - memcpy(&src, payload_addr(pkt), payload);
>> + if (!res) {
>> + res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
>> + qp->resp.res = res;
>> + }
>>
>> - dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
>> - /* check vaddr is 8 bytes aligned. */
>> - if (!dst || (uintptr_t)dst & 7)
>> - return RESPST_ERR_MISALIGNED_ATOMIC;
>> + if (res->replay)
>> + return RESPST_ACKNOWLEDGE;
>>
>> - /* Do atomic write after all prior operations have completed */
>> - smp_store_release(dst, src);
>> + mr = qp->resp.mr;
>> + value = *(u64 *)payload_addr(pkt);
>> + iova = qp->resp.va + qp->resp.offset;
>>
>> - /* decrease resp.resid to zero */
>> - qp->resp.resid -= sizeof(payload);
>> +#if defined CONFIG_64BIT
>
> Shouldn't need a #ifdef here
This avoids a new special error (i.e. NOT_64_bit) and makes it clear we
won't call the code in mr.
I really don't understand why Fujitsu did it all this way instead of just
using a spinlock for 32 bit architectures as a fallback. But if I want to
keep to the spirit of their implementation this is fairly clear I think.
Bob
>
>> + err = rxe_mr_do_atomic_write(mr, iova, value);
>> + if (unlikely(err)) {
>> + if (err == -RXE_ERR_NOT_ALIGNED)
>> + return RESPST_ERR_MISALIGNED_ATOMIC;
>> + else
>> + return RESPST_ERR_RKEY_VIOLATION;
>
> Again why not return the RESPST directly then the stub can return
> RESPST_ERR_UNSUPPORTED_OPCODE?
>
> Jason
