scsi-disk: Device Identification fixes

[libvirt] [PATCH 2/3] scsi-disk: Add device_id property

Posted by Kevin Wolf 6 years, 9 months ago

The new device_id property specifies which value to use for the vendor
specific designator in the Device Identification VPD page.

In particular, this is necessary for libvirt to maintain guest ABI
compatibility when no serial number is given and a VM is switched from
-drive (where the BlockBackend name is used) to -blockdev (where the
vendor specific designator is left out by default).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi/scsi-disk.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 93eef40b87..e74e1e7c48 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -104,6 +104,7 @@ typedef struct SCSIDiskState
     char *serial;
     char *vendor;
     char *product;
+    char *device_id;
     bool tray_open;
     bool tray_locked;
     /*
@@ -642,13 +643,8 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf)
 
     case 0x83: /* Device identification page, mandatory */
     {
-        const char *str = s->serial ?: blk_name(s->qdev.conf.blk);
-        int max_len = s->serial ? 20 : 255 - 8;
-        int id_len = strlen(str);
+        int id_len = s->device_id ? MIN(strlen(s->device_id), 255 - 8) : 0;
 
-        if (id_len > max_len) {
-            id_len = max_len;
-        }
         DPRINTF("Inquiry EVPD[Device identification] "
                 "buffer size %zd\n", req->cmd.xfer);
 
@@ -657,7 +653,7 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf)
             outbuf[buflen++] = 0;   /* not officially assigned */
             outbuf[buflen++] = 0;   /* reserved */
             outbuf[buflen++] = id_len; /* length of data following */
-            memcpy(outbuf + buflen, str, id_len);
+            memcpy(outbuf + buflen, s->device_id, id_len);
             buflen += id_len;
         }
 
@@ -2363,6 +2359,16 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
     if (!s->vendor) {
         s->vendor = g_strdup("QEMU");
     }
+    if (!s->device_id) {
+        if (s->serial) {
+            s->device_id = g_strdup_printf("%.20s", s->serial);
+        } else {
+            const char *str = blk_name(s->qdev.conf.blk);
+            if (str && *str) {
+                s->device_id = g_strdup(str);
+            }
+        }
+    }
 
     if (blk_is_sg(s->qdev.conf.blk)) {
         error_setg(errp, "unwanted /dev/sg*");
@@ -2904,7 +2910,9 @@ static const TypeInfo scsi_disk_base_info = {
     DEFINE_PROP_STRING("ver", SCSIDiskState, version),               \
     DEFINE_PROP_STRING("serial", SCSIDiskState, serial),             \
     DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor),             \
-    DEFINE_PROP_STRING("product", SCSIDiskState, product)
+    DEFINE_PROP_STRING("product", SCSIDiskState, product),           \
+    DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id)
+
 
 static Property scsi_hd_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] scsi-disk: Add device_id property

Posted by Peter Krempa 6 years, 9 months ago

On Fri, Jan 25, 2019 at 18:46:52 +0100, Kevin Wolf wrote:
> The new device_id property specifies which value to use for the vendor
> specific designator in the Device Identification VPD page.
> 
> In particular, this is necessary for libvirt to maintain guest ABI
> compatibility when no serial number is given and a VM is switched from
> -drive (where the BlockBackend name is used) to -blockdev (where the
> vendor specific designator is left out by default).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)

[...]

> @@ -2904,7 +2910,9 @@ static const TypeInfo scsi_disk_base_info = {
>      DEFINE_PROP_STRING("ver", SCSIDiskState, version),               \
>      DEFINE_PROP_STRING("serial", SCSIDiskState, serial),             \
>      DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor),             \
> -    DEFINE_PROP_STRING("product", SCSIDiskState, product)
> +    DEFINE_PROP_STRING("product", SCSIDiskState, product),           \
> +    DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id)

This adds the property only to 'scsi-disk', whereas libvirt will use
'scsi-cd' or 'scsi-hd' depending on the media type if the 'scsi-cd'
device is detected. The following logic decides:

https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_command.c;h=3e46f3ced3c1e6a4865e98e2d0cdce3214c4a5d2;hb=HEAD#l1978

This brings multiple questions:

1) Is this necssary also for scsi-cd/scsi-hd and if yes, the property
does not seem to be present for those

2) Is actually using 'scsi-cd'/'scsi-hd' the better option than
'scsi-disk'?

3) Since upstream libvirt supports qemu-1.5 and newer and 'scsi-cd' is
already supported there, can we assume that all newer versions support
it? (Basically the question is whether it can be compiled out by
upstream means).


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] scsi-disk: Add device_id property

Posted by Peter Krempa 6 years, 9 months ago

On Mon, Jan 28, 2019 at 09:50:41 +0100, Peter Krempa wrote:
> On Fri, Jan 25, 2019 at 18:46:52 +0100, Kevin Wolf wrote:
> > The new device_id property specifies which value to use for the vendor
> > specific designator in the Device Identification VPD page.
> > 
> > In particular, this is necessary for libvirt to maintain guest ABI
> > compatibility when no serial number is given and a VM is switched from
> > -drive (where the BlockBackend name is used) to -blockdev (where the
> > vendor specific designator is left out by default).
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/scsi/scsi-disk.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> [...]
> 
> > @@ -2904,7 +2910,9 @@ static const TypeInfo scsi_disk_base_info = {
> >      DEFINE_PROP_STRING("ver", SCSIDiskState, version),               \
> >      DEFINE_PROP_STRING("serial", SCSIDiskState, serial),             \
> >      DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor),             \
> > -    DEFINE_PROP_STRING("product", SCSIDiskState, product)
> > +    DEFINE_PROP_STRING("product", SCSIDiskState, product),           \
> > +    DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id)
> 
> This adds the property only to 'scsi-disk', whereas libvirt will use
> 'scsi-cd' or 'scsi-hd' depending on the media type if the 'scsi-cd'
> device is detected. The following logic decides:
> 
> https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_command.c;h=3e46f3ced3c1e6a4865e98e2d0cdce3214c4a5d2;hb=HEAD#l1978
> 
> This brings multiple questions:

I've found:

commit b443ae67130d32ad06b06fc9aa6d04d05ccd93ce
Author: Markus Armbruster <armbru@redhat.com>
Date:   Mon May 16 15:04:53 2011 +0200

    scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"

Which seems to put scsi-hd and scsi-cd as the thing to use.

Libvirt's code obviously did not adapt after that and we still keep to
probe scsi-disk instead of the split up devices, whereas we will never
use scsi-disk.

Also some of our tests seem to look as if scsi-disk was used which
confused me.

Since the new property is present also for scsi-hd and scsi-cd we really
need to adapt our code to that.

Btw, this also means that qemu can deprecate scsi-disk.

Sorry for the noise.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] scsi-disk: Add device_id property

Posted by Kevin Wolf 6 years, 9 months ago

Am 28.01.2019 um 09:50 hat Peter Krempa geschrieben:
> On Fri, Jan 25, 2019 at 18:46:52 +0100, Kevin Wolf wrote:
> > The new device_id property specifies which value to use for the vendor
> > specific designator in the Device Identification VPD page.
> > 
> > In particular, this is necessary for libvirt to maintain guest ABI
> > compatibility when no serial number is given and a VM is switched from
> > -drive (where the BlockBackend name is used) to -blockdev (where the
> > vendor specific designator is left out by default).
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/scsi/scsi-disk.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> [...]
> 
> > @@ -2904,7 +2910,9 @@ static const TypeInfo scsi_disk_base_info = {
> >      DEFINE_PROP_STRING("ver", SCSIDiskState, version),               \
> >      DEFINE_PROP_STRING("serial", SCSIDiskState, serial),             \
> >      DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor),             \
> > -    DEFINE_PROP_STRING("product", SCSIDiskState, product)
> > +    DEFINE_PROP_STRING("product", SCSIDiskState, product),           \
> > +    DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id)
> 
> This adds the property only to 'scsi-disk', whereas libvirt will use
> 'scsi-cd' or 'scsi-hd' depending on the media type if the 'scsi-cd'
> device is detected.

I'm adding this to the macro DEFINE_SCSI_DISK_PROPERTIES(), which is
used for scsi-hd, scsi-cd and scsi-disk.

It is not added to scsi-block, but there we pass through the VPD page of
the real disk, so this looks correct to me.

> The following logic decides:
> 
> https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_command.c;h=3e46f3ced3c1e6a4865e98e2d0cdce3214c4a5d2;hb=HEAD#l1978
> 
> This brings multiple questions:
> 
> 1) Is this necssary also for scsi-cd/scsi-hd and if yes, the property
> does not seem to be present for those

Yes, but the property is added there.

> 2) Is actually using 'scsi-cd'/'scsi-hd' the better option than
> 'scsi-disk'?

Yes, scsi-disk is a legacy device. Maybe we should formally deprecate
it.

> 3) Since upstream libvirt supports qemu-1.5 and newer and 'scsi-cd' is
> already supported there, can we assume that all newer versions support
> it? (Basically the question is whether it can be compiled out by
> upstream means).

I think so.

Kevin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list