Re: [Qemu-devel] [PATCH v4] spapr-vscsi: add task management

[Paolo Bonzini]

Il 26/07/2013 12:25, Alexey Kardashevskiy ha scritto:
> On 07/26/2013 07:34 PM, Paolo Bonzini wrote:
>> Il 26/07/2013 11:20, Alexey Kardashevskiy ha scritto:
>>> -    return !fn;
>>> +
>>> +    vscsi_send_rsp(s, req, resp, 0, 0);
>>
>> We're not there yet, but getting closer...
> 
> Oh. Right. Sorry and thanks (ohci + ehci + vscsi at the same time and my
> mind melted :) )
> 
> Is this better?
> 
> -    vscsi_send_rsp(s, req, resp, 0, 0);
> +    memset(iu, 0, sizeof(struct srp_rsp) + 4);
> +    iu->srp.rsp.opcode = SRP_RSP;
> +    iu->srp.rsp.req_lim_delta = cpu_to_be32(1);
> +    iu->srp.rsp.tag = tag;
> +    iu->srp.rsp.flags |= SRP_RSP_FLAG_RSPVALID;
> +    iu->srp.rsp.resp_data_len = cpu_to_be32(4);
> +    iu->srp.rsp.sol_not = (sol_not & 0x02) >> 1;
> +    iu->srp.rsp.status = GOOD;
> +    iu->srp.rsp.data[3] = resp;
> +
> +    vscsi_send_iu(s, req, sizeof(iu->srp.rsp) + 4, VIOSRP_SRP_FORMAT);
> 
> And it seems I found a bug in vscsi_send_rsp() (sol_not is always zero), heh.

I cannot see the definition of sol_not, here is what the spec says though:

The solicited notification (SOLNT) bit indicates whether the SRP
initiator port specified normal or solicited message reception
notification for this response. If the STATUS field is non-zero or if
the RSP_CODE field is present and non-zero, then the SOLNT bit shall
contain the value that was specified in the UCSOLNT bit of the
corresponding SRP_CMD or SRP_TSK_MGMT request; otherwise, the SOLNT bit
shall contain the value that was specified in the SCSOLNT bit of the
corresponding SRP_CMD or SRP_TSK_MGMT request.

So it must be bit 1 (SCSOLNT) or bit 2 (UCSOLNT) depending on whether
resp is zero or non-zero (respectively).

Paolo

> 
> 
>> These response codes are _not_ SCSI status codes that go in the
>> SRP_RSP's status field.  They go (in the error case) in the "response
>> data" field, so you have to set RSPVALID and set the first four bytes of
>> the data[] array in the format that I mentioned before.
>>
>> The status field must be zero, because the spec says: "Response data
>> shall not be provided in any SRP_RSP response that returns a non-zero
>> status code in the STATUS field".
>>
>> Paolo
>>
>>> +    return 1;
>>>  }
>>>  
>>>  static int vscsi_handle_srp_req(VSCSIState *s, vscsi_req *req)
>>> diff --git a/hw/scsi/srp.h b/hw/scsi/srp.h
>>> index 5e0cad5..d27f31d 100644
>>> --- a/hw/scsi/srp.h
>>> +++ b/hw/scsi/srp.h
>>> @@ -90,6 +90,13 @@ enum {
>>>      SRP_REV16A_IB_IO_CLASS = 0x0100
>>>  };
>>>  
>>> +enum {
>>> +    SRP_TSK_MGMT_COMPLETE       = 0x00,
>>> +    SRP_TSK_MGMT_FIELDS_INVALID = 0x02,
>>> +    SRP_TSK_MGMT_NOT_SUPPORTED  = 0x04,
>>> +    SRP_TSK_MGMT_FAILED         = 0x05
>>> +};
>>> +
>>>  struct srp_direct_buf {
>>>      uint64_t    va;
>>>      uint32_t    key;
>>>
>>
> 
>