On Fri, 12 Dec 2014 11:06:40 +0100
Thomas Huth <thuth@xxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 11 Dec 2014 14:25:07 +0100
> Cornelia Huck <cornelia.huck@xxxxxxxxxx> 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@xxxxxxxxxx>
> > @@ -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, \
> > + }
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
