From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37999) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S4uW5-0003lp-VH for qemu-devel@nongnu.org; Tue, 06 Mar 2012 08:33:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S4uW3-0002qX-Pb for qemu-devel@nongnu.org; Tue, 06 Mar 2012 08:33:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60675) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S4uW3-0002qK-H9 for qemu-devel@nongnu.org; Tue, 06 Mar 2012 08:33:23 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q26DXLMW021038 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 6 Mar 2012 08:33:21 -0500 Date: Tue, 6 Mar 2012 15:33:32 +0200 From: "Michael S. Tsirkin" Message-ID: <20120306133331.GB12096@redhat.com> References: <1331036527-7651-1-git-send-email-pbonzini@redhat.com> <1331036527-7651-2-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1331036527-7651-2-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/3] virtio: let devices be permissive on enabled features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: amit.shah@redhat.com, uobergfe@redhat.com, owasserm@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com 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 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 >