On Thu, Nov 27, 2014 at 05:15:42PM +0100, David Hildenbrand wrote:
> > From: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> >
> > It seemed like a good idea, but it's actually a pain when we get more
> > than 32 feature bits. Just change it to a u32 for now.
> >
> > Cc: Brian Swetland <swetland@xxxxxxxxxx>
> > Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> > Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> > Signed-off-by: Cornelia Huck <cornelia.huck@xxxxxxxxxx>
> > Acked-by: Pawel Moll <pawel.moll@xxxxxxx>
> > Acked-by: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
> >
> > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > ---
>
> Wouldn't it be easier to turn
> test_bit(i, dev->features)
>
> into a simple makro like
> #define test_bit(i, features) (features & (1 << i))
>
> Similar one for clear_bit and set_bit.
I don't really see why it's easier.
These wrappers resulted in a ton of code
converting from bit numbers to set_bit calls
and back.
E.g. with plan integer you can do (a|b|c)
to get a set with 3 bits.
>
> if names collide with existing functions, we could simply rename them to:
>
> set_feature_bit() ...
> clear_feature_bit() ...
In fact that's why we have BIT macro.
vdev->features & BIT_ULL(x) is only longer by 1
character than test_feature_bit(vdev, x), and
it's much clearer than a non standard wrapper.
vdev->features & (1ULL << x) is longer by 3 chars.
I guess this is a suggestion to use BIT / BIT_ULL more.
Good point.
> > diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
> > index e647947..0acd564 100644
> > --- a/drivers/misc/mic/card/mic_virtio.c
> > +++ b/drivers/misc/mic/card/mic_virtio.c
> > @@ -101,7 +101,7 @@ static void mic_finalize_features(struct virtio_device *vdev)
> > bits = min_t(unsigned, feature_len,
> > sizeof(vdev->features)) * 8;
> > for (i = 0; i < bits; i++) {
> > - if (test_bit(i, vdev->features))
> > + if (vdev->features & BIT(bits))
>
> Hm, shouldn't that also be "if (vdev->features & (1 << i))"?
Ouch. you are right of course. Will fix.
> > iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)),
> > &out_features[i / 8]);
> > }
>
> > out_free:
> > kfree(features);
> > kfree(ccw);
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index df598dd..7814b1f 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -49,9 +49,9 @@ static ssize_t features_show(struct device *_d,
> >
> > /* We actually represent this as a bitstring, as it could be
> > * arbitrary length in future. */
> > - for (i = 0; i < ARRAY_SIZE(dev->features)*BITS_PER_LONG; i++)
> > + for (i = 0; i < sizeof(dev->features)*8; i++)
>
> ... sizeof(dev->features) * 8; ...
>
> Do we have a define for 8 ?
We do: BITS_PER_BYTE.
> > len += sprintf(buf+len, "%c",
> > - test_bit(i, dev->features) ? '1' : '0');
> > + dev->features & (1ULL << i) ? '1' : '0');
> > len += sprintf(buf+len, "\n");
> > return len;
> > }
> > @@ -168,18 +168,18 @@ static int virtio_dev_probe(struct device *_d)
> > device_features = dev->config->get_features(dev);
> >
>
> [...]
>
> > @@ -483,7 +483,7 @@ int main(int argc, char *argv[])
> >
> > /* Set up host side. */
> > vring_init(&vrh.vring, RINGSIZE, __user_addr_min, ALIGN);
> > - vringh_init_user(&vrh, vdev.features[0], RINGSIZE, true,
> > + vringh_init_user(&vrh, vdev.features, RINGSIZE, true,
> > vrh.vring.desc, vrh.vring.avail, vrh.vring.used);
> >
> > /* No descriptor to get yet... */
> > @@ -652,13 +652,13 @@ int main(int argc, char *argv[])
> > }
> >
> > /* Test weird (but legal!) indirect. */
> > - if (vdev.features[0] & (1 << VIRTIO_RING_F_INDIRECT_DESC)) {
> > + if (vdev.features & (1 << VIRTIO_RING_F_INDIRECT_DESC)) {
> > char *data = __user_addr_max - USER_MEM/4;
> > struct vring_desc *d = __user_addr_max - USER_MEM/2;
> > struct vring vring;
> >
> > /* Force creation of direct, which we modify. */
> > - vdev.features[0] &= ~(1 << VIRTIO_RING_F_INDIRECT_DESC);
> > + vdev.features &= ~(1 << VIRTIO_RING_F_INDIRECT_DESC);
>
> That could then be something like
>
> clear_feature_bit(VIRTIO_RING_F_INDIRECT_DESC, vdev.features);
>
> (I would prefer such makros over repeating bit actions)
That's the whole reason for the patch.
I guess you disagree with it, but it's much easier
to deal with simple integers.
> > vq = vring_new_virtqueue(0, RINGSIZE, ALIGN, &vdev, true,
> > __user_addr_min,
> > never_notify_host,
>
>
> But on the whole this looks good to me.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
