Re: [PATCH 1/2] virtio-scsi: Set _DRIVER_OK flag before scsi target scanning
On Fri, Mar 15, 2013 at 09:45:15AM +0800, Asias He wrote: Before we start scsi target scanning, we need to set the VIRTIO_CONFIG_S_DRIVER_OK flag so the device can do setup properly. This fix a bug when booting tcm_vhost with seabios. Signed-off-by: Asias He as...@redhat.com Acked-by: Paolo Bonzini pbonz...@redhat.com Acked-by: Michael S. Tsirkin m...@redhat.com --- src/virtio-scsi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c index 879ddfb..4de1255 100644 --- a/src/virtio-scsi.c +++ b/src/virtio-scsi.c @@ -147,6 +147,9 @@ init_virtio_scsi(struct pci_device *pci) goto fail; } +vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | + VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK); + int i, tot; for (tot = 0, i = 0; i 256; i++) tot += virtio_scsi_scan_target(pci, ioaddr, vq, i); @@ -154,8 +157,6 @@ init_virtio_scsi(struct pci_device *pci) if (!tot) goto fail; -vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | - VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK); return; fail: -- 1.8.1.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-scsi: Pack struct virtio_scsi_{req_cmd,resp_cmd}
On Fri, Mar 15, 2013 at 09:45:16AM +0800, Asias He wrote: Device needs the exact size of these data structure. Prevent padding. This fixes guest hang when booting seabios + tcm_vhost. Signed-off-by: Asias He as...@redhat.com Acked-by: Michael S. Tsirkin m...@redhat.com --- src/virtio-scsi.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/virtio-scsi.h b/src/virtio-scsi.h index bbfbf30..96c3701 100644 --- a/src/virtio-scsi.h +++ b/src/virtio-scsi.h @@ -26,7 +26,7 @@ struct virtio_scsi_req_cmd { u8 prio; u8 crn; char cdb[VIRTIO_SCSI_CDB_SIZE]; -}; +} __attribute__((packed)); /* This is the first element of the in scatter-gather list. */ struct virtio_scsi_resp_cmd { @@ -36,7 +36,7 @@ struct virtio_scsi_resp_cmd { u8 status; u8 response; u8 sense[VIRTIO_SCSI_SENSE_SIZE]; -}; +} __attribute__((packed)); #define VIRTIO_SCSI_S_OK0 -- 1.8.1.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-spec: add field for scsi command size
On Thu, Mar 14, 2013 at 07:39:18PM +0200, Michael S. Tsirkin wrote: On Thu, Mar 14, 2013 at 04:15:28PM +0100, Paolo Bonzini wrote: Il 14/03/2013 12:10, Michael S. Tsirkin ha scritto: Add field for guest to specify command size for virtio-blk. Signed-off-by: Michael S. Tsirkin m...@redhat.com OK, but Rusty usually tweaks wording anyway. Rusty want to apply and make changes Paolo suggested yourself or want me to? Ping. Rusty could you indicate your preference please? --- virtio-spec.lyx | 83 + 1 file changed, 78 insertions(+), 5 deletions(-) diff --git a/virtio-spec.lyx b/virtio-spec.lyx index a8ce3f9..fea97ed 100644 --- a/virtio-spec.lyx +++ b/virtio-spec.lyx @@ -5826,6 +5826,16 @@ VIRTIO_BLK_F_TOPOLOGY (10) Device exports information on optimal I/O alignment. \change_inserted 1531152142 1341302349 VIRTIO_BLK_F_CONFIG_WCE (11) Device can toggle its cache between writeback and writethrough modes. +\change_inserted 1986246365 1363257418 + +\end_layout + +\begin_layout Description + +\change_inserted 1986246365 1363258629 +VIRTIO_BLK_F_CMD_SIZE (12) cmd_size field is valid. SCSI_CMD_SIZE and scsi_cmd_size please. +\change_inserted 1531152142 1341302349 + \end_layout \end_deeper @@ -5994,6 +6004,30 @@ struct virtio_blk_config { \change_inserted 1531152142 1341301918 u8 writeback; +\change_inserted 1986246365 1363257385 + +\end_layout + +\begin_layout Plain Layout + +\change_inserted 1986246365 1363258610 + +u8 reserved[3]; +\end_layout + +\begin_layout Plain Layout + +\change_inserted 1986246365 1363259147 + +// buffer length for cmd field in requests. size of the cmd field in VIRTIO_BLK_T_SCSI_CMD requests. + reset value 0. +\end_layout + +\begin_layout Plain Layout + +\change_inserted 1986246365 1363257810 + +u32 cmd_size; scsi_cmd_size \change_unchanged \end_layout @@ -6085,6 +6119,21 @@ Until version 1.1, QEMU remained in writeback mode even after a guest announced \end_inset . +\change_inserted 1986246365 1363258989 + +\end_layout + +\begin_layout Enumerate + +\change_inserted 1986246365 1363259237 +If VIRTIO_BLK_F_SCSI_CMD_SIZE is negotiated, Guest should set the desired + size for scsi commands to the If both VIRTIO_BLK_F_SCSI and VIRTIO_BLK_F_SCSI_CMD_SIZE are negotiated, the driver should write a non-zero value to the scsi_cmd_size field before sending a VIRTIO_BLK_T_SCSI_CMD request. +\emph on +cmd_size +\emph default +field. +\change_unchanged + \end_layout \begin_layout Section* @@ -6369,8 +6418,33 @@ cmd \emph default field is only present for scsi packet command requests, and indicates the command to perform. - This field must reside in a single, separate read-only buffer; command - length can be derived from the length of this buffer. + +\change_inserted 1986246365 1363258881 +If VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, Guest must set + the command length by writing it to the cmd_size field in configuration + space If the VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, the driver should write the length of the cmd field to the scsi_cmd_size field of the configuration space. (please make sure bold and emphasis are consistent) +\begin_inset Foot +status open + +\begin_layout Plain Layout + +\change_inserted 1986246365 1363258931 +Guest should always set cmd_size during initialization. scsi_cmd_size +\change_unchanged + +\end_layout + +\end_inset + +. + If VIRTIO_BLK_F_SCSI_CMD_SIZE is not negotiated, +\change_deleted 1986246365 1363258703 +T +\change_inserted 1986246365 1363258703 +t +\change_unchanged +his field must reside in a single, separate read-only buffer; command length + can be derived from the length of this buffer. the command length... \end_layout Thanks, Paolo ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3 0/3] tcm_vhost lock and flush fix
On Fri, Mar 15, 2013 at 09:14:04AM +0800, Asias He wrote: Hello Nicholas Michael, Could any of you pick this series up? Changes in v3 - Add more comments Changes in v2 - Use vq-private_data insteadof vs-vs_endpoint - Add label in vhost_scsi_clear_endpoint to simply unlock code. Asias He (3): tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint() tcm_vhost: Flush vhost_work in vhost_scsi_flush() tcm_vhost: Use vq-private_data to indicate if the endpoint is setup drivers/vhost/tcm_vhost.c | 59 +++ 1 file changed, 49 insertions(+), 10 deletions(-) Applied, thanks. -- 1.8.1.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: vhost questions.
On Thu, Mar 14, 2013 at 05:08:22PM +1030, Rusty Russell wrote: Michael S. Tsirkin m...@redhat.com writes: On Wed, Mar 13, 2013 at 05:19:51PM +1030, Rusty Russell wrote: OK, I've been trying to read the vhost and vhost net code, and I'm struggling with basic questions. 1) Why do we allow userspace to change an already-set-up vhost device? Why not have: open() ioctl(VHOST_GET_FEATURES) ioctl(VHOST_SET_VRING) x n (sets num, addresses, kick/call/err fds) ioctl(VHOST_NET_SETUP) ... close() You're not allowed to call things twice: to reset, you close and reopen. That would save a lot of code which I'm not sure is correct anyway. This won't work for the usecases we have in qemu. The way things are setup currently, qemu typically does not have the rights to open any char devices, instead it is passed an fd. By the way at least for the guest callback eventfd, we currently use the ability to switch it on the fly for when guest wants to mask the interrupt. At some level, this could be considered an argument that initialization order is a policy best let for userspace. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH net] vhost/net: fix heads usage of ubuf_info
ubuf info allocator uses guest controlled head as an index, so a malicious guest could put the same head entry in the ring twice, and we will get two callbacks on the same value. To fix use upend_idx which is guaranteed to be unique. Reported-by: Rusty Russell ru...@rustcorp.com.au Signed-off-by: Michael S. Tsirkin m...@redhat.com Cc: sta...@kernel.org --- Rusty's working on switching to allocating ubufs dynamically but that's not 3.9 material. This patch is against latest net master, needed for 3.9-rc2 and older kernels. drivers/vhost/net.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 959b1cd..ec6fb3f 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -339,7 +339,8 @@ static void handle_tx(struct vhost_net *net) msg.msg_controllen = 0; ubufs = NULL; } else { - struct ubuf_info *ubuf = vq-ubuf_info[head]; + struct ubuf_info *ubuf; + ubuf = vq-ubuf_info + vq-upend_idx; vq-heads[vq-upend_idx].len = VHOST_DMA_IN_PROGRESS; -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv3 vringh] caif_virtio: Introduce caif over virtio
From: Erwan Yvin erwan.y...@stericsson.com Date: Fri, 15 Mar 2013 10:42:17 +0100 caif-virtio is going to replace caif-shm. This patch should be merged in rusty's tree. (vringh) because there is a dependency with vringh wrapper. Feel free to add my: Acked-by: David S. Miller da...@davemloft.net ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net] vhost/net: fix heads usage of ubuf_info
From: Michael S. Tsirkin m...@redhat.com Date: Sun, 17 Mar 2013 14:46:09 +0200 ubuf info allocator uses guest controlled head as an index, so a malicious guest could put the same head entry in the ring twice, and we will get two callbacks on the same value. To fix use upend_idx which is guaranteed to be unique. Reported-by: Rusty Russell ru...@rustcorp.com.au Signed-off-by: Michael S. Tsirkin m...@redhat.com Applied and queued up for -stable, thanks. And thankfully you got the stable URL wrong, please do not CC: networking patches to stable, just make sure I apply them and in your post-commit text explicitly ask me to queue it up to my -stable queue. Thanks. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net] vhost/net: fix heads usage of ubuf_info
On Sun, Mar 17, 2013 at 02:29:55PM -0400, David Miller wrote: From: Michael S. Tsirkin m...@redhat.com Date: Sun, 17 Mar 2013 14:46:09 +0200 ubuf info allocator uses guest controlled head as an index, so a malicious guest could put the same head entry in the ring twice, and we will get two callbacks on the same value. To fix use upend_idx which is guaranteed to be unique. Reported-by: Rusty Russell ru...@rustcorp.com.au Signed-off-by: Michael S. Tsirkin m...@redhat.com Applied and queued up for -stable, thanks. OK I'll drop it from my tree then. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv3 vringh] caif_virtio: Introduce caif over virtio
Hi Erwan, On Fri, Mar 15, 2013 at 11:42 AM, Erwan Yvin erwan.y...@stericsson.com wrote: diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c new file mode 100644 index 000..6a745dd --- /dev/null +++ b/drivers/net/caif/caif_virtio.c @@ -0,0 +1,786 @@ +/* + * Copyright (C) ST-Ericsson AB 2013 + * Authors: Vicram Arv / vikram@stericsson.com, + * Dmitry Tarnyagin / dmitry.tarnya...@stericsson.com + * Sjur Brendeland / sjur.brandel...@stericsson.com + * License terms: GNU General Public License (GPL) version 2 + */ +#include linux/module.h +#include linux/if_arp.h +#include linux/virtio.h +#include linux/vringh.h +#include linux/debugfs.h +#include linux/spinlock.h +#include linux/genalloc.h +#include linux/interrupt.h +#include linux/netdevice.h +#include linux/rtnetlink.h +#include linux/remoteproc.h The driver looks nicely decoupled from remoteproc - do you still need remoteproc.h ? Thanks, Ohad. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] virtio: Introduce vringh wrappers in virtio_config
Hi Sjur, Sorry for the delay - just got back from you know where :) On Wed, Mar 13, 2013 at 12:12 PM, Sjur Brændeland sjurb...@gmail.com wrote: From: Sjur Brændeland sjur.brandel...@stericsson.com Add wrappers for the host vrings to support loose coupling between the virtio device and driver. A new struct vringh_config_ops with the functions find_vrhs() and del_vrhs() is added to the virtio_device struct. This enables virtio drivers to manage virtio host rings without detailed knowledge of how the vrings are created and deleted. The function vringh_notify() is added so vringh clients can notify the other side that buffers are added to the used-ring. Cc: Ohad Ben-Cohen o...@wizery.com Cc: Rusty Russell ru...@rustcorp.com.au Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com As you have been pushing for this wrapper, could you please add your Ack or Reviewed tag if you are happy with this patch? I am happy with this patch: the caif virtio driver is now decoupled from remoteproc which I think is great. Thanks for doing it! To provide a Reviewed-by tag though I need to review it from a virtio point of view, which I can try to do after I'll finish going through my backlog of pending patches. Let me finish that pile of patches I have, and if you or Rusty will still want me to provide a Reviewed-by tag here I'll do it. Thanks, Ohad. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] virtio: Introduce vringh wrappers in virtio_config
On Mon, Mar 18, 2013 at 12:29 AM, Ohad Ben-Cohen o...@wizery.com wrote: Hi Sjur, Sorry for the delay - just got back from you know where :) On Wed, Mar 13, 2013 at 12:12 PM, Sjur Brændeland sjurb...@gmail.com wrote: From: Sjur Brændeland sjur.brandel...@stericsson.com Add wrappers for the host vrings to support loose coupling between the virtio device and driver. A new struct vringh_config_ops with the functions find_vrhs() and del_vrhs() is added to the virtio_device struct. This enables virtio drivers to manage virtio host rings without detailed knowledge of how the vrings are created and deleted. The function vringh_notify() is added so vringh clients can notify the other side that buffers are added to the used-ring. Cc: Ohad Ben-Cohen o...@wizery.com Cc: Rusty Russell ru...@rustcorp.com.au Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com As you have been pushing for this wrapper, could you please add your Ack or Reviewed tag if you are happy with this patch? I am happy with this patch: the caif virtio driver is now decoupled from remoteproc which I think is great. Thanks for doing it! To provide a Reviewed-by tag though I need to review it from a virtio point of view, which I can try to do after I'll finish going through my backlog of pending patches. Let me finish that pile of patches I have, and if you or Rusty will still want me to provide a Reviewed-by tag here I'll do it. Hi Rusty, Do you have what you need to apply this patch? Please, let me know if any further changes are needed. The informal ack above from Ohad is good enough for me :-) Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH][v3.2.y] xen-netfront: delay gARP until backend switches to Connected
On Fri, 2013-03-15 at 13:56 -0400, Joseph Salisbury wrote: Hello, Please consider including upstream commit 08e34eb14fe4cfd934b5c169a7682a969457c4ea in the next v3.2.y release. It was included upstream as of v3.3-rc1. It has been tested and confirmed to resolve http://bugs.launchpad.net/bugs/1154608 . commit 08e34eb14fe4cfd934b5c169a7682a969457c4ea Author: Laszlo Ersek ler...@redhat.com Date: Sun Dec 11 01:48:59 2011 + xen-netfront: delay gARP until backend switches to Connected Added to the queue, thanks. Ben. -- Ben Hutchings Never attribute to conspiracy what can adequately be explained by stupidity. signature.asc Description: This is a digitally signed message part ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] Revert virtio_console: Initialize guest_connected=true for rproc_serial
sjur.brandel...@stericsson.com writes: From: Sjur Brændeland sjur.brandel...@stericsson.com This reverts commit 8078db789a92b10ff6e2d713231b5367e014c53b, and adds a lengthy comment explaining the problem area. The reverted patch caused opening of ports to fail for rproc_serial. In probe guest_connected was set to true, but port_fops_open() fails with -EMFILE if guest_connected already is true. Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com Needs Amit's Ack. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: vhost questions.
Michael S. Tsirkin m...@redhat.com writes: On Thu, Mar 14, 2013 at 05:08:22PM +1030, Rusty Russell wrote: Michael S. Tsirkin m...@redhat.com writes: On Wed, Mar 13, 2013 at 05:19:51PM +1030, Rusty Russell wrote: OK, I've been trying to read the vhost and vhost net code, and I'm struggling with basic questions. 1) Why do we allow userspace to change an already-set-up vhost device? Why not have: open() ioctl(VHOST_GET_FEATURES) ioctl(VHOST_SET_VRING) x n (sets num, addresses, kick/call/err fds) ioctl(VHOST_NET_SETUP) ... close() You're not allowed to call things twice: to reset, you close and reopen. That would save a lot of code which I'm not sure is correct anyway. This won't work for the usecases we have in qemu. The way things are setup currently, qemu typically does not have the rights to open any char devices, instead it is passed an fd. Ah, because creating a vhost fd is privileged? Why? Because it creates a kernel thread? Why? Using a model where a userspace process/thread manages the ring (even if it spends 99.99% of its time in the kernel) would avoid this. This is how KVM itself works, it's no longer privileged. KVM kind of has to have a thread per VCPU, no other options. Not at all! It could spin off a helper kernel thread to do the work and come up with some IPC. But there's a damn good reason that it didn't, nor did lguest. That same reasoning applies here, too. The fact that you create a kernel thread then give it the same context and cgroup etc should be the clue! Here I don't think we know what the best threading model is. We used to have a per-CPU thread, switched to thread pair vq *pair* and there are patches to go back to per-CPU threads, they seem to improve scalability though there are still some open issues wrt cgroups. Using a kernel thread means we can control this without touching userspace. But kernelspace is the wrong place to make these decisions. Let's try to imagine what it'd be like if we didn't do it this way, say a /dev/vhost-net2 device. You use an ioctl to set up the memory mapping, the (tun) fd, the location of a xmit ring and a recv ring (you specify one or both... we could have separate xmit and recv devices, but that seems a bit overkill). Now: 1) Simplest case, you want a dumb implementation with no extra threads in your code. Using select(), if tun fd is readable and vhost-net2 fd is writable, call ioctl(VHOST_NET_RECV), which processes as many packets as it can. If tun fd is writable and vhost-net2 fd is readable, call ioctl(VHOST_NET_XMIT). 2) You want an I/O thread in your code? That's easy, too. You might skip the writable fd checks, and just let the ioctl() block. 3) Want separate I/O threads? Separate /dev/vhost-net2 fds, one one only sets the xmit ring, one only sets the recv ring. They just call ioctl() over and over. 4) Multiqueue comes along? It only makes sense with separate threads per queue, so open /dev/vhost-net2 and tunfd once for each thread. Do your per-cpu binding here. Now, there may be issues if we actually coded this, but it *seems* sane. A simple implementation which supports live migration would have to have enough understanding to save and restore the net state. With a tun device (which handles all the header stuff), supporting networking is *trivial*: see lguest (though it needs MRG_RXBUF support). It's about 100 lines. You assume networking is stopped while we migrate but this doesn't work as migration might take a long time. So you have to have the full device, processing rings and sending/receiving packets all in userspace. No I don't: 100 lines of code *is* a full networking device. +* We also trigger polling periodically after each 16 packets +* (the value 16 here is more or less arbitrary, it's tuned to trigger +* less than 10% of times). If you have any suggestions on how to improve this, But this code does *not* wake every 16 packets. It wakes when the reference count hits 16, 32, etc. But that reference count will go up and down as packets are processed: in theory, it might go 0, 1, 2, 3, 2, 3, 2, 3, 2, 3, 2, 3, 2, 3... ? Anyway, what happens when we wake it? I'm assuming it's the net tx worker, which will just return the used bufs to the guest? Wouldn't it be better to do that directly? Maybe wake the tx worker if it needs to wake the guest, if we can't do that here. Sorry I don't get what you mean here. Let's check I've got this straight: Packet is fully xmitted: - skb_release_data - vhost_zerocopy_callback - vhost_poll_queue() Wakes up tx thread - handle_tx_kick - handle_tx - vhost_zerocopy_signal_used - vhost_add_used_and_signal() But what if we kmapped the ring so we could access it in vhost_zerocopy_callback? We could skip the wakeup altogether, right?
Re: [PATCHv2] Revert virtio_console: Initialize guest_connected=true for rproc_serial
On (Wed) 13 Mar 2013 [12:10:48], sjur.brandel...@stericsson.com wrote: From: Sjur Brændeland sjur.brandel...@stericsson.com This reverts commit 8078db789a92b10ff6e2d713231b5367e014c53b, and adds a lengthy comment explaining the problem area. The reverted patch caused opening of ports to fail for rproc_serial. In probe guest_connected was set to true, but port_fops_open() fails with -EMFILE if guest_connected already is true. Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com Acked-by: Amit Shah amit.s...@redhat.com Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization