thr3ads.net - Virtualization - [PATCH RFC v6 05/20] virtio: support more feature bits [Dec 2014]

If this information is useful, please help other people find it:
Share via:

Thomas Huth

2014-Dec-12 10:06 UTC

[PATCH RFC v6 05/20] virtio: support more feature bits

On Thu, 11 Dec 2014 14:25:07 +0100
Cornelia Huck <cornelia.huck at de.ibm.com> wrote:
> With virtio-1, we support more than 32 feature bits. Let's extend both
> host and guest features to 64, which should suffice for a while.
> 
> vhost and migration have been ignored for now.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> ---
>  hw/9pfs/virtio-9p-device.c      |    2 +-
>  hw/block/virtio-blk.c           |    2 +-
>  hw/char/virtio-serial-bus.c     |    2 +-
>  hw/core/qdev-properties.c       |   58
+++++++++++++++++++++++++++++++++++++++
>  hw/net/virtio-net.c             |   22 +++++++--------
>  hw/s390x/s390-virtio-bus.c      |    3 +-
>  hw/s390x/s390-virtio-bus.h      |    2 +-
>  hw/s390x/virtio-ccw.c           |   39 +++++++++++++++-----------
>  hw/s390x/virtio-ccw.h           |    5 +---
>  hw/scsi/vhost-scsi.c            |    3 +-
>  hw/scsi/virtio-scsi.c           |    4 +--
>  hw/virtio/virtio-balloon.c      |    2 +-
>  hw/virtio/virtio-bus.c          |    6 ++--
>  hw/virtio/virtio-mmio.c         |    4 +--
>  hw/virtio/virtio-pci.c          |    3 +-
>  hw/virtio/virtio-pci.h          |    2 +-
>  hw/virtio/virtio-rng.c          |    2 +-
>  hw/virtio/virtio.c              |   13 +++++----
>  include/hw/qdev-properties.h    |   12 ++++++++
>  include/hw/virtio/virtio-bus.h  |    8 +++---
>  include/hw/virtio/virtio-net.h  |   46 +++++++++++++++----------------
>  include/hw/virtio/virtio-scsi.h |    6 ++--
>  include/hw/virtio/virtio.h      |   38 ++++++++++++++-----------
>  23 files changed, 184 insertions(+), 100 deletions(-)
...> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9f3c58a..d6d1b98 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
...> @@ -514,7 +514,7 @@ static inline uint64_t
virtio_net_supported_guest_offloads(VirtIONet *n)
>      return virtio_net_guest_offloads_by_features(vdev->guest_features);
>  }
> 
> -static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> +static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
>      int i;
> @@ -1036,7 +1036,7 @@ static ssize_t virtio_net_receive(NetClientState *nc,
const uint8_t *buf, size_t
>                  return -1;
>              error_report("virtio-net unexpected empty queue: "
>                      "i %zd mergeable %d offset %zd, size %zd, "
> -                    "guest hdr len %zd, host hdr len %zd guest
features 0x%x",
> +                    "guest hdr len %zd, host hdr len %zd guest
features 0x%lx",
I think you need a different format string like PRIx64 here so that the
code also works right on a 32-bit system (where long is only 32-bit).
>                      i, n->mergeable_rx_bufs, offset, size,
>                      n->guest_hdr_len, n->host_hdr_len,
vdev->guest_features);
>              exit(1);
...> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 3fee4aa..fbd909d 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -371,8 +371,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>          } else {
>              features.index = ldub_phys(&address_space_memory,
>                                         ccw.cda +
sizeof(features.features));
> -            if (features.index < ARRAY_SIZE(dev->host_features)) {
> -                features.features = dev->host_features[features.index];
> +            if (features.index == 0) {
> +                features.features = (uint32_t)dev->host_features;
> +            } else if (features.index == 1) {
> +                features.features = (uint32_t)(dev->host_features
>> 32);
>              } else {
>                  /* Return zeroes if the guest supports more feature bits.
*/
>                  features.features = 0;
> @@ -399,8 +401,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>              features.index = ldub_phys(&address_space_memory,
>                                         ccw.cda +
sizeof(features.features));
>              features.features = ldl_le_phys(&address_space_memory,
ccw.cda);
> -            if (features.index < ARRAY_SIZE(dev->host_features)) {
> -                virtio_set_features(vdev, features.features);
> +            if (features.index == 0) {
> +                virtio_set_features(vdev,
> +                                    (vdev->guest_features &
0xffffffff00000000) |
> +                                    features.features);
> +            } else if (features.index == 1) {
> +                virtio_set_features(vdev,
> +                                    (vdev->guest_features &
0x00000000ffffffff) |
> +                                    ((uint64_t)features.features <<
32));
The long constants 0xffffffff00000000 and 0x00000000ffffffff should
probably get an ULL suffix.

...> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5814433..7f74ae5 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -593,6 +593,7 @@ void virtio_reset(void *opaque)
>      }
> 
>      vdev->guest_features = 0;
> +
Unnecessary white space change?
>      vdev->queue_sel = 0;
>      vdev->status = 0;
>      vdev->isr = 0;
> @@ -924,7 +925,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      qemu_put_8s(f, &vdev->status);
>      qemu_put_8s(f, &vdev->isr);
>      qemu_put_be16s(f, &vdev->queue_sel);
> -    qemu_put_be32s(f, &vdev->guest_features);
> +    /* XXX features >= 32 */
> +    qemu_put_be32s(f, (uint32_t *)&vdev->guest_features);
Casting a uint64_t* to a uint32_t* here sounds very wrong - this likely
only works on little endian sytems, but certainly not on big-endian
systems.
If you do not want to extend this for 64-bit right from the beginning,
I think you've got to use a temporary 32-bit variable to get this right.

...> @@ -1005,9 +1007,10 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int
version_id)
>      }
>      qemu_get_be32s(f, &features);
> 
> +    /* XXX features >= 32 */
>      if (virtio_set_features(vdev, features) < 0) {
>          supported_features = k->get_features(qbus->parent);
> -        error_report("Features 0x%x unsupported. Allowed features:
0x%x",
> +        error_report("Features 0x%x unsupported. Allowed features:
0x%lx",
>                       features, supported_features);
You'll likely need the PRIx64 format here, too.
>          return -1;
>      }
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 070006c..81e5d0b 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -6,6 +6,7 @@
>  /*** qdev-properties.c ***/
> 
>  extern PropertyInfo qdev_prop_bit;
> +extern PropertyInfo qdev_prop_bit64;
>  extern PropertyInfo qdev_prop_bool;
>  extern PropertyInfo qdev_prop_uint8;
>  extern PropertyInfo qdev_prop_uint16;
> @@ -51,6 +52,17 @@ extern PropertyInfo qdev_prop_arraylen;
>          .defval    = (bool)_defval,                              \
>          }
> 
> +#define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {  \
> +        .name      = (_name),                                    \
> +        .info      = &(qdev_prop_bit64),                           \
No need for the paranthesis around qdev_prop_bit64 here?
> +        .bitnr    = (_bit),                                      \
> +        .offset    = offsetof(_state, _field)                    \
> +            + type_check(uint64_t,typeof_field(_state, _field)), \
> +        .qtype     = QTYPE_QBOOL,                                \
> +        .defval    = (bool)_defval,                              \
> +        }
 Thomas

Cornelia Huck

2014-Dec-12 10:17 UTC

head link

[PATCH RFC v6 05/20] virtio: support more feature bits

On Fri, 12 Dec 2014 11:06:40 +0100
Thomas Huth <thuth at linux.vnet.ibm.com> wrote:
> On Thu, 11 Dec 2014 14:25:07 +0100
> Cornelia Huck <cornelia.huck at de.ibm.com> wrote:
> 
> > With virtio-1, we support more than 32 feature bits. Let's extend
both
> > host and guest features to 64, which should suffice for a while.
> > 
> > vhost and migration have been ignored for now.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> > @@ -1036,7 +1036,7 @@ static ssize_t virtio_net_receive(NetClientState
*nc, const uint8_t *buf, size_t
> >                  return -1;
> >              error_report("virtio-net unexpected empty queue:
"
> >                      "i %zd mergeable %d offset %zd, size %zd,
"
> > -                    "guest hdr len %zd, host hdr len %zd guest
features 0x%x",
> > +                    "guest hdr len %zd, host hdr len %zd guest
features 0x%lx",
> 
> I think you need a different format string like PRIx64 here so that the
> code also works right on a 32-bit system (where long is only 32-bit).
Reminder to self: set up cross-compile again.
> 
> >                      i, n->mergeable_rx_bufs, offset, size,
> >                      n->guest_hdr_len, n->host_hdr_len,
vdev->guest_features);
> >              exit(1);
> > @@ -399,8 +401,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >              features.index = ldub_phys(&address_space_memory,
> >                                         ccw.cda +
sizeof(features.features));
> >              features.features =
ldl_le_phys(&address_space_memory, ccw.cda);
> > -            if (features.index <
ARRAY_SIZE(dev->host_features)) {
> > -                virtio_set_features(vdev, features.features);
> > +            if (features.index == 0) {
> > +                virtio_set_features(vdev,
> > +                                    (vdev->guest_features &
0xffffffff00000000) |
> > +                                    features.features);
> > +            } else if (features.index == 1) {
> > +                virtio_set_features(vdev,
> > +                                    (vdev->guest_features &
0x00000000ffffffff) |
> > +                                    ((uint64_t)features.features
<< 32));
> 
> The long constants 0xffffffff00000000 and 0x00000000ffffffff should
> probably get an ULL suffix.
Probably.
> > @@ -593,6 +593,7 @@ void virtio_reset(void *opaque)
> >      }
> > 
> >      vdev->guest_features = 0;
> > +
> 
> Unnecessary white space change?
Crept in during various rebase sessions :)
> 
> >      vdev->queue_sel = 0;
> >      vdev->status = 0;
> >      vdev->isr = 0;
> > @@ -924,7 +925,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >      qemu_put_8s(f, &vdev->status);
> >      qemu_put_8s(f, &vdev->isr);
> >      qemu_put_be16s(f, &vdev->queue_sel);
> > -    qemu_put_be32s(f, &vdev->guest_features);
> > +    /* XXX features >= 32 */
> > +    qemu_put_be32s(f, (uint32_t *)&vdev->guest_features);
> 
> Casting a uint64_t* to a uint32_t* here sounds very wrong - this likely
> only works on little endian sytems, but certainly not on big-endian
> systems.
> If you do not want to extend this for 64-bit right from the beginning,
> I think you've got to use a temporary 32-bit variable to get this
right.
Hm... always store the old 32 bits here, then store the new 32 bits
later? Should be able to get that compatible.

> > +#define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {  \
> > +        .name      = (_name),                                    \
> > +        .info      = &(qdev_prop_bit64),                         
\
> 
> No need for the paranthesis around qdev_prop_bit64 here?
Straight copy&paste :) I'd prefer to keep the same style for all
#defines here.
> 
> > +        .bitnr    = (_bit),                                      \
> > +        .offset    = offsetof(_state, _field)                    \
> > +            + type_check(uint64_t,typeof_field(_state, _field)), \
> > +        .qtype     = QTYPE_QBOOL,                                \
> > +        .defval    = (bool)_defval,                              \
> > +        }

David Gibson

2015-Jan-22 01:40 UTC

head link

[Qemu-devel] [PATCH RFC v6 05/20] virtio: support more feature bits

On Fri, Dec 12, 2014 at 11:17:47AM +0100, Cornelia Huck
wrote:> On Fri, 12 Dec 2014 11:06:40 +0100
> Thomas Huth <thuth at linux.vnet.ibm.com> wrote:
> 
> > On Thu, 11 Dec 2014 14:25:07 +0100
> > Cornelia Huck <cornelia.huck at de.ibm.com> wrote:
[snip]> > >      vdev->queue_sel = 0;
> > >      vdev->status = 0;
> > >      vdev->isr = 0;
> > > @@ -924,7 +925,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile
*f)
> > >      qemu_put_8s(f, &vdev->status);
> > >      qemu_put_8s(f, &vdev->isr);
> > >      qemu_put_be16s(f, &vdev->queue_sel);
> > > -    qemu_put_be32s(f, &vdev->guest_features);
> > > +    /* XXX features >= 32 */
> > > +    qemu_put_be32s(f, (uint32_t *)&vdev->guest_features);
> > 
> > Casting a uint64_t* to a uint32_t* here sounds very wrong - this
likely
> > only works on little endian sytems, but certainly not on big-endian
> > systems.
> > If you do not want to extend this for 64-bit right from the beginning,
> > I think you've got to use a temporary 32-bit variable to get this
right.
> 
> Hm... always store the old 32 bits here, then store the new 32 bits
> later? Should be able to get that compatible.
I think Thomas' point is that since vdev->guest_features has changed
to a uint64_t, the "old" bits are now in the second 32-bit half on a
BE system.

Or maybe I'm misunderstanding your reply.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20150122/cf2e0421/attachment.sig>

Maybe Matching Threads

Search for more reasonably related threads

Virtualization - Dec 2014 - [PATCH RFC v6 05/20] virtio: support more feature bits

[PATCH RFC v6 05/20] virtio: support more feature bits

[PATCH RFC v6 05/20] virtio: support more feature bits

[Qemu-devel] [PATCH RFC v6 05/20] virtio: support more feature bits

Maybe Matching Threads