| Submitter | Michael S. Tsirkin |
|---|---|
| Date | 2009-11-02 22:23:30 |
| Message ID | <20091102222329.GB15153@redhat.com> |
| Download | mbox | patch |
| Permalink | /patch/57152/ |
| State | New |
| Headers | show |
Comments
Michael S. Tsirkin wrote: > devices should have the final say over which virtio features they > support. E.g. indirect entries may or may not make sense in the context > of virtio-console. In particular, for vhost, we do not want to report to > guest bits not supported by kernel backend. Move the common bits from > virtio-pci to an inline function and let each device call it. > > No functional changes. > This is a layering violation. There are transport specific features and device specific features. The virtio-net device should have no knowledge or nack'ing ability for transport features. If you need to change transport features, it suggests you're modeling things incorrectly and should be supplying an alternative transport implementation. Regards, Anthony Liguori -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/03/2009 12:33 AM, Anthony Liguori wrote: > Michael S. Tsirkin wrote: >> devices should have the final say over which virtio features they >> support. E.g. indirect entries may or may not make sense in the context >> of virtio-console. In particular, for vhost, we do not want to report to >> guest bits not supported by kernel backend. Move the common bits from >> virtio-pci to an inline function and let each device call it. >> >> No functional changes. > > This is a layering violation. There are transport specific features > and device specific features. The virtio-net device should have no > knowledge or nack'ing ability for transport features. It's equivalent to -cpu host. Sometimes you want to pass-through host capabilities in order to make the best use of your hardware. In fact, even -cpu !host allows the host kernel to nack features since the cost of emulation is prohibitive. > If you need to change transport features, it suggests you're modeling > things incorrectly and should be supplying an alternative transport > implementation. Since the kernel and qemu are developed independently, there's no way to ensure they support exactly the same capabilities. The kernel can always lag. The only options are to allow the host kernel to nack features, or to fall back to the userspace implementation. It needs to be finer grained (qemu invoker telling qemu what the minimum features are needed, and qemu telling the invoker what capabilties it supports) but there's no way around it IMO.
On Mon, Nov 02, 2009 at 04:33:53PM -0600, Anthony Liguori wrote: > Michael S. Tsirkin wrote: >> devices should have the final say over which virtio features they >> support. E.g. indirect entries may or may not make sense in the context >> of virtio-console. In particular, for vhost, we do not want to report to >> guest bits not supported by kernel backend. Move the common bits from >> virtio-pci to an inline function and let each device call it. >> >> No functional changes. >> > > This is a layering violation. There are transport specific features and > device specific features. The virtio-net device should have no > knowledge or nack'ing ability for transport features. We could pass "vhost" flag to virtio, and have virtio query the device for features. Would that be better? > If you need to change transport features, it suggests you're modeling > things incorrectly and should be supplying an alternative transport > implementation. > Regards, > > Anthony Liguori Yes, you can make vhost an alternative transport in qemu. This might be one way to handle this. However, this seems to go contrary to your previous proposal to make vhost a networking back end. Which will it be?
Patch
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c index 7ca783e..15b50bb 100644 --- a/hw/virtio-balloon.c +++ b/hw/virtio-balloon.c @@ -127,7 +127,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, static uint32_t virtio_balloon_get_features(VirtIODevice *vdev) { - return 0; + return virtio_common_features(); } static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target) diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 2630b99..db727b9 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -445,7 +445,7 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev) if (strcmp(s->serial_str, "0")) features |= 1 << VIRTIO_BLK_F_IDENTIFY; - return features; + return features | virtio_common_features(); } static void virtio_blk_save(QEMUFile *f, void *opaque) diff --git a/hw/virtio-console.c b/hw/virtio-console.c index 92c953c..79544bb 100644 --- a/hw/virtio-console.c +++ b/hw/virtio-console.c @@ -53,7 +53,7 @@ static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq) static uint32_t virtio_console_get_features(VirtIODevice *vdev) { - return 0; + return virtio_common_features(); } static int vcon_can_read(void *opaque) diff --git a/hw/virtio-net.c b/hw/virtio-net.c index ce8e6cb..469c6e3 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -154,7 +154,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev) } #endif - return features; + return features | virtio_common_features(); } static uint32_t virtio_net_bad_features(VirtIODevice *vdev) diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 01782e5..0716f6f 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -230,9 +230,6 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr) switch (addr) { case VIRTIO_PCI_HOST_FEATURES: ret = vdev->get_features(vdev); - ret |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY); - ret |= (1 << VIRTIO_RING_F_INDIRECT_DESC); - ret |= (1 << VIRTIO_F_BAD_FEATURE); break; case VIRTIO_PCI_GUEST_FEATURES: ret = vdev->features; diff --git a/hw/virtio.h b/hw/virtio.h index 0f9be7d..799e608 100644 --- a/hw/virtio.h +++ b/hw/virtio.h @@ -167,4 +167,14 @@ VirtIODevice *virtio_net_init(DeviceState *dev); VirtIODevice *virtio_console_init(DeviceState *dev); VirtIODevice *virtio_balloon_init(DeviceState *dev); +static inline uint32_t virtio_common_features(void) +{ + uint32_t features = 0; + features |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY); + features |= (1 << VIRTIO_RING_F_INDIRECT_DESC); + features |= (1 << VIRTIO_F_BAD_FEATURE); + + return features; +} + #endif
devices should have the final say over which virtio features they support. E.g. indirect entries may or may not make sense in the context of virtio-console. In particular, for vhost, we do not want to report to guest bits not supported by kernel backend. Move the common bits from virtio-pci to an inline function and let each device call it. No functional changes. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/virtio-balloon.c | 2 +- hw/virtio-blk.c | 2 +- hw/virtio-console.c | 2 +- hw/virtio-net.c | 2 +- hw/virtio-pci.c | 3 --- hw/virtio.h | 10 ++++++++++ 6 files changed, 14 insertions(+), 7 deletions(-)