Re: [PATCH v4 02/42] virtio: add support for 64 bit features.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 25 Nov 2014 18:41:22 +0200
"Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:

> From: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> 
> Change the u32 to a u64, and make sure to use 1ULL everywhere!
> 
> Cc: Brian Swetland <swetland@xxxxxxxxxx>
> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> [Thomas Huth: fix up virtio-ccw get_features]
> 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>
> ---
>  include/linux/virtio.h                 |  2 +-
>  include/linux/virtio_config.h          |  8 ++++----
>  tools/virtio/linux/virtio.h            |  2 +-
>  tools/virtio/linux/virtio_config.h     |  2 +-
>  drivers/char/virtio_console.c          |  2 +-
>  drivers/lguest/lguest_device.c         | 10 +++++-----
>  drivers/misc/mic/card/mic_virtio.c     | 10 +++++-----
>  drivers/remoteproc/remoteproc_virtio.c |  5 ++++-
>  drivers/s390/kvm/kvm_virtio.c          | 10 +++++-----
>  drivers/s390/kvm/virtio_ccw.c          | 29 ++++++++++++++++++++++++-----
>  drivers/virtio/virtio.c                | 12 ++++++------
>  drivers/virtio/virtio_mmio.c           | 14 +++++++++-----
>  drivers/virtio/virtio_pci.c            |  5 ++---
>  drivers/virtio/virtio_ring.c           |  2 +-
>  14 files changed, 69 insertions(+), 44 deletions(-)
> 
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 7828a7f..149284e 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -101,7 +101,7 @@ struct virtio_device {
>  	const struct virtio_config_ops *config;
>  	const struct vringh_config_ops *vringh_config;
>  	struct list_head vqs;
> -	u32 features;
> +	u64 features;
>  	void *priv;
>  };
> 
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index aa84d0e..022d904 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -66,7 +66,7 @@ struct virtio_config_ops {
>  			vq_callback_t *callbacks[],
>  			const char *names[]);
>  	void (*del_vqs)(struct virtio_device *);
> -	u32 (*get_features)(struct virtio_device *vdev);
> +	u64 (*get_features)(struct virtio_device *vdev);
>  	void (*finalize_features)(struct virtio_device *vdev);
>  	const char *(*bus_name)(struct virtio_device *vdev);
>  	int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
> @@ -86,14 +86,14 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
>  {
>  	/* Did you forget to fix assumptions on max features? */
>  	if (__builtin_constant_p(fbit))
> -		BUILD_BUG_ON(fbit >= 32);
> +		BUILD_BUG_ON(fbit >= 64);


While you're here, maybe you could derive the max value from the .features field ?

-               BUILD_BUG_ON(fbit >= 64);
+               BUILD_BUG_ON(fbit >= (sizeof(vdev->features) << 3));

Two other locations below.

>  	else
> -		BUG_ON(fbit >= 32);
> +		BUG_ON(fbit >= 64);
> 

Here...

>  	if (fbit < VIRTIO_TRANSPORT_F_START)
>  		virtio_check_driver_offered_feature(vdev, fbit);
> 
> -	return vdev->features & (1 << fbit);
> +	return vdev->features & (1ULL << fbit);
>  }
> 
>  static inline
> diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
> index 72bff70..8eb6421 100644
> --- a/tools/virtio/linux/virtio.h
> +++ b/tools/virtio/linux/virtio.h
> @@ -10,7 +10,7 @@
> 
>  struct virtio_device {
>  	void *dev;
> -	u32 features;
> +	u64 features;
>  };
> 
>  struct virtqueue {
> diff --git a/tools/virtio/linux/virtio_config.h b/tools/virtio/linux/virtio_config.h
> index 1f1636b..a254c2b 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) \
> -	((dev)->features & (1 << feature))
> +	((dev)->features & (1ULL << feature))
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 0074f9b..fda9a75 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 & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
> +	return portdev->vdev->features & (1ULL << 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 c831c47..4d29bcd 100644
> --- a/drivers/lguest/lguest_device.c
> +++ b/drivers/lguest/lguest_device.c
> @@ -94,17 +94,17 @@ static unsigned desc_size(const struct lguest_device_desc *desc)
>  }
> 
>  /* This gets the device's feature bits. */
> -static u32 lg_get_features(struct virtio_device *vdev)
> +static u64 lg_get_features(struct virtio_device *vdev)
>  {
>  	unsigned int i;
> -	u32 features = 0;
> +	u64 features = 0;
>  	struct lguest_device_desc *desc = to_lgdev(vdev)->desc;
>  	u8 *in_features = lg_features(desc);
> 
>  	/* We do this the slow but generic way. */
> -	for (i = 0; i < min(desc->feature_len * 8, 32); i++)
> +	for (i = 0; i < min(desc->feature_len * 8, 64); i++)
>  		if (in_features[i / 8] & (1 << (i % 8)))
> -			features |= (1 << i);
> +			features |= (1ULL << i);
> 
>  	return features;
>  }
> @@ -144,7 +144,7 @@ static void lg_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 (vdev->features & (1 << i))
> +		if (vdev->features & (1ULL << 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 0acd564..6d94f04 100644
> --- a/drivers/misc/mic/card/mic_virtio.c
> +++ b/drivers/misc/mic/card/mic_virtio.c
> @@ -68,10 +68,10 @@ static inline struct device *mic_dev(struct mic_vdev *mvdev)
>  }
> 
>  /* This gets the device's feature bits. */
> -static u32 mic_get_features(struct virtio_device *vdev)
> +static u64 mic_get_features(struct virtio_device *vdev)
>  {
>  	unsigned int i, bits;
> -	u32 features = 0;
> +	u64 features = 0;
>  	struct mic_device_desc __iomem *desc = to_micvdev(vdev)->desc;
>  	u8 __iomem *in_features = mic_vq_features(desc);
>  	int feature_len = ioread8(&desc->feature_len);
> @@ -79,8 +79,8 @@ static u32 mic_get_features(struct virtio_device *vdev)
>  	bits = min_t(unsigned, feature_len,
>  		sizeof(vdev->features)) * 8;
>  	for (i = 0; i < bits; i++)
> -		if (ioread8(&in_features[i / 8]) & (BIT(i % 8)))
> -			features |= BIT(i);
> +		if (ioread8(&in_features[i / 8]) & (BIT_ULL(i % 8)))
> +			features |= BIT_ULL(i);
> 
>  	return features;
>  }
> @@ -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 (vdev->features & BIT(bits))
> +		if (vdev->features & BIT_ULL(bits))
>  			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 dafaf38..627737e 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -207,7 +207,7 @@ static void rproc_virtio_reset(struct virtio_device *vdev)
>  }
> 
>  /* provide the vdev features as retrieved from the firmware */
> -static u32 rproc_virtio_get_features(struct virtio_device *vdev)
> +static u64 rproc_virtio_get_features(struct virtio_device *vdev)
>  {
>  	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
>  	struct fw_rsc_vdev *rsc;
> @@ -227,6 +227,9 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
>  	/* Give virtio_ring a chance to accept features */
>  	vring_transport_features(vdev);
> 
> +	/* Make sure we don't have any features > 32 bits! */
> +	BUG_ON((u32)vdev->features != vdev->features);
> +
>  	/*
>  	 * Remember the finalized features of our vdev, and provide it
>  	 * to the remote processor once it is powered on.
> diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
> index ac79a09..c78251d 100644
> --- a/drivers/s390/kvm/kvm_virtio.c
> +++ b/drivers/s390/kvm/kvm_virtio.c
> @@ -80,16 +80,16 @@ static unsigned desc_size(const struct kvm_device_desc *desc)
>  }
> 
>  /* This gets the device's feature bits. */
> -static u32 kvm_get_features(struct virtio_device *vdev)
> +static u64 kvm_get_features(struct virtio_device *vdev)
>  {
>  	unsigned int i;
> -	u32 features = 0;
> +	u64 features = 0;
>  	struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
>  	u8 *in_features = kvm_vq_features(desc);
> 
> -	for (i = 0; i < min(desc->feature_len * 8, 32); i++)
> +	for (i = 0; i < min(desc->feature_len * 8, 64); i++)
>  		if (in_features[i / 8] & (1 << (i % 8)))
> -			features |= (1 << i);
> +			features |= (1ULL << i);
>  	return features;
>  }
> 
> @@ -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 (vdev->features & (1 << i))
> +		if (vdev->features & (1ULL << 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 1dbee95..abba04d 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -660,11 +660,12 @@ static void virtio_ccw_reset(struct virtio_device *vdev)
>  	kfree(ccw);
>  }
> 
> -static u32 virtio_ccw_get_features(struct virtio_device *vdev)
> +static u64 virtio_ccw_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>  	struct virtio_feature_desc *features;
> -	int ret, rc;
> +	int ret;
> +	u64 rc;
>  	struct ccw1 *ccw;
> 
>  	ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
> @@ -677,7 +678,6 @@ static u32 virtio_ccw_get_features(struct virtio_device *vdev)
>  		goto out_free;
>  	}
>  	/* Read the feature bits from the host. */
> -	/* TODO: Features > 32 bits */
>  	features->index = 0;
>  	ccw->cmd_code = CCW_CMD_READ_FEAT;
>  	ccw->flags = 0;
> @@ -691,6 +691,16 @@ static u32 virtio_ccw_get_features(struct virtio_device *vdev)
> 
>  	rc = le32_to_cpu(features->features);
> 
> +	/* Read second half feature bits from the host. */
> +	features->index = 1;
> +	ccw->cmd_code = CCW_CMD_READ_FEAT;
> +	ccw->flags = 0;
> +	ccw->count = sizeof(*features);
> +	ccw->cda = (__u32)(unsigned long)features;
> +	ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_FEAT);
> +	if (ret == 0)
> +		rc |= (u64)le32_to_cpu(features->features) << 32;
> +
>  out_free:
>  	kfree(features);
>  	kfree(ccw);
> @@ -715,8 +725,17 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
>  	vring_transport_features(vdev);
> 
>  	features->index = 0;
> -	features->features = cpu_to_le32(vdev->features);
> -	/* Write the feature bits to the host. */
> +	features->features = cpu_to_le32((u32)vdev->features);
> +	/* Write the first half of 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 = 1;
> +	features->features = cpu_to_le32(vdev->features >> 32);
> +	/* Write the second half of the feature bits to the host. */
>  	ccw->cmd_code = CCW_CMD_WRITE_FEAT;
>  	ccw->flags = 0;
>  	ccw->count = sizeof(*features);
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 7814b1f..d213567 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -159,7 +159,7 @@ static int virtio_dev_probe(struct device *_d)
>  	int err, i;
>  	struct virtio_device *dev = dev_to_virtio(_d);
>  	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> -	u32 device_features;
> +	u64 device_features;
> 
>  	/* We have a driver! */
>  	add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> @@ -171,15 +171,15 @@ static int virtio_dev_probe(struct device *_d)
>  	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))
> -			dev->features |= (1 << f);
> +		BUG_ON(f >= 64);

and here.

> +		if (device_features & (1ULL << f))
> +			dev->features |= (1ULL << 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))
> -			dev->features |= (1 << i);
> +		if (device_features & (1ULL << i))
> +			dev->features |= (1ULL << i);
> 
>  	dev->config->finalize_features(dev);
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index eb5b0e2..fd01c6d 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -142,14 +142,16 @@ struct virtio_mmio_vq_info {
> 
>  /* Configuration interface */
> 
> -static u32 vm_get_features(struct virtio_device *vdev)
> +static u64 vm_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> +	u64 features;
> 
> -	/* TODO: Features > 32 bits */
>  	writel(0, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
> -
> -	return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
> +	features = readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
> +	writel(1, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
> +	features |= ((u64)readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES) << 32);
> +	return features;
>  }
> 
>  static void vm_finalize_features(struct virtio_device *vdev)
> @@ -160,7 +162,9 @@ static void vm_finalize_features(struct virtio_device *vdev)
>  	vring_transport_features(vdev);
> 
>  	writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
> -	writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
> +	writel((u32)vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
> +	writel(1, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
> +	writel(vdev->features >> 32, 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 ab95a4c..68c0711 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -102,12 +102,11 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
>  }
> 
>  /* virtio config->get_features() implementation */
> -static u32 vp_get_features(struct virtio_device *vdev)
> +static u64 vp_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> 
> -	/* When someone needs more than 32 feature bits, we'll need to
> -	 * steal a bit to indicate that the rest are somewhere else. */
> +	/* We only support 32 feature bits. */
>  	return ioread32(vp_dev->ioaddr + VIRTIO_PCI_HOST_FEATURES);
>  }
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 15a8a05..61a1fe1 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. */
> -			vdev->features &= ~(1 << i);
> +			vdev->features &= ~(1ULL << i);
>  		}
>  	}
>  }

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux