Re: [Qemu-devel] [PATCH RFC] virtio: set any_layout in virtio core

[Michael S. Tsirkin]

On Thu, Jul 23, 2015 at 12:15:22PM +0200, Christian Borntraeger wrote:
> Am 22.07.2015 um 11:36 schrieb Michael S. Tsirkin:
> > Virtio 1 requires this, and all devices are clean by now,
> > so let's do it!
> > 
> > Exceptions:
> >     - virtio-blk
> >     - compat machine types
> 
> Is this targetted for 2.4 or 2.5? Pushing all the 1.0 stuff in 2.4 while
> in hard freeze seems a bit dangerous as long as there are discussions going 
> on.
> (I just started looking into the scsi feature mail thread - no opinion yet)
> 
> Christian

As long as it's a bugfix, and it's not dangerous, we can merge it for
2.4. Whether the specific patch is dangerous needs to be argued
on a case by case basis.

I think merging 1.0 patches late in release cycle might be
ok simply because 1.0 is disabled by default.


> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> > 
> > Untested - consider this pseudo-code - it just seems easier to write it
> > in C than try to explain it.
> > 
> >  include/hw/compat.h        | 22 +++++++++++++++++++++-
> >  include/hw/virtio/virtio.h |  4 +++-
> >  hw/block/virtio-blk.c      |  1 +
> >  hw/net/virtio-net.c        |  2 --
> >  hw/scsi/virtio-scsi.c      |  2 --
> >  5 files changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 4a43466..94c8097 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -2,7 +2,27 @@
> >  #define HW_COMPAT_H
> > 
> >  #define HW_COMPAT_2_3 \
> > -        /* empty */
> > +        {\
> > +            .driver   = "virtio-blk-pci",\
> > +            .property = "any_layout",\
> > +            .value    = "off",\
> > +        },{\
> > +            .driver   = "virtio-balloon-pci",\
> > +            .property = "any_layout",\
> > +            .value    = "off",\
> > +        },{\
> > +            .driver   = "virtio-serial-pci",\
> > +            .property = "any_layout",\
> > +            .value    = "off",\
> > +        },{\
> > +            .driver   = "virtio-9p-pci",\
> > +            .property = "any_layout",\
> > +            .value    = "off",\
> > +        },{\
> > +            .driver   = "virtio-rng-pci",\
> > +            .property = "any_layout",\
> > +            .value    = "off",\
> > +        },
> > 
> >  #define HW_COMPAT_2_2 \
> >          /* empty */
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 473fb75..fbb3c06 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -214,7 +214,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> >      DEFINE_PROP_BIT64("event_idx", _state, _field,        \
> >                        VIRTIO_RING_F_EVENT_IDX, true),     \
> >      DEFINE_PROP_BIT64("notify_on_empty", _state, _field,  \
> > -                      VIRTIO_F_NOTIFY_ON_EMPTY, true)
> > +                      VIRTIO_F_NOTIFY_ON_EMPTY, true) \
> > +    DEFINE_PROP_BIT64("any_layout", _state, _field,  \
> > +                      VIRTIO_F_ANY_LAYOUT, true)
> > 
> >  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> >  hwaddr virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 6aefda4..015b9b5 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -731,6 +731,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice 
> > *vdev, uint64_t features)
> >      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);
> > +    virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > 
> >      if (s->conf.config_wce) {
> >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 304d3dd..e203058 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1777,8 +1777,6 @@ static void virtio_net_instance_init(Object *obj)
> >  }
> > 
> >  static Property virtio_net_properties[] = {
> > -    DEFINE_PROP_BIT("any_layout", VirtIONet, host_features,
> > -                    VIRTIO_F_ANY_LAYOUT, true),
> >      DEFINE_PROP_BIT("csum", VirtIONet, host_features, VIRTIO_NET_F_CSUM, 
> > true),
> >      DEFINE_PROP_BIT("guest_csum", VirtIONet, host_features,
> >                      VIRTIO_NET_F_GUEST_CSUM, true),
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index f7d3c7c..d17698d 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -953,8 +953,6 @@ static Property virtio_scsi_properties[] = {
> >                                                    0xFFFF),
> >      DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSI, 
> > parent_obj.conf.cmd_per_lun,
> >                                                    128),
> > -    DEFINE_PROP_BIT("any_layout", VirtIOSCSI, host_features,
> > -                                              VIRTIO_F_ANY_LAYOUT, true),
> >      DEFINE_PROP_BIT("hotplug", VirtIOSCSI, host_features,
> >                                             VIRTIO_SCSI_F_HOTPLUG, true),
> >      DEFINE_PROP_BIT("param_change", VirtIOSCSI, host_features,
> > 

I just noticed that scsi uses
DEFINE_PROP_BIT for feature bits.  That's also a bug
(should be DEFINE_PROP_BIT64).

My patch fixes any_layout but we need to fix the rest.

-- 
MST