Anthony Liguori wrote:
> For merging virtio, I thought I'd try a different approach from the
> fix out of tree and merge all at once approach I took with live migration.
>
> What I would like to do is make some minimal changes to virtio in
> kvm-userspace
> so that some form of virtio could be merged in upstream QEMU. We'll have to
> maintain a small diff in the kvm-userspace tree until we work out some of the
> issues, but I'd rather work out those issues in the QEMU tree instead of
> fixing
> them all outside of the tree first.
>
> The attached patch uses USE_KVM guards to separate KVM specific code. This is
> not KVM code, per say, but rather routines that aren't mergable right now. I
> think using the guards make it more clear and easier to deal with merges.
>
> When we submit virtio to qemu-devel and eventually commit, we'll strip out any
> ifdef USE_KVM block.
>
> The only real significant change in this patch is the use of accessors for
> the virtio queues. We've discussed patches like this before.
>
> The point of this patch is to make no functional change in the KVM tree. I've
> confirmed that performance does not regress in virtio-net and that both
> virtio-blk and virtio-net seem to be functional.
>
> Signed-off-by: Anthony Liguori <[EMAIL PROTECTED]>
>
> diff --git a/qemu/hw/virtio-blk.c b/qemu/hw/virtio-blk.c
> index 727119b..fb703eb 100644
> --- a/qemu/hw/virtio-blk.c
> +++ b/qemu/hw/virtio-blk.c
> @@ -276,7 +276,9 @@ void *virtio_blk_init(PCIBus *bus, uint16_t vendor,
> uint16_t device,
> BlockDriverState *bs)
> {
> VirtIOBlock *s;
> +#ifdef USE_KVM
> int cylinders, heads, secs;
> +#endif
> static int virtio_blk_id;
>
> s = (VirtIOBlock *)virtio_init_pci(bus, "virtio-blk", vendor, device,
> @@ -290,9 +292,12 @@ void *virtio_blk_init(PCIBus *bus, uint16_t vendor,
> uint16_t device,
> s->vdev.get_features = virtio_blk_get_features;
> s->vdev.reset = virtio_blk_reset;
> s->bs = bs;
> +#ifdef USE_KVM
> bs->devfn = s->vdev.pci_dev.devfn;
> + /* FIXME this can definitely go in upstream QEMU */
> bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
> bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
> +#endif
>
Why not merge these bits prior to merging virtio? They aren't kvm
specific and would be good in mainline qemu.
>
> static int virtio_net_can_receive(void *opaque)
> {
> VirtIONet *n = opaque;
>
> - if (n->rx_vq->vring.avail == NULL ||
> + if (!virtio_queue_ready(n->rx_vq) ||
> !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
> return 0;
>
> - if (n->rx_vq->vring.avail->idx == n->rx_vq->last_avail_idx) {
> - n->rx_vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY;
> + if (virtio_queue_empty(n->rx_vq)) {
> + virtio_queue_set_notification(n->rx_vq, 1);
> return 0;
> }
>
> - n->rx_vq->vring.used->flags |= VRING_USED_F_NO_NOTIFY;
> + virtio_queue_set_notification(n->rx_vq, 0);
> return 1;
> }
>
This should be in a separate patch.
>
> +#ifdef USE_KVM
> /* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
> * it never finds out that the packets don't have valid checksums. This
> * causes dhclient to get upset. Fedora's carried a patch for ages to
> @@ -181,6 +190,7 @@ static void work_around_broken_dhclient(struct
> virtio_net_hdr *hdr,
> hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> }
> }
> +#endif
>
Is this kvm specific?
>
> static void virtio_net_receive(void *opaque, const uint8_t *buf, int size)
> {
> @@ -190,6 +200,11 @@ static void virtio_net_receive(void *opaque, const
> uint8_t *buf, int size)
> int offset, i;
> int total;
>
> +#ifndef USE_KVM
> + if (!virtio_queue_ready(n->rx_vq))
> + return;
> +#endif
> +
>
Or this?
>
> +#ifdef USE_KVM
> if (tap_has_vnet_hdr(n->vc->vlan->first_client)) {
> memcpy(hdr, buf, sizeof(*hdr));
> offset += total;
> work_around_broken_dhclient(hdr, buf + offset, size - offset);
> }
> +#endif
>
Or this?
>
> /* copy in packet. ugh */
> i = 1;
> @@ -230,7 +247,11 @@ static void virtio_net_receive(void *opaque, const
> uint8_t *buf, int size)
> static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
> {
> VirtQueueElement elem;
> +#ifdef USE_KVM
> int has_vnet_hdr = tap_has_vnet_hdr(n->vc->vlan->first_client);
> +#else
> + int has_vnet_hdr = 0;
> +#endif
>
Or this?
>
> if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
> return;
> @@ -252,7 +273,32 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue
> *vq)
> len += sizeof(struct virtio_net_hdr);
> }
>
> +#ifdef USE_KVM
> len += qemu_sendv_packet(n->vc, out_sg, out_num);
> +#else
> + {
> + size_t size = 0;
> + size_t offset = 0;
> + int i;
> + void *packet;
> +
> + for (i = 0; i < out_num; i++)
> + size += out_sg[i].iov_len;
> +
> + packet = qemu_malloc(size);
> + if (packet) {
> + for (i = 0; i < out_num; i++) {
> + memcpy(packet + offset,
> + out_sg[i].iov_base,
> + out_sg[i].iov_len);
> + offset += out_sg[i].iov_len;
> + }
> + qemu_send_packet(n->vc, packet, size);
> + len += size;
> + }
> + qemu_free(packet);
> + }
> +#endif
>
Why not merge qemu_sendv_packet? Perhaps with the alternate code as the
body if some infrastructure in qemu is missing?
>
> virtqueue_push(vq, &elem, len);
> virtio_notify(&n->vdev, vq);
> @@ -264,7 +310,7 @@ static void virtio_net_handle_tx(VirtIODevice *vdev,
> VirtQueue *vq)
> VirtIONet *n = to_virtio_net(vdev);
>
> if (n->tx_timer_active) {
> - vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY;
> + virtio_queue_set_notification(vq, 1);
>
Separate patch.
> +#ifdef USE_KVM
> + VRingDesc *desc;
> + VRingAvail *avail;
> + VRingUsed *used;
> +#else
> + target_phys_addr_t desc;
> + target_phys_addr_t avail;
> + target_phys_addr_t used;
> +#endif
> +} VRing;
>
Stumped. Why?
--
Do not meddle in the internals of kernels, for they are subtle and quick to
panic.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
