[Qemu-devel] [PATCH 1/5] virtio-pci: ignore unaligned read/write in virtio_address_space_read()/write()

[Qemu-devel] [PATCH 1/5] virtio-pci: ignore unaligned read/write in virtio_address_space_read()/write()

[Jason Wang]

We abort on unaligned read/write in
virtio_address_space_read()/write() but since len in under control of
guest so qemu will simply crash when booting a modern guest (guest is
try to read when len is zero). Fix this by ignoring unaligned write or
read.

Fixes 1e40356ce5f6ccfa0bb57104a533c62952c560ce
("virtio fix cfg endian-ness for BE targets")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ccca2b6..bed9735 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -466,8 +466,8 @@ void virtio_address_space_write(AddressSpace *as, hwaddr addr,
      */
     addr &= ~(len - 1);
 
-    /* Make sure caller aligned buf properly */
-    assert(!(((uintptr_t)buf) & (len - 1)));
+    if (!(((uintptr_t)buf) & (len - 1)))
+        return;
 
     switch (len) {
     case 1:
@@ -498,8 +498,8 @@ virtio_address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int len)
      */
     addr &= ~(len - 1);
 
-    /* Make sure caller aligned buf properly */
-    assert(!(((uintptr_t)buf) & (len - 1)));
+    if (!(((uintptr_t)buf) & (len - 1)))
+        return;
 
     switch (len) {
     case 1:
-- 
2.1.4

[Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Jason Wang]

VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/block/virtio-blk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6aefda4..f30ad25 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
     virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
     virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
     virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
-    virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
+    if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
+        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
 
     if (s->conf.config_wce) {
         virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
-- 
2.1.4

[Qemu-devel] [PATCH 3/5] virtio-blk: set VIRTIO_F_ANY_LAYOUT when 1.0 is supported

[Jason Wang]

Chapter 6.3 of spec said

"
Transitional devices MUST offer, and if offered by the device
transitional drivers MUST accept the following:

VIRTIO_F_ANY_LAYOUT (27)
"

So this patch sets VIRTIO_F_ANY_LAYOUT when 1.0 is supported.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/block/virtio-blk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f30ad25..0f07e25 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -732,6 +732,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
     virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
     if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
         virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
+    else
+        virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
 
     if (s->conf.config_wce) {
         virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
-- 
2.1.4

[Qemu-devel] [PATCH 4/5] Revert "virtio-net: enable virtio 1.0"

[Jason Wang]

This reverts commit df91055db5c9cee93d70ca8c08d72119a240b987.

This is because:
- vhost support virtio 1.0 now
- transport code (e.g virtio-pci) set this feature when modern is
  enabled, setting this unconditionally will break disable-modern=on.

Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index d728233..e3c2db3 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -466,7 +466,6 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features)
     }
 
     if (!get_vhost_net(nc->peer)) {
-        virtio_add_feature(&features, VIRTIO_F_VERSION_1);
         return features;
     }
     return vhost_net_get_features(get_vhost_net(nc->peer), features);
-- 
2.1.4

[Qemu-devel] [PATCH 5/5] virtio-net: unbreak any layout

[Jason Wang]

Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
("virtio-net: byteswap virtio-net header") breaks any layout by
requiring out_sg[0].iov_len >= n->guest_hdr_len. Fixing this by
copying header to temporary buffer and copying it back after byteswap.

Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
("virtio-net: byteswap virtio-net header")
Cc: qemu-stable@nongnu.org
Cc: clg@fr.ibm.com
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e3c2db3..b42af8f 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1139,10 +1139,11 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
     }
 
     while (virtqueue_pop(q->tx_vq, &elem)) {
-        ssize_t ret, len;
+        ssize_t ret, len, s;
         unsigned int out_num = elem.out_num;
         struct iovec *out_sg = &elem.out_sg[0];
         struct iovec sg[VIRTQUEUE_MAX_SIZE];
+        struct virtio_net_hdr hdr;
 
         if (out_num < 1) {
             error_report("virtio-net header not in first element");
@@ -1150,11 +1151,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
         }
 
         if (n->has_vnet_hdr) {
-            if (out_sg[0].iov_len < n->guest_hdr_len) {
+            s = iov_to_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
+            if (s != sizeof(hdr)) {
                 error_report("virtio-net header incorrect");
                 exit(1);
             }
-            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
+            virtio_net_hdr_swap(vdev, (void *) &hdr);
+            s = iov_from_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
+            assert(s == sizeof(hdr));
         }
 
         /*
-- 
2.1.4

Re: [Qemu-devel] [PATCH 4/5] Revert "virtio-net: enable virtio 1.0"

[Cornelia Huck]

On Mon, 13 Jul 2015 13:46:50 +0800
Jason Wang <jasowang@redhat.com> wrote:

> This reverts commit df91055db5c9cee93d70ca8c08d72119a240b987.
> 
> This is because:
> - vhost support virtio 1.0 now
> - transport code (e.g virtio-pci) set this feature when modern is
>   enabled, setting this unconditionally will break disable-modern=on.

*scratches head*

Why is transport code now supposed to set VERSION_1? I thought we
wanted to have the individual devices offer it, once they are converted.

No objection on removing the dependency on !vhost.

> 
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index d728233..e3c2db3 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -466,7 +466,6 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features)
>      }
> 
>      if (!get_vhost_net(nc->peer)) {
> -        virtio_add_feature(&features, VIRTIO_F_VERSION_1);
>          return features;
>      }
>      return vhost_net_get_features(get_vhost_net(nc->peer), features);

Re: [Qemu-devel] [PATCH 5/5] virtio-net: unbreak any layout

[Paolo Bonzini]

On 13/07/2015 07:46, Jason Wang wrote:
> -            if (out_sg[0].iov_len < n->guest_hdr_len) {
> +            s = iov_to_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> +            if (s != sizeof(hdr)) {
>                  error_report("virtio-net header incorrect");
>                  exit(1);
>              }
> -            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
> +            virtio_net_hdr_swap(vdev, (void *) &hdr);
> +            s = iov_from_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> +            assert(s == sizeof(hdr));
>          }

Are the copies necessary in the common case of no swap?  In that case
you can just use iov_size.

Paolo

Re: [Qemu-devel] [PATCH 4/5] Revert "virtio-net: enable virtio 1.0"

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 08:16:17AM +0200, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 13:46:50 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> > This reverts commit df91055db5c9cee93d70ca8c08d72119a240b987.
> > 
> > This is because:
> > - vhost support virtio 1.0 now
> > - transport code (e.g virtio-pci) set this feature when modern is
> >   enabled, setting this unconditionally will break disable-modern=on.
> 
> *scratches head*
> 
> Why is transport code now supposed to set VERSION_1? I thought we
> wanted to have the individual devices offer it, once they are converted.

Because all devices have been converted now.  Excepting bugs, but we can
fix these.

> No objection on removing the dependency on !vhost.
> 
> > 
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index d728233..e3c2db3 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -466,7 +466,6 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features)
> >      }
> > 
> >      if (!get_vhost_net(nc->peer)) {
> > -        virtio_add_feature(&features, VIRTIO_F_VERSION_1);
> >          return features;
> >      }
> >      return vhost_net_get_features(get_vhost_net(nc->peer), features);

Re: [Qemu-devel] [PATCH 5/5] virtio-net: unbreak any layout

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 01:46:51PM +0800, Jason Wang wrote:
> Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
> ("virtio-net: byteswap virtio-net header") breaks any layout by
> requiring out_sg[0].iov_len >= n->guest_hdr_len. Fixing this by
> copying header to temporary buffer and copying it back after byteswap.
> 
> Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
> ("virtio-net: byteswap virtio-net header")
> Cc: qemu-stable@nongnu.org
> Cc: clg@fr.ibm.com
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index e3c2db3..b42af8f 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1139,10 +1139,11 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>      }
>  
>      while (virtqueue_pop(q->tx_vq, &elem)) {
> -        ssize_t ret, len;
> +        ssize_t ret, len, s;
>          unsigned int out_num = elem.out_num;
>          struct iovec *out_sg = &elem.out_sg[0];
>          struct iovec sg[VIRTQUEUE_MAX_SIZE];
> +        struct virtio_net_hdr hdr;
>  
>          if (out_num < 1) {
>              error_report("virtio-net header not in first element");
> @@ -1150,11 +1151,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>          }
>  
>          if (n->has_vnet_hdr) {
> -            if (out_sg[0].iov_len < n->guest_hdr_len) {
> +            s = iov_to_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> +            if (s != sizeof(hdr)) {
>                  error_report("virtio-net header incorrect");
>                  exit(1);
>              }
> -            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
> +            virtio_net_hdr_swap(vdev, (void *) &hdr);
> +            s = iov_from_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> +            assert(s == sizeof(hdr));
>          }
>  
>          /*

Looks like this will write into out_sg - that's violating the virtio spec.

> -- 
> 2.1.4

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: ignore unaligned read/write in virtio_address_space_read()/write()

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
> We abort on unaligned read/write in
> virtio_address_space_read()/write() but since len in under control of
> guest so qemu will simply crash when booting a modern guest (guest is
> try to read when len is zero).
> read.

How can len be 0? Isn't this a guest bug? Or is this
a theoretical issue?

> Fix this by ignoring unaligned write or
> 
> Fixes 1e40356ce5f6ccfa0bb57104a533c62952c560ce
> ("virtio fix cfg endian-ness for BE targets")
> Signed-off-by: Jason Wang <jasowang@redhat.com>

I guess since we ignore some illegal values (e.g. > 4)
we should just whitelist the legal ones.
So the following looks like a slightly cleaner way to
make this change.

--->
virtio-pci: don't crash on illegal length

Some guests seem to access cfg with an illegal length value.
It's worth fixing them but debugging is easier if
qemu does not crash.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 6ca0258..c5e8cc0 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -546,7 +546,8 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
         off = le32_to_cpu(cfg->cap.offset);
         len = le32_to_cpu(cfg->cap.length);
 
-        if (len <= sizeof cfg->pci_cfg_data) {
+        if (len == 1 || len == 2 || len == 4) {
+            assert(len <= sizeof cfg->pci_cfg_data);
             virtio_address_space_write(&proxy->modern_as, off,
                                        cfg->pci_cfg_data, len);
         }
@@ -570,7 +571,8 @@ static uint32_t virtio_read_config(PCIDevice *pci_dev,
         off = le32_to_cpu(cfg->cap.offset);
         len = le32_to_cpu(cfg->cap.length);
 
-        if (len <= sizeof cfg->pci_cfg_data) {
+        if (len == 1 || len == 2 || len == 4) {
+            assert(len <= sizeof cfg->pci_cfg_data);
             virtio_address_space_read(&proxy->modern_as, off,
                                       cfg->pci_cfg_data, len);
         }

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> 
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Interesting, I noticed we have a field scsi - see
	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
	Author: Paolo Bonzini <pbonzini@redhat.com>
	Date:   Fri Dec 23 15:39:03 2011 +0100

	    virtio-blk: refuse SG_IO requests with scsi=off

but it doesn't seem to be propagated to guest features in
any way.

Maybe we should fix that, making that flag AutoOnOff?
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.

Given scsi flag is currently ignored, I think
this can be a patch on top.

> ---
>  hw/block/virtio-blk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 6aefda4..f30ad25 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
>      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
>      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
>      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> -    virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> +    if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>  
>      if (s->conf.config_wce) {
>          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> -- 
> 2.1.4

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: ignore unaligned read/write in virtio_address_space_read()/write()

[Gerd Hoffmann]

On Mo, 2015-07-13 at 10:36 +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
> > We abort on unaligned read/write in
> > virtio_address_space_read()/write() but since len in under control of
> > guest so qemu will simply crash when booting a modern guest (guest is
> > try to read when len is zero).
> > read.
> 
> How can len be 0? Isn't this a guest bug? Or is this
> a theoretical issue?

Something dumping pci config space?
With pci access capability not being used before and therefore zeroed?
Then hitting the "data" field will trigger a zero-length read.

That assert actually triggers when booting a recent linux kernel with
disable-modern=off

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: ignore unaligned read/write in virtio_address_space_read()/write()

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 09:53:43AM +0200, Gerd Hoffmann wrote:
> On Mo, 2015-07-13 at 10:36 +0300, Michael S. Tsirkin wrote:
> > On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
> > > We abort on unaligned read/write in
> > > virtio_address_space_read()/write() but since len in under control of
> > > guest so qemu will simply crash when booting a modern guest (guest is
> > > try to read when len is zero).
> > > read.
> > 
> > How can len be 0? Isn't this a guest bug? Or is this
> > a theoretical issue?
> 
> Something dumping pci config space?
> With pci access capability not being used before and therefore zeroed?
> Then hitting the "data" field will trigger a zero-length read.

I suspect so, yes. All this worries me: what if length was not 0
because the capability was previously used e.g. by bios?

> That assert actually triggers when booting a recent linux kernel with
> disable-modern=off
> 
> cheers,
>   Gerd
> 

Which linux version?  Doesn't seem to trigger for me ... 

-- 
MST

Re: [Qemu-devel] [PATCH 5/5] virtio-net: unbreak any layout

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 10:24:59AM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:46:51PM +0800, Jason Wang wrote:
> > Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
> > ("virtio-net: byteswap virtio-net header") breaks any layout by
> > requiring out_sg[0].iov_len >= n->guest_hdr_len. Fixing this by
> > copying header to temporary buffer and copying it back after byteswap.
> > 
> > Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
> > ("virtio-net: byteswap virtio-net header")
> > Cc: qemu-stable@nongnu.org
> > Cc: clg@fr.ibm.com
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index e3c2db3..b42af8f 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1139,10 +1139,11 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >      }
> >  
> >      while (virtqueue_pop(q->tx_vq, &elem)) {
> > -        ssize_t ret, len;
> > +        ssize_t ret, len, s;
> >          unsigned int out_num = elem.out_num;
> >          struct iovec *out_sg = &elem.out_sg[0];
> >          struct iovec sg[VIRTQUEUE_MAX_SIZE];
> > +        struct virtio_net_hdr hdr;
> >  
> >          if (out_num < 1) {
> >              error_report("virtio-net header not in first element");
> > @@ -1150,11 +1151,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >          }
> >  
> >          if (n->has_vnet_hdr) {
> > -            if (out_sg[0].iov_len < n->guest_hdr_len) {
> > +            s = iov_to_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> > +            if (s != sizeof(hdr)) {
> >                  error_report("virtio-net header incorrect");
> >                  exit(1);
> >              }
> > -            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
> > +            virtio_net_hdr_swap(vdev, (void *) &hdr);
> > +            s = iov_from_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> > +            assert(s == sizeof(hdr));
> >          }
> >  
> >          /*
> 
> Looks like this will write into out_sg - that's violating the virtio spec.

And the fault lies with the original code upstream - not with these
patches. In my opinion the thing to do here is to completely
avoid using vnet header for cross endian unless host supports ioctls
to set endian-ness.

> > -- 
> > 2.1.4

Re: [Qemu-devel] [PATCH 4/5] Revert "virtio-net: enable virtio 1.0"

[Jason Wang]

On 07/13/2015 02:16 PM, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 13:46:50 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> This reverts commit df91055db5c9cee93d70ca8c08d72119a240b987.
>>
>> This is because:
>> - vhost support virtio 1.0 now
>> - transport code (e.g virtio-pci) set this feature when modern is
>>   enabled, setting this unconditionally will break disable-modern=on.
> *scratches head*
>
> Why is transport code now supposed to set VERSION_1? I thought we
> wanted to have the individual devices offer it, once they are converted.

As Michael pointed out, all device have been converted. And offering
this in device needs knowledge of transport capability but device should
know nothing about this.

>
> No objection on removing the dependency on !vhost.
>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  hw/net/virtio-net.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index d728233..e3c2db3 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -466,7 +466,6 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features)
>>      }
>>
>>      if (!get_vhost_net(nc->peer)) {
>> -        virtio_add_feature(&features, VIRTIO_F_VERSION_1);
>>          return features;
>>      }
>>      return vhost_net_get_features(get_vhost_net(nc->peer), features);

Re: [Qemu-devel] [PATCH 5/5] virtio-net: unbreak any layout

[Jason Wang]

On 07/13/2015 02:50 PM, Paolo Bonzini wrote:
>
> On 13/07/2015 07:46, Jason Wang wrote:
>> -            if (out_sg[0].iov_len < n->guest_hdr_len) {
>> +            s = iov_to_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
>> +            if (s != sizeof(hdr)) {
>>                  error_report("virtio-net header incorrect");
>>                  exit(1);
>>              }
>> -            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
>> +            virtio_net_hdr_swap(vdev, (void *) &hdr);
>> +            s = iov_from_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
>> +            assert(s == sizeof(hdr));
>>          }
> Are the copies necessary in the common case of no swap?  In that case
> you can just use iov_size.
>
> Paolo
>

Not necessary, will fix this in V2.

Thanks

Re: [Qemu-devel] [PATCH 5/5] virtio-net: unbreak any layout

[Jason Wang]

On 07/13/2015 03:24 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:46:51PM +0800, Jason Wang wrote:
>> Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
>> ("virtio-net: byteswap virtio-net header") breaks any layout by
>> requiring out_sg[0].iov_len >= n->guest_hdr_len. Fixing this by
>> copying header to temporary buffer and copying it back after byteswap.
>>
>> Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
>> ("virtio-net: byteswap virtio-net header")
>> Cc: qemu-stable@nongnu.org
>> Cc: clg@fr.ibm.com
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  hw/net/virtio-net.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index e3c2db3..b42af8f 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -1139,10 +1139,11 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>>      }
>>  
>>      while (virtqueue_pop(q->tx_vq, &elem)) {
>> -        ssize_t ret, len;
>> +        ssize_t ret, len, s;
>>          unsigned int out_num = elem.out_num;
>>          struct iovec *out_sg = &elem.out_sg[0];
>>          struct iovec sg[VIRTQUEUE_MAX_SIZE];
>> +        struct virtio_net_hdr hdr;
>>  
>>          if (out_num < 1) {
>>              error_report("virtio-net header not in first element");
>> @@ -1150,11 +1151,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>>          }
>>  
>>          if (n->has_vnet_hdr) {
>> -            if (out_sg[0].iov_len < n->guest_hdr_len) {
>> +            s = iov_to_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
>> +            if (s != sizeof(hdr)) {
>>                  error_report("virtio-net header incorrect");
>>                  exit(1);
>>              }
>> -            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
>> +            virtio_net_hdr_swap(vdev, (void *) &hdr);
>> +            s = iov_from_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
>> +            assert(s == sizeof(hdr));
>>          }
>>  
>>          /*
> Looks like this will write into out_sg - that's violating the virtio spec.

Right, will fix this in V2.

Thanks
>
>> -- 
>> 2.1.4

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: ignore unaligned read/write in virtio_address_space_read()/write()

[Jason Wang]

On 07/13/2015 03:36 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
>> We abort on unaligned read/write in
>> virtio_address_space_read()/write() but since len in under control of
>> guest so qemu will simply crash when booting a modern guest (guest is
>> try to read when len is zero).
>> read.
> How can len be 0? Isn't this a guest bug? Or is this
> a theoretical issue?

E.g cat /sys/bus/pci/devices/0000\:00\:03.0/config
and also happen during boot (but not virtio specific code, probably pci
core or something else).

>
>> Fix this by ignoring unaligned write or
>>
>> Fixes 1e40356ce5f6ccfa0bb57104a533c62952c560ce
>> ("virtio fix cfg endian-ness for BE targets")
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> I guess since we ignore some illegal values (e.g. > 4)
> we should just whitelist the legal ones.
> So the following looks like a slightly cleaner way to
> make this change.
>
> --->
> virtio-pci: don't crash on illegal length
>
> Some guests seem to access cfg with an illegal length value.
> It's worth fixing them but debugging is easier if
> qemu does not crash.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

I believe when we can, we should avoid guest trigger-able abort.

>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 6ca0258..c5e8cc0 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -546,7 +546,8 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>          off = le32_to_cpu(cfg->cap.offset);
>          len = le32_to_cpu(cfg->cap.length);
>  
> -        if (len <= sizeof cfg->pci_cfg_data) {
> +        if (len == 1 || len == 2 || len == 4) {
> +            assert(len <= sizeof cfg->pci_cfg_data);
>              virtio_address_space_write(&proxy->modern_as, off,
>                                         cfg->pci_cfg_data, len);
>          }
> @@ -570,7 +571,8 @@ static uint32_t virtio_read_config(PCIDevice *pci_dev,
>          off = le32_to_cpu(cfg->cap.offset);
>          len = le32_to_cpu(cfg->cap.length);
>  
> -        if (len <= sizeof cfg->pci_cfg_data) {
> +        if (len == 1 || len == 2 || len == 4) {
> +            assert(len <= sizeof cfg->pci_cfg_data);
>              virtio_address_space_read(&proxy->modern_as, off,
>                                        cfg->pci_cfg_data, len);
>          }
>

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: ignore unaligned read/write in virtio_address_space_read()/write()

[Gerd Hoffmann]

On Mo, 2015-07-13 at 11:00 +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 09:53:43AM +0200, Gerd Hoffmann wrote:
> > On Mo, 2015-07-13 at 10:36 +0300, Michael S. Tsirkin wrote:
> > > On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
> > > > We abort on unaligned read/write in
> > > > virtio_address_space_read()/write() but since len in under control of
> > > > guest so qemu will simply crash when booting a modern guest (guest is
> > > > try to read when len is zero).
> > > > read.
> > > 
> > > How can len be 0? Isn't this a guest bug? Or is this
> > > a theoretical issue?
> > 
> > Something dumping pci config space?
> > With pci access capability not being used before and therefore zeroed?
> > Then hitting the "data" field will trigger a zero-length read.
> 
> I suspect so, yes. All this worries me: what if length was not 0
> because the capability was previously used e.g. by bios?
> 
> > That assert actually triggers when booting a recent linux kernel with
> > disable-modern=off
> > 
> > cheers,
> >   Gerd
> > 
> 
> Which linux version?  Doesn't seem to trigger for me ... 

Fedora 22 guest with latest distro kernel (4.0.7)

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 4/5] Revert "virtio-net: enable virtio 1.0"

[Cornelia Huck]

On Mon, 13 Jul 2015 10:22:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2015 at 08:16:17AM +0200, Cornelia Huck wrote:
> > On Mon, 13 Jul 2015 13:46:50 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> > 
> > > This reverts commit df91055db5c9cee93d70ca8c08d72119a240b987.
> > > 
> > > This is because:
> > > - vhost support virtio 1.0 now
> > > - transport code (e.g virtio-pci) set this feature when modern is
> > >   enabled, setting this unconditionally will break disable-modern=on.
> > 
> > *scratches head*
> > 
> > Why is transport code now supposed to set VERSION_1? I thought we
> > wanted to have the individual devices offer it, once they are converted.
> 
> Because all devices have been converted now.  Excepting bugs, but we can
> fix these.

Must have missed this. Will rework the virtio-1 enabling patch for
virtio-ccw (for 2.5).

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Jason Wang]

On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
>> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
>>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: qemu-block@nongnu.org
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Interesting, I noticed we have a field scsi - see
> 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> 	Author: Paolo Bonzini <pbonzini@redhat.com>
> 	Date:   Fri Dec 23 15:39:03 2011 +0100
>
> 	    virtio-blk: refuse SG_IO requests with scsi=off
>
> but it doesn't seem to be propagated to guest features in
> any way.
>
> Maybe we should fix that, making that flag AutoOnOff?

Looks ok but auto may need some compat work since default is true.

> Then, if user explicitly requested scsi=on with a modern
> interface then we can error out cleanly.
>
> Given scsi flag is currently ignored, I think
> this can be a patch on top.

Looks like virtio_blk_handle_scsi_req() check this:

    if (!blk->conf.scsi) {
        status = VIRTIO_BLK_S_UNSUPP;
        goto fail;
    }

>
>> ---
>>  hw/block/virtio-blk.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 6aefda4..f30ad25 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
>>      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
>>      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
>>      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
>> -    virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>> +    if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
>> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>>  
>>      if (s->conf.config_wce) {
>>          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
>> -- 
>> 2.1.4

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Kevin Wolf]

Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> 
> 
> On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> >>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Cc: Kevin Wolf <kwolf@redhat.com>
> >> Cc: qemu-block@nongnu.org
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Interesting, I noticed we have a field scsi - see
> > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> >
> > 	    virtio-blk: refuse SG_IO requests with scsi=off
> >
> > but it doesn't seem to be propagated to guest features in
> > any way.
> >
> > Maybe we should fix that, making that flag AutoOnOff?
> 
> Looks ok but auto may need some compat work since default is true.
> 
> > Then, if user explicitly requested scsi=on with a modern
> > interface then we can error out cleanly.
> >
> > Given scsi flag is currently ignored, I think
> > this can be a patch on top.
> 
> Looks like virtio_blk_handle_scsi_req() check this:
> 
>     if (!blk->conf.scsi) {
>         status = VIRTIO_BLK_S_UNSUPP;
>         goto fail;
>     }

So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk->conf.scsi is set.

> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 6aefda4..f30ad25 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> >> -    virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >> +    if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
> >> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);

Coding style: Braces are needed here. (Same in the other patch you CCed
me on).

Kevin

Re: [Qemu-devel] [PATCH 5/5] virtio-net: unbreak any layout

[Greg Kurz]

On Mon, 13 Jul 2015 11:22:09 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Jul 13, 2015 at 10:24:59AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jul 13, 2015 at 01:46:51PM +0800, Jason Wang wrote:
> > > Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
> > > ("virtio-net: byteswap virtio-net header") breaks any layout by
> > > requiring out_sg[0].iov_len >= n->guest_hdr_len. Fixing this by
> > > copying header to temporary buffer and copying it back after byteswap.
> > > 
> > > Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
> > > ("virtio-net: byteswap virtio-net header")
> > > Cc: qemu-stable@nongnu.org
> > > Cc: clg@fr.ibm.com
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  hw/net/virtio-net.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index e3c2db3..b42af8f 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -1139,10 +1139,11 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> > >      }
> > >  
> > >      while (virtqueue_pop(q->tx_vq, &elem)) {
> > > -        ssize_t ret, len;
> > > +        ssize_t ret, len, s;
> > >          unsigned int out_num = elem.out_num;
> > >          struct iovec *out_sg = &elem.out_sg[0];
> > >          struct iovec sg[VIRTQUEUE_MAX_SIZE];
> > > +        struct virtio_net_hdr hdr;
> > >  
> > >          if (out_num < 1) {
> > >              error_report("virtio-net header not in first element");
> > > @@ -1150,11 +1151,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> > >          }
> > >  
> > >          if (n->has_vnet_hdr) {
> > > -            if (out_sg[0].iov_len < n->guest_hdr_len) {
> > > +            s = iov_to_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> > > +            if (s != sizeof(hdr)) {
> > >                  error_report("virtio-net header incorrect");
> > >                  exit(1);
> > >              }
> > > -            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
> > > +            virtio_net_hdr_swap(vdev, (void *) &hdr);
> > > +            s = iov_from_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> > > +            assert(s == sizeof(hdr));
> > >          }
> > >  
> > >          /*
> > 
> > Looks like this will write into out_sg - that's violating the virtio spec.
> 
> And the fault lies with the original code upstream - not with these
> patches. In my opinion the thing to do here is to completely

I fully agree: it is a hack.

> avoid using vnet header for cross endian unless host supports ioctls
> to set endian-ness.
> 

Makes sense. I had an *experimental* patch that would just deactivate
the cross-endian hack if the host kernel provides the TUNSETVNETLE/BE
ioctls. It worked but the patch was really ugly and I did not care to
post at the time... I'll try to work something out before I go on
vacation.

> > > -- 
> > > 2.1.4
> 

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH 5/5] virtio-net: unbreak any layout

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 12:54:02PM +0200, Greg Kurz wrote:
> On Mon, 13 Jul 2015 11:22:09 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, Jul 13, 2015 at 10:24:59AM +0300, Michael S. Tsirkin wrote:
> > > On Mon, Jul 13, 2015 at 01:46:51PM +0800, Jason Wang wrote:
> > > > Commit 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
> > > > ("virtio-net: byteswap virtio-net header") breaks any layout by
> > > > requiring out_sg[0].iov_len >= n->guest_hdr_len. Fixing this by
> > > > copying header to temporary buffer and copying it back after byteswap.
> > > > 
> > > > Fixes 032a74a1c0fcdd5fd1c69e56126b4c857ee36611
> > > > ("virtio-net: byteswap virtio-net header")
> > > > Cc: qemu-stable@nongnu.org
> > > > Cc: clg@fr.ibm.com
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  hw/net/virtio-net.c | 10 +++++++---
> > > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index e3c2db3..b42af8f 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -1139,10 +1139,11 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> > > >      }
> > > >  
> > > >      while (virtqueue_pop(q->tx_vq, &elem)) {
> > > > -        ssize_t ret, len;
> > > > +        ssize_t ret, len, s;
> > > >          unsigned int out_num = elem.out_num;
> > > >          struct iovec *out_sg = &elem.out_sg[0];
> > > >          struct iovec sg[VIRTQUEUE_MAX_SIZE];
> > > > +        struct virtio_net_hdr hdr;
> > > >  
> > > >          if (out_num < 1) {
> > > >              error_report("virtio-net header not in first element");
> > > > @@ -1150,11 +1151,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> > > >          }
> > > >  
> > > >          if (n->has_vnet_hdr) {
> > > > -            if (out_sg[0].iov_len < n->guest_hdr_len) {
> > > > +            s = iov_to_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> > > > +            if (s != sizeof(hdr)) {
> > > >                  error_report("virtio-net header incorrect");
> > > >                  exit(1);
> > > >              }
> > > > -            virtio_net_hdr_swap(vdev, (void *) out_sg[0].iov_base);
> > > > +            virtio_net_hdr_swap(vdev, (void *) &hdr);
> > > > +            s = iov_from_buf(out_sg, out_num, 0, &hdr, sizeof(hdr));
> > > > +            assert(s == sizeof(hdr));
> > > >          }
> > > >  
> > > >          /*
> > > 
> > > Looks like this will write into out_sg - that's violating the virtio spec.
> > 
> > And the fault lies with the original code upstream - not with these
> > patches. In my opinion the thing to do here is to completely
> 
> I fully agree: it is a hack.
> 
> > avoid using vnet header for cross endian unless host supports ioctls
> > to set endian-ness.
> > 
> 
> Makes sense. I had an *experimental* patch that would just deactivate
> the cross-endian hack if the host kernel provides the TUNSETVNETLE/BE
> ioctls. It worked but the patch was really ugly and I did not care to
> post at the time...

So my proposal would be to basically drop the hack completely:
if we are cross-endian and TUNSETVNETLE/BE are unavailable,
treat it as if has_vnet_hdr was not set.


> I'll try to work something out before I go on
> vacation.
> 
> > > > -- 
> > > > 2.1.4
> > 
> 
> Cheers.
> 
> --
> Greg

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 05:00:51PM +0800, Jason Wang wrote:
> 
> 
> On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> >>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Cc: Kevin Wolf <kwolf@redhat.com>
> >> Cc: qemu-block@nongnu.org
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Interesting, I noticed we have a field scsi - see
> > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> >
> > 	    virtio-blk: refuse SG_IO requests with scsi=off
> >
> > but it doesn't seem to be propagated to guest features in
> > any way.
> >
> > Maybe we should fix that, making that flag AutoOnOff?
> 
> Looks ok but auto may need some compat work since default is true.

Right. Auto would then mean "!modern".

> > Then, if user explicitly requested scsi=on with a modern
> > interface then we can error out cleanly.
> >
> > Given scsi flag is currently ignored, I think
> > this can be a patch on top.
> 
> Looks like virtio_blk_handle_scsi_req() check this:
> 
>     if (!blk->conf.scsi) {
>         status = VIRTIO_BLK_S_UNSUPP;
>         goto fail;
>     }
> 
> >
> >> ---
> >>  hw/block/virtio-blk.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 6aefda4..f30ad25 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> >> -    virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >> +    if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
> >> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >>  
> >>      if (s->conf.config_wce) {
> >>          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> >> -- 
> >> 2.1.4

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > 
> > 
> > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > >>
> > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > >> Cc: Kevin Wolf <kwolf@redhat.com>
> > >> Cc: qemu-block@nongnu.org
> > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > Interesting, I noticed we have a field scsi - see
> > > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> > >
> > > 	    virtio-blk: refuse SG_IO requests with scsi=off
> > >
> > > but it doesn't seem to be propagated to guest features in
> > > any way.
> > >
> > > Maybe we should fix that, making that flag AutoOnOff?
> > 
> > Looks ok but auto may need some compat work since default is true.
> > 
> > > Then, if user explicitly requested scsi=on with a modern
> > > interface then we can error out cleanly.
> > >
> > > Given scsi flag is currently ignored, I think
> > > this can be a patch on top.
> > 
> > Looks like virtio_blk_handle_scsi_req() check this:
> > 
> >     if (!blk->conf.scsi) {
> >         status = VIRTIO_BLK_S_UNSUPP;
> >         goto fail;
> >     }
> 
> So we should be checking the same condition for the feature flag and
> error out in the init function if we have a VERSION_1 device and
> blk->conf.scsi is set.

Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision >= 1.
I'd need to check for !scsi as well before I can add this feature bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 11:56:51 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > 
> > > 
> > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > >>
> > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > >> Cc: Kevin Wolf <kwolf@redhat.com>
> > > >> Cc: qemu-block@nongnu.org
> > > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > Interesting, I noticed we have a field scsi - see
> > > > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > > > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> > > >
> > > > 	    virtio-blk: refuse SG_IO requests with scsi=off
> > > >
> > > > but it doesn't seem to be propagated to guest features in
> > > > any way.
> > > >
> > > > Maybe we should fix that, making that flag AutoOnOff?
> > > 
> > > Looks ok but auto may need some compat work since default is true.
> > > 
> > > > Then, if user explicitly requested scsi=on with a modern
> > > > interface then we can error out cleanly.
> > > >
> > > > Given scsi flag is currently ignored, I think
> > > > this can be a patch on top.
> > > 
> > > Looks like virtio_blk_handle_scsi_req() check this:
> > > 
> > >     if (!blk->conf.scsi) {
> > >         status = VIRTIO_BLK_S_UNSUPP;
> > >         goto fail;
> > >     }
> > 
> > So we should be checking the same condition for the feature flag and
> > error out in the init function if we have a VERSION_1 device and
> > blk->conf.scsi is set.
> 
> Hm, I wonder how this plays with transports that want to make the
> virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> only want to offer VERSION_1 if the driver negotiated revision >= 1.
> I'd need to check for !scsi as well before I can add this feature bit
> then? Have the init function set a blocker for VERSION_1 so that the
> driver may only negotiate revision 0?


We already handle this, do we not?
            if (features.index == 0) {
                features.features = (uint32_t)vdev->host_features;
            } else if (features.index == 1) {
                features.features = (uint32_t)(vdev->host_features >> 32);
                /*
                 * Don't offer version 1 to the guest if it did not
                 * negotiate at least revision 1.
                 */
                if (dev->revision <= 0) {
                    features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
                }
            } else {
                /* Return zeroes if the guest supports more feature bits. */
                features.features = 0;
            }

So guest that doesn't negotiate revision >= 1 never gets to see
VIRTIO_F_VERSION_1.

Maybe we should go further and additionally all bits >= 32 if
VIRTIO_F_VERSION_1 is clear, but that can wait
and we have no bits like that in 2.4.

-- 
MST

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Mon, 13 Jul 2015 15:22:52 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > On Mon, 13 Jul 2015 11:56:51 +0200
> > Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > > 
> > > > 
> > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > > >>
> > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > >> Cc: Kevin Wolf <kwolf@redhat.com>
> > > > >> Cc: qemu-block@nongnu.org
> > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > Interesting, I noticed we have a field scsi - see
> > > > > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > > > > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> > > > >
> > > > > 	    virtio-blk: refuse SG_IO requests with scsi=off
> > > > >
> > > > > but it doesn't seem to be propagated to guest features in
> > > > > any way.
> > > > >
> > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > > 
> > > > Looks ok but auto may need some compat work since default is true.
> > > > 
> > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > interface then we can error out cleanly.
> > > > >
> > > > > Given scsi flag is currently ignored, I think
> > > > > this can be a patch on top.
> > > > 
> > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > > 
> > > >     if (!blk->conf.scsi) {
> > > >         status = VIRTIO_BLK_S_UNSUPP;
> > > >         goto fail;
> > > >     }
> > > 
> > > So we should be checking the same condition for the feature flag and
> > > error out in the init function if we have a VERSION_1 device and
> > > blk->conf.scsi is set.
> > 
> > Hm, I wonder how this plays with transports that want to make the
> > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > I'd need to check for !scsi as well before I can add this feature bit
> > then? Have the init function set a blocker for VERSION_1 so that the
> > driver may only negotiate revision 0?
> 
> 
> We already handle this, do we not?
(...)
> So guest that doesn't negotiate revision >= 1 never gets to see
> VIRTIO_F_VERSION_1.

Not my question :) I was wondering about scsi vs. virtio-1 devices. And
as I basically only want to make the decision on whether to offer
VERSION_1 when the guest negotiated a revision, I cannot fence scsi
during init, no?

> 
> Maybe we should go further and additionally all bits >= 32 if
> VIRTIO_F_VERSION_1 is clear, but that can wait
> and we have no bits like that in 2.4.
> 
Spec says bits >= 32 are only valid if we have VERSION_1, doesn't it?
Sounds sensible.

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 15:22:52 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > > On Mon, 13 Jul 2015 11:56:51 +0200
> > > Kevin Wolf <kwolf@redhat.com> wrote:
> > > 
> > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > > > 
> > > > > 
> > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > > > >>
> > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > >> Cc: Kevin Wolf <kwolf@redhat.com>
> > > > > >> Cc: qemu-block@nongnu.org
> > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > Interesting, I noticed we have a field scsi - see
> > > > > > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> > > > > >
> > > > > > 	    virtio-blk: refuse SG_IO requests with scsi=off
> > > > > >
> > > > > > but it doesn't seem to be propagated to guest features in
> > > > > > any way.
> > > > > >
> > > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > > > 
> > > > > Looks ok but auto may need some compat work since default is true.
> > > > > 
> > > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > > interface then we can error out cleanly.
> > > > > >
> > > > > > Given scsi flag is currently ignored, I think
> > > > > > this can be a patch on top.
> > > > > 
> > > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > > > 
> > > > >     if (!blk->conf.scsi) {
> > > > >         status = VIRTIO_BLK_S_UNSUPP;
> > > > >         goto fail;
> > > > >     }
> > > > 
> > > > So we should be checking the same condition for the feature flag and
> > > > error out in the init function if we have a VERSION_1 device and
> > > > blk->conf.scsi is set.
> > > 
> > > Hm, I wonder how this plays with transports that want to make the
> > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > > I'd need to check for !scsi as well before I can add this feature bit
> > > then? Have the init function set a blocker for VERSION_1 so that the
> > > driver may only negotiate revision 0?
> > 
> > 
> > We already handle this, do we not?
> (...)
> > So guest that doesn't negotiate revision >= 1 never gets to see
> > VIRTIO_F_VERSION_1.
> 
> Not my question :) I was wondering about scsi vs. virtio-1 devices. And
> as I basically only want to make the decision on whether to offer
> VERSION_1 when the guest negotiated a revision, I cannot fence scsi
> during init, no?

No, I don't think there's a lot of value in offering scsi only to
old guests that don't negotiate revision >= 1.

If user asked for virtio 1 support then that by proxy implies scsi
passthrough does not work, and it won't work for legacy
guests too.


> > 
> > Maybe we should go further and additionally all bits >= 32 if
> > VIRTIO_F_VERSION_1 is clear, but that can wait
> > and we have no bits like that in 2.4.
> > 
> Spec says bits >= 32 are only valid if we have VERSION_1, doesn't it?
> Sounds sensible.

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Mon, 13 Jul 2015 15:36:00 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
> > On Mon, 13 Jul 2015 15:22:52 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > > > On Mon, 13 Jul 2015 11:56:51 +0200
> > > > Kevin Wolf <kwolf@redhat.com> wrote:
> > > > 
> > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > > > > 
> > > > > > 
> > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > > > > >>
> > > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > > >> Cc: Kevin Wolf <kwolf@redhat.com>
> > > > > > >> Cc: qemu-block@nongnu.org
> > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > Interesting, I noticed we have a field scsi - see
> > > > > > > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > > > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> > > > > > >
> > > > > > > 	    virtio-blk: refuse SG_IO requests with scsi=off
> > > > > > >
> > > > > > > but it doesn't seem to be propagated to guest features in
> > > > > > > any way.
> > > > > > >
> > > > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > > > > 
> > > > > > Looks ok but auto may need some compat work since default is true.
> > > > > > 
> > > > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > > > interface then we can error out cleanly.
> > > > > > >
> > > > > > > Given scsi flag is currently ignored, I think
> > > > > > > this can be a patch on top.
> > > > > > 
> > > > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > > > > 
> > > > > >     if (!blk->conf.scsi) {
> > > > > >         status = VIRTIO_BLK_S_UNSUPP;
> > > > > >         goto fail;
> > > > > >     }
> > > > > 
> > > > > So we should be checking the same condition for the feature flag and
> > > > > error out in the init function if we have a VERSION_1 device and
> > > > > blk->conf.scsi is set.
> > > > 
> > > > Hm, I wonder how this plays with transports that want to make the
> > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > > > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > > > I'd need to check for !scsi as well before I can add this feature bit
> > > > then? Have the init function set a blocker for VERSION_1 so that the
> > > > driver may only negotiate revision 0?
> > > 
> > > 
> > > We already handle this, do we not?
> > (...)
> > > So guest that doesn't negotiate revision >= 1 never gets to see
> > > VIRTIO_F_VERSION_1.
> > 
> > Not my question :) I was wondering about scsi vs. virtio-1 devices. And
> > as I basically only want to make the decision on whether to offer
> > VERSION_1 when the guest negotiated a revision, I cannot fence scsi
> > during init, no?
> 
> No, I don't think there's a lot of value in offering scsi only to
> old guests that don't negotiate revision >= 1.
> 
> If user asked for virtio 1 support then that by proxy implies scsi
> passthrough does not work, and it won't work for legacy
> guests too.

This would imply that any transitional device cannot offer scsi,
doesn't it?

We have two layers interacting here: virtio-blk which may or may not
offer scsi support, and the transport layer which may or may not offer
VERSION_1 support. Failing scsi commands if VERSION_1 has been
negotiated makes sense to me; but I don't want to disable scsi config a
priori because the driver might negotiate VERSION_1. This would imply
that virtio-blk over virtio-ccw would never offer scsi once we enable
virtio-1 support, and it kind of defeats the purpose of a transitional
device for me.

(The other way round - fail negotiating revison 1 if the device was
configured with scsi support - makes more sense to me.)

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Paolo Bonzini]

On 13/07/2015 15:20, Cornelia Huck wrote:
> This would imply that any transitional device cannot offer scsi,
> doesn't it?
> 
> We have two layers interacting here: virtio-blk which may or may not
> offer scsi support, and the transport layer which may or may not offer
> VERSION_1 support. Failing scsi commands if VERSION_1 has been
> negotiated makes sense to me; but I don't want to disable scsi config a
> priori because the driver might negotiate VERSION_1. This would imply
> that virtio-blk over virtio-ccw would never offer scsi once we enable
> virtio-1 support, and it kind of defeats the purpose of a transitional
> device for me.
> 
> (The other way round - fail negotiating revison 1 if the device was
> configured with scsi support - makes more sense to me.)

For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI
altogether if !blk->conf.scsi.  Would that fix the problem for you too?

Paolo

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Mon, 13 Jul 2015 16:34:30 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 13/07/2015 15:20, Cornelia Huck wrote:
> > This would imply that any transitional device cannot offer scsi,
> > doesn't it?
> > 
> > We have two layers interacting here: virtio-blk which may or may not
> > offer scsi support, and the transport layer which may or may not offer
> > VERSION_1 support. Failing scsi commands if VERSION_1 has been
> > negotiated makes sense to me; but I don't want to disable scsi config a
> > priori because the driver might negotiate VERSION_1. This would imply
> > that virtio-blk over virtio-ccw would never offer scsi once we enable
> > virtio-1 support, and it kind of defeats the purpose of a transitional
> > device for me.
> > 
> > (The other way round - fail negotiating revison 1 if the device was
> > configured with scsi support - makes more sense to me.)
> 
> For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI
> altogether if !blk->conf.scsi.  Would that fix the problem for you too?

This is probably a sensible approach, and it can be contained in
virtio-block, no?

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Paolo Bonzini]

On 13/07/2015 16:41, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 16:34:30 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 13/07/2015 15:20, Cornelia Huck wrote:
>>> This would imply that any transitional device cannot offer scsi,
>>> doesn't it?
>>>
>>> We have two layers interacting here: virtio-blk which may or may not
>>> offer scsi support, and the transport layer which may or may not offer
>>> VERSION_1 support. Failing scsi commands if VERSION_1 has been
>>> negotiated makes sense to me; but I don't want to disable scsi config a
>>> priori because the driver might negotiate VERSION_1. This would imply
>>> that virtio-blk over virtio-ccw would never offer scsi once we enable
>>> virtio-1 support, and it kind of defeats the purpose of a transitional
>>> device for me.
>>>
>>> (The other way round - fail negotiating revison 1 if the device was
>>> configured with scsi support - makes more sense to me.)
>>
>> For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI
>> altogether if !blk->conf.scsi.  Would that fix the problem for you too?
> 
> This is probably a sensible approach, and it can be contained in
> virtio-block, no?

Yes, I think so.

Paolo

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Mon, Jul 13, 2015 at 03:20:59PM +0200, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 15:36:00 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
> > > On Mon, 13 Jul 2015 15:22:52 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > > > > On Mon, 13 Jul 2015 11:56:51 +0200
> > > > > Kevin Wolf <kwolf@redhat.com> wrote:
> > > > > 
> > > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > > > > > 
> > > > > > > 
> > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > > > > > >>
> > > > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > > > >> Cc: Kevin Wolf <kwolf@redhat.com>
> > > > > > > >> Cc: qemu-block@nongnu.org
> > > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > Interesting, I noticed we have a field scsi - see
> > > > > > > > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > > > > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > > > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> > > > > > > >
> > > > > > > > 	    virtio-blk: refuse SG_IO requests with scsi=off
> > > > > > > >
> > > > > > > > but it doesn't seem to be propagated to guest features in
> > > > > > > > any way.
> > > > > > > >
> > > > > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > > > > > 
> > > > > > > Looks ok but auto may need some compat work since default is true.
> > > > > > > 
> > > > > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > > > > interface then we can error out cleanly.
> > > > > > > >
> > > > > > > > Given scsi flag is currently ignored, I think
> > > > > > > > this can be a patch on top.
> > > > > > > 
> > > > > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > > > > > 
> > > > > > >     if (!blk->conf.scsi) {
> > > > > > >         status = VIRTIO_BLK_S_UNSUPP;
> > > > > > >         goto fail;
> > > > > > >     }
> > > > > > 
> > > > > > So we should be checking the same condition for the feature flag and
> > > > > > error out in the init function if we have a VERSION_1 device and
> > > > > > blk->conf.scsi is set.
> > > > > 
> > > > > Hm, I wonder how this plays with transports that want to make the
> > > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > > > > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > > > > I'd need to check for !scsi as well before I can add this feature bit
> > > > > then? Have the init function set a blocker for VERSION_1 so that the
> > > > > driver may only negotiate revision 0?
> > > > 
> > > > 
> > > > We already handle this, do we not?
> > > (...)
> > > > So guest that doesn't negotiate revision >= 1 never gets to see
> > > > VIRTIO_F_VERSION_1.
> > > 
> > > Not my question :) I was wondering about scsi vs. virtio-1 devices. And
> > > as I basically only want to make the decision on whether to offer
> > > VERSION_1 when the guest negotiated a revision, I cannot fence scsi
> > > during init, no?
> > 
> > No, I don't think there's a lot of value in offering scsi only to
> > old guests that don't negotiate revision >= 1.
> > 
> > If user asked for virtio 1 support then that by proxy implies scsi
> > passthrough does not work, and it won't work for legacy
> > guests too.
> 
> This would imply that any transitional device cannot offer scsi,
> doesn't it?

Yes, and that's because as written, transitional devices must set
ANY_LAYOUT, but that's incompatible with scsi.

> We have two layers interacting here: virtio-blk which may or may not
> offer scsi support, and the transport layer which may or may not offer
> VERSION_1 support. Failing scsi commands if VERSION_1 has been
> negotiated makes sense to me; but I don't want to disable scsi config a
> priori because the driver might negotiate VERSION_1. This would imply
> that virtio-blk over virtio-ccw would never offer scsi once we enable
> virtio-1 support, and it kind of defeats the purpose of a transitional
> device for me.
> 
> (The other way round - fail negotiating revison 1 if the device was
> configured with scsi support - makes more sense to me.)

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Mon, 13 Jul 2015 18:35:53 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2015 at 03:20:59PM +0200, Cornelia Huck wrote:
> > On Mon, 13 Jul 2015 15:36:00 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
> > > > On Mon, 13 Jul 2015 15:22:52 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > > > > > On Mon, 13 Jul 2015 11:56:51 +0200
> > > > > > Kevin Wolf <kwolf@redhat.com> wrote:
> > > > > > 
> > > > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > > > > > > >>
> > > > > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > > > > >> Cc: Kevin Wolf <kwolf@redhat.com>
> > > > > > > > >> Cc: qemu-block@nongnu.org
> > > > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > > Interesting, I noticed we have a field scsi - see
> > > > > > > > > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > > > > > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > > > > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> > > > > > > > >
> > > > > > > > > 	    virtio-blk: refuse SG_IO requests with scsi=off
> > > > > > > > >
> > > > > > > > > but it doesn't seem to be propagated to guest features in
> > > > > > > > > any way.
> > > > > > > > >
> > > > > > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > > > > > > 
> > > > > > > > Looks ok but auto may need some compat work since default is true.
> > > > > > > > 
> > > > > > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > > > > > interface then we can error out cleanly.
> > > > > > > > >
> > > > > > > > > Given scsi flag is currently ignored, I think
> > > > > > > > > this can be a patch on top.
> > > > > > > > 
> > > > > > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > > > > > > 
> > > > > > > >     if (!blk->conf.scsi) {
> > > > > > > >         status = VIRTIO_BLK_S_UNSUPP;
> > > > > > > >         goto fail;
> > > > > > > >     }
> > > > > > > 
> > > > > > > So we should be checking the same condition for the feature flag and
> > > > > > > error out in the init function if we have a VERSION_1 device and
> > > > > > > blk->conf.scsi is set.
> > > > > > 
> > > > > > Hm, I wonder how this plays with transports that want to make the
> > > > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > > > > > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > > > > > I'd need to check for !scsi as well before I can add this feature bit
> > > > > > then? Have the init function set a blocker for VERSION_1 so that the
> > > > > > driver may only negotiate revision 0?
> > > > > 
> > > > > 
> > > > > We already handle this, do we not?
> > > > (...)
> > > > > So guest that doesn't negotiate revision >= 1 never gets to see
> > > > > VIRTIO_F_VERSION_1.
> > > > 
> > > > Not my question :) I was wondering about scsi vs. virtio-1 devices. And
> > > > as I basically only want to make the decision on whether to offer
> > > > VERSION_1 when the guest negotiated a revision, I cannot fence scsi
> > > > during init, no?
> > > 
> > > No, I don't think there's a lot of value in offering scsi only to
> > > old guests that don't negotiate revision >= 1.
> > > 
> > > If user asked for virtio 1 support then that by proxy implies scsi
> > > passthrough does not work, and it won't work for legacy
> > > guests too.
> > 
> > This would imply that any transitional device cannot offer scsi,
> > doesn't it?
> 
> Yes, and that's because as written, transitional devices must set
> ANY_LAYOUT, but that's incompatible with scsi.

Hm, I had a patch before that dynamically allowed different feature
sets for legacy or modern, not only a subset. Probably won't apply
anymore, but I'd like to able to do the following:

- driver reads features without negotiating a revision: driver is
  legacy, offer legacy bits
- driver negotiates revision 0: dito
- driver negotiates revision >= 1: driver is modern, offer modern bits

That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
first two cases, and a new qemu could still offer scsi to old guests.

Would it be worth pursuing that idea?

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote:
> > Yes, and that's because as written, transitional devices must set
> > ANY_LAYOUT, but that's incompatible with scsi.
> 
> Hm, I had a patch before that dynamically allowed different feature
> sets for legacy or modern, not only a subset. Probably won't apply
> anymore, but I'd like to able to do the following:
> 
> - driver reads features without negotiating a revision: driver is
>   legacy, offer legacy bits
> - driver negotiates revision 0: dito
> - driver negotiates revision >= 1: driver is modern, offer modern bits
> 
> That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
> first two cases, and a new qemu could still offer scsi to old guests.
> 
> Would it be worth pursuing that idea?

Frankly, I don't think so: I don't see why it makes sense
to expose more features on the legacy interface than
on the modern one. Imagine updating drivers to fix a bug
and losing some features. How does this make sense?

I think the virtio TC's assumption was that the scsi passthrough was a
bad idea, so in QEMU we only keep it around for legacy devices to avoid
regressions.

If you disagree and think transitional devices need the SCSI feature,
either try to convince pbonzini or rewrite the spec youself
to support it in the virtio 1 mode.

-- 
MST

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Wed, 15 Jul 2015 13:59:00 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote:
> > > Yes, and that's because as written, transitional devices must set
> > > ANY_LAYOUT, but that's incompatible with scsi.
> > 
> > Hm, I had a patch before that dynamically allowed different feature
> > sets for legacy or modern, not only a subset. Probably won't apply
> > anymore, but I'd like to able to do the following:
> > 
> > - driver reads features without negotiating a revision: driver is
> >   legacy, offer legacy bits
> > - driver negotiates revision 0: dito
> > - driver negotiates revision >= 1: driver is modern, offer modern bits
> > 
> > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
> > first two cases, and a new qemu could still offer scsi to old guests.
> > 
> > Would it be worth pursuing that idea?
> 
> Frankly, I don't think so: I don't see why it makes sense
> to expose more features on the legacy interface than
> on the modern one. Imagine updating drivers to fix a bug
> and losing some features. How does this make sense?

I don't think one should be a strict subset of the other. But I think
we don't want to withdraw features from legacy guests on qemu updates
either?

> 
> I think the virtio TC's assumption was that the scsi passthrough was a
> bad idea, so in QEMU we only keep it around for legacy devices to avoid
> regressions.

I'm not opposing this :)

> 
> If you disagree and think transitional devices need the SCSI feature,
> either try to convince pbonzini or rewrite the spec youself
> to support it in the virtio 1 mode.

This seems to boil down to the different meaning of "transitional" for
ccw and pci, see the other thread.

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 13:59:00 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote:
> > > > Yes, and that's because as written, transitional devices must set
> > > > ANY_LAYOUT, but that's incompatible with scsi.
> > > 
> > > Hm, I had a patch before that dynamically allowed different feature
> > > sets for legacy or modern, not only a subset. Probably won't apply
> > > anymore, but I'd like to able to do the following:
> > > 
> > > - driver reads features without negotiating a revision: driver is
> > >   legacy, offer legacy bits
> > > - driver negotiates revision 0: dito
> > > - driver negotiates revision >= 1: driver is modern, offer modern bits
> > > 
> > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
> > > first two cases, and a new qemu could still offer scsi to old guests.
> > > 
> > > Would it be worth pursuing that idea?
> > 
> > Frankly, I don't think so: I don't see why it makes sense
> > to expose more features on the legacy interface than
> > on the modern one. Imagine updating drivers to fix a bug
> > and losing some features. How does this make sense?
> 
> I don't think one should be a strict subset of the other. But I think
> we don't want to withdraw features from legacy guests on qemu updates
> either?

Absolutely. For now one has to enable the modern interface
explicitly. Around 2.5 we might switch that around, we'll
need to think hard about compatibility at that point.
In any case, we must definitely keep the old capability for old machine
types.

> > 
> > I think the virtio TC's assumption was that the scsi passthrough was a
> > bad idea, so in QEMU we only keep it around for legacy devices to avoid
> > regressions.
> 
> I'm not opposing this :)
> 
> > 
> > If you disagree and think transitional devices need the SCSI feature,
> > either try to convince pbonzini or rewrite the spec youself
> > to support it in the virtio 1 mode.
> 
> This seems to boil down to the different meaning of "transitional" for
> ccw and pci, see the other thread.

Before the revision is negotiated, ccw won't know whether
it's a legacy driver - is that the difference?
Fine, but revision is negotiated way before features are
probed so why does it make a practical difference?

-- 
MST

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Wed, 15 Jul 2015 15:01:01 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote:
> > On Wed, 15 Jul 2015 13:59:00 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote:
> > > > > Yes, and that's because as written, transitional devices must set
> > > > > ANY_LAYOUT, but that's incompatible with scsi.
> > > > 
> > > > Hm, I had a patch before that dynamically allowed different feature
> > > > sets for legacy or modern, not only a subset. Probably won't apply
> > > > anymore, but I'd like to able to do the following:
> > > > 
> > > > - driver reads features without negotiating a revision: driver is
> > > >   legacy, offer legacy bits
> > > > - driver negotiates revision 0: dito
> > > > - driver negotiates revision >= 1: driver is modern, offer modern bits
> > > > 
> > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
> > > > first two cases, and a new qemu could still offer scsi to old guests.
> > > > 
> > > > Would it be worth pursuing that idea?
> > > 
> > > Frankly, I don't think so: I don't see why it makes sense
> > > to expose more features on the legacy interface than
> > > on the modern one. Imagine updating drivers to fix a bug
> > > and losing some features. How does this make sense?
> > 
> > I don't think one should be a strict subset of the other. But I think
> > we don't want to withdraw features from legacy guests on qemu updates
> > either?
> 
> Absolutely. For now one has to enable the modern interface
> explicitly. Around 2.5 we might switch that around, we'll
> need to think hard about compatibility at that point.
> In any case, we must definitely keep the old capability for old machine
> types.

ccw only offers revision 0 (legacy) in 2.4. I plan to introduce
revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4
is the first versioned ccw machine).

> 
> > > 
> > > I think the virtio TC's assumption was that the scsi passthrough was a
> > > bad idea, so in QEMU we only keep it around for legacy devices to avoid
> > > regressions.
> > 
> > I'm not opposing this :)
> > 
> > > 
> > > If you disagree and think transitional devices need the SCSI feature,
> > > either try to convince pbonzini or rewrite the spec youself
> > > to support it in the virtio 1 mode.
> > 
> > This seems to boil down to the different meaning of "transitional" for
> > ccw and pci, see the other thread.
> 
> Before the revision is negotiated, ccw won't know whether
> it's a legacy driver - is that the difference?

I'd say it doesn't know whether the driver intends to use the modern
interface.

> Fine, but revision is negotiated way before features are
> probed so why does it make a practical difference?

Legacy drivers (that don't know about the set-revision command) will
read features without revision negotiation - we need to offer them the
legacy feature set.

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 15:01:01 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote:
> > > On Wed, 15 Jul 2015 13:59:00 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote:
> > > > > > Yes, and that's because as written, transitional devices must set
> > > > > > ANY_LAYOUT, but that's incompatible with scsi.
> > > > > 
> > > > > Hm, I had a patch before that dynamically allowed different feature
> > > > > sets for legacy or modern, not only a subset. Probably won't apply
> > > > > anymore, but I'd like to able to do the following:
> > > > > 
> > > > > - driver reads features without negotiating a revision: driver is
> > > > >   legacy, offer legacy bits
> > > > > - driver negotiates revision 0: dito
> > > > > - driver negotiates revision >= 1: driver is modern, offer modern bits
> > > > > 
> > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
> > > > > first two cases, and a new qemu could still offer scsi to old guests.
> > > > > 
> > > > > Would it be worth pursuing that idea?
> > > > 
> > > > Frankly, I don't think so: I don't see why it makes sense
> > > > to expose more features on the legacy interface than
> > > > on the modern one. Imagine updating drivers to fix a bug
> > > > and losing some features. How does this make sense?
> > > 
> > > I don't think one should be a strict subset of the other. But I think
> > > we don't want to withdraw features from legacy guests on qemu updates
> > > either?
> > 
> > Absolutely. For now one has to enable the modern interface
> > explicitly. Around 2.5 we might switch that around, we'll
> > need to think hard about compatibility at that point.
> > In any case, we must definitely keep the old capability for old machine
> > types.
> 
> ccw only offers revision 0 (legacy) in 2.4. I plan to introduce
> revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4
> is the first versioned ccw machine).

I was talking about pci here actually.

> > 
> > > > 
> > > > I think the virtio TC's assumption was that the scsi passthrough was a
> > > > bad idea, so in QEMU we only keep it around for legacy devices to avoid
> > > > regressions.
> > > 
> > > I'm not opposing this :)
> > > 
> > > > 
> > > > If you disagree and think transitional devices need the SCSI feature,
> > > > either try to convince pbonzini or rewrite the spec youself
> > > > to support it in the virtio 1 mode.
> > > 
> > > This seems to boil down to the different meaning of "transitional" for
> > > ccw and pci, see the other thread.
> > 
> > Before the revision is negotiated, ccw won't know whether
> > it's a legacy driver - is that the difference?
> 
> I'd say it doesn't know whether the driver intends to use the modern
> interface.

That's also the case for pci.

> > Fine, but revision is negotiated way before features are
> > probed so why does it make a practical difference?
> 
> Legacy drivers (that don't know about the set-revision command) will
> read features without revision negotiation - we need to offer them the
> legacy feature set.

Right. So simply do if (revision < 1) return features & 0xffffffff
and that will do this, will it not?

-- 
MST

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Wed, 15 Jul 2015 16:16:07 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote:
> > On Wed, 15 Jul 2015 15:01:01 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote:
> > > > On Wed, 15 Jul 2015 13:59:00 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote:
> > > > > > > Yes, and that's because as written, transitional devices must set
> > > > > > > ANY_LAYOUT, but that's incompatible with scsi.
> > > > > > 
> > > > > > Hm, I had a patch before that dynamically allowed different feature
> > > > > > sets for legacy or modern, not only a subset. Probably won't apply
> > > > > > anymore, but I'd like to able to do the following:
> > > > > > 
> > > > > > - driver reads features without negotiating a revision: driver is
> > > > > >   legacy, offer legacy bits
> > > > > > - driver negotiates revision 0: dito
> > > > > > - driver negotiates revision >= 1: driver is modern, offer modern bits
> > > > > > 
> > > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
> > > > > > first two cases, and a new qemu could still offer scsi to old guests.
> > > > > > 
> > > > > > Would it be worth pursuing that idea?
> > > > > 
> > > > > Frankly, I don't think so: I don't see why it makes sense
> > > > > to expose more features on the legacy interface than
> > > > > on the modern one. Imagine updating drivers to fix a bug
> > > > > and losing some features. How does this make sense?
> > > > 
> > > > I don't think one should be a strict subset of the other. But I think
> > > > we don't want to withdraw features from legacy guests on qemu updates
> > > > either?
> > > 
> > > Absolutely. For now one has to enable the modern interface
> > > explicitly. Around 2.5 we might switch that around, we'll
> > > need to think hard about compatibility at that point.
> > > In any case, we must definitely keep the old capability for old machine
> > > types.
> > 
> > ccw only offers revision 0 (legacy) in 2.4. I plan to introduce
> > revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4
> > is the first versioned ccw machine).
> 
> I was talking about pci here actually.

Sure, and these are my plans for ccw ;)

> 
> > > 
> > > > > 
> > > > > I think the virtio TC's assumption was that the scsi passthrough was a
> > > > > bad idea, so in QEMU we only keep it around for legacy devices to avoid
> > > > > regressions.
> > > > 
> > > > I'm not opposing this :)
> > > > 
> > > > > 
> > > > > If you disagree and think transitional devices need the SCSI feature,
> > > > > either try to convince pbonzini or rewrite the spec youself
> > > > > to support it in the virtio 1 mode.
> > > > 
> > > > This seems to boil down to the different meaning of "transitional" for
> > > > ccw and pci, see the other thread.
> > > 
> > > Before the revision is negotiated, ccw won't know whether
> > > it's a legacy driver - is that the difference?
> > 
> > I'd say it doesn't know whether the driver intends to use the modern
> > interface.
> 
> That's also the case for pci.

But does pci know the moment it first tries to get the device's
features? And does pci assume modern as default for transitional
devices?

> 
> > > Fine, but revision is negotiated way before features are
> > > probed so why does it make a practical difference?
> > 
> > Legacy drivers (that don't know about the set-revision command) will
> > read features without revision negotiation - we need to offer them the
> > legacy feature set.
> 
> Right. So simply do if (revision < 1) return features & 0xffffffff
> and that will do this, will it not?

Not for bits that we want to offer for legacy but not for modern.

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 03:40:22PM +0200, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 16:16:07 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote:
> > > On Wed, 15 Jul 2015 15:01:01 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote:
> > > > > On Wed, 15 Jul 2015 13:59:00 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote:
> > > > > > > > Yes, and that's because as written, transitional devices must set
> > > > > > > > ANY_LAYOUT, but that's incompatible with scsi.
> > > > > > > 
> > > > > > > Hm, I had a patch before that dynamically allowed different feature
> > > > > > > sets for legacy or modern, not only a subset. Probably won't apply
> > > > > > > anymore, but I'd like to able to do the following:
> > > > > > > 
> > > > > > > - driver reads features without negotiating a revision: driver is
> > > > > > >   legacy, offer legacy bits
> > > > > > > - driver negotiates revision 0: dito
> > > > > > > - driver negotiates revision >= 1: driver is modern, offer modern bits
> > > > > > > 
> > > > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the
> > > > > > > first two cases, and a new qemu could still offer scsi to old guests.
> > > > > > > 
> > > > > > > Would it be worth pursuing that idea?
> > > > > > 
> > > > > > Frankly, I don't think so: I don't see why it makes sense
> > > > > > to expose more features on the legacy interface than
> > > > > > on the modern one. Imagine updating drivers to fix a bug
> > > > > > and losing some features. How does this make sense?
> > > > > 
> > > > > I don't think one should be a strict subset of the other. But I think
> > > > > we don't want to withdraw features from legacy guests on qemu updates
> > > > > either?
> > > > 
> > > > Absolutely. For now one has to enable the modern interface
> > > > explicitly. Around 2.5 we might switch that around, we'll
> > > > need to think hard about compatibility at that point.
> > > > In any case, we must definitely keep the old capability for old machine
> > > > types.
> > > 
> > > ccw only offers revision 0 (legacy) in 2.4. I plan to introduce
> > > revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4
> > > is the first versioned ccw machine).
> > 
> > I was talking about pci here actually.
> 
> Sure, and these are my plans for ccw ;)
> 
> > 
> > > > 
> > > > > > 
> > > > > > I think the virtio TC's assumption was that the scsi passthrough was a
> > > > > > bad idea, so in QEMU we only keep it around for legacy devices to avoid
> > > > > > regressions.
> > > > > 
> > > > > I'm not opposing this :)
> > > > > 
> > > > > > 
> > > > > > If you disagree and think transitional devices need the SCSI feature,
> > > > > > either try to convince pbonzini or rewrite the spec youself
> > > > > > to support it in the virtio 1 mode.
> > > > > 
> > > > > This seems to boil down to the different meaning of "transitional" for
> > > > > ccw and pci, see the other thread.
> > > > 
> > > > Before the revision is negotiated, ccw won't know whether
> > > > it's a legacy driver - is that the difference?
> > > 
> > > I'd say it doesn't know whether the driver intends to use the modern
> > > interface.
> > 
> > That's also the case for pci.
> 
> But does pci know the moment it first tries to get the device's
> features? And does pci assume modern as default for transitional
> devices?

I don't think it does.

> > 
> > > > Fine, but revision is negotiated way before features are
> > > > probed so why does it make a practical difference?
> > > 
> > > Legacy drivers (that don't know about the set-revision command) will
> > > read features without revision negotiation - we need to offer them the
> > > legacy feature set.
> > 
> > Right. So simply do if (revision < 1) return features & 0xffffffff
> > and that will do this, will it not?
> 
> Not for bits that we want to offer for legacy but not for modern.

I don't think this selective offering works at least for scsi.
scsi is a backend feature, if you connect a modern device
in front the device simply does not work.
It therefore makes no sense to attach a transitional device
to such a backend.

-- 
MST

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Wed, 15 Jul 2015 17:11:57 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > > > > Fine, but revision is negotiated way before features are
> > > > > probed so why does it make a practical difference?
> > > > 
> > > > Legacy drivers (that don't know about the set-revision command) will
> > > > read features without revision negotiation - we need to offer them the
> > > > legacy feature set.
> > > 
> > > Right. So simply do if (revision < 1) return features & 0xffffffff
> > > and that will do this, will it not?
> > 
> > Not for bits that we want to offer for legacy but not for modern.
> 
> I don't think this selective offering works at least for scsi.
> scsi is a backend feature, if you connect a modern device
> in front the device simply does not work.
> It therefore makes no sense to attach a transitional device
> to such a backend.

My point is that we're losing legacy features with that approach, and
it would not be possible to offer them to legacy guests with newer
qemus (at least with ccw).

What about the other way around (i.e. scsi is configured, therefore the
device is legacy-only)? We'd only retain the scsi bit if it is actually
wanted by the user's configuration. I would need to enforce a max
revision of 0 for such a device in ccw, and pci could disable modern
for it.

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 17:11:57 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > > > Fine, but revision is negotiated way before features are
> > > > > > probed so why does it make a practical difference?
> > > > > 
> > > > > Legacy drivers (that don't know about the set-revision command) will
> > > > > read features without revision negotiation - we need to offer them the
> > > > > legacy feature set.
> > > > 
> > > > Right. So simply do if (revision < 1) return features & 0xffffffff
> > > > and that will do this, will it not?
> > > 
> > > Not for bits that we want to offer for legacy but not for modern.
> > 
> > I don't think this selective offering works at least for scsi.
> > scsi is a backend feature, if you connect a modern device
> > in front the device simply does not work.
> > It therefore makes no sense to attach a transitional device
> > to such a backend.
> 
> My point is that we're losing legacy features with that approach, and
> it would not be possible to offer them to legacy guests with newer
> qemus (at least with ccw).

What's wrong with adding a disable-modern flag, like pci has?
User can set that to get a legacy device.

> What about the other way around (i.e. scsi is configured, therefore the
> device is legacy-only)? We'd only retain the scsi bit if it is actually
> wanted by the user's configuration. I would need to enforce a max
> revision of 0 for such a device in ccw, and pci could disable modern
> for it.

Will have to think about it.
But I think a flag to disable/enable modern is useful in any case,
and it seems sufficient.

-- 
MST

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Wed, 15 Jul 2015 17:39:18 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote:
> > On Wed, 15 Jul 2015 17:11:57 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > > > > > Fine, but revision is negotiated way before features are
> > > > > > > probed so why does it make a practical difference?
> > > > > > 
> > > > > > Legacy drivers (that don't know about the set-revision command) will
> > > > > > read features without revision negotiation - we need to offer them the
> > > > > > legacy feature set.
> > > > > 
> > > > > Right. So simply do if (revision < 1) return features & 0xffffffff
> > > > > and that will do this, will it not?
> > > > 
> > > > Not for bits that we want to offer for legacy but not for modern.
> > > 
> > > I don't think this selective offering works at least for scsi.
> > > scsi is a backend feature, if you connect a modern device
> > > in front the device simply does not work.
> > > It therefore makes no sense to attach a transitional device
> > > to such a backend.
> > 
> > My point is that we're losing legacy features with that approach, and
> > it would not be possible to offer them to legacy guests with newer
> > qemus (at least with ccw).
> 
> What's wrong with adding a disable-modern flag, like pci has?
> User can set that to get a legacy device.

The whole idea behind the revision-stuff was that we don't need
something like disable-modern. If the device is able to figure out on
its own if it is to act as a modern or a legacy device, why require
user intervention?

> 
> > What about the other way around (i.e. scsi is configured, therefore the
> > device is legacy-only)? We'd only retain the scsi bit if it is actually
> > wanted by the user's configuration. I would need to enforce a max
> > revision of 0 for such a device in ccw, and pci could disable modern
> > for it.
> 
> Will have to think about it.
> But I think a flag to disable/enable modern is useful in any case,
> and it seems sufficient.

I don't like the idea of disabling modern or legacy for ccw, where the
differences between both are very minor.

I also don't think requiring the user to specify a new flag on upgrade
just to present the same features as before is a good idea: it is
something that is easily missed and may lead to much headscratching.

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 17:39:18 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote:
> > > On Wed, 15 Jul 2015 17:11:57 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > > > > > Fine, but revision is negotiated way before features are
> > > > > > > > probed so why does it make a practical difference?
> > > > > > > 
> > > > > > > Legacy drivers (that don't know about the set-revision command) will
> > > > > > > read features without revision negotiation - we need to offer them the
> > > > > > > legacy feature set.
> > > > > > 
> > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff
> > > > > > and that will do this, will it not?
> > > > > 
> > > > > Not for bits that we want to offer for legacy but not for modern.
> > > > 
> > > > I don't think this selective offering works at least for scsi.
> > > > scsi is a backend feature, if you connect a modern device
> > > > in front the device simply does not work.
> > > > It therefore makes no sense to attach a transitional device
> > > > to such a backend.
> > > 
> > > My point is that we're losing legacy features with that approach, and
> > > it would not be possible to offer them to legacy guests with newer
> > > qemus (at least with ccw).
> > 
> > What's wrong with adding a disable-modern flag, like pci has?
> > User can set that to get a legacy device.
> 
> The whole idea behind the revision-stuff was that we don't need
> something like disable-modern. If the device is able to figure out on
> its own if it is to act as a modern or a legacy device, why require
> user intervention?

It's about compatibility, e.g. being able to test legacy mode
in transitional drivers in guests.
Consider also bugs, e.g. the fact that linux guests lack WCE
in modern mode ATM.

> > 
> > > What about the other way around (i.e. scsi is configured, therefore the
> > > device is legacy-only)? We'd only retain the scsi bit if it is actually
> > > wanted by the user's configuration. I would need to enforce a max
> > > revision of 0 for such a device in ccw, and pci could disable modern
> > > for it.
> > 
> > Will have to think about it.
> > But I think a flag to disable/enable modern is useful in any case,
> > and it seems sufficient.
> 
> I don't like the idea of disabling modern or legacy for ccw, where the
> differences between both are very minor.
> 
> I also don't think requiring the user to specify a new flag on upgrade
> just to present the same features as before is a good idea: it is
> something that is easily missed and may lead to much headscratching.

And doing this on a driver upgrade won't?  As I said, if you believe
this feature has value, argue that we shouldn't drop scsi off in virtio
1.0 then.

-- 
MST

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Wed, 15 Jul 2015 21:51:32 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote:
> > On Wed, 15 Jul 2015 17:39:18 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote:
> > > > On Wed, 15 Jul 2015 17:11:57 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > > > > > Fine, but revision is negotiated way before features are
> > > > > > > > > probed so why does it make a practical difference?
> > > > > > > > 
> > > > > > > > Legacy drivers (that don't know about the set-revision command) will
> > > > > > > > read features without revision negotiation - we need to offer them the
> > > > > > > > legacy feature set.
> > > > > > > 
> > > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff
> > > > > > > and that will do this, will it not?
> > > > > > 
> > > > > > Not for bits that we want to offer for legacy but not for modern.
> > > > > 
> > > > > I don't think this selective offering works at least for scsi.
> > > > > scsi is a backend feature, if you connect a modern device
> > > > > in front the device simply does not work.
> > > > > It therefore makes no sense to attach a transitional device
> > > > > to such a backend.
> > > > 
> > > > My point is that we're losing legacy features with that approach, and
> > > > it would not be possible to offer them to legacy guests with newer
> > > > qemus (at least with ccw).
> > > 
> > > What's wrong with adding a disable-modern flag, like pci has?
> > > User can set that to get a legacy device.
> > 
> > The whole idea behind the revision-stuff was that we don't need
> > something like disable-modern. If the device is able to figure out on
> > its own if it is to act as a modern or a legacy device, why require
> > user intervention?
> 
> It's about compatibility, e.g. being able to test legacy mode
> in transitional drivers in guests.

Let's step back a bit. Why do we need legacy? To support old hosts and
old guests. So what we need is a test matrix combining old/modern hosts
with old/modern guests. I don't think forcing to an old version is
something we should spend time on except during development.

> Consider also bugs, e.g. the fact that linux guests lack WCE
> in modern mode ATM.

If we consider this a bug, shouldn't VERSION_1 be forced off
unconditionally until specs and codebase are fixed?

> 
> > > 
> > > > What about the other way around (i.e. scsi is configured, therefore the
> > > > device is legacy-only)? We'd only retain the scsi bit if it is actually
> > > > wanted by the user's configuration. I would need to enforce a max
> > > > revision of 0 for such a device in ccw, and pci could disable modern
> > > > for it.
> > > 
> > > Will have to think about it.
> > > But I think a flag to disable/enable modern is useful in any case,
> > > and it seems sufficient.
> > 
> > I don't like the idea of disabling modern or legacy for ccw, where the
> > differences between both are very minor.
> > 
> > I also don't think requiring the user to specify a new flag on upgrade
> > just to present the same features as before is a good idea: it is
> > something that is easily missed and may lead to much headscratching.
> 
> And doing this on a driver upgrade won't?  As I said, if you believe
> this feature has value, argue that we shouldn't drop scsi off in virtio
> 1.0 then.

I seem to have problems explaining myself :(

I don't care about the feature, except for supporting old guests that
should continue working as before even if the host updated. And without
special configuration on the host, as this is too easily forgotten.
What else is legacy good for, other than providing backwards
compatibility?

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Michael S. Tsirkin]

On Thu, Jul 16, 2015 at 02:37:12PM +0200, Cornelia Huck wrote:
> On Wed, 15 Jul 2015 21:51:32 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote:
> > > On Wed, 15 Jul 2015 17:39:18 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote:
> > > > > On Wed, 15 Jul 2015 17:11:57 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > > > > > Fine, but revision is negotiated way before features are
> > > > > > > > > > probed so why does it make a practical difference?
> > > > > > > > > 
> > > > > > > > > Legacy drivers (that don't know about the set-revision command) will
> > > > > > > > > read features without revision negotiation - we need to offer them the
> > > > > > > > > legacy feature set.
> > > > > > > > 
> > > > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff
> > > > > > > > and that will do this, will it not?
> > > > > > > 
> > > > > > > Not for bits that we want to offer for legacy but not for modern.
> > > > > > 
> > > > > > I don't think this selective offering works at least for scsi.
> > > > > > scsi is a backend feature, if you connect a modern device
> > > > > > in front the device simply does not work.
> > > > > > It therefore makes no sense to attach a transitional device
> > > > > > to such a backend.
> > > > > 
> > > > > My point is that we're losing legacy features with that approach, and
> > > > > it would not be possible to offer them to legacy guests with newer
> > > > > qemus (at least with ccw).
> > > > 
> > > > What's wrong with adding a disable-modern flag, like pci has?
> > > > User can set that to get a legacy device.
> > > 
> > > The whole idea behind the revision-stuff was that we don't need
> > > something like disable-modern. If the device is able to figure out on
> > > its own if it is to act as a modern or a legacy device, why require
> > > user intervention?
> > 
> > It's about compatibility, e.g. being able to test legacy mode
> > in transitional drivers in guests.
> 
> Let's step back a bit. Why do we need legacy? To support old hosts and
> old guests. So what we need is a test matrix combining old/modern hosts
> with old/modern guests. I don't think forcing to an old version is
> something we should spend time on except during development.
> 
> > Consider also bugs, e.g. the fact that linux guests lack WCE
> > in modern mode ATM.
> 
> If we consider this a bug, shouldn't VERSION_1 be forced off
> unconditionally until specs and codebase are fixed?

I don't see why.  We'll never fix all bugs.  It causes a performance
regression but otherwise things work correctly.
modern is off by default for now, this seems sufficient, if we
never even ship it as an option we'll never find all bugs.

> > 
> > > > 
> > > > > What about the other way around (i.e. scsi is configured, therefore the
> > > > > device is legacy-only)? We'd only retain the scsi bit if it is actually
> > > > > wanted by the user's configuration. I would need to enforce a max
> > > > > revision of 0 for such a device in ccw, and pci could disable modern
> > > > > for it.
> > > > 
> > > > Will have to think about it.
> > > > But I think a flag to disable/enable modern is useful in any case,
> > > > and it seems sufficient.
> > > 
> > > I don't like the idea of disabling modern or legacy for ccw, where the
> > > differences between both are very minor.
> > > 
> > > I also don't think requiring the user to specify a new flag on upgrade
> > > just to present the same features as before is a good idea: it is
> > > something that is easily missed and may lead to much headscratching.
> > 
> > And doing this on a driver upgrade won't?  As I said, if you believe
> > this feature has value, argue that we shouldn't drop scsi off in virtio
> > 1.0 then.
> 
> I seem to have problems explaining myself :(
> 
> I don't care about the feature, except for supporting old guests that
> should continue working as before even if the host updated. And without
> special configuration on the host, as this is too easily forgotten.
> What else is legacy good for, other than providing backwards
> compatibility?

Maybe it's me having trouble explaining.

There are two kind of blk devices:

- those that allow scsi passthrough
	Such devices can't work properly through the modern interface.
	We could add a modern interface but device can not operate through it.

- those that don't allow scsi passthrough
	Such devices can't work properly through the modern interface.
	We could add a modern interface and we'll get a transitional
	device.


I think for 2.4 it's a good idea to avoid enabling modern interface
by default. Therefore, for 2.4 we can keep scsi enabled unless modern
is requested by user. I am also fine with just doing

	if (modern && scsi)
		exit;


-- 
MST

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Paolo Bonzini]

On 16/07/2015 14:47, Michael S. Tsirkin wrote:
> I think for 2.4 it's a good idea to avoid enabling modern interface
> by default. Therefore, for 2.4 we can keep scsi enabled unless modern
> is requested by user.

I agree.

> I am also fine with just doing
> 
> 	if (modern && scsi)
> 		exit;

exit is evil.

Paolo

Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Thu, 16 Jul 2015 19:22:21 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 16/07/2015 14:47, Michael S. Tsirkin wrote:
> > I think for 2.4 it's a good idea to avoid enabling modern interface
> > by default. Therefore, for 2.4 we can keep scsi enabled unless modern
> > is requested by user.
> 
> I agree.

For 2.4, ccw isn't supporting revisions > 0 anyway, so keeping scsi
enabled for 2.4 sounds good to me as well.