Michael S. Tsirkin
2014-Dec-04 17:26 UTC
[PATCH RFC 1/3] virtio: add API to detect legacy devices
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 <mst at redhat.com> --- 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
Michael S. Tsirkin
2014-Dec-04 17:26 UTC
[PATCH RFC 2/3] virtio_ccw: legacy: don't negotiate rev 1/features
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 <mst at redhat.com> --- drivers/s390/kvm/virtio_ccw.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c index 4a3e6e5..088bdf1 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; @@ -1182,9 +1185,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)) { + ret = virtio_ccw_set_transport_rev(vcdev); + if (ret) + goto out_free; + } else { + vcdev->revision = 0; + } ret = register_virtio_device(&vcdev->vdev); if (ret) { -- MST
Michael S. Tsirkin
2014-Dec-04 17:26 UTC
[PATCH RFC 3/3] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1
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: skip attempts to finalize features or set status. virtio core will detect this and bail out. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c index 088bdf1..0ceeb25 100644 --- a/drivers/s390/kvm/virtio_ccw.c +++ b/drivers/s390/kvm/virtio_ccw.c @@ -65,6 +65,7 @@ struct virtio_ccw_device { bool is_thinint; bool going_away; bool device_lost; + bool features_invalid; void *airq_info; }; @@ -746,6 +747,12 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) if (ret == 0) rc |= (u64)le32_to_cpu(features->features) << 32; + /* Devices MUST set VIRTIO_F_VERSION_1 */ + if (!(rc & BIT_ULL(VIRTIO_F_VERSION_1))) { + vcdev->features_invalid = true; + rc |= BIT_ULL(VIRTIO_F_VERSION_1); + } + out_free: kfree(features); kfree(ccw); @@ -758,6 +765,10 @@ static void virtio_ccw_finalize_features(struct virtio_device *vdev) struct virtio_feature_desc *features; struct ccw1 *ccw; + /* Invalid features? Let's not try to drive this device. */ + if (vcdev->features_invalid) + return; + ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL); if (!ccw) return; @@ -869,6 +880,10 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) struct ccw1 *ccw; int ret; + /* Invalid features? Let's not try to drive this device. */ + if (vcdev->features_invalid) + return; + ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL); if (!ccw) return; -- MST
Cornelia Huck
2014-Dec-04 17:54 UTC
[PATCH RFC 1/3] virtio: add API to detect legacy devices
On Thu, 4 Dec 2014 19:26:42 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> 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 <mst at redhat.com> > --- > include/linux/virtio.h | 2 ++ > drivers/virtio/virtio.c | 6 ++++++ > 2 files changed, 8 insertions(+)Acked-by: Cornelia Huck <cornelia.huck at de.ibm.com>
Cornelia Huck
2014-Dec-04 17:56 UTC
[PATCH RFC 2/3] virtio_ccw: legacy: don't negotiate rev 1/features
On Thu, 4 Dec 2014 19:26:45 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> 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 <mst at redhat.com> > --- > drivers/s390/kvm/virtio_ccw.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c > index 4a3e6e5..088bdf1 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; > @@ -1182,9 +1185,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)) {Inverted logic?> + ret = virtio_ccw_set_transport_rev(vcdev); > + if (ret) > + goto out_free; > + } else { > + vcdev->revision = 0; > + } > > ret = register_virtio_device(&vcdev->vdev); > if (ret) {
Cornelia Huck
2014-Dec-04 17:59 UTC
[PATCH RFC 3/3] virtio_ccw: rev 1 devices set VIRTIO_F_VERSION_1
On Thu, 4 Dec 2014 19:26:50 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> 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: > skip attempts to finalize features or set status.Why not set status_failed?> virtio core will detect this and bail out. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) >I'm not sure yet whether I like this approach. I'll think about it over the weekend.
Apparently Analagous Threads
- [PATCH RFC 2/3] virtio_ccw: legacy: don't negotiate rev 1/features
- [PATCH RFC 2/3] virtio_ccw: legacy: don't negotiate rev 1/features
- [PATCH RFC 2/3] virtio_ccw: legacy: don't negotiate rev 1/features
- [PATCH RFC 1/3] virtio: add API to detect legacy devices
- [PATCH RFC 1/3] virtio: add API to detect legacy devices