* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
From: Michael S. Tsirkin @ 2010-02-23 17:32 UTC (permalink / raw)
To: Gleb Natapov
Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, markmc, herbert.xu, quintela, dlaor, avi
In-Reply-To: <20100223173434.GB9834@redhat.com>
On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote:
> On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote:
> > get_user_pages_fast returns number of pages on success, negative value
> > on failure, but never 0. Fix vhost code to match this logic.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vhost/vhost.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index d4f8fdf..d003504 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr)
> > int bit = nr + (log % PAGE_SIZE) * 8;
> > int r;
> > r = get_user_pages_fast(log, 1, 1, &page);
> > - if (r)
> > + if (r < 0)
> > return r;
> > + BUG_ON(r != 1);
> Can't this be easily triggered from user space?
I think no. get_user_pages_fast always returns number of pages
pinned (in this case always 1) or an error (< 0).
Anything else is a kernel bug.
> > base = kmap_atomic(page, KM_USER0);
> > set_bit(bit, base);
> > kunmap_atomic(base, KM_USER0);
> > --
> > 1.7.0.18.g0d53a5
>
> --
> Gleb.
^ permalink raw reply
* virtio: console: Barrier needed before dropping early_put_chars?
From: Amit Shah @ 2010-02-23 17:10 UTC (permalink / raw)
To: borntraeger; +Cc: hch, Virtualization List
Hey Rusty, Christian,
Christoph Hellwig asked why we don't need a barrier before this code in
virtcons_probe():
> + /* Start using the new console output. */
> + early_put_chars = NULL;
> return 0;
Since only s390 uses early_put_chars so far, you'd know why it's not
needed / why we're safe.
Thanks,
Amit
^ permalink raw reply
* [PATCH 3/3] vhost: fix get_user_pages_fast error handling
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, ma
In-Reply-To: <cover.1266943453.git.mst@redhat.com>
get_user_pages_fast returns number of pages on success, negative value
on failure, but never 0. Fix vhost code to match this logic.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d4f8fdf..d003504 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr)
int bit = nr + (log % PAGE_SIZE) * 8;
int r;
r = get_user_pages_fast(log, 1, 1, &page);
- if (r)
+ if (r < 0)
return r;
+ BUG_ON(r != 1);
base = kmap_atomic(page, KM_USER0);
set_bit(bit, base);
kunmap_atomic(base, KM_USER0);
--
1.7.0.18.g0d53a5
^ permalink raw reply related
* [PATCH 2/3] vhost: initialize log eventfd context pointer
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, ma
In-Reply-To: <cover.1266943453.git.mst@redhat.com>
vq log eventfd context pointer needs to be initialized, otherwise
operation may fail or oops if log is enabled but log eventfd not set by
userspace.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c767279..d4f8fdf 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -121,6 +121,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->kick = NULL;
vq->call_ctx = NULL;
vq->call = NULL;
+ vq->log_ctx = NULL;
}
long vhost_dev_init(struct vhost_dev *dev,
--
1.7.0.18.g0d53a5
^ permalink raw reply related
* [PATCH 1/3] vhost: logging math fix
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, ma
In-Reply-To: <cover.1266943453.git.mst@redhat.com>
vhost was dong some complex math to get
offset to log at, and got it wrong by a couple of bytes,
while in fact it's simple: get address where we write,
subtract start of buffer, add log base.
Do it this way.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 6eb1525..c767279 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1004,10 +1004,12 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
if (unlikely(vq->log_used)) {
/* Make sure data is seen before log. */
smp_wmb();
- log_write(vq->log_base, vq->log_addr + sizeof *vq->used->ring *
- (vq->last_used_idx % vq->num),
- sizeof *vq->used->ring);
- log_write(vq->log_base, vq->log_addr, sizeof *vq->used->ring);
+ log_write(vq->log_base,
+ vq->log_addr + ((void *)used - (void *)vq->used),
+ sizeof *used);
+ log_write(vq->log_base,
+ vq->log_addr + offsetof(struct vring_used, idx),
+ sizeof vq->used->idx);
if (vq->log_ctx)
eventfd_signal(vq->log_ctx, 1);
}
--
1.7.0.18.g0d53a5
^ permalink raw reply related
* [PATCH 0/3] vhost: logging fixes
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, gl
The following patches on top of net-next fix issues related to write
logging in vhost. This fixes all known to me logging issues, migration
now works for me while under stress in both TX and RX directions.
Rusty's going on vacation, I am guessing he won't have time to review
this: Gleb, Juan, Herbert, could one of you review this patchset please?
There's also the send queue full issue reported by
Sridhar Samudrala which I'm testing various fixes for,
that patch is contained to vhost/net though,
so there's no conflict, patch will be posted separately.
Michael S. Tsirkin (3):
vhost: logging thinko fix
vhost: initialize log eventfd context pointer
vhost: fix get_user_pages_fast error handling
drivers/vhost/vhost.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
^ permalink raw reply
* [PATCH] virtio-spec: document block CMD and FLUSH
From: Michael S. Tsirkin @ 2010-02-18 22:22 UTC (permalink / raw)
To: Rusty Russell; +Cc: kvm, hch, qemu-devel, Anthony Liguori, virtualization
I took a stub at documenting CMD and FLUSH request types in virtio
block. Christoph, could you look over this please?
I note that the interface seems full of warts to me,
this might be a first step to cleaning them.
One issue I struggled with especially is how type
field mixes bits and non-bit values. I ended up
simply defining all legal values, so that we have
CMD = 2, CMD_OUT = 3 and so on.
I also avoided instroducing inhdr/outhdr structures
that virtio blk driver in linux uses, I was concerned
that nesting tables will confuse the reader.
Comments welcome.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
--
diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index d16104a..ed35893 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -67,7 +67,11 @@ IBM Corporation
\end_layout
\begin_layout Standard
+
+\change_deleted 0 1266531118
FIXME: virtio block scsi passthrough section
+\change_unchanged
+
\end_layout
\begin_layout Standard
@@ -4376,7 +4380,7 @@ struct virtio_net_ctrl_mac {
The device can filter incoming packets by any number of destination MAC
addresses.
\begin_inset Foot
-status open
+status collapsed
\begin_layout Plain Layout
Since there are no guarentees, it can use a hash filter orsilently switch
@@ -4549,6 +4553,22 @@ blk_size
\end_inset
.
+\change_inserted 0 1266444580
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1266471229
+VIRTIO_BLK_F_SCSI (7) Device supports scsi packet commands.
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1266444605
+VIRTIO_BLK_F_FLUSH (9) Cache flush command support.
+\change_unchanged
+
\end_layout
\begin_layout Description
@@ -4700,17 +4720,25 @@ struct virtio_blk_req {
\begin_layout Plain Layout
+\change_deleted 0 1266472188
+
#define VIRTIO_BLK_T_IN 0
\end_layout
\begin_layout Plain Layout
+\change_deleted 0 1266472188
+
#define VIRTIO_BLK_T_OUT 1
\end_layout
\begin_layout Plain Layout
+\change_deleted 0 1266472188
+
#define VIRTIO_BLK_T_BARRIER 0x80000000
+\change_unchanged
+
\end_layout
\begin_layout Plain Layout
@@ -4735,11 +4763,15 @@ struct virtio_blk_req {
\begin_layout Plain Layout
+\change_deleted 0 1266472204
+
#define VIRTIO_BLK_S_OK 0
\end_layout
\begin_layout Plain Layout
+\change_deleted 0 1266472204
+
#define VIRTIO_BLK_S_IOERR 1
\end_layout
@@ -4759,32 +4791,481 @@ struct virtio_blk_req {
\end_layout
\begin_layout Standard
-The type of the request is either a read (VIRTIO_BLK_T_IN) or a write (VIRTIO_BL
-K_T_OUT); the high bit indicates that this request acts as a barrier and
- that all preceeding requests must be complete before this one, and all
- following requests must not be started until this is complete.
+
+\change_inserted 0 1266472490
+If the device has VIRTIO_BLK_F_SCSI feature, it can also support scsi packet
+ command requests, each of these requests is of form:
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472395
+
+struct virtio_scsi_pc_req {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472375
+
+ u32 type;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472375
+
+ u32 ioprio;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266474298
+
+ u64 sector;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266474308
+
+ char cmd[];
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266505809
+
+ char data[][512];
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266505825
+
+#define SCSI_SENSE_BUFFERSIZE 96
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266505848
+
+ u8 sense[SCSI_SENSE_BUFFERSIZE];
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472969
+
+ u32 errors;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472979
+
+ u32 data_len;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472984
+
+ u32 sense_len;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472987
+
+ u32 residual;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472375
+
+ u8 status;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472375
+
+};
+\end_layout
+
+\end_inset
+
+
+\change_unchanged
+
\end_layout
\begin_layout Standard
-The ioprio field is a hint about the relative priorities of requests to
- the device: higher numbers indicate more important requests.
+The
+\emph on
+type
+\emph default
+ of the request is either a read (VIRTIO_BLK_T_IN)
+\change_inserted 0 1266495815
+,
+\change_unchanged
+
+\change_deleted 0 1266495817
+or
+\change_unchanged
+ a write (VIRTIO_BLK_T_OUT)
+\change_inserted 0 1266497316
+, a scsi packet command (VIRTIO_BLK_T_SCSI_CMD or VIRTIO_BLK_T_SCSI_CMD_OUT
+\begin_inset Foot
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266497390
+the SCSI_CMD and SCSI_CMD_OUT types are equivalent, the device does not
+ distinguish between them
+\change_unchanged
+
+\end_layout
+
+\end_inset
+
+) or a flush (VIRTIO_BLK_T_FLUSH or VIRTIO_BLK_T_FLUSH_OUT
+\begin_inset Foot
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266497402
+the FLUSH and FLUSH_OUT types are equivalent, the device does not distinguish
+ between them
+\change_unchanged
+
+\end_layout
+
+\end_inset
+
+)
+\change_deleted 0 1266503753
+;
+\change_inserted 0 1266503758
+.
+
+\change_unchanged
+
+\change_inserted 0 1266497301
+If the device has VIRTIO_BLK_F_BARRIER feature
+\begin_inset space ~
+\end_inset
+
+
+\change_unchanged
+the high bit
+\change_inserted 0 1266497301
+ (VIRTIO_BLK_T_BARRIER)
+\change_unchanged
+ indicates that this request acts as a barrier and that all preceeding requests
+ must be complete before this one, and all following requests must not be
+ started until this is complete.
+
+\change_inserted 0 1266504385
+ Note that a barrier does not flush caches in the underlying backend device
+ in host, and thus does not serve as data consistency guarantee.
+ Driver must use FLUSH request to flush the host cache.
+\change_unchanged
+
\end_layout
\begin_layout Standard
-The sector number indicates the offset (multiplied by 512) where the read
- or write is to occur.
+
+\change_inserted 0 1266472135
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266495783
+
+#define VIRTIO_BLK_T_IN 0
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266495782
+
+#define VIRTIO_BLK_T_OUT 1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266495781
+
+#define VIRTIO_BLK_T_SCSI_CMD 2
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266495799
+
+#define VIRTIO_BLK_T_SCSI_CMD_OUT 3
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266497225
+
+#define VIRTIO_BLK_T_FLUSH 4
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266497237
+
+#define VIRTIO_BLK_T_FLUSH_OUT 5
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266472135
+
+#define VIRTIO_BLK_T_BARRIER 0x80000000
+\end_layout
+
+\end_inset
+
+
\end_layout
\begin_layout Standard
-Note that these first three fields are always read-only: the data field
- is either read-only or write-only, depending on the type of the request.
+The
+\emph on
+ioprio
+\emph default
+ field is a hint about the relative priorities of requests to the device:
+ higher numbers indicate more important requests.
+\end_layout
+
+\begin_layout Standard
+The
+\emph on
+sector
+\emph default
+ number indicates the offset (multiplied by 512) where the read or write
+ is to occur.
+
+\change_inserted 0 1266504683
+ This field is unused and set to 0 for scsi packet commands and for flush
+ commands.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1266527996
+The
+\emph on
+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_unchanged
+
+\end_layout
+
+\begin_layout Standard
+Note that these first three
+\change_inserted 0 1266504407
+ (four for scsi packet commands)
+\change_unchanged
+ fields are always read-only: the
+\emph on
+data
+\emph default
+ field is either read-only or write-only, depending on
+\change_deleted 0 1266505122
+the type of
+\change_unchanged
+ the request.
The size of the read or write can be derived from the total size of the
- request buffer.
+ request buffer
+\change_inserted 0 1266504916
+s
+\change_unchanged
+.
+\change_inserted 0 1266506030
+
\end_layout
\begin_layout Standard
-The final byte is written by the device: either VIRTIO_BLK_S_OK or VIRTIO_BLK_S_
-IOERR.
+
+\change_inserted 0 1266528308
+The
+\emph on
+ sense
+\emph default
+ field is only present for scsi packet command requests, and indicates the
+ buffer for scsi sense data.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1266528658
+The
+\emph on
+data_len
+\emph default
+ field is only present for scsi packet command requests, this field is deprecate
+d, and should be ignored by the driver.
+ Historically, devices copied data length there.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1266528675
+The
+\emph on
+sense_len
+\emph default
+ field is only present for scsi packet command requests and indicates the
+ number of bytes actually written to the
+\emph on
+sense
+\emph default
+ buffer.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1266528717
+The
+\emph on
+residual
+\emph default
+ field is only present for scsi packet command requests and indicates the
+ residual size, calculated as data length - number of bytes actually transferred.
+\change_unchanged
+
+\end_layout
+
+\begin_layout Standard
+The final
+\change_inserted 0 1266471813
+
+\emph on
+status
+\emph default
+
+\change_unchanged
+byte is written by the device: either VIRTIO_BLK_S_OK
+\change_inserted 0 1266528888
+ for success,
+\change_deleted 0 1266528889
+ or
+\change_unchanged
+ VIRTIO_BLK_S_IOERR
+\change_inserted 0 1266529171
+ for host or guest error or VIRTIO_BLK_S_UNSUPP for a request unsupported
+ by host:
+\change_deleted 0 1266471769
+.
+\change_inserted 0 1266471782
+
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266471782
+
+#define VIRTIO_BLK_S_OK 0
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266528846
+
+#define VIRTIO_BLK_S_IOERR 1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1266528863
+
+#define VIRTIO_BLK_S_UNSUPP 2
+\change_unchanged
+
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1266528247
+Historically, devices assumed that the fields
+\emph on
+type
+\emph default
+,
+\emph on
+ioprio
+\emph default
+ and
+\emph on
+sector
+\emph default
+ reside in a single, separate read-only buffer; the fields
+\emph on
+errors
+\emph default
+,
+\emph on
+data_len
+\emph default
+,
+\emph on
+sense_len
+\emph default
+ and
+\emph on
+residual
+\emph default
+ reside in a single, separate write-only buffer; the
+\emph on
+sense
+\emph default
+ field in a separate write-only buffer of size 96 bytes, by itself; the
+ fields
+\emph on
+errors
+\emph default
+,
+\emph on
+data_len
+\emph default
+,
+\emph on
+sense_len
+\emph default
+ and
+\emph on
+residual
+\emph default
+ in a single write-only buffer; and the
+\emph on
+status
+\emph default
+ field is a separate read-only buffer of size 1 byte, by itself.
\end_layout
\begin_layout Chapter*
^ permalink raw reply related
* Re: [PATCH] vhost-net: switch to smp barriers
From: David Miller @ 2010-02-14 19:55 UTC (permalink / raw)
To: mst; +Cc: rusty, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20100213173911.GA5040@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Sat, 13 Feb 2010 19:39:11 +0200
> Dave, I see it's marked "not applicable":
> http://patchwork.ozlabs.org/patch/44207/
> the patch applies to net-next as of
> b3b3f04fb587ecb61b5baa6c1c5f0e666fd12d73.
> Can this be queued up please?
> Should I resubmit with Rusty's ack?
Sorry about that, I must have thought Rusty would queue
it up.
I'll fix the state to under-review and process it in my
backlog.
Thanks.
^ permalink raw reply
* Re: [PATCH] vhost-net: switch to smp barriers
From: Michael S. Tsirkin @ 2010-02-13 17:39 UTC (permalink / raw)
To: Rusty Russell; +Cc: kvm, virtualization, netdev, linux-kernel, David Miller
In-Reply-To: <201002081849.39512.rusty@rustcorp.com.au>
On Mon, Feb 08, 2010 at 06:49:39PM +1030, Rusty Russell wrote:
> On Sun, 7 Feb 2010 07:37:49 pm Michael S. Tsirkin wrote:
> > On Mon, Feb 01, 2010 at 07:21:02PM +0200, Michael S. Tsirkin wrote:
> > > vhost-net only uses memory barriers to control SMP effects
> > > (communication with userspace potentially running on a different CPU),
> > > so it should use SMP barriers and not mandatory barriers for memory
> > > access ordering, as suggested by Documentation/memory-barriers.txt
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > Rusty, any feedback on this one?
> > Thanks!
>
> Yep. barrier() is correct on UP to guard against preemption.
>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Thanks,
> Rusty.
Dave, I see it's marked "not applicable":
http://patchwork.ozlabs.org/patch/44207/
the patch applies to net-next as of
b3b3f04fb587ecb61b5baa6c1c5f0e666fd12d73.
Can this be queued up please?
Should I resubmit with Rusty's ack?
Thanks!
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-spec: document MSI-X
From: Michael S. Tsirkin @ 2010-02-12 10:47 UTC (permalink / raw)
To: Rusty Russell; +Cc: markmc, kvm, qemu-devel, Anthony Liguori, virtualization
In-Reply-To: <201002122017.55536.rusty@rustcorp.com.au>
On Fri, Feb 12, 2010 at 08:17:55PM +1030, Rusty Russell wrote:
> On Fri, 12 Feb 2010 03:52:36 am Michael S. Tsirkin wrote:
> > This documents MSI-X support in virtio.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Wow, great!
>
> I reworked one paragraph for better grammar. Mainly adding "the":
>
> (pseudo-patch):
> Devices report such failures by returning +the+ NO_VECTOR value
> when the relevant Vector field is read. After mapping an event to vector,
> +the+ driver must verify success by reading the Vector field value: on
> success, +the+ previously written value is returned-;-+, and+ on failure,
> NO_VECTOR -value- is returned. If +a+ mapping failure is detected, +the+
> driver can retry mapping with +fewer+-less- vectors, or disable MSI-X.
Looks good, thanks for the corrections!
> I really liked the conversational style: standards can be intimidating and
> unfriendly documents if they concentrate too much on partitioning all
> information into precise sections.
>
> That makes it 0.8.6. I will re-read the entire document for consistency
> before releasing 0.9.
>
> Thanks!
> Rusty.
^ permalink raw reply
* Re: [PATCH 1/6] virtio: Initialize vq->data entries to NULL
From: Rusty Russell @ 2010-02-12 9:58 UTC (permalink / raw)
To: Amit Shah; +Cc: Shirley Ma, virtualization
In-Reply-To: <1265950939-12359-2-git-send-email-amit.shah@redhat.com>
On Fri, 12 Feb 2010 03:32:14 pm Amit Shah wrote:
> vq operations depend on vq->data[i] being NULL to figure out if the vq
> entry is in use.
>
> We have to initialize them to NULL to ensure we don't work with junk
> data and trigger false BUG_ONs.
Not really. We wouldn't detect some bogus get_buf() calls if it's not NULL,
but we only *need* this since detach_bufs was added.
I've updated the commit message to reflect it.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-spec: document MSI-X
From: Rusty Russell @ 2010-02-12 9:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: markmc, kvm, qemu-devel, Anthony Liguori, virtualization
In-Reply-To: <20100211172236.GA20357@redhat.com>
On Fri, 12 Feb 2010 03:52:36 am Michael S. Tsirkin wrote:
> This documents MSI-X support in virtio.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Wow, great!
I reworked one paragraph for better grammar. Mainly adding "the":
(pseudo-patch):
Devices report such failures by returning +the+ NO_VECTOR value
when the relevant Vector field is read. After mapping an event to vector,
+the+ driver must verify success by reading the Vector field value: on
success, +the+ previously written value is returned-;-+, and+ on failure,
NO_VECTOR -value- is returned. If +a+ mapping failure is detected, +the+
driver can retry mapping with +fewer+-less- vectors, or disable MSI-X.
I really liked the conversational style: standards can be intimidating and
unfriendly documents if they concentrate too much on partitioning all
information into precise sections.
That makes it 0.8.6. I will re-read the entire document for consistency
before releasing 0.9.
Thanks!
Rusty.
^ permalink raw reply
* Re: [PATCH 0/6] virtio: console: Fixes
From: Amit Shah @ 2010-02-12 9:07 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization
In-Reply-To: <201002121934.58477.rusty@rustcorp.com.au>
On (Fri) Feb 12 2010 [19:34:58], Rusty Russell wrote:
> On Fri, 12 Feb 2010 03:32:13 pm Amit Shah wrote:
> > Hey Rusty,
> >
> > Here are a few fixes for virtio and virtio_console.
> >
> > The first patch ensures the data elements of vqs are properly
> > initialised at allocation-time so that we don't trigger BUG_ONs. I found
> > this when hot-unplugging ports and there was just one unused buffer.
> > detach_unused_buffers() kept returning pointers that were invalid. I
> > didn't catch this earlier as I had the in_vq filled completely.
> >
> > Patches 2, 4 and 5 can be folded into the series as they are bugfixes
> > for the functionality present there.
> >
> > About patch 5: When running a test that transfers a 260M file from the
> > host to the guest, qemu-kvm.git takes 17m with a single outstanding
> > buffer in the in_vq vs. 1m when the entire in_vq is filled. This is a
> > bug in qemu-kvm.git's scheduling, but since it's a big difference and
> > not much change involved, we could merge this now.
> >
> > Comments?
> >
> > If these patches are favourable, I could send you a tarball in private
> > so that the bugfixes are folded in the series and just patches 1, 3 and
> > 6 are added.
>
> I prefer to fold them myself, after they've spent some time in linux-next.
Fine by me.
BTW should patch 1 be considered for stable?
Thanks!
Amit
^ permalink raw reply
* Re: [PATCH 0/6] virtio: console: Fixes
From: Rusty Russell @ 2010-02-12 9:04 UTC (permalink / raw)
To: Amit Shah; +Cc: virtualization
In-Reply-To: <1265950939-12359-1-git-send-email-amit.shah@redhat.com>
On Fri, 12 Feb 2010 03:32:13 pm Amit Shah wrote:
> Hey Rusty,
>
> Here are a few fixes for virtio and virtio_console.
>
> The first patch ensures the data elements of vqs are properly
> initialised at allocation-time so that we don't trigger BUG_ONs. I found
> this when hot-unplugging ports and there was just one unused buffer.
> detach_unused_buffers() kept returning pointers that were invalid. I
> didn't catch this earlier as I had the in_vq filled completely.
>
> Patches 2, 4 and 5 can be folded into the series as they are bugfixes
> for the functionality present there.
>
> About patch 5: When running a test that transfers a 260M file from the
> host to the guest, qemu-kvm.git takes 17m with a single outstanding
> buffer in the in_vq vs. 1m when the entire in_vq is filled. This is a
> bug in qemu-kvm.git's scheduling, but since it's a big difference and
> not much change involved, we could merge this now.
>
> Comments?
>
> If these patches are favourable, I could send you a tarball in private
> so that the bugfixes are folded in the series and just patches 1, 3 and
> 6 are added.
I prefer to fold them myself, after they've spent some time in linux-next.
Thanks!
Rusty.
^ permalink raw reply
* [PATCH 6/6] Add MAINTAINERS entry for virtio_console
From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw)
To: rusty; +Cc: Amit Shah, virtualization
In-Reply-To: <1265950939-12359-6-git-send-email-amit.shah@redhat.com>
I'm taking ownership of the virtio_console module; but I'll continue
feeding patches via Rusty.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
MAINTAINERS | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 03f38c1..3118dfa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2394,6 +2394,12 @@ L: linuxppc-dev@ozlabs.org
S: Odd Fixes
F: drivers/char/hvc_*
+VIRTIO CONSOLE DRIVER
+M: Amit Shah <amit.shah@redhat.com>
+L: virtualization@lists.linux-foundation.org
+S: Maintained
+F: drivers/char/virtio_console.c
+
GSPCA FINEPIX SUBDRIVER
M: Frank Zago <frank@zago.net>
L: linux-media@vger.kernel.org
--
1.6.2.5
^ permalink raw reply related
* [PATCH 5/6] virtio: console: Fill ports' entire in_vq with buffers
From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw)
To: rusty; +Cc: Amit Shah, virtualization
In-Reply-To: <1265950939-12359-5-git-send-email-amit.shah@redhat.com>
Instead of allocating just one buffer for a port's in_vq, fill
the entire in_vq with buffers so the host need not stall while
an application consumes the data and makes the buffer available
again for the host.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 53 ++++++++++++++++++++++++----------------
1 files changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c407037..213373b 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -330,6 +330,7 @@ static void discard_port_data(struct port *port)
struct port_buffer *buf;
struct virtqueue *vq;
unsigned int len;
+ int ret;
vq = port->in_vq;
if (port->inbuf)
@@ -337,16 +338,18 @@ static void discard_port_data(struct port *port)
else
buf = vq->vq_ops->get_buf(vq, &len);
- if (!buf)
- return;
-
- if (add_inbuf(vq, buf) < 0) {
- buf->len = buf->offset = 0;
- dev_warn(port->dev, "Error adding buffer back to vq\n");
- return;
+ ret = 0;
+ while (buf) {
+ if (add_inbuf(vq, buf) < 0) {
+ ret++;
+ free_buf(buf);
+ }
+ buf = vq->vq_ops->get_buf(vq, &len);
}
-
port->inbuf = NULL;
+ if (ret)
+ dev_warn(port->dev, "Errors adding %d buffers back to vq\n",
+ ret);
}
static bool port_has_data(struct port *port)
@@ -354,12 +357,19 @@ static bool port_has_data(struct port *port)
unsigned long flags;
bool ret;
- ret = false;
spin_lock_irqsave(&port->inbuf_lock, flags);
- if (port->inbuf)
+ if (port->inbuf) {
ret = true;
+ goto out;
+ }
+ port->inbuf = get_inbuf(port);
+ if (port->inbuf) {
+ ret = true;
+ goto out;
+ }
+ ret = false;
+out:
spin_unlock_irqrestore(&port->inbuf_lock, flags);
-
return ret;
}
@@ -1011,7 +1021,8 @@ static void in_intr(struct virtqueue *vq)
return;
spin_lock_irqsave(&port->inbuf_lock, flags);
- port->inbuf = get_inbuf(port);
+ if (!port->inbuf)
+ port->inbuf = get_inbuf(port);
/*
* Don't queue up data when port is closed. This condition
@@ -1087,7 +1098,7 @@ static int add_port(struct ports_device *portdev, u32 id)
{
char debugfs_name[16];
struct port *port;
- struct port_buffer *inbuf;
+ struct port_buffer *buf;
dev_t devt;
int err;
@@ -1132,22 +1143,21 @@ static int add_port(struct ports_device *portdev, u32 id)
spin_lock_init(&port->inbuf_lock);
init_waitqueue_head(&port->waitqueue);
- inbuf = alloc_buf(PAGE_SIZE);
- if (!inbuf) {
+ /* Fill the in_vq with buffers so the host can send us data. */
+ err = fill_queue(port->in_vq, &port->inbuf_lock);
+ if (!err) {
+ dev_err(port->dev, "Error allocating inbufs\n");
err = -ENOMEM;
goto free_device;
}
- /* Register the input buffer the first time. */
- add_inbuf(port->in_vq, inbuf);
-
/*
* If we're not using multiport support, this has to be a console port
*/
if (!use_multiport(port->portdev)) {
err = init_port_console(port);
if (err)
- goto free_inbuf;
+ goto free_inbufs;
}
spin_lock_irq(&portdev->ports_lock);
@@ -1175,8 +1185,9 @@ static int add_port(struct ports_device *portdev, u32 id)
}
return 0;
-free_inbuf:
- free_buf(inbuf);
+free_inbufs:
+ while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
+ free_buf(buf);
free_device:
device_destroy(pdrvdata.class, port->dev->devt);
free_cdev:
--
1.6.2.5
^ permalink raw reply related
* [PATCH 4/6] virtio: console: Error out if we can't allocate buffers for control queue
From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw)
To: rusty; +Cc: Amit Shah, virtualization
In-Reply-To: <1265950939-12359-4-git-send-email-amit.shah@redhat.com>
With MULTIPORT support, the control queue is an integral part of the
functioning of the device. If we can't get any buffers allocated, the
host won't be able to relay important information and the device may not
function as intended.
Ensure 'probe' doesn't succeed until the control queue has at least one
buffer allocated for its ivq.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 27 +++++++++++++++++++++------
1 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0057bae..c407037 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1057,25 +1057,30 @@ static void config_intr(struct virtio_device *vdev)
resize_console(find_port_by_id(portdev, 0));
}
-static void fill_queue(struct virtqueue *vq, spinlock_t *lock)
+static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
{
struct port_buffer *buf;
- int ret;
+ unsigned int ret;
+ int err;
+ ret = 0;
do {
buf = alloc_buf(PAGE_SIZE);
if (!buf)
break;
spin_lock_irq(lock);
- ret = add_inbuf(vq, buf);
- if (ret < 0) {
+ err = add_inbuf(vq, buf);
+ if (err < 0) {
spin_unlock_irq(lock);
free_buf(buf);
break;
}
+ ret++;
spin_unlock_irq(lock);
- } while (ret > 0);
+ } while (err > 0);
+
+ return ret;
}
static int add_port(struct ports_device *portdev, u32 id)
@@ -1430,7 +1435,13 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
INIT_WORK(&portdev->control_work, &control_work_handler);
INIT_WORK(&portdev->config_work, &config_work_handler);
- fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+ err = fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+ if (!err) {
+ dev_err(&vdev->dev,
+ "Error allocating buffers for control queue\n");
+ err = -ENOMEM;
+ goto free_vqs;
+ }
}
for (i = 0; i < portdev->config.nr_ports; i++)
@@ -1440,6 +1451,10 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
early_put_chars = NULL;
return 0;
+free_vqs:
+ vdev->config->del_vqs(vdev);
+ kfree(portdev->in_vqs);
+ kfree(portdev->out_vqs);
free_chrdev:
unregister_chrdev(portdev->chr_major, "virtio-portsdev");
free:
--
1.6.2.5
^ permalink raw reply related
* [PATCH 3/6] virtio: console: Add ability to remove module
From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw)
To: rusty; +Cc: Amit Shah, virtualization
In-Reply-To: <1265950939-12359-3-git-send-email-amit.shah@redhat.com>
Add the ability to remove the virtio_console module.
This aids debugging.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 41 insertions(+), 0 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 69d2e61..0057bae 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1448,6 +1448,36 @@ fail:
return err;
}
+static void virtcons_remove(struct virtio_device *vdev)
+{
+ struct ports_device *portdev;
+ struct port *port, *port2;
+ struct port_buffer *buf;
+ unsigned int len;
+
+ portdev = vdev->priv;
+
+ cancel_work_sync(&portdev->control_work);
+ cancel_work_sync(&portdev->config_work);
+
+ list_for_each_entry_safe(port, port2, &portdev->ports, list)
+ remove_port(port);
+
+ unregister_chrdev(portdev->chr_major, "virtio-portsdev");
+
+ while ((buf = portdev->c_ivq->vq_ops->get_buf(portdev->c_ivq, &len)))
+ free_buf(buf);
+
+ while ((buf = portdev->c_ivq->vq_ops->detach_unused_buf(portdev->c_ivq)))
+ free_buf(buf);
+
+ vdev->config->del_vqs(vdev);
+ kfree(portdev->in_vqs);
+ kfree(portdev->out_vqs);
+
+ kfree(portdev);
+}
+
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_CONSOLE, VIRTIO_DEV_ANY_ID },
{ 0 },
@@ -1465,6 +1495,7 @@ static struct virtio_driver virtio_console = {
.driver.owner = THIS_MODULE,
.id_table = id_table,
.probe = virtcons_probe,
+ .remove = virtcons_remove,
.config_changed = config_intr,
};
@@ -1488,7 +1519,17 @@ static int __init init(void)
return register_virtio_driver(&virtio_console);
}
+
+static void __exit fini(void)
+{
+ unregister_virtio_driver(&virtio_console);
+
+ class_destroy(pdrvdata.class);
+ if (pdrvdata.debugfs_dir)
+ debugfs_remove_recursive(pdrvdata.debugfs_dir);
+}
module_init(init);
+module_exit(fini);
MODULE_DEVICE_TABLE(virtio, id_table);
MODULE_DESCRIPTION("Virtio console driver");
--
1.6.2.5
^ permalink raw reply related
* [PATCH 2/6] virtio: console: Ensure no memleaks in case of unused buffers
From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw)
To: rusty; +Cc: Amit Shah, virtualization
In-Reply-To: <1265950939-12359-2-git-send-email-amit.shah@redhat.com>
If unused data exists in in_vq, ensure we flush that first and then
detach unused buffers, which will ensure all buffers from the in_vq are
removed.
Also ensure we free the buffers after detaching them.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index a3f1f73..69d2e61 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -838,6 +838,8 @@ static const struct file_operations port_debugfs_ops = {
/* Remove all port-specific data. */
static int remove_port(struct port *port)
{
+ struct port_buffer *buf;
+
spin_lock_irq(&port->portdev->ports_lock);
list_del(&port->list);
spin_unlock_irq(&port->portdev->ports_lock);
@@ -851,14 +853,17 @@ static int remove_port(struct port *port)
if (port->guest_connected)
send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
- while (port->in_vq->vq_ops->detach_unused_buf(port->in_vq))
- ;
-
sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
device_destroy(pdrvdata.class, port->dev->devt);
cdev_del(&port->cdev);
+ /* Remove unused data this port might have received. */
discard_port_data(port);
+
+ /* Remove buffers we queued up for the Host to send us data in. */
+ while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
+ free_buf(buf);
+
kfree(port->name);
debugfs_remove(port->debugfs_file);
--
1.6.2.5
^ permalink raw reply related
* [PATCH 1/6] virtio: Initialize vq->data entries to NULL
From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw)
To: rusty; +Cc: Amit Shah, virtualization
In-Reply-To: <1265950939-12359-1-git-send-email-amit.shah@redhat.com>
vq operations depend on vq->data[i] being NULL to figure out if the vq
entry is in use.
We have to initialize them to NULL to ensure we don't work with junk
data and trigger false BUG_ONs.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/virtio/virtio_ring.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71929ee..9bcfe95 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -431,8 +431,11 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
/* Put everything in free lists. */
vq->num_free = num;
vq->free_head = 0;
- for (i = 0; i < num-1; i++)
+ for (i = 0; i < num-1; i++) {
vq->vring.desc[i].next = i+1;
+ vq->data[i] = NULL;
+ }
+ vq->data[i] = NULL;
return &vq->vq;
}
--
1.6.2.5
^ permalink raw reply related
* [PATCH 0/6] virtio: console: Fixes
From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw)
To: rusty; +Cc: Amit Shah, virtualization
Hey Rusty,
Here are a few fixes for virtio and virtio_console.
The first patch ensures the data elements of vqs are properly
initialised at allocation-time so that we don't trigger BUG_ONs. I found
this when hot-unplugging ports and there was just one unused buffer.
detach_unused_buffers() kept returning pointers that were invalid. I
didn't catch this earlier as I had the in_vq filled completely.
Patches 2, 4 and 5 can be folded into the series as they are bugfixes
for the functionality present there.
About patch 5: When running a test that transfers a 260M file from the
host to the guest, qemu-kvm.git takes 17m with a single outstanding
buffer in the in_vq vs. 1m when the entire in_vq is filled. This is a
bug in qemu-kvm.git's scheduling, but since it's a big difference and
not much change involved, we could merge this now.
Comments?
If these patches are favourable, I could send you a tarball in private
so that the bugfixes are folded in the series and just patches 1, 3 and
6 are added.
Thanks,
Amit.
Amit Shah (6):
virtio: Initialize vq->data entries to NULL
virtio: console: Ensure no memleaks in case of unused buffers
virtio: console: Add ability to remove module
virtio: console: Error out if we can't allocate buffers for control
queue
virtio: console: Fill ports' entire in_vq with buffers
Add MAINTAINERS entry for virtio_console
MAINTAINERS | 6 ++
drivers/char/virtio_console.c | 132 +++++++++++++++++++++++++++++++---------
drivers/virtio/virtio_ring.c | 5 +-
3 files changed, 112 insertions(+), 31 deletions(-)
^ permalink raw reply
* [PATCH] virtio-spec: document MSI-X
From: Michael S. Tsirkin @ 2010-02-11 17:22 UTC (permalink / raw)
To: rusty, virtualization; +Cc: markmc, qemu-devel, Anthony Liguori, kvm
This documents MSI-X support in virtio.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
virtio-spec.lyx | 358 +++++++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 332 insertions(+), 26 deletions(-)
diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 49ed612..d16104a 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -1,4 +1,4 @@
-#LyX 1.6.4 created this file. For more info see http://www.lyx.org/
+#LyX 1.6.5 created this file. For more info see http://www.lyx.org/
\lyxformat 345
\begin_document
\begin_header
@@ -35,9 +35,8 @@
\papersides 1
\paperpagestyle default
\tracking_changes true
-\output_changes true
-\author ""
-\author ""
+\output_changes false
+\author "Michael S. Tsirkin"
\author ""
\end_header
@@ -72,7 +71,11 @@ FIXME: virtio block scsi passthrough section
\end_layout
\begin_layout Standard
+
+\change_deleted 0 1265908736
FIXME: MSI-X documentation
+\change_unchanged
+
\end_layout
\begin_layout Chapter
@@ -590,8 +593,11 @@ The DRIVER status bit is set: we know how to drive the device.
\begin_layout Enumerate
Device-specific setup, including reading the Device Feature Bits, discovery
- of virtqueues for the device, and reading and possibly writing the virtio
- configuration space.
+ of virtqueues for the device,
+\change_inserted 0 1265905891
+optional MSI-X setup,
+\change_unchanged
+and reading and possibly writing the virtio configuration space.
\end_layout
\begin_layout Enumerate
@@ -636,7 +642,7 @@ Virtio Header
\begin_layout Standard
\begin_inset Tabular
-<lyxtabular version="3" rows="4" columns="10">
+<lyxtabular version="3" rows="4" columns="12">
<features>
<column alignment="left" valignment="top" width="0">
<column alignment="left" valignment="top" width="0">
@@ -648,6 +654,8 @@ Virtio Header
<column alignment="left" valignment="top" width="0">
<column alignment="left" valignment="top" width="0">
<column alignment="left" valignment="top" width="0">
+<column alignment="left" valignment="top" width="0">
+<column alignment="left" valignment="top" width="0">
<row>
<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
\begin_inset Text
@@ -730,6 +738,28 @@ Bits
\end_inset
</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265895519
+16 (optional)
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265895525
+16 (optional)
+\end_layout
+
+\end_inset
+</cell>
<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
\begin_inset Text
@@ -822,6 +852,28 @@ R
\end_inset
</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265895422
+R+W
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265895531
+R+W
+\end_layout
+
+\end_inset
+</cell>
<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
\begin_inset Text
@@ -930,6 +982,28 @@ ISR
\end_inset
</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265895579
+Configuration
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265895618
+Queue
+\end_layout
+
+\end_inset
+</cell>
<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
\begin_inset Text
@@ -1040,6 +1114,28 @@ Status
\end_inset
</cell>
+<cell alignment="center" valignment="top" bottomline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265895695
+Vector
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" bottomline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265895623
+Vector
+\end_layout
+
+\end_inset
+</cell>
<cell alignment="center" valignment="top" bottomline="true" leftline="true" rightline="true" usebox="none">
\begin_inset Text
@@ -1181,6 +1277,88 @@ This allows for forwards and backwards compatibility: if the device is enhanced
support, it will not see that feature bit in the Device Features field
and can go into backwards compatibility mode (or, for poor implementations,
set the FAILED Device Status bit).
+\change_inserted 0 1265896046
+
+\end_layout
+
+\begin_layout Subsubsection
+
+\change_inserted 0 1265896301
+Configuration/Queue Vectors
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1265908336
+When MSI-X capability is present and enabled in the device (through standard
+ PCI configuration space) 4 bytes at byte offset 20 are used to map configuratio
+n change and queue interrupts to MSI-X vectors.
+ In this case, the ISR Status field is unused, and device specific configuration
+ starts at byte offset 24 in virtio header structure.
+ When MSI-X capability is not enabled, device specific configuration starts
+ at byte offset 20 in virtio header.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1265907969
+Writing a valid MSI-X Table entry number, 0 to 0x7FF, to one of Configuration/Qu
+eue Vector registers,
+\emph on
+maps
+\emph default
+ interrupts triggered by the configuration change/selected queue events
+ respectively to the corresponding MSI-X vector.
+ To disable interrupts for a specific event type, unmap it by writing a
+ special NO_VECTOR value:
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1265902253
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265902147
+
+/* Vector value used to disable MSI for queue */
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1265902136
+
+#define VIRTIO_MSI_NO_VECTOR 0xffff
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1265905829
+Reading these registers returns vector mapped to a given event, or NO_VECTOR
+ if unmapped.
+ All queue and configuration change events are unmapped by default.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1265907870
+Note that mapping an event to vector might require allocating internal device
+ resources, and might fail.
+ Devices report such failures by returning NO_VECTOR value when the relevant
+ Vector field is read.
+ After mapping an event to vector, driver must verify success by reading
+ the Vector field valueon success, previously written value is returned;
+ on failure, NO_VECTOR value is returned.
+ If mapping failure is detected, driver can retry mapping with less vectors,
+ or disable MSI-X.
\end_layout
\begin_layout Section
@@ -1224,6 +1402,19 @@ The 4096 is based on the x86 page size, but it's also large enough to ensure
\end_inset
+\change_inserted 0 1265902802
+
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 0 1265907664
+Optionally, if MSI-X capability is present and enabled on the device, select
+ a vector to use to request interrupts triggered by virtqueue events.
+ Write the MSI-X Table entry number corresponding to this vector in Queue
+ Vector field.
+ Read the Queue Vector field: on success, previously written value is returned;
+ on failure, NO_VECTOR value is returned.
\end_layout
\begin_layout Standard
@@ -2107,6 +2298,17 @@ Update the used ring idx.
\begin_layout Enumerate
If the VRING_AVAIL_F_NO_INTERRUPT flag is not set in avail->flags:
+\change_inserted 0 1265903387
+
+\end_layout
+
+\begin_deeper
+\begin_layout Enumerate
+
+\change_inserted 0 1265903435
+If MSI-X capability is disabled:
+\change_unchanged
+
\end_layout
\begin_deeper
@@ -2116,16 +2318,66 @@ Set the lower bit of the ISR Status field for the device.
\begin_layout Enumerate
Send the appropriate PCI interrupt for the device.
+\change_inserted 0 1265904154
+
\end_layout
\end_deeper
+\begin_layout Enumerate
+
+\change_inserted 0 1265903452
+If MSI-X capability is enabled:
+\end_layout
+
+\begin_deeper
+\begin_layout Enumerate
+
+\change_inserted 0 1265907522
+Request the appropriate MSI-X interrupt message for the device, Queue Vector
+ field sets the MSI-X Table entry number.
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 0 1265907541
+If Queue Vector field value is NO_VECTOR, no interrupt message is requested
+ for this event.
+\change_unchanged
+
+\end_layout
+
+\end_deeper
+\end_deeper
\begin_layout Standard
-The guest interrupt handler should read the ISR Status field, which will
- reset it to zero.
+The guest interrupt handler should
+\change_inserted 0 1265904434
+:
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 0 1265904449
+If MSI-X capability is disabled:
+\change_deleted 0 1265904425
+
+\change_unchanged
+read the ISR Status field, which will reset it to zero.
If the lower bit is zero, the interrupt was not for this device.
Otherwise, the guest driver should look through the used rings of each
virtqueue for the device, to see if any progress has been made by the device
which requires servicing.
+\change_inserted 0 1265904489
+
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 0 1265904546
+If MSI-X capability is enabled: look through the used rings of each virtqueue
+ mapped to the specific MSI-X vector for the device, to see if any progress
+ has been made by the device which requires servicing.
+\change_unchanged
+
\end_layout
\begin_layout Standard
@@ -2170,12 +2422,23 @@ Dealing With Configuration Changes
\begin_layout Standard
Some virtio PCI devices can change the device configuration state, as reflected
in the virtio header in the PCI configuration space.
- In this case, an interrupt is delivered and the second highest bit is set
- in the ISR Status field to indicate that the driver should re-examine the
- configuration space.
+ In this case
+\change_inserted 0 1265904732
+:
\end_layout
-\begin_layout Standard
+\begin_layout Enumerate
+
+\change_inserted 0 1265904810
+If MSI-X capability is disabled:
+\change_deleted 0 1265904811
+,
+\change_unchanged
+ an interrupt is delivered and the second highest bit is set in the ISR
+ Status field to indicate that the driver should re-examine the configuration
+ space.
+\change_deleted 0 1265905023
+
\begin_inset listings
inline false
status open
@@ -2188,12 +2451,31 @@ status open
\end_inset
+\change_inserted 0 1265905350
+Note that a single interrupt can indicate both that one or more virtqueue
+ has been used and that the configuration space has changed: even if the
+ config bit is set, virtqueues must be scanned.
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 0 1265907476
+If MSI-X capability is enabled: an interrupt message is requested.
+ The Configuration Vector field sets the MSI-X Table entry number to use.
+ If Configuration Vector field value is NO_VECTOR, no interrupt message
+ is requested for this event.
+\change_unchanged
+
\end_layout
\begin_layout Standard
+
+\change_deleted 0 1265905342
Note that a single interrupt can indicate both that one or more virtqueue
has been used and that the configuration space has changed: even if the
config bit is set, virtqueues must be scanned.
+\change_inserted 0 1265905057
+
\end_layout
\begin_layout Chapter
@@ -2259,6 +2541,30 @@ Meanwhile for experimental drivers, use 65535 and work backwards.
\end_layout
\begin_layout Section*
+
+\change_inserted 0 1265906688
+How many MSI-X vectors?
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1265907268
+Using the optional MSI-X capability devices can speed up interrupt processing
+ by removing the need to read ISR Status register by guest driver (which
+ might be an expensive operation), reducing interrupt sharing between devices
+ and queues within the device, and handling interrupts from multiple CPUs.
+ However, some systems impose a limit (which might be as low as 256) on
+ the total number of MSI-X vectors that can be allocated to all devices.
+ Devices and/or device drivers should take this into account, limiting the
+ number of vectors used unless the device is expected to cause a high volume
+ of interrupts.
+ Devices can control the number of vectors used by limiting the MSI-X Table
+ Size or not presenting MSI-X capability in PCI configuration space.
+ Drivers can control this by mapping events to as small number of vectors
+ as possible, or disabling MSI-X capability altogether.
+\end_layout
+
+\begin_layout Section*
Message Framing
\end_layout
@@ -2276,7 +2582,7 @@ The descriptors used for a buffer should not effect the semantics of the
In particular, no implementation should use the descriptor boundaries to
determine the size of any header in a request.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
The current qemu device implementations mistakenly insist that the first
@@ -2298,7 +2604,7 @@ Any change to configuration space, or new virtqueues, or behavioural changes,
should be indicated be negotiation of a new feature bit.
This establishes clarity
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
Even if it does mean documenting design or implementation mistakes!
@@ -3092,7 +3398,7 @@ Virtqueues 0:receiveq.
1:transmitq.
2:controlq
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
Only if VIRTIO_NET_F_CTRL_VQ set
@@ -3143,7 +3449,7 @@ VIRTIO_NET_F_GSO
(6) (Deprecated) device handles packets with any GSO type.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
It was supposed to indicate segmentation offload support, but upon further
@@ -3412,7 +3718,7 @@ This is a common restriction in real, older network cards.
The converse features are also available: a driver can save the virtual
device some work by negotiating these features.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
For example, a network packet transported between two guests on the same
@@ -3576,7 +3882,7 @@ csum_start is set to the offset within the packet to begin checksumming,
csum_offset indicates how many bytes after the csum_start the new (16 bit
ones' complement) checksum should be placed.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
For example, consider a partially checksummed TCP (IPv4) packet.
@@ -3653,7 +3959,7 @@ gso_type
as well, indicating that the TCP packet has the ECN bit set.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
This case is not handled by some older hardware, so is called out specifically
@@ -3682,7 +3988,7 @@ reference "sub:Notifying-The-Device"
).
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
Note that the header will be two bytes longer for the VIRTIO_NET_F_MRG_RXBUF
@@ -4070,7 +4376,7 @@ struct virtio_net_ctrl_mac {
The device can filter incoming packets by any number of destination MAC
addresses.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
Since there are no guarentees, it can use a hash filter orsilently switch
@@ -4633,7 +4939,7 @@ Device Operation
\begin_layout Enumerate
For output, a buffer containing the characters is placed in the port's transmitq.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
Because this is high importance and low bandwidth, the current Linux implementat
@@ -4843,7 +5149,7 @@ Virtqueues 0:inflateq.
1:deflateq.
2:statsq.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
Only if VIRTIO_BALLON_F_STATS_VQ set
@@ -5001,7 +5307,7 @@ To supply memory to the balloon (aka.
The driver constructs an array of addresses of unused memory pages.
These addresses are divided by 4096
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
This is historical, and independent of the guest page size
@@ -5062,7 +5368,7 @@ actual
field of the configuration should be updated to reflect the new number
of pages in the balloon.
\begin_inset Foot
-status collapsed
+status open
\begin_layout Plain Layout
As updates to configuration space are not atomic, this field isn't particularly
--
1.6.6.144.g5c3af
^ permalink raw reply related
* Re: [PATCH] virtio-spec: document indirect descriptors
From: Rusty Russell @ 2010-02-10 23:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: markmc, kvm, qemu-devel, Anthony Liguori, virtualization
In-Reply-To: <20100210130026.GA19271@redhat.com>
On Wed, 10 Feb 2010 11:30:39 pm Michael S. Tsirkin wrote:
> Add documentation for indirect descriptors
Thanks, that's awesome!
I added an entry to the Reserved Feature table in appendix B, and applied
it. We're now at 0.8.5.
Cheers,
Rusty.
^ permalink raw reply
* Re: Paravirt compile failure with gcc33
From: H. Peter Anvin @ 2010-02-10 21:08 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Linux Kernel Mailing List, virtualization, Ingo Molnar,
Thomas Gleixner
In-Reply-To: <alpine.LSU.2.01.1002102019430.6708@obet.zrqbmnf.qr>
On 02/10/2010 11:20 AM, Jan Engelhardt wrote:
> On Wednesday 2010-02-10 19:38, Jeremy Fitzhardinge wrote:
>
>> On 02/10/2010 10:13 AM, Jan Engelhardt wrote:
>>> As I was compile-testing 2.6.33-rc with gcc-3.3,
>>> binutils-2.19.51-10.26.4.x86_64, I observed a failure when
>>> CONFIG_PARAVIRT is turned on:
>>>
>>
>> Yeah, there's a gcc bug of some kind there, and its very hard to see how to
>> work around it. When we last discussed this, I think we were close to deciding
>> to obsolete gcc 3.3.
>>
>> HPA, do you remember?
>
> In fact, there's such a big bug in there that I'll get a runaway
> allocation later on when kvm.o is to be compiled. This is getting fun,
> but I am absolutely fine with obsoleting 3.x :)
I have heard of some people using 3.4, but I'm not sure if anyone cares
for current kernels and for x86, in particular. Some other
architectures might still be stuck on 3.x.
For x86 in particular 3.x support is becoming more of a headache than a
help.
-hpa
^ permalink raw reply
* Re: Paravirt compile failure with gcc33
From: Jan Engelhardt @ 2010-02-10 19:20 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Jeremy Fitzhardinge, virtualization, Linux Kernel Mailing List,
H. Peter Anvin
In-Reply-To: <4B72FD09.1020202@goop.org>
On Wednesday 2010-02-10 19:38, Jeremy Fitzhardinge wrote:
> On 02/10/2010 10:13 AM, Jan Engelhardt wrote:
>> As I was compile-testing 2.6.33-rc with gcc-3.3,
>> binutils-2.19.51-10.26.4.x86_64, I observed a failure when
>> CONFIG_PARAVIRT is turned on:
>>
>
> Yeah, there's a gcc bug of some kind there, and its very hard to see how to
> work around it. When we last discussed this, I think we were close to deciding
> to obsolete gcc 3.3.
>
> HPA, do you remember?
In fact, there's such a big bug in there that I'll get a runaway
allocation later on when kvm.o is to be compiled. This is getting fun,
but I am absolutely fine with obsoleting 3.x :)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox