qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v1 3/3] scsi-block: adding flag at realize to en


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH v1 3/3] scsi-block: adding flag at realize to enable Block Limits emulation
Date: Thu, 21 Jun 2018 12:01:12 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 08/06/2018 22:07, Daniel Henrique Barboza wrote:
> The previous patches implemented a way to deliver an emulated
> Block Limits (BL) response for the guest in case the underlying
> hardware does not support this page.
> 
> However, the approach used is crude. We're executing the logic for
> all SCSI devices, regardless of whether they need it or not. There's
> also a possibility that we'll end up masking a legitimate SCSI error
> of a device that does implement the BL page (thus not needing any
> BL emulation).
> 
> This patch refines the solution used in the previous patches by
> adding a new SCSIDevice attribute called 'needs_vpl_bl_emulation'.
> This flag is set at scsi_block_realize using a new function called
> 'scsi_block_set_vpd_bl_emulation'. This new function queries the
> Inquiry Supported Pages of the device and checks if it supports
> the BL message. If it doesn't, the emulation flag is set to 'true'.
> 
> This flag is then used at scsi_read_complete to isolate the emulation
> logic from the devices that does not require it.
> 
> Signed-off-by: Daniel Henrique Barboza <address@hidden>
> ---
>  hw/scsi/scsi-disk.c    | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/scsi/scsi-generic.c | 32 +++++++++++++++-----------------
>  include/hw/scsi/scsi.h |  1 +
>  3 files changed, 65 insertions(+), 17 deletions(-)

Please squash this in the previous patch.  I wonder if it should be
limited to scsi-block, or it should be done for all TYPE_DISK
passthrough devices.

In that case, you could do the check in
scsi_generic_read_device_identification (possibly renaming it to
scsi_generic_read_device_inquiry).

Thanks,

Paolo

> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 4461a592e5..cb53d0fdab 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2599,6 +2599,54 @@ static int get_device_type(SCSIDiskState *s)
>      return 0;
>  }
>  
> +static void scsi_block_set_vpd_bl_emulation(SCSIDevice *s)
> +{
> +    uint8_t cmd[6];
> +    uint8_t buf[250];
> +    uint8_t sensebuf[8];
> +    uint8_t page_len;
> +    sg_io_hdr_t io_header;
> +    int ret, i;
> +
> +    memset(cmd, 0, sizeof(cmd));
> +    memset(buf, 0, sizeof(buf));
> +    cmd[0] = INQUIRY;
> +    cmd[1] = 1;
> +    cmd[2] = 0x00;
> +    cmd[4] = sizeof(buf);
> +
> +    memset(&io_header, 0, sizeof(io_header));
> +    io_header.interface_id = 'S';
> +    io_header.dxfer_direction = SG_DXFER_FROM_DEV;
> +    io_header.dxfer_len = sizeof(buf);
> +    io_header.dxferp = buf;
> +    io_header.cmdp = cmd;
> +    io_header.cmd_len = sizeof(cmd);
> +    io_header.mx_sb_len = sizeof(sensebuf);
> +    io_header.sbp = sensebuf;
> +    io_header.timeout = 6000; /* XXX */
> +
> +    ret = blk_ioctl(s->conf.blk, SG_IO, &io_header);
> +    if (ret < 0 || io_header.driver_status || io_header.host_status) {
> +        /*
> +         * Do not assume anything if we can't retrieve the
> +         * INQUIRY response to assert the VPD Block Limits
> +         * support.
> +         */
> +        s->needs_vpd_bl_emulation = false;
> +        return;
> +    }
> +
> +    page_len = buf[3];
> +    for (i = 4; i < page_len + 4; i++) {
> +        if (buf[i] == 0xb0) {
> +            s->needs_vpd_bl_emulation = false;
> +            return;
> +        }
> +    }
> +    s->needs_vpd_bl_emulation = true;
> +}
> +
>  static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> @@ -2648,6 +2696,7 @@ static void scsi_block_realize(SCSIDevice *dev, Error 
> **errp)
>  
>      scsi_realize(&s->qdev, errp);
>      scsi_generic_read_device_identification(&s->qdev);
> +    scsi_block_set_vpd_bl_emulation(dev);
>  }
>  
>  typedef struct SCSIBlockReq {
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 64d3b79518..e08ffaa38c 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -243,9 +243,8 @@ static void scsi_read_complete(void * opaque, int ret)
>  {
>      SCSIGenericReq *r = (SCSIGenericReq *)opaque;
>      SCSIDevice *s = r->req.dev;
> -    SCSISense sense;
>      uint8_t page, page_len;
> -    int len, i;
> +    int len;
>  
>      assert(r->req.aiocb != NULL);
>      r->req.aiocb = NULL;
> @@ -328,11 +327,17 @@ static void scsi_read_complete(void * opaque, int ret)
>                   * the buffer, clean up the io_header to avoid firing up
>                   * the sense error.
>                   */
> -                if (sg_io_sense_from_errno(-ret, &r->io_header, &sense)) {
> +                if (s->needs_vpd_bl_emulation) {
> +
>                      r->buflen = scsi_emulate_vpd_bl_page(s, r->buf);
>                      r->io_header.sb_len_wr = 0;
>  
> -                    /* Clean sg_io_sense */
> +                    /*
> +                     * We have valid contents in the reply buffer but the
> +                     * io_header will report a sense error coming from
> +                     * the hardware in scsi_command_complete_noio. Clean it
> +                     * up the io_header to avoid reporting it.
> +                     */
>                      r->io_header.driver_status = 0;
>                      r->io_header.status = 0;
>  
> @@ -346,26 +351,19 @@ static void scsi_read_complete(void * opaque, int ret)
>                      stl_be_p(&r->buf[12],
>                               MIN_NON_ZERO(max_transfer, 
> ldl_be_p(&r->buf[12])));
>                  }
> -            } else if (page == 0x00) {
> +            } else if (page == 0x00 && s->needs_vpd_bl_emulation) {
>                  /*
>                   * Now we're capable of supplying the VPD Block Limits
> -                 * response if the hardware can't. Inspect if the INQUIRY
> -                 * response contains support for the VPD Block Limits page.
> -                 * Add it if it doesn't.
> +                 * response if the hardware can't. Add it in the INQUIRY
> +                 * Supported VPD pages response in case we are using the
> +                 * emulation for this device.
>                   *
>                   * This way, the guest kernel will be aware of the support
>                   * and will use it to proper setup the SCSI device.
>                   */
>                  page_len = r->buf[3];
> -                for (i = 4; i < page_len + 4; i++) {
> -                    if (r->buf[i] == 0xb0) {
> -                        break;
> -                    }
> -                }
> -                if (i == page_len + 4) {
> -                    r->buf[i] = 0xb0;
> -                    r->buf[3] = ++page_len;
> -                }
> +                r->buf[page_len + 4] = 0xb0;
> +                r->buf[3] = ++page_len;
>              }
>          }
>      }
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 4fdde102b8..5fba858b11 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -90,6 +90,7 @@ struct SCSIDevice
>      uint64_t port_wwn;
>      int scsi_version;
>      int default_scsi_version;
> +    bool needs_vpd_bl_emulation;
>  };
>  
>  extern const VMStateDescription vmstate_scsi_device;
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]