[Qemu-devel] [PATCH 0/3] scsi-disk: Device Identification fixes

[Qemu-devel] [PATCH 0/3] scsi-disk: Device Identification fixes

[Kevin Wolf]

The vendor specific designator in the Device Identification VPD page has
two problems:

1. It defaults to the BlockBackend name (-drive id=...), which everyone
   expected to be a host detail that the guest never sees

2. With -blockdev based setups it defaults to an empty string; if this
   default is used with more than one disk, the guest OS will interpret
   this as a single multipath disk.

We can address problem 2 immediately, and start running the deprecation
clock for problem 1.

Related bug reports:
https://bugzilla.redhat.com/show_bug.cgi?id=1669446
https://bugzilla.redhat.com/show_bug.cgi?id=1668248

Kevin Wolf (3):
  scsi-disk: Don't use empty string as device id
  scsi-disk: Add device_id property
  scsi-disk: Deprecate device_id fallback to BlockBackend name

 hw/scsi/scsi-disk.c  | 41 ++++++++++++++++++++++++++++-------------
 qemu-deprecated.texi | 22 +++++++++++++++++++++-
 2 files changed, 49 insertions(+), 14 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/3] scsi-disk: Don't use empty string as device id

[Kevin Wolf]

scsi-disk includes in the Device Identification VPD page, depending on
configuration amongst others, a vendor specific designator that consists
either of the serial number if given or the BlockBackend name (which is
a host detail that better shouldn't have been leaked to the guest, but
now we have to maintain it for compatibility).

With anonymous BlockBackends, i.e. scsi-disk devices constructed with
drive=<node-name>, and no serial number explicitly specified, this ends
up as an empty string. If this happens to more than one disk, we have
accidentally signalled to the OS that this is a multipath setup, which
is obviously not what was intended.

Instead of using an empty string for the vendor specific designator,
simply leave out that designator, which makes Linux detect such setups
as separate disks again.

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

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 0e9027c8f3..93eef40b87 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -652,12 +652,14 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf)
         DPRINTF("Inquiry EVPD[Device identification] "
                 "buffer size %zd\n", req->cmd.xfer);
 
-        outbuf[buflen++] = 0x2; /* ASCII */
-        outbuf[buflen++] = 0;   /* not officially assigned */
-        outbuf[buflen++] = 0;   /* reserved */
-        outbuf[buflen++] = id_len; /* length of data following */
-        memcpy(outbuf + buflen, str, id_len);
-        buflen += id_len;
+        if (id_len) {
+            outbuf[buflen++] = 0x2; /* ASCII */
+            outbuf[buflen++] = 0;   /* not officially assigned */
+            outbuf[buflen++] = 0;   /* reserved */
+            outbuf[buflen++] = id_len; /* length of data following */
+            memcpy(outbuf + buflen, str, id_len);
+            buflen += id_len;
+        }
 
         if (s->qdev.wwn) {
             outbuf[buflen++] = 0x1; /* Binary */
-- 
2.20.1

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

[Kevin Wolf]

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

[Qemu-devel] [PATCH 3/3] scsi-disk: Deprecate device_id fallback to BlockBackend name

[Kevin Wolf]

We should never have exposed BlockBackend names to the guest, it's a
host detail. Deprecate this behaviour. Users who need to maintain the
guest ABI can explicitly set the value with the device_id property.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi/scsi-disk.c  |  5 +++++
 qemu-deprecated.texi | 22 +++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e74e1e7c48..38f1fe2570 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2366,6 +2366,11 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
             const char *str = blk_name(s->qdev.conf.blk);
             if (str && *str) {
                 s->device_id = g_strdup(str);
+                warn_report("Using the backend drive ID for the Device "
+                            "Identification VPD page is deprecated. "
+                            "Please specify the serial or device_id options "
+                            "explicitly to avoid guest-visible changes in "
+                            "future QEMU versions.");
             }
         }
     }
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 219206a836..a426d8245d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -146,7 +146,7 @@ This machine type uses an unmaintained firmware, broken in lots of ways,
 and unable to start post-2004 operating systems. 40p machine type should be
 used instead.
 
-@section Device options
+@section Devices
 
 @subsection Block device options
 
@@ -170,6 +170,26 @@ The above, converted to the current supported format:
 
 @code{json:@{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"@}}
 
+@subsection scsi-disk device identification (since 4.0.0)
+
+The Device Identification VPD page of ``scsi-disk'' devices contains a vendor
+specific designator that can be explicitly specified using the ``device_id''
+property since 4.0.0.
+
+If ``device_id'' is not given, the implementation falls back to reusing any
+specified serial number for this field. If the serial number is not given
+either and ``drive=X'' was used where ``X'' is a drive ID, this drive ID is
+given to the guest instead. This is backend information that should not
+to be exposed to guests.
+
+Therefore, this behaviour is deprecated and future versions will change the
+guest-visible behaviour (e.g. by leaving out the vendor specific designator)
+for the case that neither ``device_id'' nor a serial number are given, but a
+drive ID is used to create the device.
+
+If you need the guest-visible information to stay unchanged, add an explicit
+``device_id'' option to your QEMU invocation.
+
 @subsection vio-spapr-device device options
 
 @subsubsection "irq": "" (since 3.0.0)
-- 
2.20.1

Re: [Qemu-devel] [PATCH 0/3] scsi-disk: Device Identification fixes

[Eric Blake]

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

On 1/25/19 11:46 AM, Kevin Wolf wrote:
> The vendor specific designator in the Device Identification VPD page has
> two problems:
> 
> 1. It defaults to the BlockBackend name (-drive id=...), which everyone
>    expected to be a host detail that the guest never sees
> 
> 2. With -blockdev based setups it defaults to an empty string; if this
>    default is used with more than one disk, the guest OS will interpret
>    this as a single multipath disk.
> 
> We can address problem 2 immediately, and start running the deprecation
> clock for problem 1.
> 
> Related bug reports:
> https://bugzilla.redhat.com/show_bug.cgi?id=1669446
> https://bugzilla.redhat.com/show_bug.cgi?id=1668248
> 
> Kevin Wolf (3):
>   scsi-disk: Don't use empty string as device id
>   scsi-disk: Add device_id property
>   scsi-disk: Deprecate device_id fallback to BlockBackend name

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Peter Krempa]

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

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).



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Krempa]

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Kevin Wolf]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.01.2019 um 09:50 hat Peter Krempa geschrieben:
[...]
>> 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.

There's an internal use in scsi_bus_legacy_add_drive(), which in turn
powers two legacy features:

1. -drive if=scsi

   Creates scsi-disk frontends.

   Only works with onboard HBAs since commit 14545097267, v2.12.0.

2. -device usb-storage

   Bad magic: usb-storage pretends to be a block device, but it's really
   a SCSI bus that can serve only a single device, which it creates
   automatically.

If we deprecate scsi-disk, we should deprecate these, too.  Can't say
whether that's practical right now.

>> 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.

Compiling out scsi-hd or scsi-cd, but not scsi-disk would be silly.  All
three devices are in scsi-disk.c.  You'd have to hack that up to be
silly.

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

[Kevin Wolf]

Am 28.01.2019 um 17:55 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 28.01.2019 um 09:50 hat Peter Krempa geschrieben:
> [...]
> >> 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.
> 
> There's an internal use in scsi_bus_legacy_add_drive(), which in turn
> powers two legacy features:
> 
> 1. -drive if=scsi
> 
>    Creates scsi-disk frontends.
> 
>    Only works with onboard HBAs since commit 14545097267, v2.12.0.
> 
> 2. -device usb-storage
> 
>    Bad magic: usb-storage pretends to be a block device, but it's really
>    a SCSI bus that can serve only a single device, which it creates
>    automatically.
> 
> If we deprecate scsi-disk, we should deprecate these, too.  Can't say
> whether that's practical right now.

Most likely not worth the effort anyway. I don't think it's blocking
anything.

> >> 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.
> 
> Compiling out scsi-hd or scsi-cd, but not scsi-disk would be silly.  All
> three devices are in scsi-disk.c.  You'd have to hack that up to be
> silly.

I understood this as a question about libvirt, i.e. whether libvirt can
drop/compile out their scsi-disk code and instead assume that scsi-hd/cd
are always present. Maybe I misunderstood, though?

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.01.2019 um 17:55 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 28.01.2019 um 09:50 hat Peter Krempa geschrieben:
>> [...]
>> >> 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.
>> 
>> There's an internal use in scsi_bus_legacy_add_drive(), which in turn
>> powers two legacy features:
>> 
>> 1. -drive if=scsi
>> 
>>    Creates scsi-disk frontends.
>> 
>>    Only works with onboard HBAs since commit 14545097267, v2.12.0.
>> 
>> 2. -device usb-storage
>> 
>>    Bad magic: usb-storage pretends to be a block device, but it's really
>>    a SCSI bus that can serve only a single device, which it creates
>>    automatically.
>> 
>> If we deprecate scsi-disk, we should deprecate these, too.  Can't say
>> whether that's practical right now.
>
> Most likely not worth the effort anyway. I don't think it's blocking
> anything.

We could also wean them off the legacy device models.

>> >> 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.
>> 
>> Compiling out scsi-hd or scsi-cd, but not scsi-disk would be silly.  All
>> three devices are in scsi-disk.c.  You'd have to hack that up to be
>> silly.
>
> I understood this as a question about libvirt, i.e. whether libvirt can
> drop/compile out their scsi-disk code and instead assume that scsi-hd/cd
> are always present. Maybe I misunderstood, though?

If questions remain, I trust Peter will ask.

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

[Peter Krempa]

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

On Mon, Jan 28, 2019 at 17:55:14 +0100, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 28.01.2019 um 09:50 hat Peter Krempa geschrieben:
> [...]
> >> 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.
> 
> There's an internal use in scsi_bus_legacy_add_drive(), which in turn
> powers two legacy features:
> 
> 1. -drive if=scsi
> 
>    Creates scsi-disk frontends.
> 
>    Only works with onboard HBAs since commit 14545097267, v2.12.0.
> 
> 2. -device usb-storage
> 
>    Bad magic: usb-storage pretends to be a block device, but it's really
>    a SCSI bus that can serve only a single device, which it creates
>    automatically.
> 
> If we deprecate scsi-disk, we should deprecate these, too.  Can't say
> whether that's practical right now.

Unfortunately we did not bother replacing usb-storage yet.

scsi-disk was unused for some time (if scsi-hd was supported). I just
deleted any mentions of it from libvirt now.

> 
> >> 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.
> 
> Compiling out scsi-hd or scsi-cd, but not scsi-disk would be silly.  All
> three devices are in scsi-disk.c.  You'd have to hack that up to be
> silly.

That would be a downstream modification of qemu thus libvirt will
not want to support that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Krempa]

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

On Tue, Jan 29, 2019 at 08:10:19 +0100, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 28.01.2019 um 17:55 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 28.01.2019 um 09:50 hat Peter Krempa geschrieben:
> >> [...]
> >> >> 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.
> >> 
> >> There's an internal use in scsi_bus_legacy_add_drive(), which in turn
> >> powers two legacy features:
> >> 
> >> 1. -drive if=scsi
> >> 
> >>    Creates scsi-disk frontends.
> >> 
> >>    Only works with onboard HBAs since commit 14545097267, v2.12.0.
> >> 
> >> 2. -device usb-storage
> >> 
> >>    Bad magic: usb-storage pretends to be a block device, but it's really
> >>    a SCSI bus that can serve only a single device, which it creates
> >>    automatically.
> >> 
> >> If we deprecate scsi-disk, we should deprecate these, too.  Can't say
> >> whether that's practical right now.
> >
> > Most likely not worth the effort anyway. I don't think it's blocking
> > anything.
> 
> We could also wean them off the legacy device models.

In libvirt we should get rid of usb-storage usage, but I remember there
were a few ABI-related problems so that we could not plainly switch to
uas.

> >> >> 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.
> >> 
> >> Compiling out scsi-hd or scsi-cd, but not scsi-disk would be silly.  All
> >> three devices are in scsi-disk.c.  You'd have to hack that up to be
> >> silly.
> >
> > I understood this as a question about libvirt, i.e. whether libvirt can
> > drop/compile out their scsi-disk code and instead assume that scsi-hd/cd
> > are always present. Maybe I misunderstood, though?
> 
> If questions remain, I trust Peter will ask.

I in fact wanted to know whether it's possible to compile it out of qemu
somehow. Removing it from libvirt is then easy.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/3] scsi-disk: Don't use empty string as device id

[Philippe Mathieu-Daudé]

On 1/25/19 6:46 PM, Kevin Wolf wrote:
> scsi-disk includes in the Device Identification VPD page, depending on
> configuration amongst others, a vendor specific designator that consists
> either of the serial number if given or the BlockBackend name (which is
> a host detail that better shouldn't have been leaked to the guest, but
> now we have to maintain it for compatibility).
> 
> With anonymous BlockBackends, i.e. scsi-disk devices constructed with
> drive=<node-name>, and no serial number explicitly specified, this ends
> up as an empty string. If this happens to more than one disk, we have
> accidentally signalled to the OS that this is a multipath setup, which
> is obviously not what was intended.
> 
> Instead of using an empty string for the vendor specific designator,
> simply leave out that designator, which makes Linux detect such setups
> as separate disks again.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/scsi/scsi-disk.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 0e9027c8f3..93eef40b87 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -652,12 +652,14 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf)
>          DPRINTF("Inquiry EVPD[Device identification] "
>                  "buffer size %zd\n", req->cmd.xfer);
>  
> -        outbuf[buflen++] = 0x2; /* ASCII */
> -        outbuf[buflen++] = 0;   /* not officially assigned */
> -        outbuf[buflen++] = 0;   /* reserved */
> -        outbuf[buflen++] = id_len; /* length of data following */
> -        memcpy(outbuf + buflen, str, id_len);
> -        buflen += id_len;
> +        if (id_len) {
> +            outbuf[buflen++] = 0x2; /* ASCII */
> +            outbuf[buflen++] = 0;   /* not officially assigned */
> +            outbuf[buflen++] = 0;   /* reserved */
> +            outbuf[buflen++] = id_len; /* length of data following */
> +            memcpy(outbuf + buflen, str, id_len);
> +            buflen += id_len;
> +        }
>  
>          if (s->qdev.wwn) {
>              outbuf[buflen++] = 0x1; /* Binary */
> 

Re: [Qemu-devel] [PATCH 3/3] scsi-disk: Deprecate device_id fallback to BlockBackend name

[Daniel P. Berrangé]

On Fri, Jan 25, 2019 at 06:46:53PM +0100, Kevin Wolf wrote:
> We should never have exposed BlockBackend names to the guest, it's a
> host detail. Deprecate this behaviour. Users who need to maintain the
> guest ABI can explicitly set the value with the device_id property.

I don't think we've deprecated things in the past which would cause
a guest ABI change when finally deleted.

Shouldn't we be instead setting using machine type versioning here,
so that existing machine types keep using the blk_name() fallback
forever, while QEMU 4.0+ machine types avoid blk_name() fallback
immediately.

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/scsi/scsi-disk.c  |  5 +++++
>  qemu-deprecated.texi | 22 +++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e74e1e7c48..38f1fe2570 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2366,6 +2366,11 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
>              const char *str = blk_name(s->qdev.conf.blk);
>              if (str && *str) {
>                  s->device_id = g_strdup(str);
> +                warn_report("Using the backend drive ID for the Device "
> +                            "Identification VPD page is deprecated. "
> +                            "Please specify the serial or device_id options "
> +                            "explicitly to avoid guest-visible changes in "
> +                            "future QEMU versions.");
>              }
>          }
>      }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/3] scsi-disk: Don't use empty string as device id

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> scsi-disk includes in the Device Identification VPD page, depending on
> configuration amongst others, a vendor specific designator that consists
> either of the serial number if given or the BlockBackend name (which is
> a host detail that better shouldn't have been leaked to the guest, but
> now we have to maintain it for compatibility).
>
> With anonymous BlockBackends, i.e. scsi-disk devices constructed with
> drive=<node-name>, and no serial number explicitly specified, this ends
> up as an empty string. If this happens to more than one disk, we have
> accidentally signalled to the OS that this is a multipath setup, which
> is obviously not what was intended.
>
> Instead of using an empty string for the vendor specific designator,
> simply leave out that designator, which makes Linux detect such setups
> as separate disks again.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 0e9027c8f3..93eef40b87 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -652,12 +652,14 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf)
>          DPRINTF("Inquiry EVPD[Device identification] "
>                  "buffer size %zd\n", req->cmd.xfer);
>  
> -        outbuf[buflen++] = 0x2; /* ASCII */
> -        outbuf[buflen++] = 0;   /* not officially assigned */
> -        outbuf[buflen++] = 0;   /* reserved */
> -        outbuf[buflen++] = id_len; /* length of data following */
> -        memcpy(outbuf + buflen, str, id_len);
> -        buflen += id_len;
> +        if (id_len) {
> +            outbuf[buflen++] = 0x2; /* ASCII */
> +            outbuf[buflen++] = 0;   /* not officially assigned */
> +            outbuf[buflen++] = 0;   /* reserved */
> +            outbuf[buflen++] = id_len; /* length of data following */
> +            memcpy(outbuf + buflen, str, id_len);
> +            buflen += id_len;
> +        }
>  
>          if (s->qdev.wwn) {
>              outbuf[buflen++] = 0x1; /* Binary */

Before the patch, we always add this descriptor, but as you explain in
your commit message, its contents can be wrong.

After the patch, we add this descriptor only when we have a suitable
name (we use serial number, else falling back to BlockBackend name).
It's possible we add *no* descriptors.  I wonder whether that's okay.  I
consulted section SPC-4 section 7.8.5 Device Identification VPD page,
but failed to penetrate the dense prose there.

Re: [Qemu-devel] [PATCH 1/3] scsi-disk: Don't use empty string as device id

[Kevin Wolf]

Am 29.01.2019 um 17:37 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > scsi-disk includes in the Device Identification VPD page, depending on
> > configuration amongst others, a vendor specific designator that consists
> > either of the serial number if given or the BlockBackend name (which is
> > a host detail that better shouldn't have been leaked to the guest, but
> > now we have to maintain it for compatibility).
> >
> > With anonymous BlockBackends, i.e. scsi-disk devices constructed with
> > drive=<node-name>, and no serial number explicitly specified, this ends
> > up as an empty string. If this happens to more than one disk, we have
> > accidentally signalled to the OS that this is a multipath setup, which
> > is obviously not what was intended.
> >
> > Instead of using an empty string for the vendor specific designator,
> > simply leave out that designator, which makes Linux detect such setups
> > as separate disks again.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/scsi/scsi-disk.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index 0e9027c8f3..93eef40b87 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -652,12 +652,14 @@ static int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf)
> >          DPRINTF("Inquiry EVPD[Device identification] "
> >                  "buffer size %zd\n", req->cmd.xfer);
> >  
> > -        outbuf[buflen++] = 0x2; /* ASCII */
> > -        outbuf[buflen++] = 0;   /* not officially assigned */
> > -        outbuf[buflen++] = 0;   /* reserved */
> > -        outbuf[buflen++] = id_len; /* length of data following */
> > -        memcpy(outbuf + buflen, str, id_len);
> > -        buflen += id_len;
> > +        if (id_len) {
> > +            outbuf[buflen++] = 0x2; /* ASCII */
> > +            outbuf[buflen++] = 0;   /* not officially assigned */
> > +            outbuf[buflen++] = 0;   /* reserved */
> > +            outbuf[buflen++] = id_len; /* length of data following */
> > +            memcpy(outbuf + buflen, str, id_len);
> > +            buflen += id_len;
> > +        }
> >  
> >          if (s->qdev.wwn) {
> >              outbuf[buflen++] = 0x1; /* Binary */
> 
> Before the patch, we always add this descriptor, but as you explain in
> your commit message, its contents can be wrong.
> 
> After the patch, we add this descriptor only when we have a suitable
> name (we use serial number, else falling back to BlockBackend name).
> It's possible we add *no* descriptors.  I wonder whether that's okay.  I
> consulted section SPC-4 section 7.8.5 Device Identification VPD page,
> but failed to penetrate the dense prose there.

I wasn't completely sure about the interpretation either, but to me the
most likely one is that according to SPC-4 not having a descriptor is
illegal because 7.8.5.2.1 requires that at least one descriptor of type
1, 2, 3 or 8 shall be included (all of them contain some sort of a
registered vendor ID, which we don't have).

The one that I'm removing is type 0, so it didn't meet the requirement
even before and I concluded that if this patch doesn't make things worse
in terms of spec compliance and fixes things in practice, it can't be
completely wrong.

Kevin

Re: [Qemu-devel] [PATCH 0/3] scsi-disk: Device Identification fixes

[Kevin Wolf]

Am 25.01.2019 um 18:46 hat Kevin Wolf geschrieben:
> The vendor specific designator in the Device Identification VPD page has
> two problems:
> 
> 1. It defaults to the BlockBackend name (-drive id=...), which everyone
>    expected to be a host detail that the guest never sees
> 
> 2. With -blockdev based setups it defaults to an empty string; if this
>    default is used with more than one disk, the guest OS will interpret
>    this as a single multipath disk.
> 
> We can address problem 2 immediately, and start running the deprecation
> clock for problem 1.
> 
> Related bug reports:
> https://bugzilla.redhat.com/show_bug.cgi?id=1669446
> https://bugzilla.redhat.com/show_bug.cgi?id=1668248

Applied patches 1 and 2 to the block branch so that the bug is fixed.
I'll try to rewrite patch 3 along the lines Dan suggested.

Kevin

Re: [Qemu-devel] [PATCH 3/3] scsi-disk: Deprecate device_id fallback to BlockBackend name

[Kevin Wolf]

Am 29.01.2019 um 16:58 hat Daniel P. Berrangé geschrieben:
> On Fri, Jan 25, 2019 at 06:46:53PM +0100, Kevin Wolf wrote:
> > We should never have exposed BlockBackend names to the guest, it's a
> > host detail. Deprecate this behaviour. Users who need to maintain the
> > guest ABI can explicitly set the value with the device_id property.
> 
> I don't think we've deprecated things in the past which would cause
> a guest ABI change when finally deleted.
> 
> Shouldn't we be instead setting using machine type versioning here,
> so that existing machine types keep using the blk_name() fallback
> forever, while QEMU 4.0+ machine types avoid blk_name() fallback
> immediately.

I was going to implement this, but then I wondered if that's actually
worthwhile. With -blockdev, you don't get the bad behaviour anyway, and
tools that would select new machine types will hopefully also use new
backend options.

So if we need to keep the bad behaviour around anyway instead of
completely removing it as I intended originally, maybe -drive vs.
-blockdev is enough to make the difference and we shouldn't change
anything at the device model level at all.

Kevin

Re: [Qemu-devel] [PATCH 3/3] scsi-disk: Deprecate device_id fallback to BlockBackend name

[Daniel P. Berrangé]

On Mon, Feb 04, 2019 at 10:56:28AM +0100, Kevin Wolf wrote:
> Am 29.01.2019 um 16:58 hat Daniel P. Berrangé geschrieben:
> > On Fri, Jan 25, 2019 at 06:46:53PM +0100, Kevin Wolf wrote:
> > > We should never have exposed BlockBackend names to the guest, it's a
> > > host detail. Deprecate this behaviour. Users who need to maintain the
> > > guest ABI can explicitly set the value with the device_id property.
> > 
> > I don't think we've deprecated things in the past which would cause
> > a guest ABI change when finally deleted.
> > 
> > Shouldn't we be instead setting using machine type versioning here,
> > so that existing machine types keep using the blk_name() fallback
> > forever, while QEMU 4.0+ machine types avoid blk_name() fallback
> > immediately.
> 
> I was going to implement this, but then I wondered if that's actually
> worthwhile. With -blockdev, you don't get the bad behaviour anyway, and
> tools that would select new machine types will hopefully also use new
> backend options.
> 
> So if we need to keep the bad behaviour around anyway instead of
> completely removing it as I intended originally, maybe -drive vs.
> -blockdev is enough to make the difference and we shouldn't change
> anything at the device model level at all.

Yeah, that rationale sounds ok to me.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|