Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752554AbaKZQs2 (ORCPT ); Wed, 26 Nov 2014 11:48:28 -0500 Received: from e06smtp16.uk.ibm.com ([195.75.94.112]:57848 "EHLO e06smtp16.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750828AbaKZQs0 (ORCPT ); Wed, 26 Nov 2014 11:48:26 -0500 Date: Wed, 26 Nov 2014 17:48:09 +0100 From: Greg Kurz To: "Michael S. Tsirkin" Cc: linux-kernel@vger.kernel.org, rusty@au1.ibm.com, Heiko Carstens , Sudeep Dutt , virtualization@lists.linux-foundation.org, linux-s390@vger.kernel.org, lguest@lists.ozlabs.org, Pawel Moll , Christian Borntraeger , Joel Stanley , Arnd Bergmann , Siva Yerramreddy , Martin Schwidefsky , pbonzini@redhat.com, Brian Swetland , Ashutosh Dixit , Greg Kroah-Hartman , Amit Shah , linux390@de.ibm.com, David Miller Subject: Re: [PATCH v4 02/42] virtio: add support for 64 bit features. Message-ID: <20141126174809.24f6be4d@bahia.local> In-Reply-To: <1416933600-21398-3-git-send-email-mst@redhat.com> References: <1416933600-21398-1-git-send-email-mst@redhat.com> <1416933600-21398-3-git-send-email-mst@redhat.com> Organization: IBM X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.24; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14112616-0025-0000-0000-0000029AB15D Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 25 Nov 2014 18:41:22 +0200 "Michael S. Tsirkin" wrote: > From: Rusty Russell > > Change the u32 to a u64, and make sure to use 1ULL everywhere! > > Cc: Brian Swetland > Cc: Christian Borntraeger > [Thomas Huth: fix up virtio-ccw get_features] > Signed-off-by: Rusty Russell > Signed-off-by: Cornelia Huck > Acked-by: Pawel Moll > Acked-by: Ohad Ben-Cohen > > Signed-off-by: Michael S. Tsirkin > --- > 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); > } > } > } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/