Michael S. Tsirkin
2014-Nov-27 20:07 UTC
[PATCH v6 02/46] virtio: use u32, not bitmap for features
It seemed like a good idea to use bitmap for features in struct virtio_device, but it's actually a pain, and seems to become even more painful when we get more than 32 feature bits. Just change it to a u32 for now. Based on patch by Rusty. Suggested-by: David Hildenbrand <dahi at linux.vnet.ibm.com> Signed-off-by: Rusty Russell <rusty at rustcorp.com.au> Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- include/linux/virtio.h | 3 +-- include/linux/virtio_config.h | 6 +++--- tools/virtio/linux/virtio.h | 22 +--------------------- tools/virtio/linux/virtio_config.h | 2 +- drivers/char/virtio_console.c | 2 +- drivers/lguest/lguest_device.c | 8 ++++---- drivers/misc/mic/card/mic_virtio.c | 2 +- drivers/remoteproc/remoteproc_virtio.c | 2 +- drivers/s390/kvm/kvm_virtio.c | 2 +- drivers/s390/kvm/virtio_ccw.c | 23 +++++++++-------------- drivers/virtio/virtio.c | 10 +++++----- drivers/virtio/virtio_mmio.c | 8 ++------ drivers/virtio/virtio_pci.c | 3 +-- drivers/virtio/virtio_ring.c | 2 +- tools/virtio/virtio_test.c | 5 ++--- tools/virtio/vringh_test.c | 16 ++++++++-------- 16 files changed, 42 insertions(+), 74 deletions(-) diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 65261a7..7828a7f 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -101,8 +101,7 @@ struct virtio_device { const struct virtio_config_ops *config; const struct vringh_config_ops *vringh_config; struct list_head vqs; - /* Note that this is a Linux set_bit-style bitmap. */ - unsigned long features[1]; + u32 features; void *priv; }; diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 249fcd6..1761106 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -92,7 +92,7 @@ static inline bool __virtio_test_bit(const struct virtio_device *vdev, else BUG_ON(fbit >= 32); - return test_bit(fbit, vdev->features); + return vdev->features & BIT(fbit); } /** @@ -109,7 +109,7 @@ static inline void __virtio_set_bit(struct virtio_device *vdev, else BUG_ON(fbit >= 32); - set_bit(fbit, vdev->features); + vdev->features |= BIT(fbit); } /** @@ -126,7 +126,7 @@ static inline void __virtio_clear_bit(struct virtio_device *vdev, else BUG_ON(fbit >= 32); - clear_bit(fbit, vdev->features); + vdev->features &= ~BIT(fbit); } /** diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h index 5a2d1f0..72bff70 100644 --- a/tools/virtio/linux/virtio.h +++ b/tools/virtio/linux/virtio.h @@ -6,31 +6,11 @@ /* TODO: empty stubs for now. Broken but enough for virtio_ring.c */ #define list_add_tail(a, b) do {} while (0) #define list_del(a) do {} while (0) - -#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) -#define BITS_PER_BYTE 8 -#define BITS_PER_LONG (sizeof(long) * BITS_PER_BYTE) -#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) - -/* TODO: Not atomic as it should be: - * we don't use this for anything important. */ -static inline void clear_bit(int nr, volatile unsigned long *addr) -{ - unsigned long mask = BIT_MASK(nr); - unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); - - *p &= ~mask; -} - -static inline int test_bit(int nr, const volatile unsigned long *addr) -{ - return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1))); -} /* end of stubs */ struct virtio_device { void *dev; - unsigned long features[1]; + u32 features; }; struct virtqueue { diff --git a/tools/virtio/linux/virtio_config.h b/tools/virtio/linux/virtio_config.h index 5049967..83b27e8 100644 --- a/tools/virtio/linux/virtio_config.h +++ b/tools/virtio/linux/virtio_config.h @@ -2,5 +2,5 @@ #define VIRTIO_TRANSPORT_F_END 32 #define virtio_has_feature(dev, feature) \ - test_bit((feature), (dev)->features) + (__virtio_test_bit((dev), feature)) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index cf7a561..8d00aa7 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device *portdev) */ if (!portdev->vdev) return 0; - return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT); + return __virtio_test_bit(portdev->vdev, VIRTIO_CONSOLE_F_MULTIPORT); } static DEFINE_SPINLOCK(dma_bufs_lock); diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c index d0a1d8a..97aeb7d 100644 --- a/drivers/lguest/lguest_device.c +++ b/drivers/lguest/lguest_device.c @@ -137,14 +137,14 @@ static void lg_finalize_features(struct virtio_device *vdev) vring_transport_features(vdev); /* - * The vdev->feature array is a Linux bitmask: this isn't the same as a - * the simple array of bits used by lguest devices for features. So we - * do this slow, manual conversion which is completely general. + * Since lguest is currently x86-only, we're little-endian. That + * means we could just memcpy. But it's not time critical, and in + * case someone copies this code, we do it the slow, obvious way. */ memset(out_features, 0, desc->feature_len); bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8; for (i = 0; i < bits; i++) { - if (test_bit(i, vdev->features)) + if (__virtio_test_bit(vdev, i)) out_features[i / 8] |= (1 << (i % 8)); } diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c index e647947..4f070ad 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 (__virtio_test_bit(vdev, i)) iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)), &out_features[i / 8]); } diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c index a34b506..dafaf38 100644 --- a/drivers/remoteproc/remoteproc_virtio.c +++ b/drivers/remoteproc/remoteproc_virtio.c @@ -231,7 +231,7 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev) * Remember the finalized features of our vdev, and provide it * to the remote processor once it is powered on. */ - rsc->gfeatures = vdev->features[0]; + rsc->gfeatures = vdev->features; } static void rproc_virtio_get(struct virtio_device *vdev, unsigned offset, diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c index 6431290..fcd312d 100644 --- a/drivers/s390/kvm/kvm_virtio.c +++ b/drivers/s390/kvm/kvm_virtio.c @@ -106,7 +106,7 @@ static void kvm_finalize_features(struct virtio_device *vdev) memset(out_features, 0, desc->feature_len); bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8; for (i = 0; i < bits; i++) { - if (test_bit(i, vdev->features)) + if (__virtio_test_bit(vdev, i)) out_features[i / 8] |= (1 << (i % 8)); } } diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c index bda52f1..1dbee95 100644 --- a/drivers/s390/kvm/virtio_ccw.c +++ b/drivers/s390/kvm/virtio_ccw.c @@ -701,7 +701,6 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev) { struct virtio_ccw_device *vcdev = to_vc_device(vdev); struct virtio_feature_desc *features; - int i; struct ccw1 *ccw; ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL); @@ -715,19 +714,15 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev) /* Give virtio_ring a chance to accept features. */ vring_transport_features(vdev); - for (i = 0; i < sizeof(*vdev->features) / sizeof(features->features); - i++) { - int highbits = i % 2 ? 32 : 0; - features->index = i; - features->features = cpu_to_le32(vdev->features[i / 2] - >> highbits); - /* Write the feature bits to the host. */ - ccw->cmd_code = CCW_CMD_WRITE_FEAT; - ccw->flags = 0; - ccw->count = sizeof(*features); - ccw->cda = (__u32)(unsigned long)features; - ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT); - } + features->index = 0; + features->features = cpu_to_le32(vdev->features); + /* Write the feature bits to the host. */ + ccw->cmd_code = CCW_CMD_WRITE_FEAT; + ccw->flags = 0; + ccw->count = sizeof(*features); + ccw->cda = (__u32)(unsigned long)features; + ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT); + out_free: kfree(features); kfree(ccw); diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index df598dd..2b9aafb 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++) len += sprintf(buf+len, "%c", - test_bit(i, dev->features) ? '1' : '0'); + __virtio_test_bit(dev, 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); /* Features supported by both device and driver into dev->features. */ - memset(dev->features, 0, sizeof(dev->features)); + dev->features = 0; for (i = 0; i < drv->feature_table_size; i++) { unsigned int f = drv->feature_table[i]; BUG_ON(f >= 32); if (device_features & (1 << f)) - set_bit(f, dev->features); + __virtio_set_bit(dev, f); } /* Transport features always preserved to pass to finalize_features. */ for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++) if (device_features & (1 << i)) - set_bit(i, dev->features); + __virtio_set_bit(dev, i); dev->config->finalize_features(dev); diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c index ef9a165..eb5b0e2 100644 --- a/drivers/virtio/virtio_mmio.c +++ b/drivers/virtio/virtio_mmio.c @@ -155,16 +155,12 @@ static u32 vm_get_features(struct virtio_device *vdev) static void vm_finalize_features(struct virtio_device *vdev) { struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev); - int i; /* Give virtio_ring a chance to accept features. */ vring_transport_features(vdev); - for (i = 0; i < ARRAY_SIZE(vdev->features); i++) { - writel(i, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL); - writel(vdev->features[i], - vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES); - } + writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL); + writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES); } static void vm_get(struct virtio_device *vdev, unsigned offset, diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index d34ebfa..ab95a4c 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -120,8 +120,7 @@ static void vp_finalize_features(struct virtio_device *vdev) vring_transport_features(vdev); /* We only support 32 feature bits. */ - BUILD_BUG_ON(ARRAY_SIZE(vdev->features) != 1); - iowrite32(vdev->features[0], vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES); + iowrite32(vdev->features, vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES); } /* virtio config->get() implementation */ diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 3b1f89b..839247c 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -781,7 +781,7 @@ void vring_transport_features(struct virtio_device *vdev) break; default: /* We don't understand this bit. */ - clear_bit(i, vdev->features); + __virtio_clear_bit(vdev, i); } } } diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c index 00ea679..db3437c 100644 --- a/tools/virtio/virtio_test.c +++ b/tools/virtio/virtio_test.c @@ -60,7 +60,7 @@ void vhost_vq_setup(struct vdev_info *dev, struct vq_info *info) { struct vhost_vring_state state = { .index = info->idx }; struct vhost_vring_file file = { .index = info->idx }; - unsigned long long features = dev->vdev.features[0]; + unsigned long long features = dev->vdev.features; struct vhost_vring_addr addr = { .index = info->idx, .desc_user_addr = (uint64_t)(unsigned long)info->vring.desc, @@ -113,8 +113,7 @@ static void vdev_info_init(struct vdev_info* dev, unsigned long long features) { int r; memset(dev, 0, sizeof *dev); - dev->vdev.features[0] = features; - dev->vdev.features[1] = features >> 32; + dev->vdev.features = features; dev->buf_size = 1024; dev->buf = malloc(dev->buf_size); assert(dev->buf); diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c index 14a4f4c..9d4b1bc 100644 --- a/tools/virtio/vringh_test.c +++ b/tools/virtio/vringh_test.c @@ -304,7 +304,7 @@ static int parallel_test(unsigned long features, close(to_guest[1]); close(to_host[0]); - gvdev.vdev.features[0] = features; + gvdev.vdev.features = features; gvdev.to_host_fd = to_host[1]; gvdev.notifies = 0; @@ -449,13 +449,13 @@ int main(int argc, char *argv[]) bool fast_vringh = false, parallel = false; getrange = getrange_iov; - vdev.features[0] = 0; + vdev.features = 0; while (argv[1]) { if (strcmp(argv[1], "--indirect") == 0) - vdev.features[0] |= (1 << VIRTIO_RING_F_INDIRECT_DESC); + __virtio_set_bit(&vdev, VIRTIO_RING_F_INDIRECT_DESC); else if (strcmp(argv[1], "--eventidx") == 0) - vdev.features[0] |= (1 << VIRTIO_RING_F_EVENT_IDX); + __virtio_set_bit(&vdev, VIRTIO_RING_F_EVENT_IDX); else if (strcmp(argv[1], "--slow-range") == 0) getrange = getrange_slow; else if (strcmp(argv[1], "--fast-vringh") == 0) @@ -468,7 +468,7 @@ int main(int argc, char *argv[]) } if (parallel) - return parallel_test(vdev.features[0], getrange, fast_vringh); + return parallel_test(vdev.features, getrange, fast_vringh); if (posix_memalign(&__user_addr_min, PAGE_SIZE, USER_MEM) != 0) abort(); @@ -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 (__virtio_test_bit(&vdev, 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); + __virtio_clear_bit(&vdev, VIRTIO_RING_F_INDIRECT_DESC); vq = vring_new_virtqueue(0, RINGSIZE, ALIGN, &vdev, true, __user_addr_min, never_notify_host, -- MST
Cornelia Huck
2014-Nov-28 12:44 UTC
[PATCH v6 02/46] virtio: use u32, not bitmap for features
On Thu, 27 Nov 2014 22:07:41 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> It seemed like a good idea to use bitmap for features > in struct virtio_device, but it's actually a pain, > and seems to become even more painful when we get more > than 32 feature bits. Just change it to a u32 for now. > > Based on patch by Rusty. > > Suggested-by: David Hildenbrand <dahi at linux.vnet.ibm.com> > Signed-off-by: Rusty Russell <rusty at rustcorp.com.au> > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c > index d34ebfa..ab95a4c 100644 > --- a/drivers/virtio/virtio_pci.c > +++ b/drivers/virtio/virtio_pci.c > @@ -120,8 +120,7 @@ static void vp_finalize_features(struct virtio_device *vdev) > vring_transport_features(vdev); > > /* We only support 32 feature bits. */I think you can kill this comment...> - BUILD_BUG_ON(ARRAY_SIZE(vdev->features) != 1); > - iowrite32(vdev->features[0], vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES); > + iowrite32(vdev->features, vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);...and add blanks around '+', as you're touching this line anyway.> } > > /* virtio config->get() implementation */Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>
Michael S. Tsirkin
2014-Nov-29 17:34 UTC
[PATCH v6 02/46] virtio: use u32, not bitmap for features
On Fri, Nov 28, 2014 at 01:44:57PM +0100, Cornelia Huck wrote:> On Thu, 27 Nov 2014 22:07:41 +0200 > "Michael S. Tsirkin" <mst at redhat.com> wrote: > > > It seemed like a good idea to use bitmap for features > > in struct virtio_device, but it's actually a pain, > > and seems to become even more painful when we get more > > than 32 feature bits. Just change it to a u32 for now. > > > > Based on patch by Rusty. > > > > Suggested-by: David Hildenbrand <dahi at linux.vnet.ibm.com> > > Signed-off-by: Rusty Russell <rusty at rustcorp.com.au> > > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com> > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c > > index d34ebfa..ab95a4c 100644 > > --- a/drivers/virtio/virtio_pci.c > > +++ b/drivers/virtio/virtio_pci.c > > @@ -120,8 +120,7 @@ static void vp_finalize_features(struct virtio_device *vdev) > > vring_transport_features(vdev); > > > > /* We only support 32 feature bits. */ > > I think you can kill this comment...Why? It's still true for virtio pci at this point, that's why we just do iowrite32 here.> > - BUILD_BUG_ON(ARRAY_SIZE(vdev->features) != 1); > > - iowrite32(vdev->features[0], vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES); > > + iowrite32(vdev->features, vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES); > > ...and add blanks around '+', as you're touching this line anyway. > > > } > > > > /* virtio config->get() implementation */ > > Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>
Reasonably Related Threads
- [PATCH v6 02/46] virtio: use u32, not bitmap for features
- [PATCH v7 02/46] virtio: use u32, not bitmap for features
- [PATCH v8 02/50] virtio: use u32, not bitmap for features
- [PATCH v7 02/46] virtio: use u32, not bitmap for features
- [PATCH v8 02/50] virtio: use u32, not bitmap for features