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 <address@hidden> wrote:
>
> On 27/06/19 00:46, Alistair Francis wrote:
> > From: Shin'ichiro Kawasaki <address@hidden>
> >
> > 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 <address@hidden>
> > Signed-off-by: Alistair Francis <address@hidden>
> > ---
> >  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;
> >
>