[Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Alistair Francis]

From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

When host block devices are bridged to a guest system through
virtio-scsi-pci and scsi-block driver, scsi_handle_rw_error() in
hw/scsi/scsi-disk.c checks the error number to judge which error to
report to the guests. EIO and EINVAL are not reported and ignored. Once
EIO or EINVAL happen, eternal wait of guest system happens. This problem
was observed with zoned block devices on the host system attached to the
guest via virtio-scsi-pci. To avoid the eternal wait, add EIO and EINVAL
to the list of error numbers to report to the guest.

On top of this, it is required to report SCSI sense data to the guest
so that the guest can handle the error correctly. However,
scsi_handle_rw_error() does not passthrough sense data that host
scsi-block device reported. Instead, it newly generates fixed sense
data only for certain error numbers. This is inflexible to support new
error codes to report to guest. To avoid this inflexiblity, pass the SCSI
sense data that the host scsi-block device reported as is. To be more
precise, set valid sense_len in the SCSIDiskReq referring sb_len_wr that
host SCSI device SG_IO ioctl reported. Add update_sense callback to
SCSIDiskClass to refer the SG_IO ioctl result only when scsi-block device
is targeted.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/scsi/scsi-disk.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ed7295bfd7..6801e3a0d0 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -62,6 +62,7 @@ typedef struct SCSIDiskClass {
     DMAIOFunc       *dma_readv;
     DMAIOFunc       *dma_writev;
     bool            (*need_fua_emulation)(SCSICommand *cmd);
+    void            (*update_sense)(SCSIRequest *r);
 } SCSIDiskClass;
 
 typedef struct SCSIDiskReq {
@@ -438,6 +439,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
 {
     bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
     BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
                                                    is_read, error);
 
@@ -452,9 +454,12 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
              * pause the host.
              */
             assert(r->status && *r->status);
+            if (sdc->update_sense) {
+                sdc->update_sense(&r->req);
+            }
             error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
             if (error == ECANCELED || error == EAGAIN || error == ENOTCONN ||
-                error == 0)  {
+                error == EIO || error == EINVAL || error == 0)  {
                 /* These errors are handled by guest. */
                 scsi_req_complete(&r->req, *r->status);
                 return true;
@@ -2894,6 +2899,13 @@ static int scsi_block_parse_cdb(SCSIDevice *d, SCSICommand *cmd,
     }
 }
 
+static void scsi_block_update_sense(SCSIRequest *req)
+{
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+    SCSIBlockReq *br = DO_UPCAST(SCSIBlockReq, req, r);
+    r->req.sense_len = MIN(br->io_header.sb_len_wr, sizeof(r->req.sense));
+}
+
 #endif
 
 static
@@ -3059,6 +3071,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
     sc->parse_cdb    = scsi_block_parse_cdb;
     sdc->dma_readv   = scsi_block_dma_readv;
     sdc->dma_writev  = scsi_block_dma_writev;
+    sdc->update_sense = scsi_block_update_sense;
     sdc->need_fua_emulation = scsi_block_no_fua;
     dc->desc = "SCSI block device passthrough";
     dc->props = scsi_block_properties;
-- 
2.22.0

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Paolo Bonzini]

On 27/06/19 00:46, Alistair Francis wrote:
> From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> 
> When host block devices are bridged to a guest system through
> virtio-scsi-pci and scsi-block driver, scsi_handle_rw_error() in
> hw/scsi/scsi-disk.c checks the error number to judge which error to
> report to the guests. EIO and EINVAL are not reported and ignored. Once
> EIO or EINVAL happen, eternal wait of guest system happens. This problem
> was observed with zoned block devices on the host system attached to the
> guest via virtio-scsi-pci. To avoid the eternal wait, add EIO and EINVAL
> to the list of error numbers to report to the guest.

This is certainly correct for EINVAL, I am not sure about EIO.  Did you
run the VM with "-drive ...,rerror=report,werror=report"?

The update_sense part is okay too.

Paolo

> On top of this, it is required to report SCSI sense data to the guest
> so that the guest can handle the error correctly. However,
> scsi_handle_rw_error() does not passthrough sense data that host
> scsi-block device reported. Instead, it newly generates fixed sense
> data only for certain error numbers. This is inflexible to support new
> error codes to report to guest. To avoid this inflexiblity, pass the SCSI
> sense data that the host scsi-block device reported as is. To be more
> precise, set valid sense_len in the SCSIDiskReq referring sb_len_wr that
> host SCSI device SG_IO ioctl reported. Add update_sense callback to
> SCSIDiskClass to refer the SG_IO ioctl result only when scsi-block device
> is targeted.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/scsi/scsi-disk.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index ed7295bfd7..6801e3a0d0 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -62,6 +62,7 @@ typedef struct SCSIDiskClass {
>      DMAIOFunc       *dma_readv;
>      DMAIOFunc       *dma_writev;
>      bool            (*need_fua_emulation)(SCSICommand *cmd);
> +    void            (*update_sense)(SCSIRequest *r);
>  } SCSIDiskClass;
>  
>  typedef struct SCSIDiskReq {
> @@ -438,6 +439,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
>  {
>      bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +    SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
>      BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
>                                                     is_read, error);
>  
> @@ -452,9 +454,12 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
>               * pause the host.
>               */
>              assert(r->status && *r->status);
> +            if (sdc->update_sense) {
> +                sdc->update_sense(&r->req);
> +            }
>              error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
>              if (error == ECANCELED || error == EAGAIN || error == ENOTCONN ||
> -                error == 0)  {
> +                error == EIO || error == EINVAL || error == 0)  {
>                  /* These errors are handled by guest. */
>                  scsi_req_complete(&r->req, *r->status);
>                  return true;
> @@ -2894,6 +2899,13 @@ static int scsi_block_parse_cdb(SCSIDevice *d, SCSICommand *cmd,
>      }
>  }
>  
> +static void scsi_block_update_sense(SCSIRequest *req)
> +{
> +    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> +    SCSIBlockReq *br = DO_UPCAST(SCSIBlockReq, req, r);
> +    r->req.sense_len = MIN(br->io_header.sb_len_wr, sizeof(r->req.sense));
> +}
> +
>  #endif
>  
>  static
> @@ -3059,6 +3071,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
>      sc->parse_cdb    = scsi_block_parse_cdb;
>      sdc->dma_readv   = scsi_block_dma_readv;
>      sdc->dma_writev  = scsi_block_dma_writev;
> +    sdc->update_sense = scsi_block_update_sense;
>      sdc->need_fua_emulation = scsi_block_no_fua;
>      dc->desc = "SCSI block device passthrough";
>      dc->props = scsi_block_properties;
> 

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Alistair Francis]

On Thu, Jun 27, 2019 at 2:01 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 27/06/19 00:46, Alistair Francis wrote:
> > From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> >
> > When host block devices are bridged to a guest system through
> > virtio-scsi-pci and scsi-block driver, scsi_handle_rw_error() in
> > hw/scsi/scsi-disk.c checks the error number to judge which error to
> > report to the guests. EIO and EINVAL are not reported and ignored. Once
> > EIO or EINVAL happen, eternal wait of guest system happens. This problem
> > was observed with zoned block devices on the host system attached to the
> > guest via virtio-scsi-pci. To avoid the eternal wait, add EIO and EINVAL
> > to the list of error numbers to report to the guest.
>
> This is certainly correct for EINVAL, I am not sure about EIO.  Did you
> run the VM with "-drive ...,rerror=report,werror=report"?

This is from Shin'ichiro Kawasaki:

I tried to run my VM with option "-drive
...,rerror=report,werror=report" as he noted, but the eternal loop
symptom still happens when host block-scsi device returns EIO. Then I
believe EIO should be added to the report target error list.

>
> The update_sense part is okay too.

Cool!

Alistair

>
> Paolo
>
> > On top of this, it is required to report SCSI sense data to the guest
> > so that the guest can handle the error correctly. However,
> > scsi_handle_rw_error() does not passthrough sense data that host
> > scsi-block device reported. Instead, it newly generates fixed sense
> > data only for certain error numbers. This is inflexible to support new
> > error codes to report to guest. To avoid this inflexiblity, pass the SCSI
> > sense data that the host scsi-block device reported as is. To be more
> > precise, set valid sense_len in the SCSIDiskReq referring sb_len_wr that
> > host SCSI device SG_IO ioctl reported. Add update_sense callback to
> > SCSIDiskClass to refer the SG_IO ioctl result only when scsi-block device
> > is targeted.
> >
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/scsi/scsi-disk.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index ed7295bfd7..6801e3a0d0 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -62,6 +62,7 @@ typedef struct SCSIDiskClass {
> >      DMAIOFunc       *dma_readv;
> >      DMAIOFunc       *dma_writev;
> >      bool            (*need_fua_emulation)(SCSICommand *cmd);
> > +    void            (*update_sense)(SCSIRequest *r);
> >  } SCSIDiskClass;
> >
> >  typedef struct SCSIDiskReq {
> > @@ -438,6 +439,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
> >  {
> >      bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
> >      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> > +    SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
> >      BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
> >                                                     is_read, error);
> >
> > @@ -452,9 +454,12 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
> >               * pause the host.
> >               */
> >              assert(r->status && *r->status);
> > +            if (sdc->update_sense) {
> > +                sdc->update_sense(&r->req);
> > +            }
> >              error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
> >              if (error == ECANCELED || error == EAGAIN || error == ENOTCONN ||
> > -                error == 0)  {
> > +                error == EIO || error == EINVAL || error == 0)  {
> >                  /* These errors are handled by guest. */
> >                  scsi_req_complete(&r->req, *r->status);
> >                  return true;
> > @@ -2894,6 +2899,13 @@ static int scsi_block_parse_cdb(SCSIDevice *d, SCSICommand *cmd,
> >      }
> >  }
> >
> > +static void scsi_block_update_sense(SCSIRequest *req)
> > +{
> > +    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> > +    SCSIBlockReq *br = DO_UPCAST(SCSIBlockReq, req, r);
> > +    r->req.sense_len = MIN(br->io_header.sb_len_wr, sizeof(r->req.sense));
> > +}
> > +
> >  #endif
> >
> >  static
> > @@ -3059,6 +3071,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
> >      sc->parse_cdb    = scsi_block_parse_cdb;
> >      sdc->dma_readv   = scsi_block_dma_readv;
> >      sdc->dma_writev  = scsi_block_dma_writev;
> > +    sdc->update_sense = scsi_block_update_sense;
> >      sdc->need_fua_emulation = scsi_block_no_fua;
> >      dc->desc = "SCSI block device passthrough";
> >      dc->props = scsi_block_properties;
> >
>

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Paolo Bonzini]

On 28/06/19 23:57, Alistair Francis wrote:
> 
> I tried to run my VM with option "-drive
> ...,rerror=report,werror=report" as he noted, but the eternal loop
> symptom still happens when host block-scsi device returns EIO. Then I
> believe EIO should be added to the report target error list.

What is the sense data he's seeing?  EIO is a catch-all return value
for scsi_sense_buf_to_errno so I'd rather be more specific.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Alistair Francis]

On Fri, Jun 28, 2019 at 3:14 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 28/06/19 23:57, Alistair Francis wrote:
> >
> > I tried to run my VM with option "-drive
> > ...,rerror=report,werror=report" as he noted, but the eternal loop
> > symptom still happens when host block-scsi device returns EIO. Then I
> > believe EIO should be added to the report target error list.
>
> What is the sense data he's seeing?  EIO is a catch-all return value
> for scsi_sense_buf_to_errno so I'd rather be more specific.

I'm not sure, he is CCed to this email so he should respond with more
information.

Alistair

>
> Thanks,
>
> Paolo

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Shinichiro Kawasaki]

On 6/29/19 7:21 AM, Alistair Francis wrote:
> On Fri, Jun 28, 2019 at 3:14 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 28/06/19 23:57, Alistair Francis wrote:
>>>
>>> I tried to run my VM with option "-drive
>>> ...,rerror=report,werror=report" as he noted, but the eternal loop
>>> symptom still happens when host block-scsi device returns EIO. Then I
>>> believe EIO should be added to the report target error list.
>>
>> What is the sense data he's seeing?  EIO is a catch-all return value
>> for scsi_sense_buf_to_errno so I'd rather be more specific.
> 
> I'm not sure, he is CCed to this email so he should respond with more
> information.

Hi Paolo, thank you very much for your review and comments.
(Alistair, thank you for your care for the patch post)

The sense data I observe are related to 'zoned storage'. Now I'm trying to make 
zoned storage on host visible to qemu guests through scsi-block driver, to 
utilize qemu guest environment for zoned storage system development.

     'zonedstroage.io' is the good reference for details of the zoned storage.
     http://zonedstorage.io/introduction/zoned-storage/

The zoned storage introduced "zone" and "write pointer" concepts, and SCSI spec 
documents were updated to define additional commands for zoned storage as well 
as Additional Sense Codes. The latest SPC-5 defines a number of ASCs that zoned 
SCSI storage devices return. I observe four of them listed below in sense data, 
when I ran basic operations to the zoned storage from the guest via scsi-block.

     21h 04h: UNALIGNED WRITE COMMAND
     21h 05h: WRITE BOUNDARY VIOLATION
     21h 06h: ATTEMPT TO READ INVALID DATA
     55h 0Eh: INSUFFICIENT ZONE RESOURCES

These ASCs can be reported for write or read commands due to unexpected zone
status or write pointer status. Reporting these ASCs to the guest, the user
applications can handle them to manage zone/write pointer status, or help the
user application developers to understand the failure reason and fix bugs.

I took a look in scsi_sense_to_errno() and learned that ASCs are grouped in 
errnos. To report the ASCs above to the guest, is it good to add them in EINVAL 
group defined in scsi_sense_to_errno()? The ASCs are reported with sense key 
ILLEGAL_REQUEST or DATA_PROTECT, then I think it fits in the function.

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Paolo Bonzini]

On 01/07/19 12:14, Shinichiro Kawasaki wrote:
> I observe four of them listed below in sense data, 
> when I ran basic operations to the zoned storage from the guest via scsi-block.
> 
>      21h 04h: UNALIGNED WRITE COMMAND
>      21h 05h: WRITE BOUNDARY VIOLATION
>      21h 06h: ATTEMPT TO READ INVALID DATA
>      55h 0Eh: INSUFFICIENT ZONE RESOURCES
> 
> These ASCs can be reported for write or read commands due to unexpected zone
> status or write pointer status. Reporting these ASCs to the guest, the user
> applications can handle them to manage zone/write pointer status, or help the
> user application developers to understand the failure reason and fix bugs.
> 
> I took a look in scsi_sense_to_errno() and learned that ASCs are grouped in 
> errnos. To report the ASCs above to the guest, is it good to add them in EINVAL 
> group defined in scsi_sense_to_errno()? The ASCs are reported with sense key 
> ILLEGAL_REQUEST or DATA_PROTECT, then I think it fits in the function.

The grouping by errno is historical and pretty much broken.  It should
be possible to change it to return just a bool.

Paolo

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Shinichiro Kawasaki]

On 7/1/19 8:56 PM, Paolo Bonzini wrote:
> On 01/07/19 12:14, Shinichiro Kawasaki wrote:
>> I observe four of them listed below in sense data,
>> when I ran basic operations to the zoned storage from the guest via scsi-block.
>>
>>       21h 04h: UNALIGNED WRITE COMMAND
>>       21h 05h: WRITE BOUNDARY VIOLATION
>>       21h 06h: ATTEMPT TO READ INVALID DATA
>>       55h 0Eh: INSUFFICIENT ZONE RESOURCES
>>
>> These ASCs can be reported for write or read commands due to unexpected zone
>> status or write pointer status. Reporting these ASCs to the guest, the user
>> applications can handle them to manage zone/write pointer status, or help the
>> user application developers to understand the failure reason and fix bugs.
>>
>> I took a look in scsi_sense_to_errno() and learned that ASCs are grouped in
>> errnos. To report the ASCs above to the guest, is it good to add them in EINVAL
>> group defined in scsi_sense_to_errno()? The ASCs are reported with sense key
>> ILLEGAL_REQUEST or DATA_PROTECT, then I think it fits in the function.
> 
> The grouping by errno is historical and pretty much broken.  It should
> be possible to change it to return just a bool.

The errno grouping of scsi_sense_to_errno() is used not only by scsi-disk but 
also by block/iscsi for error reporting. Can we avoid errno grouping for iscsi also?

If the errno grouping is not valuable for iscsi, single error code (EIO?) can be 
reported for all iscsi failures. If the errno grouping is useful for iscsi, I 
can create a patch to avoid errno grouping only for scsi-disk, leaving 
scsi_sense_to_errno() for iscsi.

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Paolo Bonzini]

On 02/07/19 08:44, Shinichiro Kawasaki wrote:
> On 7/1/19 8:56 PM, Paolo Bonzini wrote:
>> On 01/07/19 12:14, Shinichiro Kawasaki wrote:
>>> I observe four of them listed below in sense data,
>>> when I ran basic operations to the zoned storage from the guest via scsi-block.
>>>
>>>       21h 04h: UNALIGNED WRITE COMMAND
>>>       21h 05h: WRITE BOUNDARY VIOLATION
>>>       21h 06h: ATTEMPT TO READ INVALID DATA
>>>       55h 0Eh: INSUFFICIENT ZONE RESOURCES
>>>
>>> These ASCs can be reported for write or read commands due to unexpected zone
>>> status or write pointer status. Reporting these ASCs to the guest, the user
>>> applications can handle them to manage zone/write pointer status, or help the
>>> user application developers to understand the failure reason and fix bugs.
>>>
>>> I took a look in scsi_sense_to_errno() and learned that ASCs are grouped in
>>> errnos. To report the ASCs above to the guest, is it good to add them in EINVAL
>>> group defined in scsi_sense_to_errno()? The ASCs are reported with sense key
>>> ILLEGAL_REQUEST or DATA_PROTECT, then I think it fits in the function.
>>
>> The grouping by errno is historical and pretty much broken.  It should
>> be possible to change it to return just a bool.
> 
> The errno grouping of scsi_sense_to_errno() is used not only by scsi-disk but 
> also by block/iscsi for error reporting. Can we avoid errno grouping for iscsi also?

No, but we can do something like

    if (scsi_sense_buf_is_guest_recoverable(r->req.sense,
sizeof(r->req.sense))) {
        /* These errors are handled by guest. */
        sdc->update_sense(&r->req);
        scsi_req_complete(&r->req, *r->status);
        return true;
    }
    error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));

This way there's generally no need to shoehorn ASC codes into errno.  I
still have to test my changes, but I hope to send something within a
couple of days.

Paolo

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Shinichiro Kawasaki]

On 7/2/19 7:23 PM, Paolo Bonzini wrote:
> On 02/07/19 08:44, Shinichiro Kawasaki wrote:
>> On 7/1/19 8:56 PM, Paolo Bonzini wrote:
>>> On 01/07/19 12:14, Shinichiro Kawasaki wrote:
>>>> I observe four of them listed below in sense data,
>>>> when I ran basic operations to the zoned storage from the guest via scsi-block.
>>>>
>>>>        21h 04h: UNALIGNED WRITE COMMAND
>>>>        21h 05h: WRITE BOUNDARY VIOLATION
>>>>        21h 06h: ATTEMPT TO READ INVALID DATA
>>>>        55h 0Eh: INSUFFICIENT ZONE RESOURCES
>>>>
>>>> These ASCs can be reported for write or read commands due to unexpected zone
>>>> status or write pointer status. Reporting these ASCs to the guest, the user
>>>> applications can handle them to manage zone/write pointer status, or help the
>>>> user application developers to understand the failure reason and fix bugs.
>>>>
>>>> I took a look in scsi_sense_to_errno() and learned that ASCs are grouped in
>>>> errnos. To report the ASCs above to the guest, is it good to add them in EINVAL
>>>> group defined in scsi_sense_to_errno()? The ASCs are reported with sense key
>>>> ILLEGAL_REQUEST or DATA_PROTECT, then I think it fits in the function.
>>>
>>> The grouping by errno is historical and pretty much broken.  It should
>>> be possible to change it to return just a bool.
>>
>> The errno grouping of scsi_sense_to_errno() is used not only by scsi-disk but
>> also by block/iscsi for error reporting. Can we avoid errno grouping for iscsi also?
> 
> No, but we can do something like
> 
>      if (scsi_sense_buf_is_guest_recoverable(r->req.sense,
> sizeof(r->req.sense))) {
>          /* These errors are handled by guest. */
>          sdc->update_sense(&r->req);
>          scsi_req_complete(&r->req, *r->status);
>          return true;
>      }
>      error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
> 
> This way there's generally no need to shoehorn ASC codes into errno.  I
> still have to test my changes, but I hope to send something within a
> couple of days.

Thanks for sharing your thoughts. Now I understand your idea.

I'm awaiting your patch. In case you want me to create the patch based on your 
idea, please let me know. I can afford some time next week to work on it.

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

On 05/07/19 12:31, Shinichiro Kawasaki wrote:
>> This way there's generally no need to shoehorn ASC codes into errno.  I
>> still have to test my changes, but I hope to send something within a
>> couple of days.
> 
> Thanks for sharing your thoughts. Now I understand your idea.
> 
> I'm awaiting your patch. In case you want me to create the patch based on your 
> idea, please let me know. I can afford some time next week to work on it.

I didn't have time to finish, but since I will be out for part of next
week, here is what I currently have (untested, somewhat uncompiled).
It's a bugfix so it can be included after hard freeze.

Thanks!

Paolo

[-- Attachment #2: scsi-sense.patch --]
[-- Type: text/x-patch, Size: 14051 bytes --]

From fdccbd77192ca4450f6e8900bef392fa36242200 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Fri, 5 Jul 2019 19:07:17 +0200
Subject: [PATCH 0/5] *** SUBJECT HERE ***

*** BLURB HERE ***

Paolo Bonzini (4):
  scsi: explicitly list guest-recoverable sense codes
  scsi: add guest-recoverable ZBC errors
  iscsi: fix busy/timeout/task set full
  iscsi: base all handling of check condition on scsi_sense_to_errno

Shinichiro Kawasaki (1):
  scsi-disk: pass sense correctly for guest-recoverable errors

 block/iscsi.c        | 28 +++++++++++------------
 hw/scsi/scsi-disk.c  | 15 ++++++++++---
 include/scsi/utils.h |  1 +
 scsi/utils.c         | 53 +++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 77 insertions(+), 20 deletions(-)

-- 
2.21.0

From f8a4b9cd0cd878aacbdf4ad4e7a2451c87339dab Mon Sep 17 00:00:00 2001
From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Date: Tue, 2 Jul 2019 10:08:00 +0200
Subject: [PATCH 1/5] scsi-disk: pass sense correctly for guest-recoverable
 errors

When an error was passed down to the guest because it was recoverable,
the sense length was not copied from the SG_IO data.  As a result,
the guest saw the CHECK CONDITION status but not the sense data.

Signed-off-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ed7295bfd7..5d3fb3c9d5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -62,6 +62,7 @@ typedef struct SCSIDiskClass {
     DMAIOFunc       *dma_readv;
     DMAIOFunc       *dma_writev;
     bool            (*need_fua_emulation)(SCSICommand *cmd);
+    void            (*update_sense)(SCSIRequest *r);
 } SCSIDiskClass;
 
 typedef struct SCSIDiskReq {
@@ -438,6 +439,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
 {
     bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
     BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
                                                    is_read, error);
 
@@ -456,6 +458,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
             if (error == ECANCELED || error == EAGAIN || error == ENOTCONN ||
                 error == 0)  {
                 /* These errors are handled by guest. */
+                sdc->update_sense(&r->req);
                 scsi_req_complete(&r->req, *r->status);
                 return true;
             }
@@ -2894,6 +2897,12 @@ static int scsi_block_parse_cdb(SCSIDevice *d, SCSICommand *cmd,
     }
 }
 
+static void scsi_block_update_sense(SCSIRequest *req)
+{
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+    SCSIBlockReq *br = DO_UPCAST(SCSIBlockReq, req, r);
+    r->req.sense_len = MIN(br->io_header.sb_len_wr, sizeof(r->req.sense));
+}
 #endif
 
 static
@@ -3059,6 +3068,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
     sc->parse_cdb    = scsi_block_parse_cdb;
     sdc->dma_readv   = scsi_block_dma_readv;
     sdc->dma_writev  = scsi_block_dma_writev;
+    sdc->update_sense = scsi_block_update_sense;
     sdc->need_fua_emulation = scsi_block_no_fua;
     dc->desc = "SCSI block device passthrough";
     dc->props = scsi_block_properties;
-- 
2.21.0


From ab60dfcd4c2bbad79d2bdfae37a88b031917ec99 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 2 Jul 2019 10:23:20 +0200
Subject: [PATCH 2/5] scsi: explicitly list guest-recoverable sense codes

It's not really possible to fit all sense codes into errno codes, especially
in such a way that sense codes can be properly categorized as either.
guest-recoverable and host-handled.  Just create a new function that
checks for guest recoverable sense, then scsi_sense_buf_to_errno only
needs to be called for host handled sense codes.
---
 hw/scsi/scsi-disk.c  |  5 ++---
 include/scsi/utils.h |  1 +
 scsi/utils.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5d3fb3c9d5..8e95e3e38d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -454,14 +454,13 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed)
              * pause the host.
              */
             assert(r->status && *r->status);
-            error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
-            if (error == ECANCELED || error == EAGAIN || error == ENOTCONN ||
-                error == 0)  {
+            if (scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
                 /* These errors are handled by guest. */
                 sdc->update_sense(&r->req);
                 scsi_req_complete(&r->req, *r->status);
                 return true;
             }
+            error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
             break;
         case ENOMEDIUM:
             scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index 9351b21ead..fee6212e85 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -106,6 +106,7 @@ extern const struct SCSISense sense_code_SPACE_ALLOC_FAILED;
 
 int scsi_sense_to_errno(int key, int asc, int ascq);
 int scsi_sense_buf_to_errno(const uint8_t *sense, size_t sense_size);
+int scsi_sense_buf_is_guest_recoverable(const uint8_t *sense, size_t sense_size);
 
 int scsi_convert_sense(uint8_t *in_buf, int in_len,
                        uint8_t *buf, int len, bool fixed);
diff --git a/scsi/utils.c b/scsi/utils.c
index 8738522955..3ad28face1 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -336,6 +336,38 @@ int scsi_convert_sense(uint8_t *in_buf, int in_len,
     }
 }
 
+static bool scsi_sense_is_guest_recoverable(int key, int asc, int ascq)
+{
+    switch (key) {
+    case NO_SENSE:
+    case RECOVERED_ERROR:
+    case UNIT_ATTENTION:
+    case ABORTED_COMMAND:
+        return true;
+    case NOT_READY:
+    case ILLEGAL_REQUEST:
+    case DATA_PROTECT:
+        /* Parse ASCQ */
+        break;
+    default:
+        return false;
+    }
+
+    switch ((asc << 8) | ascq) {
+    case 0x1a00: /* PARAMETER LIST LENGTH ERROR */
+    case 0x2000: /* INVALID OPERATION CODE */
+    case 0x2400: /* INVALID FIELD IN CDB */
+    case 0x2500: /* LOGICAL UNIT NOT SUPPORTED */
+    case 0x2600: /* INVALID FIELD IN PARAMETER LIST */
+
+    case 0x0401: /* NOT READY, IN PROGRESS OF BECOMING READY */
+    case 0x0402: /* NOT READY, INITIALIZING COMMAND REQUIRED */
+        return true;
+    default:
+        return false;
+    }
+}
+
 int scsi_sense_to_errno(int key, int asc, int ascq)
 {
     switch (key) {
@@ -391,6 +423,17 @@ int scsi_sense_buf_to_errno(const uint8_t *in_buf, size_t in_len)
     return scsi_sense_to_errno(sense.key, sense.asc, sense.ascq);
 }
 
+bool scsi_sense_buf_is_guest_recoverable(const uint8_t *in_buf, size_t in_len)
+{
+    SCSISense sense;
+    if (in_len < 1) {
+        return false;
+    }
+
+    sense = scsi_parse_sense_buf(in_buf, in_len);
+    return scsi_sense_to_errno(sense.key, sense.asc, sense.ascq);
+}
+
 const char *scsi_command_name(uint8_t cmd)
 {
     static const char *names[] = {
-- 
2.21.0


From eca5917fcdf3ea690ee80d6f073d3b915691a499 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 2 Jul 2019 10:01:03 +0200
Subject: [PATCH 3/5] scsi: add guest-recoverable ZBC errors

When I ran basic operations to the zoned storage from the guest via
scsi-block the following ASCs are reported for write or read commands
due to unexpected zone status or write pointer status:

     21h 04h: UNALIGNED WRITE COMMAND
     21h 05h: WRITE BOUNDARY VIOLATION
     21h 06h: ATTEMPT TO READ INVALID DATA
     55h 0Eh: INSUFFICIENT ZONE RESOURCES

Reporting these ASCs to the guest, the user applications can handle
them to manage zone/write pointer status, or help the user application
developers to understand the failure reason and fix bugs.

Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scsi/utils.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scsi/utils.c b/scsi/utils.c
index 3ad28face1..53274f62d7 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -360,6 +360,11 @@ static bool scsi_sense_is_guest_recoverable(int key, int asc, int ascq)
     case 0x2500: /* LOGICAL UNIT NOT SUPPORTED */
     case 0x2600: /* INVALID FIELD IN PARAMETER LIST */
 
+    case 0x2104: /* UNALIGNED WRITE COMMAND */
+    case 0x2105: /* WRITE BOUNDARY VIOLATION */
+    case 0x2106: /* ATTEMPT TO READ INVALID DATA */
+    case 0x550e: /* INSUFFICIENT ZONE RESOURCES */
+
     case 0x0401: /* NOT READY, IN PROGRESS OF BECOMING READY */
     case 0x0402: /* NOT READY, INITIALIZING COMMAND REQUIRED */
         return true;
-- 
2.21.0


From 594c17e8f6d617d5f234e1d3701cfc54e6804d1a Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 2 Jul 2019 10:45:54 +0200
Subject: [PATCH 4/5] iscsi: fix busy/timeout/task set full

In this case, do_retry was set without calling aio_co_wake, thus never
waking up the coroutine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 267f160bf6..6e238bf0ad 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -272,7 +272,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
                 timer_mod(&iTask->retry_timer,
                           qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + retry_time);
                 iTask->do_retry = 1;
-                return;
+                goto out;
             }
         }
         iTask->err_code = iscsi_translate_sense(&task->sense);
-- 
2.21.0


From fdccbd77192ca4450f6e8900bef392fa36242200 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 2 Jul 2019 11:40:41 +0200
Subject: [PATCH 5/5] iscsi: base all handling of check condition on
 scsi_sense_to_errno

Now that scsi-disk is not using scsi_sense_to_errno to separate guest-recoverable
sense codes, we can modify it to simplify iscsi's own sense handling.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 28 ++++++++++++++--------------
 scsi/utils.c  |  5 ++---
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6e238bf0ad..5817967205 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -225,9 +225,9 @@ static inline unsigned exp_random(double mean)
 
 static int iscsi_translate_sense(struct scsi_sense *sense)
 {
-    return - scsi_sense_to_errno(sense->key,
-                                 (sense->ascq & 0xFF00) >> 8,
-                                 sense->ascq & 0xFF);
+    return scsi_sense_to_errno(sense->key,
+                               (sense->ascq & 0xFF00) >> 8,
+                               sense->ascq & 0xFF);
 }
 
 /* Called (via iscsi_service) with QemuMutex held.  */
@@ -244,13 +244,6 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
 
     if (status != SCSI_STATUS_GOOD) {
         if (iTask->retries++ < ISCSI_CMD_RETRIES) {
-            if (status == SCSI_STATUS_CHECK_CONDITION
-                && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
-                error_report("iSCSI CheckCondition: %s",
-                             iscsi_get_error(iscsi));
-                iTask->do_retry = 1;
-                goto out;
-            }
             if (status == SCSI_STATUS_BUSY ||
                 status == SCSI_STATUS_TIMEOUT ||
                 status == SCSI_STATUS_TASK_SET_FULL) {
@@ -272,11 +265,18 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
                 timer_mod(&iTask->retry_timer,
                           qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + retry_time);
                 iTask->do_retry = 1;
-                goto out;
+            }
+        } else if (status == SCSI_STATUS_CHECK_CONDITION) {
+            int error = iscsi_translate_sense(&task->sense);
+            if (error == EAGAIN) {
+                error_report("iSCSI CheckCondition: %s",
+                             iscsi_get_error(iscsi));
+                iTask->do_retry = 1;
+            } else {
+                iTask->err_code = -error;
+                iTask->err_str = g_strdup(iscsi_get_error(iscsi));
             }
         }
-        iTask->err_code = iscsi_translate_sense(&task->sense);
-        iTask->err_str = g_strdup(iscsi_get_error(iscsi));
     }
 
 out:
@@ -974,7 +974,7 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
     if (status < 0) {
         error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",
                      iscsi_get_error(iscsi));
-        acb->status = iscsi_translate_sense(&acb->task->sense);
+        acb->status = -iscsi_translate_sense(&acb->task->sense);
     }
 
     acb->ioh->driver_status = 0;
diff --git a/scsi/utils.c b/scsi/utils.c
index 53274f62d7..7f332ebf1f 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -379,8 +379,7 @@ int scsi_sense_to_errno(int key, int asc, int ascq)
     case NO_SENSE:
     case RECOVERED_ERROR:
     case UNIT_ATTENTION:
-        /* These sense keys are not errors */
-        return 0;
+        return EAGAIN;
     case ABORTED_COMMAND: /* COMMAND ABORTED */
         return ECANCELED;
     case NOT_READY:
@@ -409,7 +408,7 @@ int scsi_sense_to_errno(int key, int asc, int ascq)
     case 0x2700: /* WRITE PROTECTED */
         return EACCES;
     case 0x0401: /* NOT READY, IN PROGRESS OF BECOMING READY */
-        return EAGAIN;
+        return EINPROGRESS;
     case 0x0402: /* NOT READY, INITIALIZING COMMAND REQUIRED */
         return ENOTCONN;
     default:
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 1/1] hw/scsi: Report errors and sense to guests through scsi-block

[Shinichiro Kawasaki]

On 7/6/19 2:08 AM, Paolo Bonzini wrote:
> On 05/07/19 12:31, Shinichiro Kawasaki wrote:
>>> This way there's generally no need to shoehorn ASC codes into errno.  I
>>> still have to test my changes, but I hope to send something within a
>>> couple of days.
>>
>> Thanks for sharing your thoughts. Now I understand your idea.
>>
>> I'm awaiting your patch. In case you want me to create the patch based on your
>> idea, please let me know. I can afford some time next week to work on it.
> 
> I didn't have time to finish, but since I will be out for part of next
> week, here is what I currently have (untested, somewhat uncompiled).

Paolo, thanks for the patch! With a few simple compile error fixes, the patch 
series worked with my system as expected. Zoned storage devices are visible on 
the guest through scsi-block driver. Good.

Here I list the compile error fixes for your reference:

1) changed return type of scsi_sense_buf_is_guest_recoverable() from int to bool
    in include/scsi/utils.h
2) replaced scsi_sense_to_errno() call in scsi_sense_buf_is_guest_recoverable()
    with scsi_sense_is_guest_recoverable()
3) removed unused "out:" label in block/iscsi.c

-- 
Best Regards,
Shin'ichiro Kawasaki