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

[Paolo Bonzini]

Il 21/07/2013 23:34, Benjamin Herrenschmidt ha scritto:
> On Sun, 2013-07-21 at 20:04 +1000, Alexey Kardashevskiy wrote:
>> At the moment the guest kernel issues two types of task management requests
>> to the hypervisor - task about and lun reset. This adds handling for
>> these tasks.
> 
> My worry is that the specification calls for all of them, and we don't
> have ways to advertize that we support only a subset, so we might end
> up with backward compatibility problems with future clients.
> 
> Paolo, how did you generally test the task mgmnt implementation you did
> in virtio-scsi ? The set of tasks looks fairly similar

Yeah, it's all coming from the SCSI standard (SAM).

> (and similarly,
> itl looks like the Linux client will only use a couple of them).

I only tested abort task and LUN reset.  Luckily the code for the others
is very similar to these.

Paolo

>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
>>
>> I still do not have really good test to test the task management, any 
>> working ideas? :)
>>
>> This is made on top of "pseries: rework PAPR virtual SCSI" which some day 
>> may make it to upstream.
>>
>> ---
>>  hw/scsi/spapr_vscsi.c | 84 
>> +++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 64 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
>> index a86199b..6b90c9c 100644
>> --- a/hw/scsi/spapr_vscsi.c
>> +++ b/hw/scsi/spapr_vscsi.c
>> @@ -117,6 +117,20 @@ static struct vscsi_req *vscsi_get_req(VSCSIState *s)
>>      return NULL;
>>  }
>>  
>> +static struct vscsi_req *vscsi_find_req(VSCSIState *s, uint64_t srp_tag)
>> +{
>> +    vscsi_req *req;
>> +    int i;
>> +
>> +    for (i = 0; i < VSCSI_REQ_LIMIT; i++) {
>> +        req = &s->reqs[i];
>> +        if (req->iu.srp.cmd.tag == srp_tag) {
>> +            return req;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>>  static void vscsi_put_req(vscsi_req *req)
>>  {
>>      if (req->sreq != NULL) {
>> @@ -641,6 +655,13 @@ static void *vscsi_load_request(QEMUFile *f, 
>> SCSIRequest *sreq)
>>      return req;
>>  }
>>  
>> +static void vscsi_free_request(SCSIBus *bus, void *priv)
>> +{
>> +    vscsi_req *req = priv;
>> +
>> +    vscsi_put_req(req);
>> +}
>> +
>>  static void vscsi_process_login(VSCSIState *s, vscsi_req *req)
>>  {
>>      union viosrp_iu *iu = &req->iu;
>> @@ -750,43 +771,65 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req 
>> *req)
>>      return 0;
>>  }
>>  
>> +/*
>> + * The SRP services request the SRP initiator port issue an SRP_TSK_MGMT
>> + * request (see 6.7) with a TASK MANAGEMENT FLAGS field set to indicate
>> + * an ABORT TASK function to be sent to the selected SCSI device.
>> + */
>> +static int vscsi_tsk_mgmt_abort_task(VSCSIState *s, vscsi_req *req)
>> +{
>> +    vscsi_req *task_req = vscsi_find_req(s, req->iu.srp.tsk_mgmt.task_tag);
>> +
>> +    scsi_req_cancel(task_req->sreq);
>> +
>> +    return 1;
>> +}
>> +
>> +/*
>> + * The SRP services request the SRP initiator port issue an SRP_TSK_MGMT
>> + * request (see 6.7) with a TASK MANAGEMENT FLAGS field set to indicate
>> + * a LOGICAL UNIT RESET function to be sent to the selected SCSI device.
>> + */
>> +static int vscsi_tsk_mgmt_lun_reset(VSCSIState *s, vscsi_req *req)
>> +{
>> +    int i;
>> +    vscsi_req *tmpreq;
>> +
>> +    for (i = 0; i < VSCSI_REQ_LIMIT; i++) {
>> +        tmpreq = &s->reqs[i];
>> +        if ((tmpreq->iu.srp.cmd.lun == req->iu.srp.tsk_mgmt.lun) &&
>> +                tmpreq->active && tmpreq->sreq) {
>> +            scsi_req_cancel(tmpreq->sreq);
>> +        }
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>>  static int vscsi_process_tsk_mgmt(VSCSIState *s, vscsi_req *req)
>>  {
>>      union viosrp_iu *iu = &req->iu;
>> -    int fn;
>> +    int ret = 0;
>>  
>>      fprintf(stderr, "vscsi_process_tsk_mgmt %02x\n",
>>              iu->srp.tsk_mgmt.tsk_mgmt_func);
>>  
>>      switch (iu->srp.tsk_mgmt.tsk_mgmt_func) {
>> -#if 0 /* We really don't deal with these for now */
>>      case SRP_TSK_ABORT_TASK:
>> -        fn = ABORT_TASK;
>> +        ret = vscsi_tsk_mgmt_abort_task(s, req);
>> +        break;
>> +    case SRP_TSK_LUN_RESET:
>> +        ret = vscsi_tsk_mgmt_lun_reset(s, req);
>>          break;
>>      case SRP_TSK_ABORT_TASK_SET:
>> -        fn = ABORT_TASK_SET;
>> -        break;
>>      case SRP_TSK_CLEAR_TASK_SET:
>> -        fn = CLEAR_TASK_SET;
>> -        break;
>> -    case SRP_TSK_LUN_RESET:
>> -        fn = LOGICAL_UNIT_RESET;
>> -        break;
>>      case SRP_TSK_CLEAR_ACA:
>> -        fn = CLEAR_ACA;
>> -        break;
>> -#endif
>>      default:
>> -        fn = 0;
>> -    }
>> -    if (fn) {
>> -        /* XXX Send/Handle target task management */
>> -        ;
>> -    } else {
>>          vscsi_makeup_sense(s, req, ILLEGAL_REQUEST, 0x20, 0);
>>          vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
>> +        ret = 1;
>>      }
>> -    return !fn;
>> +    return ret;
>>  }
>>  
>>  static int vscsi_handle_srp_req(VSCSIState *s, vscsi_req *req)
>> @@ -998,6 +1041,7 @@ static const struct SCSIBusInfo vscsi_scsi_info = {
>>      .cancel = vscsi_request_cancelled,
>>      .save_request = vscsi_save_request,
>>      .load_request = vscsi_load_request,
>> +    .free_request = vscsi_free_request,
>>  };
> 
> The addition of free_request is specific to the task management ? Or
> something that should be done regardless (ie. in a separate patch) ?
> 
> Cheers,
> Ben.
> 
>>  static void spapr_vscsi_reset(VIOsPAPRDevice *dev)
> 
> 
> 
>