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

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

[Alexey Kardashevskiy]

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.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

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,
 };
 
 static void spapr_vscsi_reset(VIOsPAPRDevice *dev)
-- 
1.8.3.2

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

[Benjamin Herrenschmidt]

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 (and similarly,
itl looks like the Linux client will only use a couple of them).

> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> 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)

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 <aik@ozlabs.ru>
>> ---
>>
>> 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)
> 
> 
> 
> 

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

[Alexey Kardashevskiy]

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. As spapr-vscsi starts calling scsi_req_cancel(),
free_request callback was implemented.

As virtio-vscsi, spapr-vscsi does not handle CLEAR_ACA either as CDB
control byte does not seem to be used at all so NACA bit is not
set to the guest so the guest has no good reason to call CLEAR_ACA task.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
2013/07/22:
* fixed LUN_RESET (it used to clear requests while it should reset a device)
* added handling of ABORT_TASK_SET/CLEAR_TASK_SET

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/scsi/spapr_vscsi.c | 73 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 17 deletions(-)

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index a86199b..f1e0e8f 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) {
@@ -578,6 +592,13 @@ static void vscsi_request_cancelled(SCSIRequest *sreq)
     vscsi_put_req(req);
 }
 
+static void vscsi_free_request(SCSIBus *bus, void *priv)
+{
+    vscsi_req *req = priv;
+
+    vscsi_put_req(req);
+}
+
 static const VMStateDescription vmstate_spapr_vscsi_req = {
     .name = "spapr_vscsi_req",
     .version_id = 1,
@@ -753,40 +774,57 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
 static int vscsi_process_tsk_mgmt(VSCSIState *s, vscsi_req *req)
 {
     union viosrp_iu *iu = &req->iu;
-    int fn;
+    vscsi_req *tmpreq;
+    SCSIDevice *sdev;
+    int i, lun = 0, error = 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;
+        tmpreq = vscsi_find_req(s, req->iu.srp.tsk_mgmt.task_tag);
+        if (tmpreq && tmpreq->sreq) {
+            assert(tmpreq->sreq->hba_private);
+            scsi_req_cancel(tmpreq->sreq);
+        }
         break;
+
+    case SRP_TSK_LUN_RESET:
+        sdev = vscsi_device_find(&s->bus, req->iu.srp.tsk_mgmt.lun, &lun);
+        if (sdev) {
+            qdev_reset_all(&sdev->qdev);
+        }
+        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;
+        for (i = 0; i < VSCSI_REQ_LIMIT; i++) {
+            tmpreq = &s->reqs[i];
+            if (tmpreq->iu.srp.cmd.lun != req->iu.srp.tsk_mgmt.lun) {
+                continue;
+            }
+            if (!tmpreq->active || !tmpreq->sreq) {
+                continue;
+            }
+            assert(tmpreq->sreq->hba_private);
+            scsi_req_cancel(tmpreq->sreq);
+        }
         break;
+
     case SRP_TSK_CLEAR_ACA:
-        fn = CLEAR_ACA;
-        break;
-#endif
     default:
-        fn = 0;
+        error = 1;
     }
-    if (fn) {
-        /* XXX Send/Handle target task management */
-        ;
+
+    if (!error) {
+        vscsi_send_rsp(s, req, GOOD, 0, 0);
     } else {
         vscsi_makeup_sense(s, req, ILLEGAL_REQUEST, 0x20, 0);
         vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
     }
-    return !fn;
+
+    return 1;
 }
 
 static int vscsi_handle_srp_req(VSCSIState *s, vscsi_req *req)
@@ -998,6 +1036,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,
 };
 
 static void spapr_vscsi_reset(VIOsPAPRDevice *dev)
-- 
1.8.3.2

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

[Alexey Kardashevskiy]

On 07/22/2013 07:34 AM, Benjamin Herrenschmidt wrote:
> 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 (and similarly,
> itl looks like the Linux client will only use a couple of them).
> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>
>> 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) ?

May be. But there was no way to get this callback called till I started
calling scsi_req_cancel in this patch so I would not split.



> 
> Cheers,
> Ben.
> 
>>  static void spapr_vscsi_reset(VIOsPAPRDevice *dev)
> 
> 


-- 
Alexey

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

[Benjamin Herrenschmidt]

On Mon, 2013-07-22 at 10:20 +1000, Alexey Kardashevskiy wrote:
> May be. But there was no way to get this callback called till I started
> calling scsi_req_cancel in this patch so I would not split.

You probably still should. The smaller each individual patch, the better
(in part because that makes them easier to review).

So a first patch adding the new callback, then a second patch adding the
task management.

Cheers,
Ben.

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

[Alexey Kardashevskiy]

On 07/22/2013 10:23 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2013-07-22 at 10:20 +1000, Alexey Kardashevskiy wrote:
>> May be. But there was no way to get this callback called till I started
>> calling scsi_req_cancel in this patch so I would not split.
> 
> You probably still should. The smaller each individual patch, the better
> (in part because that makes them easier to review).


Should make it easier but in fact it does not for me :)


> So a first patch adding the new callback, then a second patch adding the
> task management.

I can even add tasks one-by-one :)

Come on, is this patch really big or even close to be called big?


-- 
Alexey

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

[Benjamin Herrenschmidt]

On Mon, 2013-07-22 at 10:57 +1000, Alexey Kardashevskiy wrote:
> On 07/22/2013 10:23 AM, Benjamin Herrenschmidt wrote:
> > On Mon, 2013-07-22 at 10:20 +1000, Alexey Kardashevskiy wrote:
> >> May be. But there was no way to get this callback called till I started
> >> calling scsi_req_cancel in this patch so I would not split.
> > 
> > You probably still should. The smaller each individual patch, the better
> > (in part because that makes them easier to review).
> 
> 
> Should make it easier but in fact it does not for me :)

But you aren't the one reviewing it. This is how you are supposed to
submit patches. If you willingly decide to not follow that process then
don't be surprised if people complain or don't review them.

> > So a first patch adding the new callback, then a second patch adding the
> > task management.
> 
> I can even add tasks one-by-one :)
> 
> Come on, is this patch really big or even close to be called big?

Doesn't matter. At this point it's RFC so that's not a big deal, for a
final patch, it's about logical separation. The free_request callback
is not directly "linked" to task management though it happens to only
be called in that case. It's a pre-requisite to task management, make it
a separate patch.

Task management per-se could be a single patch, or could be split, up to
you really, but keep in mind that the smaller a patch, the more chances
it gets reviewed quickly.

Cheers,
Ben.

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

[Paolo Bonzini]

Il 22/07/2013 03:04, Benjamin Herrenschmidt ha scritto:
> The free_request callback
> is not directly "linked" to task management though it happens to only
> be called in that case. It's a pre-requisite to task management, make it
> a separate patch.

Ben is right, in fact I'm not sure why you need free_request at all
(since vscsi_put_req is called from both vscsi_command_complete and
vscsi_request_cancelled).

free_request is only needed by hw/usb/dev-uas.c because it does
scsi_req_ref/scsi_req_unref in the HBA code.  There is no need for this,
AFAIK, in the current spapr_vscsi code; if it is done in the
reorganization patches, the free_request callback should be added there.

This part looks weird:

>      default:
>          vscsi_makeup_sense(s, req, ILLEGAL_REQUEST, 0x20, 0);
>          vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);

I suggest you check in the relevant part of the vscsi spec how you are
supposed to send back errors ("FUNCTION REJECTED", "INCORRECT LUN").
Once you do that, implementing the "query" TMFs is trivial.

Paolo

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

[Benjamin Herrenschmidt]

On Mon, 2013-07-22 at 08:30 +0200, Paolo Bonzini wrote:
> I suggest you check in the relevant part of the vscsi spec how you are
> supposed to send back errors ("FUNCTION REJECTED", "INCORRECT LUN").
> Once you do that, implementing the "query" TMFs is trivial.

The vscsi spec is just the transport around SRP, so not very useful,
mostly it's the SRP spec that matters. IE. VSCSI is just SRP inside a
special hypervisor tube :-)

Cheers,
Ben.

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

[Alexey Kardashevskiy]

On 07/22/2013 04:30 PM, Paolo Bonzini wrote:
> Il 22/07/2013 03:04, Benjamin Herrenschmidt ha scritto:
>> The free_request callback
>> is not directly "linked" to task management though it happens to only
>> be called in that case. It's a pre-requisite to task management, make it
>> a separate patch.
> 
> Ben is right, in fact I'm not sure why you need free_request at all
> (since vscsi_put_req is called from both vscsi_command_complete and
> vscsi_request_cancelled).
>
> free_request is only needed by hw/usb/dev-uas.c because it does
> scsi_req_ref/scsi_req_unref in the HBA code.  There is no need for this,
> AFAIK, in the current spapr_vscsi code; if it is done in the
> reorganization patches, the free_request callback should be added there.


Hmmm. It looks like you're right :-/ I am just not very familiar with this
stuff yet so some misunderstanding is expected :)

For now I cannot find really good testing sequence. I do some delay hacks
in various places but I am not sure I am testing what I want to test as
"req->enqueued" is always false when I debug task management.



-- 
Alexey