transports need to be able to detect legacy-only
devices (ATM balloon only) to use legacy path
to drive them.
Add a core API to do just that.
The implementation just blacklists balloon:
not too pretty, but let's not over-engineer.
Signed-off-by: Michael S. Tsirkin <[email protected]>
Acked-by: Cornelia Huck <[email protected]>
---
include/linux/virtio.h | 2 ++
drivers/virtio/virtio.c | 6 ++++++
2 files changed, 8 insertions(+)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 2bbf626..d666bcb 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -108,6 +108,8 @@ struct virtio_device {
void *priv;
};
+bool virtio_device_is_legacy_only(struct virtio_device_id id);
+
static inline struct virtio_device *dev_to_virtio(struct device *_dev)
{
return container_of(_dev, struct virtio_device, dev);
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index fa6b75d..6b4c1113 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -267,6 +267,12 @@ static struct bus_type virtio_bus = {
.remove = virtio_dev_remove,
};
+bool virtio_device_is_legacy_only(struct virtio_device_id id)
+{
+ return id.device == VIRTIO_ID_BALLOON;
+}
+EXPORT_SYMBOL_GPL(register_virtio_driver);
+
int register_virtio_driver(struct virtio_driver *driver)
{
/* Catch this early. */
--
MST
Legacy balloon device doesn't pretend to support revision 1 or 64 bit
features.
But just in case someone implements a broken one that does, let's not
even try to drive legacy only devices using revision 1, and let's not
give them a chance to say they support VIRTIO_F_VERSION_1 by not reading
high feature bits.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/s390/kvm/virtio_ccw.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 4a3e6e5..c792b5f 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -733,6 +733,9 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
rc = le32_to_cpu(features->features);
+ if (vcdev->revision == 0)
+ goto out_free;
+
/* Read second half of the feature bits from the host. */
features->index = 1;
ccw->cmd_code = CCW_CMD_READ_FEAT;
@@ -775,6 +778,9 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
ccw->cda = (__u32)(unsigned long)features;
ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
+ if (vcdev->revision == 0)
+ goto out_free;
+
features->index = 1;
features->features = cpu_to_le32(vdev->features >> 32);
/* Write the second half of the feature bits to the host. */
@@ -1182,9 +1188,13 @@ static int virtio_ccw_online(struct ccw_device *cdev)
vcdev->vdev.id.vendor = cdev->id.cu_type;
vcdev->vdev.id.device = cdev->id.cu_model;
- ret = virtio_ccw_set_transport_rev(vcdev);
- if (ret)
- goto out_free;
+ if (virtio_device_is_legacy_only(vcdev->vdev.id)) {
+ vcdev->revision = 0;
+ } else {
+ ret = virtio_ccw_set_transport_rev(vcdev);
+ if (ret)
+ goto out_free;
+ }
ret = register_virtio_device(&vcdev->vdev);
if (ret) {
--
MST
What does it mean if rev 1 device does not set
VIRTIO_F_VERSION_1? E.g. is it native endian?
Let's not even try to drive such devices:
fail attempts to finalize features.
virtio core will detect this and bail out.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/s390/kvm/virtio_ccw.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 789275f..f9f87ba 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -758,6 +758,13 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
struct virtio_feature_desc *features;
struct ccw1 *ccw;
+ if (vcdev->revision == 1 &&
+ !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
+ dev_err(&vdev->dev, "virtio: device uses revision 1 "
+ "but does not have VIRTIO_F_VERSION_1\n");
+ return -EINVAL;
+ }
+
ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
if (!ccw)
return 0;
--
MST
This will make it easy for transports to validate features and return
failure.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
include/linux/virtio_config.h | 3 ++-
drivers/lguest/lguest_device.c | 4 +++-
drivers/misc/mic/card/mic_virtio.c | 4 +++-
drivers/remoteproc/remoteproc_virtio.c | 4 +++-
drivers/s390/kvm/kvm_virtio.c | 4 +++-
drivers/s390/kvm/virtio_ccw.c | 6 ++++--
drivers/virtio/virtio.c | 4 +++-
drivers/virtio/virtio_mmio.c | 4 +++-
drivers/virtio/virtio_pci.c | 4 +++-
9 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 1fa5faa..7979f85 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -47,6 +47,7 @@
* vdev: the virtio_device
* This gives the final feature bits for the device: it can change
* the dev->feature bits if it wants.
+ * Returns 0 on success or error status
* @bus_name: return the bus name associated with the device
* vdev: the virtio_device
* This returns a pointer to the bus name a la pci_name from which
@@ -68,7 +69,7 @@ struct virtio_config_ops {
const char *names[]);
void (*del_vqs)(struct virtio_device *);
u64 (*get_features)(struct virtio_device *vdev);
- void (*finalize_features)(struct virtio_device *vdev);
+ int (*finalize_features)(struct virtio_device *vdev);
const char *(*bus_name)(struct virtio_device *vdev);
int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
};
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 9b77b66..89088d6 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -126,7 +126,7 @@ static void status_notify(struct virtio_device *vdev)
* sorted out, this routine is called so we can tell the Host which features we
* understand and accept.
*/
-static void lg_finalize_features(struct virtio_device *vdev)
+static int lg_finalize_features(struct virtio_device *vdev)
{
unsigned int i, bits;
struct lguest_device_desc *desc = to_lgdev(vdev)->desc;
@@ -153,6 +153,8 @@ static void lg_finalize_features(struct virtio_device *vdev)
/* Tell Host we've finished with this device's feature negotiation */
status_notify(vdev);
+
+ return 0;
}
/* Once they've found a field, getting a copy of it is easy. */
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index d027d29..e486a0c 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -84,7 +84,7 @@ static u64 mic_get_features(struct virtio_device *vdev)
return features;
}
-static void mic_finalize_features(struct virtio_device *vdev)
+static int mic_finalize_features(struct virtio_device *vdev)
{
unsigned int i, bits;
struct mic_device_desc __iomem *desc = to_micvdev(vdev)->desc;
@@ -107,6 +107,8 @@ static void mic_finalize_features(struct virtio_device *vdev)
iowrite8(ioread8(&out_features[i / 8]) | (1 << (i % 8)),
&out_features[i / 8]);
}
+
+ return 0;
}
/*
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 627737e..e1a1023 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -217,7 +217,7 @@ static u64 rproc_virtio_get_features(struct virtio_device *vdev)
return rsc->dfeatures;
}
-static void rproc_virtio_finalize_features(struct virtio_device *vdev)
+static int rproc_virtio_finalize_features(struct virtio_device *vdev)
{
struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
struct fw_rsc_vdev *rsc;
@@ -235,6 +235,8 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
* to the remote processor once it is powered on.
*/
rsc->gfeatures = vdev->features;
+
+ return 0;
}
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 f5575cc..dd65c8b 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -93,7 +93,7 @@ static u64 kvm_get_features(struct virtio_device *vdev)
return features;
}
-static void kvm_finalize_features(struct virtio_device *vdev)
+static int kvm_finalize_features(struct virtio_device *vdev)
{
unsigned int i, bits;
struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
@@ -112,6 +112,8 @@ static void kvm_finalize_features(struct virtio_device *vdev)
if (__virtio_test_bit(vdev, i))
out_features[i / 8] |= (1 << (i % 8));
}
+
+ return 0;
}
/*
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index c792b5f..789275f 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -752,7 +752,7 @@ out_free:
return rc;
}
-static void virtio_ccw_finalize_features(struct virtio_device *vdev)
+static int virtio_ccw_finalize_features(struct virtio_device *vdev)
{
struct virtio_ccw_device *vcdev = to_vc_device(vdev);
struct virtio_feature_desc *features;
@@ -760,7 +760,7 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
if (!ccw)
- return;
+ return 0;
features = kzalloc(sizeof(*features), GFP_DMA | GFP_KERNEL);
if (!features)
@@ -793,6 +793,8 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev)
out_free:
kfree(features);
kfree(ccw);
+
+ return 0;
}
static void virtio_ccw_get_config(struct virtio_device *vdev,
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 6b4c1113..7ddebbc 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -211,7 +211,9 @@ static int virtio_dev_probe(struct device *_d)
if (device_features & (1ULL << i))
__virtio_set_bit(dev, i);
- dev->config->finalize_features(dev);
+ err = dev->config->finalize_features(dev);
+ if (err)
+ goto err;
if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index aec1dae..5219210 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -152,7 +152,7 @@ static u64 vm_get_features(struct virtio_device *vdev)
return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
}
-static void vm_finalize_features(struct virtio_device *vdev)
+static int vm_finalize_features(struct virtio_device *vdev)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
@@ -164,6 +164,8 @@ static void vm_finalize_features(struct virtio_device *vdev)
writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
+
+ return 0;
}
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 98f9c6e..c7649f3 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -111,7 +111,7 @@ static u64 vp_get_features(struct virtio_device *vdev)
}
/* virtio config->finalize_features() implementation */
-static void vp_finalize_features(struct virtio_device *vdev)
+static int vp_finalize_features(struct virtio_device *vdev)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -124,6 +124,8 @@ static void vp_finalize_features(struct virtio_device *vdev)
/* We only support 32 feature bits. */
iowrite32(vdev->features, vp_dev->ioaddr +
VIRTIO_PCI_LEGACY_GUEST_FEATURES);
+
+ return 0;
}
/* virtio config->get() implementation */
--
MST
> This will make it easy for transports to validate features and return
> failure.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> include/linux/virtio_config.h | 3 ++-
> drivers/lguest/lguest_device.c | 4 +++-
> drivers/misc/mic/card/mic_virtio.c | 4 +++-
> drivers/remoteproc/remoteproc_virtio.c | 4 +++-
> drivers/s390/kvm/kvm_virtio.c | 4 +++-
> drivers/s390/kvm/virtio_ccw.c | 6 ++++--
> drivers/virtio/virtio.c | 4 +++-
> drivers/virtio/virtio_mmio.c | 4 +++-
> drivers/virtio/virtio_pci.c | 4 +++-
> 9 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 1fa5faa..7979f85 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -47,6 +47,7 @@
> * vdev: the virtio_device
> * This gives the final feature bits for the device: it can change
> * the dev->feature bits if it wants.
> + * Returns 0 on success or error status
"Returns 0 on success, otherwise the error status."
> * @bus_name: return the bus name associated with the device
> * vdev: the virtio_device
> * This returns a pointer to the bus name a la pci_name from which
> @@ -68,7 +69,7 @@ struct virtio_config_ops {
> const char *names[]);
> void (*del_vqs)(struct virtio_device *);
> u64 (*get_features)(struct virtio_device *vdev);
> - void (*finalize_features)(struct virtio_device *vdev);
> + int (*finalize_features)(struct virtio_device *vdev);
> const char *(*bus_name)(struct virtio_device *vdev);
> int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
> };
...
>
> static void virtio_ccw_get_config(struct virtio_device *vdev,
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 6b4c1113..7ddebbc 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -211,7 +211,9 @@ static int virtio_dev_probe(struct device *_d)
> if (device_features & (1ULL << i))
> __virtio_set_bit(dev, i);
>
> - dev->config->finalize_features(dev);
> + err = dev->config->finalize_features(dev);
> + if (err)
> + goto err;
>
> if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index aec1dae..5219210 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -152,7 +152,7 @@ static u64 vm_get_features(struct virtio_device *vdev)
> return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
> }
>
Do we have to take care of fails in virtio_device_restore()?
Otherwise looks good to me.
David
On Fri, Dec 05, 2014 at 10:53:54AM +0100, David Hildenbrand wrote:
> > This will make it easy for transports to validate features and return
> > failure.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > include/linux/virtio_config.h | 3 ++-
> > drivers/lguest/lguest_device.c | 4 +++-
> > drivers/misc/mic/card/mic_virtio.c | 4 +++-
> > drivers/remoteproc/remoteproc_virtio.c | 4 +++-
> > drivers/s390/kvm/kvm_virtio.c | 4 +++-
> > drivers/s390/kvm/virtio_ccw.c | 6 ++++--
> > drivers/virtio/virtio.c | 4 +++-
> > drivers/virtio/virtio_mmio.c | 4 +++-
> > drivers/virtio/virtio_pci.c | 4 +++-
> > 9 files changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 1fa5faa..7979f85 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -47,6 +47,7 @@
> > * vdev: the virtio_device
> > * This gives the final feature bits for the device: it can change
> > * the dev->feature bits if it wants.
> > + * Returns 0 on success or error status
>
> "Returns 0 on success, otherwise the error status."
It's what other functions say.
I'm ok with changing this but let's do it in a separate patch.
> > * @bus_name: return the bus name associated with the device
> > * vdev: the virtio_device
> > * This returns a pointer to the bus name a la pci_name from which
> > @@ -68,7 +69,7 @@ struct virtio_config_ops {
> > const char *names[]);
> > void (*del_vqs)(struct virtio_device *);
> > u64 (*get_features)(struct virtio_device *vdev);
> > - void (*finalize_features)(struct virtio_device *vdev);
> > + int (*finalize_features)(struct virtio_device *vdev);
> > const char *(*bus_name)(struct virtio_device *vdev);
> > int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
> > };
>
> ...
>
> >
> > static void virtio_ccw_get_config(struct virtio_device *vdev,
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 6b4c1113..7ddebbc 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -211,7 +211,9 @@ static int virtio_dev_probe(struct device *_d)
> > if (device_features & (1ULL << i))
> > __virtio_set_bit(dev, i);
> >
> > - dev->config->finalize_features(dev);
> > + err = dev->config->finalize_features(dev);
> > + if (err)
> > + goto err;
> >
> > if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> > add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> > index aec1dae..5219210 100644
> > --- a/drivers/virtio/virtio_mmio.c
> > +++ b/drivers/virtio/virtio_mmio.c
> > @@ -152,7 +152,7 @@ static u64 vm_get_features(struct virtio_device *vdev)
> > return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
> > }
> >
>
> Do we have to take care of fails in virtio_device_restore()?
>
> Otherwise looks good to me.
>
> David