Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
On Tue, May 04, 2021 at 03:32:55PM +0100, Stefan Hajnoczi wrote: > On Thu, Apr 29, 2021 at 02:03:52PM -0400, Eduardo Habkost wrote: > > On Thu, Apr 29, 2021 at 04:52:21PM +0100, Stefan Hajnoczi wrote: > > > Live migrating old guests from an old QEMU with the SCSI feature bit > > > enabled will fail with "Features 0x... unsupported. Allowed features: > > > 0x...". We've followed the QEMU deprecation policy so users have been > > > warned... > > > > > > > Were they really warned, though? People running > > "-machine pc-i440fx-2.4" might be completely unaware that it was > > silently enabling a deprecated feature. > > > > Can we have this documented in a more explicit way? Maybe just a > > comment at hw_compat_2_4 would be enough, to warn people doing > > backports and rebases downstream. > > > > Can we make QEMU refuse to start if using pc-2.4 + virtio-blk > > together, just to be sure? > > On second thought, do we really want to break pc-2.4 user's QEMU > command-lines if they have a virtio-blk device? It depends which command line you are talking about. I believe we _must_ break the following: "-machine pc-i440fx-2.4 -device virtio-blk", and "-machine pc-i440fx-2.4 -device virtio-blk,scsi=on". Your patch breaks only the latter. Your patch also breaks the following: "-machine pc-i440fx-2.4 -device virtio-blk,scsi=off", which I don't think we should break. > > BTW Peter mentioned libvirt avoids the unnecessary scsi=off: > https://gitlab.com/libvirt/libvirt/-/commit/ec69f0190be731d12faeac08dbf63325836509a9 > > Stefan -- Eduardo
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
On Thu, Apr 29, 2021 at 02:03:52PM -0400, Eduardo Habkost wrote: > On Thu, Apr 29, 2021 at 04:52:21PM +0100, Stefan Hajnoczi wrote: > > Live migrating old guests from an old QEMU with the SCSI feature bit > > enabled will fail with "Features 0x... unsupported. Allowed features: > > 0x...". We've followed the QEMU deprecation policy so users have been > > warned... > > > > Were they really warned, though? People running > "-machine pc-i440fx-2.4" might be completely unaware that it was > silently enabling a deprecated feature. > > Can we have this documented in a more explicit way? Maybe just a > comment at hw_compat_2_4 would be enough, to warn people doing > backports and rebases downstream. > > Can we make QEMU refuse to start if using pc-2.4 + virtio-blk > together, just to be sure? On second thought, do we really want to break pc-2.4 user's QEMU command-lines if they have a virtio-blk device? BTW Peter mentioned libvirt avoids the unnecessary scsi=off: https://gitlab.com/libvirt/libvirt/-/commit/ec69f0190be731d12faeac08dbf63325836509a9 Stefan signature.asc Description: PGP signature
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
On Thu, Apr 29, 2021 at 06:16:28PM +0200, Peter Krempa wrote: > On Thu, Apr 29, 2021 at 16:52:21 +0100, Stefan Hajnoczi wrote: > > The scsi=on|off property was deprecated in QEMU 5.0 and can be removed > > completely at this point. > > > > Drop the scsi=on|off option. It was only available on Legacy virtio-blk > > devices. Linux v5.6 already dropped support for it. > > > > Remove the hw_compat_2_4[] property assignment since scsi=on|off no > > longer exists. Old guests with Legacy virtio-blk devices no longer see > > the SCSI host features bit. > > Does this mean that qemu rejects it if it's explicitly enabled on the > commandline? Yes. Stefan signature.asc Description: PGP signature
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
On Thu, Apr 29, 2021 at 02:03:52PM -0400, Eduardo Habkost wrote: > On Thu, Apr 29, 2021 at 04:52:21PM +0100, Stefan Hajnoczi wrote: > > The scsi=on|off property was deprecated in QEMU 5.0 and can be removed > > completely at this point. > > > > Drop the scsi=on|off option. It was only available on Legacy virtio-blk > > devices. Linux v5.6 already dropped support for it. > > > > Remove the hw_compat_2_4[] property assignment since scsi=on|off no > > longer exists. Old guests with Legacy virtio-blk devices no longer see > > the SCSI host features bit. > > > > This means pc-2.4 will now break guest ABI if using virtio-blk > devices, correct? Yes. However, this feature was only enabled on Linux hosts, so cross-host OS migration was always broken and no one noticed. Maybe that configuration is too niche and QEMU never supported cross-host OS migration, but it still means that the "pc-2.4" ABI was never solid to begin with :). > > > Live migrating old guests from an old QEMU with the SCSI feature bit > > enabled will fail with "Features 0x... unsupported. Allowed features: > > 0x...". We've followed the QEMU deprecation policy so users have been > > warned... > > > > Were they really warned, though? People running > "-machine pc-i440fx-2.4" might be completely unaware that it was > silently enabling a deprecated feature. > > Can we have this documented in a more explicit way? Maybe just a > comment at hw_compat_2_4 would be enough, to warn people doing > backports and rebases downstream. > > Can we make QEMU refuse to start if using pc-2.4 + virtio-blk > together, just to be sure? > > An alternative would be keeping the property (and the > hw_compat_2_4 entry) just to keep pc-2.4 working (until pc-2.4 is > deprecated and removed), but refusing to realize the device if > the feature is enabled. Yes, the least invasive approach is to leave the property in place but refuse to realize the virtio-blk device when scsi=on. The cost is more cruft, including a useless scsi=off command-line option that will continue to show up in libvirt-generated QEMU command-lines. The cautious approach makes sense to me and I'll send a new revision. Stefan signature.asc Description: PGP signature
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
* Peter Krempa (pkre...@redhat.com) wrote: > On Fri, Apr 30, 2021 at 09:42:05 +0200, Markus Armbruster wrote: > > Eduardo Habkost writes: > > > > > On Thu, Apr 29, 2021 at 04:52:21PM +0100, Stefan Hajnoczi wrote: > > >> The scsi=on|off property was deprecated in QEMU 5.0 and can be removed > > >> completely at this point. > > >> > > >> Drop the scsi=on|off option. It was only available on Legacy virtio-blk > > >> devices. Linux v5.6 already dropped support for it. > > >> > > >> Remove the hw_compat_2_4[] property assignment since scsi=on|off no > > >> longer exists. Old guests with Legacy virtio-blk devices no longer see > > >> the SCSI host features bit. > > >> > > > > > > This means pc-2.4 will now break guest ABI if using virtio-blk > > > devices, correct? > > > > > > This looks like a sign we should have deprecated pc-2.4 a long > > > time ago. > > > > The last batch of PC machine type retiring was pc-1.0 to pc-1.3: > > deprecated in 5.0 (commit 30d2a17b4, Dec 2019), dropped in 6.0 (commit > > f862ddbb1, just weeks ago). pc-1.3 was a bit over seven years old when > > we released 5.0. pc-2.4 will be six years old by the time we release > > 6.1. Fair game? > > As a data-point, libvirt will be dropping support for (release, not the machine type) in the upcomming release. I'm not sure > whether that justifies more deprecation though. What qemu features will you then be relying on? Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
On Fri, Apr 30, 2021 at 09:42:05 +0200, Markus Armbruster wrote: > Eduardo Habkost writes: > > > On Thu, Apr 29, 2021 at 04:52:21PM +0100, Stefan Hajnoczi wrote: > >> The scsi=on|off property was deprecated in QEMU 5.0 and can be removed > >> completely at this point. > >> > >> Drop the scsi=on|off option. It was only available on Legacy virtio-blk > >> devices. Linux v5.6 already dropped support for it. > >> > >> Remove the hw_compat_2_4[] property assignment since scsi=on|off no > >> longer exists. Old guests with Legacy virtio-blk devices no longer see > >> the SCSI host features bit. > >> > > > > This means pc-2.4 will now break guest ABI if using virtio-blk > > devices, correct? > > > > This looks like a sign we should have deprecated pc-2.4 a long > > time ago. > > The last batch of PC machine type retiring was pc-1.0 to pc-1.3: > deprecated in 5.0 (commit 30d2a17b4, Dec 2019), dropped in 6.0 (commit > f862ddbb1, just weeks ago). pc-1.3 was a bit over seven years old when > we released 5.0. pc-2.4 will be six years old by the time we release > 6.1. Fair game? As a data-point, libvirt will be dropping support for
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
Eduardo Habkost writes: > On Thu, Apr 29, 2021 at 04:52:21PM +0100, Stefan Hajnoczi wrote: >> The scsi=on|off property was deprecated in QEMU 5.0 and can be removed >> completely at this point. >> >> Drop the scsi=on|off option. It was only available on Legacy virtio-blk >> devices. Linux v5.6 already dropped support for it. >> >> Remove the hw_compat_2_4[] property assignment since scsi=on|off no >> longer exists. Old guests with Legacy virtio-blk devices no longer see >> the SCSI host features bit. >> > > This means pc-2.4 will now break guest ABI if using virtio-blk > devices, correct? > > This looks like a sign we should have deprecated pc-2.4 a long > time ago. The last batch of PC machine type retiring was pc-1.0 to pc-1.3: deprecated in 5.0 (commit 30d2a17b4, Dec 2019), dropped in 6.0 (commit f862ddbb1, just weeks ago). pc-1.3 was a bit over seven years old when we released 5.0. pc-2.4 will be six years old by the time we release 6.1. Fair game? >> Live migrating old guests from an old QEMU with the SCSI feature bit >> enabled will fail with "Features 0x... unsupported. Allowed features: >> 0x...". We've followed the QEMU deprecation policy so users have been >> warned... >> > > Were they really warned, though? People running > "-machine pc-i440fx-2.4" might be completely unaware that it was > silently enabling a deprecated feature. We've gotten better at documenting deprecations, but we're still bad at warning on use of deprecated features. [...]
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
On 4/29/21 5:52 PM, Stefan Hajnoczi wrote: > The scsi=on|off property was deprecated in QEMU 5.0 and can be removed > completely at this point. > > Drop the scsi=on|off option. It was only available on Legacy virtio-blk > devices. Linux v5.6 already dropped support for it. > > Remove the hw_compat_2_4[] property assignment since scsi=on|off no > longer exists. Old guests with Legacy virtio-blk devices no longer see > the SCSI host features bit. > > Live migrating old guests from an old QEMU with the SCSI feature bit > enabled will fail with "Features 0x... unsupported. Allowed features: > 0x...". We've followed the QEMU deprecation policy so users have been > warned... > > I have tested that libvirt still works when the property is absent. It > no longer adds scsi=on|off to the command-line. > > Cc: Markus Armbruster > Cc: Christoph Hellwig > Cc: Peter Krempa > Cc: Dr. David Alan Gilbert > Signed-off-by: Stefan Hajnoczi > --- > docs/specs/tpm.rst | 2 +- > docs/system/deprecated.rst | 13 --- > docs/pci_expander_bridge.txt | 2 +- > hw/block/virtio-blk.c| 192 +-- > hw/core/machine.c| 2 - > 5 files changed, 3 insertions(+), 208 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
On Thu, Apr 29, 2021 at 04:52:21PM +0100, Stefan Hajnoczi wrote: > The scsi=on|off property was deprecated in QEMU 5.0 and can be removed > completely at this point. > > Drop the scsi=on|off option. It was only available on Legacy virtio-blk > devices. Linux v5.6 already dropped support for it. > > Remove the hw_compat_2_4[] property assignment since scsi=on|off no > longer exists. Old guests with Legacy virtio-blk devices no longer see > the SCSI host features bit. > This means pc-2.4 will now break guest ABI if using virtio-blk devices, correct? This looks like a sign we should have deprecated pc-2.4 a long time ago. > Live migrating old guests from an old QEMU with the SCSI feature bit > enabled will fail with "Features 0x... unsupported. Allowed features: > 0x...". We've followed the QEMU deprecation policy so users have been > warned... > Were they really warned, though? People running "-machine pc-i440fx-2.4" might be completely unaware that it was silently enabling a deprecated feature. Can we have this documented in a more explicit way? Maybe just a comment at hw_compat_2_4 would be enough, to warn people doing backports and rebases downstream. Can we make QEMU refuse to start if using pc-2.4 + virtio-blk together, just to be sure? An alternative would be keeping the property (and the hw_compat_2_4 entry) just to keep pc-2.4 working (until pc-2.4 is deprecated and removed), but refusing to realize the device if the feature is enabled. > I have tested that libvirt still works when the property is absent. It > no longer adds scsi=on|off to the command-line. > > Cc: Markus Armbruster > Cc: Christoph Hellwig > Cc: Peter Krempa > Cc: Dr. David Alan Gilbert > Signed-off-by: Stefan Hajnoczi > --- [...] > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 40def78183..286f18ec6d 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -194,8 +194,6 @@ GlobalProperty hw_compat_2_5[] = { > const size_t hw_compat_2_5_len = G_N_ELEMENTS(hw_compat_2_5); > > GlobalProperty hw_compat_2_4[] = { > -/* Optional because the 'scsi' property is Linux-only */ > -{ "virtio-blk-device", "scsi", "true", .optional = true }, > { "e1000", "extra_mac_registers", "off" }, > { "virtio-pci", "x-disable-pcie", "on" }, > { "virtio-pci", "migrate-extra", "off" }, -- Eduardo
Re: [PATCH] virtio-blk: drop deprecated scsi=on|off property
On Thu, Apr 29, 2021 at 16:52:21 +0100, Stefan Hajnoczi wrote: > The scsi=on|off property was deprecated in QEMU 5.0 and can be removed > completely at this point. > > Drop the scsi=on|off option. It was only available on Legacy virtio-blk > devices. Linux v5.6 already dropped support for it. > > Remove the hw_compat_2_4[] property assignment since scsi=on|off no > longer exists. Old guests with Legacy virtio-blk devices no longer see > the SCSI host features bit. Does this mean that qemu rejects it if it's explicitly enabled on the commandline? > Live migrating old guests from an old QEMU with the SCSI feature bit > enabled will fail with "Features 0x... unsupported. Allowed features: > 0x...". We've followed the QEMU deprecation policy so users have been > warned... > > I have tested that libvirt still works when the property is absent. It > no longer adds scsi=on|off to the command-line. Yup, we deliberately don't format it unless the user requested it since: https://gitlab.com/libvirt/libvirt/-/commit/ec69f0190be731d12faeac08dbf63325836509a9 Depending on your answer above I might need to dig through the code again to see whether we do the correct thing if it's no longer available. > > Cc: Markus Armbruster > Cc: Christoph Hellwig > Cc: Peter Krempa > Cc: Dr. David Alan Gilbert > Signed-off-by: Stefan Hajnoczi > --- > docs/specs/tpm.rst | 2 +- > docs/system/deprecated.rst | 13 --- > docs/pci_expander_bridge.txt | 2 +- > hw/block/virtio-blk.c| 192 +-- > hw/core/machine.c| 2 - > 5 files changed, 3 insertions(+), 208 deletions(-)
[PATCH] virtio-blk: drop deprecated scsi=on|off property
The scsi=on|off property was deprecated in QEMU 5.0 and can be removed completely at this point. Drop the scsi=on|off option. It was only available on Legacy virtio-blk devices. Linux v5.6 already dropped support for it. Remove the hw_compat_2_4[] property assignment since scsi=on|off no longer exists. Old guests with Legacy virtio-blk devices no longer see the SCSI host features bit. Live migrating old guests from an old QEMU with the SCSI feature bit enabled will fail with "Features 0x... unsupported. Allowed features: 0x...". We've followed the QEMU deprecation policy so users have been warned... I have tested that libvirt still works when the property is absent. It no longer adds scsi=on|off to the command-line. Cc: Markus Armbruster Cc: Christoph Hellwig Cc: Peter Krempa Cc: Dr. David Alan Gilbert Signed-off-by: Stefan Hajnoczi --- docs/specs/tpm.rst | 2 +- docs/system/deprecated.rst | 13 --- docs/pci_expander_bridge.txt | 2 +- hw/block/virtio-blk.c| 192 +-- hw/core/machine.c| 2 - 5 files changed, 3 insertions(+), 208 deletions(-) diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst index 3be190343a..0ec017ab95 100644 --- a/docs/specs/tpm.rst +++ b/docs/specs/tpm.rst @@ -328,7 +328,7 @@ In case a pSeries machine is emulated, use the following command line: -tpmdev emulator,id=tpm0,chardev=chrtpm \ -device tpm-spapr,tpmdev=tpm0 \ -device spapr-vscsi,id=scsi0,reg=0x2000 \ --device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0 \ -drive file=test.img,format=raw,if=none,id=drive-virtio-disk0 In case an Arm virt machine is emulated, use the following command line: diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 80cae86252..1abb64b669 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -248,19 +248,6 @@ machines have been renamed ``raspi2b`` and ``raspi3b``. Device options -- -Emulated device options -''' - -``-device virtio-blk,scsi=on|off`` (since 5.0.0) - - -The virtio-blk SCSI passthrough feature is a legacy VIRTIO feature. VIRTIO 1.0 -and later do not support it because the virtio-scsi device was introduced for -full SCSI support. Use virtio-scsi instead when SCSI passthrough is required. - -Note this also applies to ``-device virtio-blk-pci,scsi=on|off``, which is an -alias. - Block device options diff --git a/docs/pci_expander_bridge.txt b/docs/pci_expander_bridge.txt index 36750273bb..540191f5e0 100644 --- a/docs/pci_expander_bridge.txt +++ b/docs/pci_expander_bridge.txt @@ -25,7 +25,7 @@ A detailed command line would be: -object memory-backend-ram,size=1024M,policy=bind,host-nodes=1,id=ram-node1 -numa node,nodeid=1,cpus=1,memdev=ram-node1 -device pxb,id=bridge1,bus=pci.0,numa_node=1,bus_nr=4 -netdev user,id=nd -device e1000,bus=bridge1,addr=0x4,netdev=nd -device pxb,id=bridge2,bus=pci.0,numa_node=0,bus_nr=8 -device e1000,bus=bridge2,addr=0x3 --device pxb,id=bridge3,bus=pci.0,bus_nr=40 -drive if=none,id=drive0,file=[img] -device virtio-blk-pci,drive=drive0,scsi=off,bus=bridge3,addr=1 +-device pxb,id=bridge3,bus=pci.0,bus_nr=40 -drive if=none,id=drive0,file=[img] -device virtio-blk-pci,drive=drive0,bus=bridge3,addr=1 Here you have: - 2 NUMA nodes for the guest, 0 and 1. (both mapped to the same NUMA node in host, but you can and should put it in different host NUMA nodes) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index d28979efb8..5023e046fc 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -25,10 +25,6 @@ #include "sysemu/runstate.h" #include "hw/virtio/virtio-blk.h" #include "dataplane/virtio-blk.h" -#include "scsi/constants.h" -#ifdef __linux__ -# include -#endif #include "hw/virtio/virtio-bus.h" #include "migration/qemu-file-types.h" #include "hw/virtio/virtio-access.h" @@ -200,59 +196,6 @@ out: aio_context_release(blk_get_aio_context(s->conf.conf.blk)); } -#ifdef __linux__ - -typedef struct { -VirtIOBlockReq *req; -struct sg_io_hdr hdr; -} VirtIOBlockIoctlReq; - -static void virtio_blk_ioctl_complete(void *opaque, int status) -{ -VirtIOBlockIoctlReq *ioctl_req = opaque; -VirtIOBlockReq *req = ioctl_req->req; -VirtIOBlock *s = req->dev; -VirtIODevice *vdev = VIRTIO_DEVICE(s); -struct virtio_scsi_inhdr *scsi; -struct sg_io_hdr *hdr; - -scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base; - -if (status) { -status = VIRTIO_BLK_S_UNSUPP; -virtio_stl_p(vdev, >errors, 255); -goto out; -} - -hdr = _req->hdr; -/* - * From SCSI-Generic-HOWTO: "Some lower level drivers (e.g. ide-scsi) - * clear the masked_status field [hence status gets cleared