Re: [Qemu-devel] [PATCH v1 3/3] scsi-block: adding flag at realize to enable Block Limits emulation
On 06/21/2018 07:01 AM, Paolo Bonzini wrote:
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
---
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.
I'll end up doing like you suggested in the patch 1/3 review (a cleanup
patch first, the whole feature later on in a single patch).
In that case, you could do the check in
scsi_generic_read_device_identification (possibly renaming it to
scsi_generic_read_device_inquiry).
Sounds good. I'll see how it goes in v2.
Thanks,
Daniel
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
+ * th
Re: [Qemu-devel] [PATCH v1 3/3] scsi-block: adding flag at realize to enable Block Limits emulation
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
> ---
> 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
[Qemu-devel] [PATCH v1 3/3] scsi-block: adding flag at realize to enable Block Limits emulation
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
---
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(-)
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 VP
