[Qemu-devel] [PATCH 0/3] Make virtio_load permissive when possible

[Qemu-devel] [PATCH 0/3] Make virtio_load permissive when possible

[Paolo Bonzini]

virtio_load checks features that are enabled by the guest and
blocks migration if they are not available in the destination
host.  However, in some cases we can let features through because
we know that guests will be able to proceed even without it.

The patch is on top of Orit's at
http://permalink.gmane.org/gmane.comp.emulators.qemu/138806.

Paolo Bonzini (3):
  virtio: let devices be permissive on enabled features
  virtio-balloon: note optional features
  virtio-blk: note optional features

 hw/virtio-balloon.c    |    2 +-
 hw/virtio-blk.c        |    2 +-
 hw/virtio-net.c        |    2 +-
 hw/virtio-scsi.c       |    2 +-
 hw/virtio-serial-bus.c |    2 +-
 hw/virtio.c            |    8 +++++---
 hw/virtio.h            |    2 +-
 7 files changed, 11 insertions(+), 9 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/3] virtio: let devices be permissive on enabled features

[Paolo Bonzini]

virtio_load checks features that are enabled by the guest and
blocks migration if they are not available in the destination
host.  However, in some cases we can let features through because
we know that guests will be able to proceed even without it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-balloon.c    |    2 +-
 hw/virtio-blk.c        |    2 +-
 hw/virtio-net.c        |    2 +-
 hw/virtio-scsi.c       |    2 +-
 hw/virtio-serial-bus.c |    2 +-
 hw/virtio.c            |    8 +++++---
 hw/virtio.h            |    2 +-
 7 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 075ed87..0ade8b0 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -216,7 +216,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 1)
         return -EINVAL;
 
-    ret = virtio_load(&s->vdev, f);
+    ret = virtio_load(&s->vdev, f, 0);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index d4bb400..c95f8fc 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -542,7 +542,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(&s->vdev, f);
+    ret = virtio_load(&s->vdev, f, 0);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 3f190d4..719ba96 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -896,7 +896,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
 
-    ret = virtio_load(&n->vdev, f);
+    ret = virtio_load(&n->vdev, f, 0);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c
index 9797847..da281e2 100644
--- a/hw/virtio-scsi.c
+++ b/hw/virtio-scsi.c
@@ -560,7 +560,7 @@ static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
     VirtIOSCSI *s = opaque;
     int ret;
 
-    ret = virtio_load(&s->vdev, f);
+    ret = virtio_load(&s->vdev, f, 0);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 4a33872..5ef7d30 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -597,7 +597,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     /* The virtio device */
-    ret = virtio_load(&s->vdev, f);
+    ret = virtio_load(&s->vdev, f, 0);
     if (ret) {
         return ret;
     }
diff --git a/hw/virtio.c b/hw/virtio.c
index 064aecf..fdbb286 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -777,11 +777,12 @@ int virtio_set_features(VirtIODevice *vdev, uint32_t val)
     return bad ? -1 : 0;
 }
 
-int virtio_load(VirtIODevice *vdev, QEMUFile *f)
+int virtio_load(VirtIODevice *vdev, QEMUFile *f, int additional_features)
 {
     int num, i, ret;
     uint32_t features;
     uint32_t supported_features;
+    uint32_t disabled_features;
 
     if (vdev->binding->load_config) {
         ret = vdev->binding->load_config(vdev->binding_opaque, f);
@@ -794,8 +795,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
     qemu_get_be16s(f, &vdev->queue_sel);
     qemu_get_be32s(f, &features);
 
-    if (virtio_set_features(vdev, features) < 0) {
-        supported_features = vdev->binding->get_features(vdev->binding_opaque);
+    supported_features = vdev->binding->get_features(vdev->binding_opaque);
+    disabled_features = additional_features & ~supported_features;
+    if (virtio_set_features(vdev, features & ~disabled_features) < 0) {
         error_report("Features 0x%x unsupported. Allowed features: 0x%x",
                      features, supported_features);
         return -1;
diff --git a/hw/virtio.h b/hw/virtio.h
index 400c092..2e8dd20 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -154,7 +154,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
 
 void virtio_save(VirtIODevice *vdev, QEMUFile *f);
 
-int virtio_load(VirtIODevice *vdev, QEMUFile *f);
+int virtio_load(VirtIODevice *vdev, QEMUFile *f, int additional_features);
 
 void virtio_cleanup(VirtIODevice *vdev);
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/3] virtio-balloon: note optional features

[Paolo Bonzini]

It is not a problem if the destination does not have
VIRTIO_BALLOON_F_MUST_TELL_HOST.  In that case, the guest
will simply do useless virtqueue traffic, but the destination
does not have a problem.

(In fact, it _is_ a problem if the destination has
VIRTIO_BALLOON_F_MUST_TELL_HOST but the source does not.
The feature bit should have been backwards!  Luckily,
our implementation is free from this problem).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-balloon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 0ade8b0..d10df2e 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -216,7 +216,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 1)
         return -EINVAL;
 
-    ret = virtio_load(&s->vdev, f, 0);
+    ret = virtio_load(&s->vdev, f, VIRTIO_BALLOON_F_MUST_TELL_HOST);
     if (ret) {
         return ret;
     }
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/3] virtio-blk: note optional features

[Paolo Bonzini]

The guest must already be prepared to see SG_IO support
disappear from under its feet, for example if migration
refers to a block device on the source and file-based
storage on the destination; or more likely, if the source
kernel allows (gasp) SG_IO on a partition and the destination
does not.  So, we can migrate safely even if the source
had VIRTIO_BLK_F_SCSI and the destination does not.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-blk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index c95f8fc..9a4158a 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -542,7 +542,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(&s->vdev, f, 0);
+    ret = virtio_load(&s->vdev, f, VIRTIO_BLK_F_SCSI);
     if (ret) {
         return ret;
     }
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/3] virtio: let devices be permissive on enabled features

[Michael S. Tsirkin]

On Tue, Mar 06, 2012 at 01:22:05PM +0100, Paolo Bonzini wrote:
> virtio_load checks features that are enabled by the guest and
> blocks migration if they are not available in the destination
> host.  However, in some cases we can let features through because
> we know that guests will be able to proceed even without it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

So why do we need these hacks? You are saying things
work fine without this information. Then,
why not just have the bit set in supported features always?

> ---
>  hw/virtio-balloon.c    |    2 +-
>  hw/virtio-blk.c        |    2 +-
>  hw/virtio-net.c        |    2 +-
>  hw/virtio-scsi.c       |    2 +-
>  hw/virtio-serial-bus.c |    2 +-
>  hw/virtio.c            |    8 +++++---
>  hw/virtio.h            |    2 +-
>  7 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 075ed87..0ade8b0 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -216,7 +216,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
>      if (version_id != 1)
>          return -EINVAL;
>  
> -    ret = virtio_load(&s->vdev, f);
> +    ret = virtio_load(&s->vdev, f, 0);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index d4bb400..c95f8fc 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -542,7 +542,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>      if (version_id != 2)
>          return -EINVAL;
>  
> -    ret = virtio_load(&s->vdev, f);
> +    ret = virtio_load(&s->vdev, f, 0);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 3f190d4..719ba96 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -896,7 +896,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>      if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
>          return -EINVAL;
>  
> -    ret = virtio_load(&n->vdev, f);
> +    ret = virtio_load(&n->vdev, f, 0);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c
> index 9797847..da281e2 100644
> --- a/hw/virtio-scsi.c
> +++ b/hw/virtio-scsi.c
> @@ -560,7 +560,7 @@ static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
>      VirtIOSCSI *s = opaque;
>      int ret;
>  
> -    ret = virtio_load(&s->vdev, f);
> +    ret = virtio_load(&s->vdev, f, 0);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 4a33872..5ef7d30 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -597,7 +597,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>      }
>  
>      /* The virtio device */
> -    ret = virtio_load(&s->vdev, f);
> +    ret = virtio_load(&s->vdev, f, 0);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 064aecf..fdbb286 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -777,11 +777,12 @@ int virtio_set_features(VirtIODevice *vdev, uint32_t val)
>      return bad ? -1 : 0;
>  }
>  
> -int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> +int virtio_load(VirtIODevice *vdev, QEMUFile *f, int additional_features)

int? Should be uint32_t?

>  {
>      int num, i, ret;
>      uint32_t features;
>      uint32_t supported_features;
> +    uint32_t disabled_features;
>  
>      if (vdev->binding->load_config) {
>          ret = vdev->binding->load_config(vdev->binding_opaque, f);
> @@ -794,8 +795,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>      qemu_get_be16s(f, &vdev->queue_sel);
>      qemu_get_be32s(f, &features);
>  
> -    if (virtio_set_features(vdev, features) < 0) {
> -        supported_features = vdev->binding->get_features(vdev->binding_opaque);
> +    supported_features = vdev->binding->get_features(vdev->binding_opaque);
> +    disabled_features = additional_features & ~supported_features;
> +    if (virtio_set_features(vdev, features & ~disabled_features) < 0) {
>          error_report("Features 0x%x unsupported. Allowed features: 0x%x",
>                       features, supported_features);
>          return -1;
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 400c092..2e8dd20 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -154,7 +154,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>  
>  void virtio_save(VirtIODevice *vdev, QEMUFile *f);
>  
> -int virtio_load(VirtIODevice *vdev, QEMUFile *f);
> +int virtio_load(VirtIODevice *vdev, QEMUFile *f, int additional_features);
>  
>  void virtio_cleanup(VirtIODevice *vdev);
>  
> -- 
> 1.7.7.6
> 

Re: [Qemu-devel] [PATCH 1/3] virtio: let devices be permissive on enabled features

[Paolo Bonzini]

Il 06/03/2012 14:33, Michael S. Tsirkin ha scritto:
>> > virtio_load checks features that are enabled by the guest and
>> > blocks migration if they are not available in the destination
>> > host.  However, in some cases we can let features through because
>> > we know that guests will be able to proceed even without it.
>> > 
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> So why do we need these hacks? You are saying things
> work fine without this information. Then,
> why not just have the bit set in supported features always?

Not sure what you mean.

virtio-balloon works fine only because our implementation does not set
the bit.  I found it just by inspection, but it's wrong.  If QEMU
started setting VIRTIO_F_BALLOON_MUST_TELL_HOST, migration would fail to
a version that does not set it.

virtio-blk is what prompted the patch and it is a real bug.  Updating
libvirt on the destination causes the source's scsi=on to become
scsi=off on the destination.  Then migration fails (it used to crash,
Orit fixed the crash, I'm fixing the rest).  However, a kernel update
can cause the same behavioral change can happen even if
VIRTIO_BLK_F_SCSI remains on, so it is not a good reason to fail migration.

Paolo

Re: [Qemu-devel] [PATCH 1/3] virtio: let devices be permissive on enabled features

[Orit Wasserman]

On 03/06/2012 03:57 PM, Paolo Bonzini wrote:
> Il 06/03/2012 14:33, Michael S. Tsirkin ha scritto:
>>>> virtio_load checks features that are enabled by the guest and
>>>> blocks migration if they are not available in the destination
>>>> host.  However, in some cases we can let features through because
>>>> we know that guests will be able to proceed even without it.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> So why do we need these hacks? You are saying things
>> work fine without this information. Then,
>> why not just have the bit set in supported features always?
> 
> Not sure what you mean.
> 
> virtio-balloon works fine only because our implementation does not set
> the bit.  I found it just by inspection, but it's wrong.  If QEMU
> started setting VIRTIO_F_BALLOON_MUST_TELL_HOST, migration would fail to
> a version that does not set it.
> 
> virtio-blk is what prompted the patch and it is a real bug.  Updating
> libvirt on the destination causes the source's scsi=on to become
> scsi=off on the destination.  Then migration fails (it used to crash,
> Orit fixed the crash, I'm fixing the rest). 

To be more accurate we don't crash but give an error message:
"Features 0x100006d4 unsupported. Allowed features: 0x71000654"
The migration succeeds (from the user point of view ) but the virtio-blk device is not 
working properly.
I changed the code to fail the migration.
Paolo patch fixes the migration.

Orit

 However, a kernel update
> can cause the same behavioral change can happen even if
> VIRTIO_BLK_F_SCSI remains on, so it is not a good reason to fail migration.
> 
> Paolo

Re: [Qemu-devel] [PATCH 3/3] virtio-blk: note optional features

[Michael S. Tsirkin]

On Tue, Mar 06, 2012 at 01:22:07PM +0100, Paolo Bonzini wrote:
> The guest must already be prepared to see SG_IO support
> disappear from under its feet, for example if migration
> refers to a block device on the source and file-based
> storage on the destination; or more likely, if the source
> kernel allows (gasp) SG_IO on a partition and the destination
> does not.  So, we can migrate safely even if the source
> had VIRTIO_BLK_F_SCSI and the destination does not.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

My first reaction is you want a new non guest
visible flag to control whether SG_IO fails on host.
guest visible ones must be consistent across migration.

> ---
>  hw/virtio-blk.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index c95f8fc..9a4158a 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -542,7 +542,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>      if (version_id != 2)
>          return -EINVAL;
>  
> -    ret = virtio_load(&s->vdev, f, 0);
> +    ret = virtio_load(&s->vdev, f, VIRTIO_BLK_F_SCSI);
>      if (ret) {
>          return ret;
>      }
> -- 
> 1.7.7.6

Re: [Qemu-devel] [PATCH 2/3] virtio-balloon: note optional features

[Michael S. Tsirkin]

On Tue, Mar 06, 2012 at 01:22:06PM +0100, Paolo Bonzini wrote:
> It is not a problem if the destination does not have
> VIRTIO_BALLOON_F_MUST_TELL_HOST.  In that case, the guest
> will simply do useless virtqueue traffic, but the destination
> does not have a problem.
> (In fact, it _is_ a problem if the destination has
> VIRTIO_BALLOON_F_MUST_TELL_HOST but the source does not.
> The feature bit should have been backwards!  Luckily,
> our implementation is free from this problem).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Can't we just add a flag to control this feature?

> ---
>  hw/virtio-balloon.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 0ade8b0..d10df2e 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -216,7 +216,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
>      if (version_id != 1)
>          return -EINVAL;
>  
> -    ret = virtio_load(&s->vdev, f, 0);
> +    ret = virtio_load(&s->vdev, f, VIRTIO_BALLOON_F_MUST_TELL_HOST);
>      if (ret) {
>          return ret;
>      }
> -- 
> 1.7.7.6
> 

Re: [Qemu-devel] [PATCH 2/3] virtio-balloon: note optional features

[Paolo Bonzini]

Il 06/03/2012 15:55, Michael S. Tsirkin ha scritto:
> > It is not a problem if the destination does not have
> > VIRTIO_BALLOON_F_MUST_TELL_HOST.  In that case, the guest
> > will simply do useless virtqueue traffic, but the destination
> > does not have a problem.
> > (In fact, it _is_ a problem if the destination has
> > VIRTIO_BALLOON_F_MUST_TELL_HOST but the source does not.
> > The feature bit should have been backwards!  Luckily,
> > our implementation is free from this problem).
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Can't we just add a flag to control this feature?

I think the sane thing here would be to remove it from the spec.

Paolo

Re: [Qemu-devel] [PATCH 2/3] virtio-balloon: note optional features

[Michael S. Tsirkin]

On Tue, Mar 06, 2012 at 03:59:53PM +0100, Paolo Bonzini wrote:
> Il 06/03/2012 15:55, Michael S. Tsirkin ha scritto:
> > > It is not a problem if the destination does not have
> > > VIRTIO_BALLOON_F_MUST_TELL_HOST.  In that case, the guest
> > > will simply do useless virtqueue traffic, but the destination
> > > does not have a problem.
> > > (In fact, it _is_ a problem if the destination has
> > > VIRTIO_BALLOON_F_MUST_TELL_HOST but the source does not.
> > > The feature bit should have been backwards!  Luckily,
> > > our implementation is free from this problem).
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Can't we just add a flag to control this feature?
> 
> I think the sane thing here would be to remove it from the spec.
> 
> Paolo

I guess we could but what does it buy us?

-- 
MST

Re: [Qemu-devel] [PATCH 2/3] virtio-balloon: note optional features

[Paolo Bonzini]

Il 06/03/2012 16:08, Michael S. Tsirkin ha scritto:
> > > Can't we just add a flag to control this feature?
> > 
> > I think the sane thing here would be to remove it from the spec.
> 
> I guess we could but what does it buy us?

Not having a broken spec. :)

The pedantically correct thing to do would be to replace it with
VIRTIO_BALLOON_F_DONT_TELL_HOST (so that it breaks in the 1->0 direction
rather than 0->1).  But indeed it doesn't buy enough, so it's not worth
the effort of updating guests+QEMU+spec.

Paolo

Re: [Qemu-devel] [PATCH 3/3] virtio-blk: note optional features

[Paolo Bonzini]

Il 06/03/2012 15:53, Michael S. Tsirkin ha scritto:
> > The guest must already be prepared to see SG_IO support
> > disappear from under its feet, for example if migration
> > refers to a block device on the source and file-based
> > storage on the destination; or more likely, if the source
> > kernel allows (gasp) SG_IO on a partition and the destination
> > does not.  So, we can migrate safely even if the source
> > had VIRTIO_BLK_F_SCSI and the destination does not.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> My first reaction is you want a new non guest
> visible flag to control whether SG_IO fails on host.
> guest visible ones must be consistent across migration.

So scsi=off/on would control VIRTIO_BLK_F_SCSI, while the new flag would
cause requests to fail.  Then it's simpler to do the other way round.
Make the "new non guest-visible flag" be scsi=on/off and set
VIRTIO_BLK_F_SCSI unconditionally as you suggested first.

Paolo

Re: [Qemu-devel] [PATCH 3/3] virtio-blk: note optional features

[Anthony Liguori]

On 03/06/2012 06:22 AM, Paolo Bonzini wrote:
> The guest must already be prepared to see SG_IO support
> disappear from under its feet, for example if migration
> refers to a block device on the source and file-based
> storage on the destination; or more likely, if the source
> kernel allows (gasp) SG_IO on a partition and the destination
> does not.  So, we can migrate safely even if the source
> had VIRTIO_BLK_F_SCSI and the destination does not.

I don't know how comfortable I feel about this.

You can't just remove a feature in flight.  The guest is going to behave 
differently in such a way that the host isn't expecting.  Yes, it should fail 
gracefully, but nonetheless it will fail.

Aren't you just delaying the inevitable?  Instead of having migration fail, the 
guest workload is going to fail.  How is this an improvement?

Regards,

Anthony Liguorig

>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/virtio-blk.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index c95f8fc..9a4158a 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -542,7 +542,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>       if (version_id != 2)
>           return -EINVAL;
>
> -    ret = virtio_load(&s->vdev, f, 0);
> +    ret = virtio_load(&s->vdev, f, VIRTIO_BLK_F_SCSI);
>       if (ret) {
>           return ret;
>       }

Re: [Qemu-devel] [PATCH 3/3] virtio-blk: note optional features

[Paolo Bonzini]

Il 06/03/2012 18:03, Anthony Liguori ha scritto:
> I don't know how comfortable I feel about this.
> 
> You can't just remove a feature in flight.  The guest is going to behave
> differently in such a way that the host isn't expecting.  Yes, it should
> fail gracefully, but nonetheless it will fail.
> 
> Aren't you just delaying the inevitable?  Instead of having migration
> fail, the guest workload is going to fail.  How is this an improvement?

VIRTIO_BLK_F_SCSI feature was almost never used but was always marked as
available.  Because of possible security problems connected to it,
libvirt started making it an opt-in feature.

In practice, you need to configure your host specially if you want to
use SCSI passthrough (e.g. you must not use labels and UUIDs in your
/etc/fstab), so it's safe to assume that guests that have SG_IO disabled
under their feet will keep working.

That said, instead of this hack we can just decouple scsi=on/off from
VIRTIO_BLK_F_SCSI, and just report the feature.  After all we do not
clear VIRTIO_BLK_F_SCSI just because the device is backed by a file or
partition, yet SG_IO is still unavailable in those cases.  I'll send
patches for this tomorrow.

Paolo

Re: [Qemu-devel] [PATCH 3/3] virtio-blk: note optional features

[Anthony Liguori]

On 03/06/2012 11:12 AM, Paolo Bonzini wrote:
> Il 06/03/2012 18:03, Anthony Liguori ha scritto:
>> I don't know how comfortable I feel about this.
>>
>> You can't just remove a feature in flight.  The guest is going to behave
>> differently in such a way that the host isn't expecting.  Yes, it should
>> fail gracefully, but nonetheless it will fail.
>>
>> Aren't you just delaying the inevitable?  Instead of having migration
>> fail, the guest workload is going to fail.  How is this an improvement?
>
> VIRTIO_BLK_F_SCSI feature was almost never used but was always marked as
> available.  Because of possible security problems connected to it,
> libvirt started making it an opt-in feature.
>
> In practice, you need to configure your host specially if you want to
> use SCSI passthrough (e.g. you must not use labels and UUIDs in your
> /etc/fstab), so it's safe to assume that guests that have SG_IO disabled
> under their feet will keep working.
>
> That said, instead of this hack we can just decouple scsi=on/off from
> VIRTIO_BLK_F_SCSI, and just report the feature.  After all we do not
> clear VIRTIO_BLK_F_SCSI just because the device is backed by a file or
> partition, yet SG_IO is still unavailable in those cases.  I'll send
> patches for this tomorrow.

Okay, that makes more sense to me.  I think this is an exceptional circumstance 
so handling it as a one-off verses a generic mechanism would be preferred.

Regards,

Anthony Liguori

>
> Paolo