[libvirt] [PATCH 0/3] scsi-disk: Device Identification fixes
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
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
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 */
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.
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
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
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=3e... 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).
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=3e...
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.
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=3e...
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
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.
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
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.
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.
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.
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
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 :|
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
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 :|
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
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
participants (6)
-
Daniel P. Berrangé -
Eric Blake -
Kevin Wolf -
Markus Armbruster -
Peter Krempa -
Philippe Mathieu-Daudé