* Re: Using PCI config space to indicate config location
From: Michael S. Tsirkin @ 2012-10-10 8:30 UTC (permalink / raw)
To: Rusty Russell; +Cc: Anthony Liguori, qemu-devel, kvm, virtualization
In-Reply-To: <87y5jhpuu2.fsf@rustcorp.com.au>
On Mon, Oct 08, 2012 at 12:51:25PM +1030, Rusty Russell wrote:
> (Topic updated, cc's trimmed).
>
> Anthony Liguori <aliguori@us.ibm.com> writes:
> > Rusty Russell <rusty@rustcorp.com.au> writes:
> >> 4) The only significant change to the spec is that we use PCI
> >> capabilities, so we can have infinite feature bits.
> >> (see http://lists.linuxfoundation.org/pipermail/virtualization/2011-December/019198.html)
> >
> > We discussed this on IRC last night. I don't think PCI capabilites are
> > a good mechanism to use...
> >
> > PCI capabilities are there to organize how the PCI config space is
> > allocated to allow vendor extensions to co-exist with future PCI
> > extensions.
> >
> > But we've never used the PCI config space within virtio-pci. We do
> > everything in BAR0. I don't think there's any real advantage of using
> > the config space vs. a BAR for virtio-pci.
>
> Note before anyone gets confused; we were talking about using the PCI
> config space to indicate what BAR(s) the virtio stuff is in. An
> alternative would be to simply specify a new layout format in BAR1.
One problem we are still left with is this: device specific
config accesses are still non atomic.
This is a problem for multibyte fields such as MAC address
where MAC could change while we are accessing it.
I was thinking about some backwards compatible way to solve this, but if
we are willing to break compatiblity or use some mode switch, how about
we give up on virtio config space completely, and do everything besides
IO and ISR through guest memory?
--
MST
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Michael S. Tsirkin @ 2012-10-10 8:34 UTC (permalink / raw)
To: Rusty Russell; +Cc: Anthony Liguori, virtualization, kvm, qemu-devel
In-Reply-To: <87sj9o8qn7.fsf@rustcorp.com.au>
On Tue, Oct 09, 2012 at 10:26:12AM +1030, Rusty Russell wrote:
> Anthony Liguori <aliguori@us.ibm.com> writes:
> > Gerd Hoffmann <kraxel@redhat.com> writes:
> >
> >> Hi,
> >>
> >>>> So we could have for virtio something like this:
> >>>>
> >>>> Capabilities: [??] virtio-regs:
> >>>> legacy: BAR=0 offset=0
> >>>> virtio-pci: BAR=1 offset=1000
> >>>> virtio-cfg: BAR=1 offset=1800
> >>>
> >>> This would be a vendor specific PCI capability so lspci wouldn't
> >>> automatically know how to parse it.
> >>
> >> Sure, would need a patch to actually parse+print the cap,
> >> /me was just trying to make my point clear in a simple way.
> >>
> >>>>>> 2) ISTR an argument about mapping the ISR register separately, for
> >>>>>> performance, but I can't find a reference to it.
> >>>>>
> >>>>> I think the rationale is that ISR really needs to be PIO but everything
> >>>>> else doesn't. PIO is much faster on x86 because it doesn't require
> >>>>> walking page tables or instruction emulation to handle the exit.
> >>>>
> >>>> Is this still a pressing issue? With MSI-X enabled ISR isn't needed,
> >>>> correct? Which would imply that pretty much only old guests without
> >>>> MSI-X support need this, and we don't need to worry that much when
> >>>> designing something new ...
> >>>
> >>> It wasn't that long ago that MSI-X wasn't supported.. I think we should
> >>> continue to keep ISR as PIO as it is a fast path.
> >>
> >> No problem if we allow to have both legacy layout and new layout at the
> >> same time. Guests can continue to use ISR @ BAR0 in PIO space for
> >> existing virtio devices, even in case they want use mmio for other
> >> registers -> all fine.
> >>
> >> New virtio devices can support MSI-X from day one and decide to not
> >> expose a legacy layout PIO bar.
> >
> > I think having BAR1 be an MMIO mirror of the registers + a BAR2 for
> > virtio configuration space is probably not that bad of a solution.
>
> Well, we also want to clean up the registers, so how about:
>
> BAR0: legacy, as is. If you access this, don't use the others.
> BAR1: new format virtio-pci layout. If you use this, don't use BAR0.
> BAR2: virtio-cfg. If you use this, don't use BAR0.
> BAR3: ISR. If you use this, don't use BAR0.
One problem here is there are only 3 64-bit BARs under PCI.
There's no point to make IO BAR 64-bit on x86, but there might
be for memory.
> I prefer the cases exclusive (ie. use one or the other) as a clear path
> to remove the legacy layout; and leaving the ISR in BAR0 leaves us with
> an ugly corner case in future (ISR is BAR0 + 19? WTF?).
>
> As to MMIO vs PIO, the BARs are self-describing, so we should explicitly
> endorse that and leave it to the devices.
>
> The detection is simple: if BAR1 has non-zero length, it's new-style,
> otherwise legacy.
>
> Thoughts?
> Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Michael S. Tsirkin @ 2012-10-10 11:37 UTC (permalink / raw)
To: Rusty Russell
Cc: Anthony Liguori, kvm, Benjamin Herrenschmidt, qemu-devel,
virtualization
In-Reply-To: <87wqyz6lf8.fsf@rustcorp.com.au>
On Wed, Oct 10, 2012 at 02:14:11PM +1030, Rusty Russell wrote:
> > See above. A guest could happily just use BAR1/BAR2 and completely
> > ignore BAR0 provided that BAR1/BAR2 are present.
>
> But x86 guests want BAR0 because IO space is faster than MMIO. Right?
Or to be a bit more precise, ATM on x86 emulating IO instructions is
faster than MMIO. IIUC this is since you get the address/data in
registers and don't need to decode them.
--
MST
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Anthony Liguori @ 2012-10-10 13:36 UTC (permalink / raw)
To: Rusty Russell, Gerd Hoffmann
Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87zk3v6npm.fsf@rustcorp.com.au>
Rusty Russell <rusty@rustcorp.com.au> writes:
> Gerd Hoffmann <kraxel@redhat.com> writes:
>> So how about this:
>>
>> (1) Add a vendor specific pci capability for new-style virtio.
>> Specifies the pci bar used for new-style virtio registers.
>> Guests can use it to figure whenever new-style virtio is
>> supported and to map the correct bar (which will probably
>> be bar 1 in most cases).
>
> This was closer to the original proposal[1], which I really liked (you
> can layout bars however you want). Anthony thought that vendor
> capabilities were a PCI-e feature, but it seems they're blessed in PCI
> 2.3.
2.3 was standardized in 2002. Are we confident that vendor extensions
play nice with pre-2.3 OSes like Win2k, WinXP, etc?
I still think it's a bad idea to rely on something so "new" in something
as fundamental as virtio-pci unless we have to.
Regards,
Anthony Liguori
>
> So let's return to that proposal, giving something like this:
>
> /* IDs for different capabilities. Must all exist. */
> /* FIXME: Do we win from separating ISR, NOTIFY and COMMON? */
> /* Common configuration */
> #define VIRTIO_PCI_CAP_COMMON_CFG 1
> /* Notifications */
> #define VIRTIO_PCI_CAP_NOTIFY_CFG 2
> /* ISR access */
> #define VIRTIO_PCI_CAP_ISR_CFG 3
> /* Device specific confiuration */
> #define VIRTIO_PCI_CAP_DEVICE_CFG 4
>
> /* This is the PCI capability header: */
> struct virtio_pci_cap {
> u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
> u8 cap_next; /* Generic PCI field: next ptr. */
> u8 cap_len; /* Generic PCI field: sizeof(struct virtio_pci_cap). */
> u8 cfg_type; /* One of the VIRTIO_PCI_CAP_*_CFG. */
> u8 bar; /* Where to find it. */
> u8 unused;
> __le16 offset; /* Offset within bar. */
> __le32 length; /* Length. */
> };
>
> This means qemu can point the isr_cfg into the legacy area if it wants.
> In fact, it can put everything in BAR0 if it wants.
>
> Thoughts?
> Rusty.
> --
> 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
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Michael S. Tsirkin @ 2012-10-10 13:41 UTC (permalink / raw)
To: Anthony Liguori; +Cc: virtualization, kvm, qemu-devel
In-Reply-To: <87vceiih4j.fsf@codemonkey.ws>
On Wed, Oct 10, 2012 at 08:36:12AM -0500, Anthony Liguori wrote:
> Rusty Russell <rusty@rustcorp.com.au> writes:
>
> > Gerd Hoffmann <kraxel@redhat.com> writes:
> >> So how about this:
> >>
> >> (1) Add a vendor specific pci capability for new-style virtio.
> >> Specifies the pci bar used for new-style virtio registers.
> >> Guests can use it to figure whenever new-style virtio is
> >> supported and to map the correct bar (which will probably
> >> be bar 1 in most cases).
> >
> > This was closer to the original proposal[1], which I really liked (you
> > can layout bars however you want). Anthony thought that vendor
> > capabilities were a PCI-e feature, but it seems they're blessed in PCI
> > 2.3.
>
> 2.3 was standardized in 2002. Are we confident that vendor extensions
> play nice with pre-2.3 OSes like Win2k, WinXP, etc?
>
> I still think it's a bad idea to rely on something so "new" in something
> as fundamental as virtio-pci unless we have to.
>
> Regards,
>
> Anthony Liguori
Pre 2.3 it was the case that *all* space outside
the capability linked list, and any capability not
recognized by space, was considered vendor specific.
So there's no problem really.
> >
> > So let's return to that proposal, giving something like this:
> >
> > /* IDs for different capabilities. Must all exist. */
> > /* FIXME: Do we win from separating ISR, NOTIFY and COMMON? */
> > /* Common configuration */
> > #define VIRTIO_PCI_CAP_COMMON_CFG 1
> > /* Notifications */
> > #define VIRTIO_PCI_CAP_NOTIFY_CFG 2
> > /* ISR access */
> > #define VIRTIO_PCI_CAP_ISR_CFG 3
> > /* Device specific confiuration */
> > #define VIRTIO_PCI_CAP_DEVICE_CFG 4
> >
> > /* This is the PCI capability header: */
> > struct virtio_pci_cap {
> > u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
> > u8 cap_next; /* Generic PCI field: next ptr. */
> > u8 cap_len; /* Generic PCI field: sizeof(struct virtio_pci_cap). */
> > u8 cfg_type; /* One of the VIRTIO_PCI_CAP_*_CFG. */
> > u8 bar; /* Where to find it. */
> > u8 unused;
> > __le16 offset; /* Offset within bar. */
> > __le32 length; /* Length. */
> > };
> >
> > This means qemu can point the isr_cfg into the legacy area if it wants.
> > In fact, it can put everything in BAR0 if it wants.
> >
> > Thoughts?
> > Rusty.
> > --
> > 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
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Rusty Russell @ 2012-10-11 0:03 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Sasha Levin, avi, Thomas Lendacky
In-Reply-To: <5073D1D8.3060905@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 09/10/2012 06:59, Rusty Russell ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> Il 05/10/2012 07:43, Rusty Russell ha scritto:
>>>> That's good. But virtio_blk's scsi command is insoluble AFAICT. As I
>>>> said to Anthony, the best rules are "always" and "never", so I'd really
>>>> rather not have to grandfather that in.
>>>
>>> It is, but we can add a rule that if the (transport) flag
>>> VIRTIO_RING_F_ANY_HEADER_SG is set, the cdb field is always 32 bytes in
>>> virtio-blk.
>>
>> Could we do that? It's the cmd length I'm concerned about; is it always
>> 32 in practice for some reason?
>
> It is always 32 or less except in very obscure cases that are pretty
> much confined to iSCSI. We don't care about the obscure cases, and the
> extra bytes don't hurt.
>
> BTW, 32 is the default cdb_size used by virtio-scsi.
>
>> Currently qemu does:
>>
>> struct sg_io_hdr hdr;
>> memset(&hdr, 0, sizeof(struct sg_io_hdr));
>> hdr.interface_id = 'S';
>> hdr.cmd_len = req->elem.out_sg[1].iov_len;
>> hdr.cmdp = req->elem.out_sg[1].iov_base;
>> hdr.dxfer_len = 0;
>>
>> If it's a command which expects more output data, there's no way to
>> guess where the boundary is between that command and the data.
>
> Yep, so I understood the problem right.
OK. Well, Anthony wants qemu to be robust in this regard, so I am
tempted to rework all the qemu drivers to handle arbitrary layouts.
They could use a good audit anyway.
This would become a glaring exception, but I'm tempted to fix it to 32
bytes at the same time as we get the new pci layout (ie. for the virtio
1.0 spec). The Linux driver would carefully be backwards compatible, of
course, and the spec would document why.
Cheers,
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-11 0:08 UTC (permalink / raw)
To: Jamie Lokier
Cc: Anthony Liguori, virtualization, qemu-devel, kvm,
Michael S. Tsirkin
In-Reply-To: <87txu36le8.fsf@rustcorp.com.au>
Rusty Russell <rusty@rustcorp.com.au> writes:
> Jamie Lokier <jamie@shareable.org> writes:
>
>> Rusty Russell wrote:
>>> I don't think it'll be that bad; reset clears the device to unknown,
>>> bar0 moves it from unknown->legacy mode, bar1/2/3 changes it from
>>> unknown->modern mode, and anything else is bad (I prefer being strict so
>>> we catch bad implementations from the beginning).
>>
>> Will that work, if the guest with kernel that uses modern mode, kexecs
>> to an older (but presumed reliable) kernel that only knows about legacy mode?
>>
>> I.e. will the replacement kernel, or (ideally) replacement driver on
>> the rare occasion that is needed on a running kernel, be able to reset
>> the device hard enough?
>
> Well, you need to reset the device, so yes.
MST said something which made me think harder about this case.
Either there needs to be a way to tell what mode the device is in, or
legacy reset has to work, even in modern mode. The latter is
preferable, since it allows an older kernel to do the reset.
Now, since qemu would almost certainly have to add code to stop that
working, it'll probably be fine. But I'd really like to add a "strict
mode" to qemu virtio which does extra sanity checking for driver
authors, and that might break this. That's OK.
Thanks,
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-11 0:43 UTC (permalink / raw)
To: Michael S. Tsirkin, Anthony Liguori; +Cc: virtualization, kvm, qemu-devel
In-Reply-To: <20121010134157.GA7922@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Wed, Oct 10, 2012 at 08:36:12AM -0500, Anthony Liguori wrote:
>> Rusty Russell <rusty@rustcorp.com.au> writes:
>>
>> > Gerd Hoffmann <kraxel@redhat.com> writes:
>> >> So how about this:
>> >>
>> >> (1) Add a vendor specific pci capability for new-style virtio.
>> >> Specifies the pci bar used for new-style virtio registers.
>> >> Guests can use it to figure whenever new-style virtio is
>> >> supported and to map the correct bar (which will probably
>> >> be bar 1 in most cases).
>> >
>> > This was closer to the original proposal[1], which I really liked (you
>> > can layout bars however you want). Anthony thought that vendor
>> > capabilities were a PCI-e feature, but it seems they're blessed in PCI
>> > 2.3.
>>
>> 2.3 was standardized in 2002. Are we confident that vendor extensions
>> play nice with pre-2.3 OSes like Win2k, WinXP, etc?
2.2 (1998) had the capability IDs linked list, indicated by bit 4 in the
status register, but reserved ids 7 and above. ID 9 (vendor specific)
was added in 2.3; it should be ignored, but will require testing of
course, like any change.
2.1 didn't have the capability ID linked list at all; bit 4 in the
status register was reserved.
QEMU's pci.c has capability support, and sets the capabiliy status bit
and list pointer when the driver requests (eg. the eepro100).
> Pre 2.3 it was the case that *all* space outside
> the capability linked list, and any capability not
> recognized by space, was considered vendor specific.
> So there's no problem really.
Oh in theory, sure. In practice, almost certainly. But this needs to
be proved with actual testing.
Cheers,
Rusty.
^ permalink raw reply
* [PATCH] virtio-pci: use module_pci_driver to simplify the code
From: Wei Yongjun @ 2012-10-11 1:10 UTC (permalink / raw)
To: rusty, mst; +Cc: yongjun_wei, linux-kernel, virtualization
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Use the module_pci_driver() macro to make the code simpler
by eliminating module_init and module_exit calls.
dpatch engine is used to auto generate this patch.
(https://github.com/weiyj/dpatch)
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/virtio/virtio_pci.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index c33aea3..b59237c 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -830,16 +830,4 @@ static struct pci_driver virtio_pci_driver = {
#endif
};
-static int __init virtio_pci_init(void)
-{
- return pci_register_driver(&virtio_pci_driver);
-}
-
-module_init(virtio_pci_init);
-
-static void __exit virtio_pci_exit(void)
-{
- pci_unregister_driver(&virtio_pci_driver);
-}
-
-module_exit(virtio_pci_exit);
+module_pci_driver(virtio_pci_driver);
^ permalink raw reply related
* Re: Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-11 1:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Anthony Liguori, qemu-devel, kvm, virtualization
In-Reply-To: <20121010083033.GA4799@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Oct 08, 2012 at 12:51:25PM +1030, Rusty Russell wrote:
>> Note before anyone gets confused; we were talking about using the PCI
>> config space to indicate what BAR(s) the virtio stuff is in. An
>> alternative would be to simply specify a new layout format in BAR1.
>
> One problem we are still left with is this: device specific
> config accesses are still non atomic.
> This is a problem for multibyte fields such as MAC address
> where MAC could change while we are accessing it.
It's also a problem for related fields, eg. console width and height, or
disk geometry.
> I was thinking about some backwards compatible way to solve this, but if
> we are willing to break compatiblity or use some mode switch, how about
> we give up on virtio config space completely, and do everything besides
> IO and ISR through guest memory?
I think there's still a benefit in the simple publishing of information:
I don't really want to add a control queue for the console. But
inevitably, once-static information can change in later versions, and
it's horrible to have config information plus a bit that says "don't use
this, use the control queue".
Here's a table from a quick audit:
Driver Config Device changes Driver writes... after init?
net Y Y N N
block Y Y Y Y
console Y Y N N
rng N N N N
balloon Y Y Y Y
scsi Y N Y N
9p Y N N N
For config space reads, I suggest the driver publish a generation count.
For writes, the standard seems to be a commit latch. We could abuse the
generation count for this: the driver writes to it to commit config
changes.
ie:
/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
struct virtio_pci_common_cfg {
/* About the whole device. */
__le32 device_feature_select; /* read-write */
__le32 device_feature; /* read-only */
__le32 guest_feature_select; /* read-write */
__le32 guest_feature; /* read-only */
__le32 config_gen_and_latch; /* read-write */
__le16 msix_config; /* read-write */
__u8 device_status; /* read-write */
__u8 unused;
/* About a specific virtqueue. */
__le16 queue_select; /* read-write */
__le16 queue_align; /* read-write, power of 2. */
__le16 queue_size; /* read-write, power of 2. */
__le16 queue_msix_vector;/* read-write */
__le64 queue_address; /* read-write: 0xFFFFFFFFFFFFFFFF == DNE. */
};
Thoughts?
Rusty.
PS. Let's make all the virtio-device config LE, too...
^ permalink raw reply
* [PATCH] [SCSI] virtio-scsi: Add real 2-clause BSD license to header
From: Bryan Venteicher @ 2012-10-11 6:41 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, Cong Meng
This is analogous to commit a1b383870a made by Rusty Russell to all
the VirtIO headers at the time. This eases the use of the header as
is by other OSes.
Signed-off-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
---
include/linux/virtio_scsi.h | 28 ++++++++++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index dc8d305..a91fb52 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -1,7 +1,31 @@
+/*
+ * This header is BSD licensed so anyone can use the definitions to implement
+ * compatible drivers/servers.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
#ifndef _LINUX_VIRTIO_SCSI_H
#define _LINUX_VIRTIO_SCSI_H
-/* This header is BSD licensed so anyone can use the definitions to implement
- * compatible drivers/servers. */
#define VIRTIO_SCSI_CDB_SIZE 32
#define VIRTIO_SCSI_SENSE_SIZE 96
--
1.7.11.4
^ permalink raw reply related
* Re: [PATCH] virtio-pci: use module_pci_driver to simplify the code
From: Rusty Russell @ 2012-10-11 8:32 UTC (permalink / raw)
To: Wei Yongjun, mst; +Cc: yongjun_wei, linux-kernel, virtualization
In-Reply-To: <CAPgLHd-GzSDQ+E-zboLA8RfbOVk5bHFK+NTDe2WkWihVFVCtmA@mail.gmail.com>
Wei Yongjun <weiyj.lk@gmail.com> writes:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> Use the module_pci_driver() macro to make the code simpler
> by eliminating module_init and module_exit calls.
>
> dpatch engine is used to auto generate this patch.
> (https://github.com/weiyj/dpatch)
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Thanks, applied!
Rusty.
^ permalink raw reply
* Re: Using PCI config space to indicate config location
From: Michael S. Tsirkin @ 2012-10-11 10:23 UTC (permalink / raw)
To: Rusty Russell; +Cc: Anthony Liguori, qemu-devel, kvm, virtualization
In-Reply-To: <87bog9957l.fsf@rustcorp.com.au>
On Thu, Oct 11, 2012 at 11:48:22AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Mon, Oct 08, 2012 at 12:51:25PM +1030, Rusty Russell wrote:
> >> Note before anyone gets confused; we were talking about using the PCI
> >> config space to indicate what BAR(s) the virtio stuff is in. An
> >> alternative would be to simply specify a new layout format in BAR1.
> >
> > One problem we are still left with is this: device specific
> > config accesses are still non atomic.
> > This is a problem for multibyte fields such as MAC address
> > where MAC could change while we are accessing it.
>
> It's also a problem for related fields, eg. console width and height, or
> disk geometry.
>
> > I was thinking about some backwards compatible way to solve this, but if
> > we are willing to break compatiblity or use some mode switch, how about
> > we give up on virtio config space completely, and do everything besides
> > IO and ISR through guest memory?
>
> I think there's still a benefit in the simple publishing of information:
> I don't really want to add a control queue for the console.
One reason I thought using a vq is handy is because this would
let us get by with a single MSI vector. Currently we need at least 2
for config changes + a shared one for vqs.
But I don't insist.
> But
> inevitably, once-static information can change in later versions, and
> it's horrible to have config information plus a bit that says "don't use
> this, use the control queue".
>
> Here's a table from a quick audit:
>
> Driver Config Device changes Driver writes... after init?
> net Y Y N N
> block Y Y Y Y
> console Y Y N N
> rng N N N N
> balloon Y Y Y Y
> scsi Y N Y N
> 9p Y N N N
>
> For config space reads, I suggest the driver publish a generation count.
You mean device?
> For writes, the standard seems to be a commit latch. We could abuse the
> generation count for this: the driver writes to it to commit config
> changes.
I think this will work. There are a couple of things that bother me:
This assumes read accesses have no side effects, and these are sometimes handy.
Also the semantics for write aren't very clear to me.
I guess device must buffer data until generation count write?
This assumes the device has a buffer to store writes,
and it must track each byte written. I kind of dislike this
tracking of accessed bytes. Also, device would need to resolve conflicts
if any in some device specific way.
Maybe it's a good idea to make the buffer accesses explicit instead?
IOW require driver to declare intent to read/request write of a specific
config range. We could for example do it like this:
__le32 config_offset;
__le32 config_len;
__u8 config_cmd; /* write-only: 0 - read, 1 - write
config_len bytes at config_offset
from/to config space to/from device memory */
But maybe this is over-engineering?
> ie:
> /* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
> struct virtio_pci_common_cfg {
> /* About the whole device. */
> __le32 device_feature_select; /* read-write */
> __le32 device_feature; /* read-only */
> __le32 guest_feature_select; /* read-write */
> __le32 guest_feature; /* read-only */
> __le32 config_gen_and_latch; /* read-write */
> __le16 msix_config; /* read-write */
> __u8 device_status; /* read-write */
> __u8 unused;
>
> /* About a specific virtqueue. */
> __le16 queue_select; /* read-write */
> __le16 queue_align; /* read-write, power of 2. */
> __le16 queue_size; /* read-write, power of 2. */
> __le16 queue_msix_vector;/* read-write */
> __le64 queue_address; /* read-write: 0xFFFFFFFFFFFFFFFF == DNE. */
> };
>
> Thoughts?
> Rusty.
> PS. Let's make all the virtio-device config LE, too...
We'll need some new API for devices then: currently we pass bytes.
--
MST
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Michael S. Tsirkin @ 2012-10-11 11:04 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, netdev, linux-kernel, virtualization, Sasha Levin,
Paolo Bonzini, avi, Thomas Lendacky
In-Reply-To: <87k3ux98oc.fsf@rustcorp.com.au>
On Thu, Oct 11, 2012 at 10:33:31AM +1030, Rusty Russell wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> > Il 09/10/2012 06:59, Rusty Russell ha scritto:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >>> Il 05/10/2012 07:43, Rusty Russell ha scritto:
> >>>> That's good. But virtio_blk's scsi command is insoluble AFAICT. As I
> >>>> said to Anthony, the best rules are "always" and "never", so I'd really
> >>>> rather not have to grandfather that in.
> >>>
> >>> It is, but we can add a rule that if the (transport) flag
> >>> VIRTIO_RING_F_ANY_HEADER_SG is set, the cdb field is always 32 bytes in
> >>> virtio-blk.
> >>
> >> Could we do that? It's the cmd length I'm concerned about; is it always
> >> 32 in practice for some reason?
> >
> > It is always 32 or less except in very obscure cases that are pretty
> > much confined to iSCSI. We don't care about the obscure cases, and the
> > extra bytes don't hurt.
> >
> > BTW, 32 is the default cdb_size used by virtio-scsi.
> >
> >> Currently qemu does:
> >>
> >> struct sg_io_hdr hdr;
> >> memset(&hdr, 0, sizeof(struct sg_io_hdr));
> >> hdr.interface_id = 'S';
> >> hdr.cmd_len = req->elem.out_sg[1].iov_len;
> >> hdr.cmdp = req->elem.out_sg[1].iov_base;
> >> hdr.dxfer_len = 0;
> >>
> >> If it's a command which expects more output data, there's no way to
> >> guess where the boundary is between that command and the data.
> >
> > Yep, so I understood the problem right.
>
> OK. Well, Anthony wants qemu to be robust in this regard, so I am
> tempted to rework all the qemu drivers to handle arbitrary layouts.
> They could use a good audit anyway.
I agree here. Still trying to understand whether we can agree to use
a feature bit for this, or not.
> This would become a glaring exception, but I'm tempted to fix it to 32
> bytes at the same time as we get the new pci layout (ie. for the virtio
> 1.0 spec).
But this isn't a virtio-pci only issue, is it?
qemu has s390 bus with same limmitation.
How can we tie it to pci layout?
Maybe what you mean is to use a transport feature for this
and tie *that* to new layout in case of pci?
> The Linux driver would carefully be backwards compatible, of
> course, and the spec would document why.
>
> Cheers,
> Rusty.
^ permalink raw reply
* Re: [PATCH] [SCSI] virtio-scsi: Add real 2-clause BSD license to header
From: Paolo Bonzini @ 2012-10-11 11:49 UTC (permalink / raw)
To: Bryan Venteicher
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, Cong Meng
In-Reply-To: <1349937691-60084-1-git-send-email-bryanv@daemoninthecloset.org>
Il 11/10/2012 08:41, Bryan Venteicher ha scritto:
> This is analogous to commit a1b383870a made by Rusty Russell to all
> the VirtIO headers at the time. This eases the use of the header as
> is by other OSes.
>
> Signed-off-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/linux/virtio_scsi.h | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
> index dc8d305..a91fb52 100644
> --- a/include/linux/virtio_scsi.h
> +++ b/include/linux/virtio_scsi.h
> @@ -1,7 +1,31 @@
> +/*
> + * This header is BSD licensed so anyone can use the definitions to implement
> + * compatible drivers/servers.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> #ifndef _LINUX_VIRTIO_SCSI_H
> #define _LINUX_VIRTIO_SCSI_H
> -/* This header is BSD licensed so anyone can use the definitions to implement
> - * compatible drivers/servers. */
>
> #define VIRTIO_SCSI_CDB_SIZE 32
> #define VIRTIO_SCSI_SENSE_SIZE 96
>
^ permalink raw reply
* Re: [PATCH] vhost-blk: Add vhost-blk support v2
From: Michael S. Tsirkin @ 2012-10-11 12:41 UTC (permalink / raw)
To: Asias He, Rusty Russell; +Cc: kvm, virtualization
In-Reply-To: <1349769918-8611-1-git-send-email-asias@redhat.com>
On Tue, Oct 09, 2012 at 04:05:18PM +0800, Asias He wrote:
> vhost-blk is an in-kernel virito-blk device accelerator.
>
> Due to lack of proper in-kernel AIO interface, this version converts
> guest's I/O request to bio and use submit_bio() to submit I/O directly.
> So this version any supports raw block device as guest's disk image,
> e.g. /dev/sda, /dev/ram0. We can add file based image support to
> vhost-blk once we have in-kernel AIO interface. There are some work in
> progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
>
> http://marc.info/?l=linux-fsdevel&m=133312234313122
>
> Performance evaluation:
> -----------------------------
> 1) LKVM
> Fio with libaio ioengine on Fusion IO device using kvm tool
> IOPS Before After Improvement
> seq-read 107 121 +13.0%
> seq-write 130 179 +37.6%
> rnd-read 102 122 +19.6%
> rnd-write 125 159 +27.0%
>
> 2) QEMU
> Fio with libaio ioengine on Fusion IO device using QEMU
> IOPS Before After Improvement
> seq-read 76 123 +61.8%
> seq-write 139 173 +24.4%
> rnd-read 73 120 +64.3%
> rnd-write 75 156 +108.0%
>
> Userspace bits:
> -----------------------------
> 1) LKVM
> The latest vhost-blk userspace bits for kvm tool can be found here:
> git@github.com:asias/linux-kvm.git blk.vhost-blk
>
> 2) QEMU
> The latest vhost-blk userspace prototype for QEMU can be found here:
> git@github.com:asias/qemu.git blk.vhost-blk
>
> Signed-off-by: Asias He <asias@redhat.com>
> ---
> drivers/vhost/Kconfig | 1 +
> drivers/vhost/Kconfig.blk | 10 +
> drivers/vhost/Makefile | 2 +
> drivers/vhost/blk.c | 641 ++++++++++++++++++++++++++++++++++++++++++++++
> drivers/vhost/blk.h | 8 +
> 5 files changed, 662 insertions(+)
> create mode 100644 drivers/vhost/Kconfig.blk
> create mode 100644 drivers/vhost/blk.c
> create mode 100644 drivers/vhost/blk.h
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 202bba6..acd8038 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -11,4 +11,5 @@ config VHOST_NET
>
> if STAGING
> source "drivers/vhost/Kconfig.tcm"
> +source "drivers/vhost/Kconfig.blk"
> endif
> diff --git a/drivers/vhost/Kconfig.blk b/drivers/vhost/Kconfig.blk
> new file mode 100644
> index 0000000..ff8ab76
> --- /dev/null
> +++ b/drivers/vhost/Kconfig.blk
> @@ -0,0 +1,10 @@
> +config VHOST_BLK
> + tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
> + depends on BLOCK && EXPERIMENTAL && m
> + ---help---
> + This kernel module can be loaded in host kernel to accelerate
> + guest block with virtio_blk. Not to be confused with virtio_blk
> + module itself which needs to be loaded in guest kernel.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called vhost_blk.
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index a27b053..1a8a4a5 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -2,3 +2,5 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o
> vhost_net-y := vhost.o net.o
>
> obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
> +obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
> +vhost_blk-y := blk.o
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> new file mode 100644
> index 0000000..6b2445a
> --- /dev/null
> +++ b/drivers/vhost/blk.c
> @@ -0,0 +1,641 @@
> +/*
> + * Copyright (C) 2011 Taobao, Inc.
> + * Author: Liu Yuan <tailai.ly@taobao.com>
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + * Author: Asias He <asias@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + *
> + * virtio-blk server in host kernel.
> + */
> +
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/vhost.h>
> +#include <linux/virtio_blk.h>
> +#include <linux/mutex.h>
> +#include <linux/file.h>
> +#include <linux/kthread.h>
> +#include <linux/blkdev.h>
> +
> +#include "vhost.c"
> +#include "vhost.h"
> +#include "blk.h"
> +
> +#define BLK_HDR 0
What's this for, exactly? Please add a comment.
> +
> +static DEFINE_IDA(vhost_blk_index_ida);
> +
> +enum {
> + VHOST_BLK_VQ_REQ = 0,
> + VHOST_BLK_VQ_MAX = 1,
> +};
> +
> +struct req_page_list {
> + struct page **pages;
> + int pages_nr;
> +};
> +
> +struct vhost_blk_req {
> + struct llist_node llnode;
> + struct req_page_list *pl;
> + struct vhost_blk *blk;
> +
> + struct iovec *iov;
> + int iov_nr;
> +
> + struct bio **bio;
> + atomic_t bio_nr;
> +
> + sector_t sector;
> + int write;
> + u16 head;
> + long len;
> +
> + u8 *status;
Is this a userspace pointer? If yes it must be tagged as such.
Please run code checker - it will catch other bugs for you too.
> +};
> +
> +struct vhost_blk {
> + struct task_struct *host_kick;
> + struct vhost_blk_req *reqs;
> + struct vhost_virtqueue vq;
> + struct llist_head llhead;
> + struct vhost_dev dev;
> + u16 reqs_nr;
> + int index;
> +};
> +
> +static inline int iov_num_pages(struct iovec *iov)
> +{
> + return (PAGE_ALIGN((unsigned long)iov->iov_base + iov->iov_len) -
> + ((unsigned long)iov->iov_base & PAGE_MASK)) >> PAGE_SHIFT;
> +}
> +
> +static int vhost_blk_setup(struct vhost_blk *blk)
> +{
> + blk->reqs_nr = blk->vq.num;
> +
> + blk->reqs = kmalloc(sizeof(struct vhost_blk_req) * blk->reqs_nr,
> + GFP_KERNEL);
> + if (!blk->reqs)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static inline int vhost_blk_set_status(struct vhost_blk_req *req, u8 status)
> +{
> + struct vhost_blk *blk = req->blk;
> +
> + if (copy_to_user(req->status, &status, sizeof(status))) {
Does this write into guest memory, right? This write needs to be tracked in
log in case it's enabled.
Also, __copy_to_user should be enough here, right?
> + vq_err(&blk->vq, "Failed to write status\n");
> + vhost_discard_vq_desc(&blk->vq, 1);
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> +static void vhost_blk_enable_vq(struct vhost_blk *blk,
> + struct vhost_virtqueue *vq)
> +{
> + wake_up_process(blk->host_kick);
> +}
> +
> +static void vhost_blk_req_done(struct bio *bio, int err)
> +{
> + struct vhost_blk_req *req = bio->bi_private;
> + struct vhost_blk *blk = req->blk;
> +
> + if (err)
> + req->len = err;
> +
> + if (atomic_dec_and_test(&req->bio_nr)) {
> + llist_add(&req->llnode, &blk->llhead);
> + wake_up_process(blk->host_kick);
> + }
> +
> + bio_put(bio);
> +}
> +
> +static void vhost_blk_req_umap(struct vhost_blk_req *req)
> +{
> + struct req_page_list *pl;
> + int i, j;
> +
> + if (!req->pl)
> + return;
> +
> + for (i = 0; i < req->iov_nr; i++) {
> + pl = &req->pl[i];
> + for (j = 0; j < pl->pages_nr; j++) {
> + if (!req->write)
> + set_page_dirty_lock(pl->pages[j]);
> + page_cache_release(pl->pages[j]);
> + }
> + }
> +
> + kfree(req->pl);
> +}
> +
> +static int vhost_blk_bio_make(struct vhost_blk_req *req,
> + struct block_device *bdev)
> +{
> + int pages_nr_total, i, j, ret;
> + struct iovec *iov = req->iov;
> + int iov_nr = req->iov_nr;
> + struct page **pages, *page;
> + struct bio *bio = NULL;
> + int bio_nr = 0;
> +
> + req->len = 0;
> + pages_nr_total = 0;
> + for (i = 0; i < iov_nr; i++) {
> + req->len += iov[i].iov_len;
> + pages_nr_total += iov_num_pages(&iov[i]);
> + }
> +
> + req->pl = kmalloc((iov_nr * sizeof(struct req_page_list)) +
> + (pages_nr_total * sizeof(struct page *)) +
> + (pages_nr_total * sizeof(struct bio *)),
> + GFP_KERNEL);
> + if (!req->pl)
> + return -ENOMEM;
> + pages = (struct page **)&req->pl[iov_nr];
> + req->bio = (struct bio **)&pages[pages_nr_total];
> +
> + req->iov_nr = 0;
> + for (i = 0; i < iov_nr; i++) {
> + int pages_nr = iov_num_pages(&iov[i]);
> + unsigned long iov_base, iov_len;
> + struct req_page_list *pl;
> +
> + iov_base = (unsigned long)iov[i].iov_base;
> + iov_len = (unsigned long)iov[i].iov_len;
> +
> + ret = get_user_pages_fast(iov_base, pages_nr,
> + !req->write, pages);
> + if (ret != pages_nr)
> + goto fail;
> +
> + req->iov_nr++;
> + pl = &req->pl[i];
> + pl->pages_nr = pages_nr;
> + pl->pages = pages;
> +
> + for (j = 0; j < pages_nr; j++) {
> + unsigned int off, len;
> + page = pages[j];
> + off = iov_base & ~PAGE_MASK;
> + len = PAGE_SIZE - off;
> + if (len > iov_len)
> + len = iov_len;
> +
> + while (!bio || bio_add_page(bio, page, len, off) <= 0) {
> + bio = bio_alloc(GFP_KERNEL, pages_nr);
> + if (!bio)
> + goto fail;
> + bio->bi_sector = req->sector;
> + bio->bi_bdev = bdev;
> + bio->bi_private = req;
> + bio->bi_end_io = vhost_blk_req_done;
> + req->bio[bio_nr++] = bio;
> + }
> + req->sector += len >> 9;
> + iov_base += len;
> + iov_len -= len;
> + }
> +
> + pages += pages_nr;
> + }
> + atomic_set(&req->bio_nr, bio_nr);
> +
> + return 0;
> +
> +fail:
> + for (i = 0; i < bio_nr; i++)
> + bio_put(req->bio[i]);
> + vhost_blk_req_umap(req);
> + return -ENOMEM;
> +}
> +
> +static inline void vhost_blk_bio_send(struct vhost_blk_req *req)
> +{
> + struct blk_plug plug;
> + int i, bio_nr;
> +
> + bio_nr = atomic_read(&req->bio_nr);
> + blk_start_plug(&plug);
> + for (i = 0; i < bio_nr; i++)
> + submit_bio(req->write, req->bio[i]);
> + blk_finish_plug(&plug);
> +}
> +
> +static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file *file)
> +{
> +
> + struct inode *inode = file->f_mapping->host;
> + struct block_device *bdev = inode->i_bdev;
> + int ret;
> +
> + ret = vhost_blk_bio_make(req, bdev);
> + if (ret < 0)
> + return ret;
> +
> + vhost_blk_bio_send(req);
> +
> + return ret;
> +}
> +
> +static int vhost_blk_req_done_thread(void *data)
> +{
> + mm_segment_t oldfs = get_fs();
> + struct vhost_blk *blk = data;
> + struct vhost_virtqueue *vq;
> + struct llist_node *llnode;
> + struct vhost_blk_req *req;
> + bool added;
> + u8 status;
> + int ret;
> +
> + vq = &blk->vq;
> + set_fs(USER_DS);
> + use_mm(blk->dev.mm);
> + for (;;) {
> + llnode = llist_del_all(&blk->llhead);
Interesting, I didn't consider llist - maybe vhost.c
could switch to that too? If we do how to handle flushing?
If we do we can move some common code out here.
> + if (!llnode) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule();
> + if (unlikely(kthread_should_stop()))
> + break;
> + continue;
> + }
I think you need to call something like
if (need_resched())
schedule();
once in a while even if the list is not empty.
> + added = false;
> + while (llnode) {
> + req = llist_entry(llnode, struct vhost_blk_req, llnode);
> + llnode = llist_next(llnode);
> +
> + vhost_blk_req_umap(req);
> +
> + status = req->len > 0 ?
> + VIRTIO_BLK_S_OK : VIRTIO_BLK_S_IOERR;
> + ret = copy_to_user(req->status, &status,
> + sizeof(status));
use vhost_blk_set_status? Why not?
> + if (unlikely(ret)) {
> + vq_err(&blk->vq, "Failed to write status\n");
> + return -1;
This will kill this thread. Likely not what was intended.
> + }
> + vhost_add_used(&blk->vq, req->head, req->len);
> + added = true;
> + }
> + if (likely(added))
> + vhost_signal(&blk->dev, &blk->vq);
> +
Pls dont add empty line here.
> + }
> + unuse_mm(blk->dev.mm);
> + set_fs(oldfs);
> + return 0;
> +}
> +
> +static void vhost_blk_flush(struct vhost_blk *blk)
> +{
> + vhost_poll_flush(&blk->vq.poll);
Hmm but blk kthread could still be processing requests, no?
Need to flush these too I think?
> +}
> +
> +static struct file *vhost_blk_stop_vq(struct vhost_blk *blk,
> + struct vhost_virtqueue *vq)
> +{
> + struct file *file;
> +
> + mutex_lock(&vq->mutex);
> + file = rcu_dereference_protected(vq->private_data,
> + lockdep_is_held(&vq->mutex));
> + rcu_assign_pointer(vq->private_data, NULL);
> + mutex_unlock(&vq->mutex);
> +
> + return file;
> +
> +}
> +
> +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
> +{
> +
> + *file = vhost_blk_stop_vq(blk, &blk->vq);
Is this wrapper worth it? Also maybe just return file?
> +}
> +
> +/* Handle guest request */
> +static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
> + struct virtio_blk_outhdr *hdr,
> + u16 head, u16 out, u16 in,
> + struct file *file)
> +{
> + struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
> + struct vhost_blk_req *req;
> + int iov_nr, ret;
> + u8 status;
> +
> + if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID)
> + iov_nr = in - 1;
> + else
> + iov_nr = out - 1;
> +
> + req = &blk->reqs[head];
> + req->head = head;
> + req->status = blk->vq.iov[iov_nr + 1].iov_base;
> + req->blk = blk;
> + req->iov = &vq->iov[BLK_HDR + 1];
Lots of manual mangling of iovecs here and elsewhere is scary.
First, there should not be so many assumptions about how buffers
are laid out. Second, there seems to be no validation that iovec
is large enough. It is preferable to use memcpy_toiovecend and friends
which validate input. This applied to many places below, please
audit code for such uses. Where you find it necessary to
handle iovec directly, please add comments explaining where
it's validated and why it's safe.
> + req->iov_nr = iov_nr;
> + req->sector = hdr->sector;
> +
> + switch (hdr->type) {
> + case VIRTIO_BLK_T_OUT:
> + req->write = WRITE;
> + ret = vhost_blk_req_submit(req, file);
> + break;
> + case VIRTIO_BLK_T_IN:
> + req->write = READ;
> + ret = vhost_blk_req_submit(req, file);
> + break;
> + case VIRTIO_BLK_T_FLUSH:
> + ret = vfs_fsync(file, 1);
> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> + if (!vhost_blk_set_status(req, status))
> + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
This should discard on error, no? Also return error to caller?
r = vhost_blk_set_status(req, status);
if (r) {
ret = r;
break;
}
vhost_add_used_and_signal(&blk->dev, vq, head, ret);
return 0;
and move discard outside switch below.
> + break;
> + case VIRTIO_BLK_T_GET_ID:
> + ret = snprintf(vq->iov[BLK_HDR + 1].iov_base,
> + VIRTIO_BLK_ID_BYTES, "vhost-blk%d", blk->index);
snprintf into a userspace buffer? Uh oh.
> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> + if (!vhost_blk_set_status(req, status))
> + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
> + break;
> + default:
> + pr_warn("Unsupported request type %d\n", hdr->type);
This can be triggered by userspace so vq_err?
> + vhost_discard_vq_desc(vq, 1);
Note this does not skip this descriptor - it gives userspace
chance to correct it and retry. Is this the intended behaviour?
Should not we fail request instead?
> + ret = -EFAULT;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +/* Guest kick us for IO request */
> +static void vhost_blk_handle_guest_kick(struct vhost_work *work)
> +{
> + struct virtio_blk_outhdr hdr;
> + struct vhost_virtqueue *vq;
> + struct vhost_blk *blk;
> + struct blk_plug plug;
> + struct file *f;
> + int in, out;
> + u16 head;
> +
> + vq = container_of(work, struct vhost_virtqueue, poll.work);
> + blk = container_of(vq->dev, struct vhost_blk, dev);
> +
> + /* TODO: check that we are running from vhost_worker? */
> + f = rcu_dereference_check(vq->private_data, 1);
> + if (!f)
> + return;
> +
> + vhost_disable_notify(&blk->dev, vq);
> + blk_start_plug(&plug);
> + for (;;) {
> + head = vhost_get_vq_desc(&blk->dev, vq, vq->iov,
> + ARRAY_SIZE(vq->iov),
> + &out, &in, NULL, NULL);
> + if (unlikely(head < 0))
> + break;
> +
> + if (unlikely(head == vq->num)) {
> + if (unlikely(vhost_enable_notify(&blk->dev, vq))) {
> + vhost_disable_notify(&blk->dev, vq);
> + continue;
> + }
> + break;
> + }
> +
> + if (unlikely(copy_from_user(&hdr, vq->iov[BLK_HDR].iov_base,
> + sizeof(hdr)))) {
> + vq_err(vq, "Failed to get block header!\n");
> + vhost_discard_vq_desc(vq, 1);
> + break;
> + }
> +
> + if (vhost_blk_req_handle(vq, &hdr, head, out, in, f) < 0)
> + break;
> + }
> + blk_finish_plug(&plug);
> +}
> +
> +static int vhost_blk_open(struct inode *inode, struct file *file)
> +{
> + struct vhost_blk *blk;
> + int ret;
> +
> + blk = kzalloc(sizeof(*blk), GFP_KERNEL);
> + if (!blk) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = ida_simple_get(&vhost_blk_index_ida, 0, 0, GFP_KERNEL);
> + if (ret < 0)
> + goto out_dev;
> + blk->index = ret;
> +
> + blk->vq.handle_kick = vhost_blk_handle_guest_kick;
> +
> + ret = vhost_dev_init(&blk->dev, &blk->vq, VHOST_BLK_VQ_MAX);
> + if (ret < 0)
> + goto out_dev;
> + file->private_data = blk;
> +
> + blk->host_kick = kthread_create(vhost_blk_req_done_thread,
> + blk, "vhost-blk-%d", current->pid);
> + if (IS_ERR(blk->host_kick)) {
> + ret = PTR_ERR(blk->host_kick);
> + goto out_dev;
> + }
> +
> + return ret;
> +out_dev:
> + kfree(blk);
> +out:
> + return ret;
> +}
> +
> +static int vhost_blk_release(struct inode *inode, struct file *f)
> +{
> + struct vhost_blk *blk = f->private_data;
> + struct file *file;
> +
> + ida_simple_remove(&vhost_blk_index_ida, blk->index);
> + vhost_blk_stop(blk, &file);
> + vhost_blk_flush(blk);
> + vhost_dev_cleanup(&blk->dev, false);
> + if (file)
> + fput(file);
> + kthread_stop(blk->host_kick);
> + kfree(blk->reqs);
> + kfree(blk);
> +
> + return 0;
> +}
> +
> +static int vhost_blk_set_features(struct vhost_blk *blk, u64 features)
> +{
> + mutex_lock(&blk->dev.mutex);
> + blk->dev.acked_features = features;
> + mutex_unlock(&blk->dev.mutex);
We also need to flush outstanding requets normally.
If not needed here pls add a comment why.
> +
> + return 0;
> +}
> +
> +static long vhost_blk_set_backend(struct vhost_blk *blk, unsigned index, int fd)
> +{
> + struct vhost_virtqueue *vq = &blk->vq;
> + struct file *file, *oldfile;
> + int ret;
> +
> + mutex_lock(&blk->dev.mutex);
> + ret = vhost_dev_check_owner(&blk->dev);
> + if (ret)
> + goto out_dev;
> +
> + if (index >= VHOST_BLK_VQ_MAX) {
> + ret = -ENOBUFS;
> + goto out_dev;
> + }
> +
> + mutex_lock(&vq->mutex);
> +
> + if (!vhost_vq_access_ok(vq)) {
> + ret = -EFAULT;
> + goto out_vq;
> + }
> +
> + file = fget(fd);
> + if (IS_ERR(file)) {
> + ret = PTR_ERR(file);
> + goto out_vq;
> + }
> +
> + oldfile = rcu_dereference_protected(vq->private_data,
> + lockdep_is_held(&vq->mutex));
> + if (file != oldfile) {
> + rcu_assign_pointer(vq->private_data, file);
> + vhost_blk_enable_vq(blk, vq);
> +
> + ret = vhost_init_used(vq);
> + if (ret)
> + goto out_vq;
> + }
> +
> + mutex_unlock(&vq->mutex);
> +
> + if (oldfile) {
> + vhost_blk_flush(blk);
> + fput(oldfile);
> + }
> +
> + mutex_unlock(&blk->dev.mutex);
> + return 0;
> +
> +out_vq:
> + mutex_unlock(&vq->mutex);
> +out_dev:
> + mutex_unlock(&blk->dev.mutex);
> + return ret;
> +}
> +
> +static long vhost_blk_reset_owner(struct vhost_blk *blk)
> +{
> + struct file *file = NULL;
> + int err;
> +
> + mutex_lock(&blk->dev.mutex);
> + err = vhost_dev_check_owner(&blk->dev);
> + if (err)
> + goto done;
> + vhost_blk_stop(blk, &file);
> + vhost_blk_flush(blk);
> + err = vhost_dev_reset_owner(&blk->dev);
> +done:
> + mutex_unlock(&blk->dev.mutex);
> + if (file)
> + fput(file);
> + return err;
> +}
> +
> +static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
> + unsigned long arg)
> +{
> + struct vhost_blk *blk = f->private_data;
> + void __user *argp = (void __user *)arg;
> + struct vhost_vring_file backend;
> + u64 __user *featurep = argp;
> + u64 features;
> + int ret;
> +
> + switch (ioctl) {
> + case VHOST_BLK_SET_BACKEND:
> + if (copy_from_user(&backend, argp, sizeof(backend)))
> + return -EFAULT;
> + return vhost_blk_set_backend(blk, backend.index, backend.fd);
> + case VHOST_GET_FEATURES:
> + features = VHOST_BLK_FEATURES;
> + if (copy_to_user(featurep, &features, sizeof(features)))
> + return -EFAULT;
> + return 0;
> + case VHOST_SET_FEATURES:
> + if (copy_from_user(&features, featurep, sizeof(features)))
> + return -EFAULT;
> + if (features & ~VHOST_BLK_FEATURES)
> + return -EOPNOTSUPP;
> + return vhost_blk_set_features(blk, features);
> + case VHOST_RESET_OWNER:
> + return vhost_blk_reset_owner(blk);
> + default:
> + mutex_lock(&blk->dev.mutex);
> + ret = vhost_dev_ioctl(&blk->dev, ioctl, arg);
> + if (!ret && ioctl == VHOST_SET_VRING_NUM)
> + ret = vhost_blk_setup(blk);
> + vhost_blk_flush(blk);
> + mutex_unlock(&blk->dev.mutex);
> + return ret;
> + }
> +}
> +
> +static const struct file_operations vhost_blk_fops = {
> + .owner = THIS_MODULE,
> + .open = vhost_blk_open,
> + .release = vhost_blk_release,
> + .llseek = noop_llseek,
> + .unlocked_ioctl = vhost_blk_ioctl,
> +};
> +
> +static struct miscdevice vhost_blk_misc = {
> + MISC_DYNAMIC_MINOR,
> + "vhost-blk",
> + &vhost_blk_fops,
> +};
> +
> +int vhost_blk_init(void)
> +{
> + return misc_register(&vhost_blk_misc);
> +}
> +
> +void vhost_blk_exit(void)
> +{
> + misc_deregister(&vhost_blk_misc);
> +}
> +
> +module_init(vhost_blk_init);
> +module_exit(vhost_blk_exit);
> +
> +MODULE_VERSION("0.0.3");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Asias He");
> +MODULE_DESCRIPTION("Host kernel accelerator for virtio_blk");
> diff --git a/drivers/vhost/blk.h b/drivers/vhost/blk.h
> new file mode 100644
> index 0000000..2f674f0
> --- /dev/null
> +++ b/drivers/vhost/blk.h
> @@ -0,0 +1,8 @@
> +#include <linux/vhost.h>
> +
> +enum {
> + VHOST_BLK_FEATURES = (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> + (1ULL << VIRTIO_RING_F_EVENT_IDX),
> +};
> +/* VHOST_BLK specific defines */
> +#define VHOST_BLK_SET_BACKEND _IOW(VHOST_VIRTIO, 0x50, struct vhost_vring_file)
> --
> 1.7.11.4
^ permalink raw reply
* Re: Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-11 22:29 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Anthony Liguori, qemu-devel, kvm, virtualization
In-Reply-To: <20121011102333.GA5552@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Oct 11, 2012 at 11:48:22AM +1030, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Mon, Oct 08, 2012 at 12:51:25PM +1030, Rusty Russell wrote:
>> >> Note before anyone gets confused; we were talking about using the PCI
>> >> config space to indicate what BAR(s) the virtio stuff is in. An
>> >> alternative would be to simply specify a new layout format in BAR1.
>> >
>> > One problem we are still left with is this: device specific
>> > config accesses are still non atomic.
>> > This is a problem for multibyte fields such as MAC address
>> > where MAC could change while we are accessing it.
>>
>> It's also a problem for related fields, eg. console width and height, or
>> disk geometry.
>>
>> > I was thinking about some backwards compatible way to solve this, but if
>> > we are willing to break compatiblity or use some mode switch, how about
>> > we give up on virtio config space completely, and do everything besides
>> > IO and ISR through guest memory?
>>
>> I think there's still a benefit in the simple publishing of information:
>> I don't really want to add a control queue for the console.
>
> One reason I thought using a vq is handy is because this would
> let us get by with a single MSI vector. Currently we need at least 2
> for config changes + a shared one for vqs.
> But I don't insist.
Hmmm, that is true.
>> Here's a table from a quick audit:
>>
>> Driver Config Device changes Driver writes... after init?
>> net Y Y N N
>> block Y Y Y Y
>> console Y Y N N
>> rng N N N N
>> balloon Y Y Y Y
>> scsi Y N Y N
>> 9p Y N N N
>>
>> For config space reads, I suggest the driver publish a generation count.
>
> You mean device?
Yes, sorry.
>> For writes, the standard seems to be a commit latch. We could abuse the
>> generation count for this: the driver writes to it to commit config
>> changes.
>
> I think this will work. There are a couple of things that bother me:
>
> This assumes read accesses have no side effects, and these are sometimes handy.
> Also the semantics for write aren't very clear to me.
> I guess device must buffer data until generation count write?
> This assumes the device has a buffer to store writes,
> and it must track each byte written. I kind of dislike this
> tracking of accessed bytes. Also, device would need to resolve conflicts
> if any in some device specific way.
It should be trivial to implement: you keep a scratch copy of the config
space, and copy it to the master copy when they hit the latch.
Implementation of this will show whether I've missed anything here, I
think.
> Maybe it's a good idea to make the buffer accesses explicit instead?
> IOW require driver to declare intent to read/request write of a specific
> config range. We could for example do it like this:
> __le32 config_offset;
> __le32 config_len;
> __u8 config_cmd; /* write-only: 0 - read, 1 - write
> config_len bytes at config_offset
> from/to config space to/from device memory */
> But maybe this is over-engineering?
Seems overengineering since the config space is quite small in practice.
>> PS. Let's make all the virtio-device config LE, too...
>
> We'll need some new API for devices then: currently we pass bytes.
Yes, but driver authors expect that anyway. We can retro-define
virito-mmio to be LE (since all current users are), so this is
v. tempting.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Rusty Russell @ 2012-10-11 22:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, Sasha Levin,
Paolo Bonzini, avi, Thomas Lendacky
In-Reply-To: <20121011110430.GF5552@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Oct 11, 2012 at 10:33:31AM +1030, Rusty Russell wrote:
>> OK. Well, Anthony wants qemu to be robust in this regard, so I am
>> tempted to rework all the qemu drivers to handle arbitrary layouts.
>> They could use a good audit anyway.
>
> I agree here. Still trying to understand whether we can agree to use
> a feature bit for this, or not.
I'd *like* to imply it by the new PCI layout, but if it doesn't work
we'll add a new feature bit.
I'm resisting a feature bit, since it constrains future implementations
which could otherwise assume it.
>> This would become a glaring exception, but I'm tempted to fix it to 32
>> bytes at the same time as we get the new pci layout (ie. for the virtio
>> 1.0 spec).
>
> But this isn't a virtio-pci only issue, is it?
> qemu has s390 bus with same limmitation.
> How can we tie it to pci layout?
They can use a transport feature if they need to, of course. But
perhaps the timing with ccw will coincide with the fix, in which they
don't need to, but it might be a bit late.
Cornelia?
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] [SCSI] virtio-scsi: Add real 2-clause BSD license to header
From: Rusty Russell @ 2012-10-11 22:38 UTC (permalink / raw)
To: Paolo Bonzini, Bryan Venteicher
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, Cong Meng
In-Reply-To: <5076B241.1080506@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 11/10/2012 08:41, Bryan Venteicher ha scritto:
>> This is analogous to commit a1b383870a made by Rusty Russell to all
>> the VirtIO headers at the time. This eases the use of the header as
>> is by other OSes.
>>
>> Signed-off-by: Bryan Venteicher <bryanv@daemoninthecloset.org>
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Applied!
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Paolo Bonzini @ 2012-10-12 7:38 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Sasha Levin, avi, Thomas Lendacky
In-Reply-To: <87wqyw63et.fsf@rustcorp.com.au>
Il 12/10/2012 00:37, Rusty Russell ha scritto:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> On Thu, Oct 11, 2012 at 10:33:31AM +1030, Rusty Russell wrote:
>>> OK. Well, Anthony wants qemu to be robust in this regard, so I am
>>> tempted to rework all the qemu drivers to handle arbitrary layouts.
>>> They could use a good audit anyway.
>>
>> I agree here. Still trying to understand whether we can agree to use
>> a feature bit for this, or not.
>
> I'd *like* to imply it by the new PCI layout, but if it doesn't work
> we'll add a new feature bit.
>
> I'm resisting a feature bit, since it constrains future implementations
> which could otherwise assume it.
Future implementations may certainly refuse to start if the feature is
not there. Whether it's a good idea or not, well, that depends on how
much future they are.
Paolo
>>> This would become a glaring exception, but I'm tempted to fix it to 32
>>> bytes at the same time as we get the new pci layout (ie. for the virtio
>>> 1.0 spec).
>>
>> But this isn't a virtio-pci only issue, is it?
>> qemu has s390 bus with same limmitation.
>> How can we tie it to pci layout?
>
> They can use a transport feature if they need to, of course. But
> perhaps the timing with ccw will coincide with the fix, in which they
> don't need to, but it might be a bit late.
>
> Cornelia?
>
> Cheers,
> Rusty.
>
^ permalink raw reply
* Re: Using PCI config space to indicate config location
From: Michael S. Tsirkin @ 2012-10-12 9:33 UTC (permalink / raw)
To: Rusty Russell; +Cc: Anthony Liguori, qemu-devel, kvm, virtualization
In-Reply-To: <87391k7icv.fsf@rustcorp.com.au>
On Fri, Oct 12, 2012 at 08:59:36AM +1030, Rusty Russell wrote:
> >> For writes, the standard seems to be a commit latch. We could abuse the
> >> generation count for this: the driver writes to it to commit config
> >> changes.
> >
> > I think this will work. There are a couple of things that bother me:
> >
> > This assumes read accesses have no side effects, and these are sometimes handy.
> > Also the semantics for write aren't very clear to me.
> > I guess device must buffer data until generation count write?
> > This assumes the device has a buffer to store writes,
> > and it must track each byte written. I kind of dislike this
> > tracking of accessed bytes. Also, device would need to resolve conflicts
> > if any in some device specific way.
>
> It should be trivial to implement: you keep a scratch copy of the config
> space, and copy it to the master copy when they hit the latch.
>
> Implementation of this will show whether I've missed anything here, I
> think.
What I refer to: what happens if driver does:
- write offset 1
- write offset 3
- hit commit latch
?
--
MST
^ permalink raw reply
* Re: Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-12 9:51 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Anthony Liguori, qemu-devel, kvm, virtualization
In-Reply-To: <20121012093330.GA32014@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Oct 12, 2012 at 08:59:36AM +1030, Rusty Russell wrote:
>> >> For writes, the standard seems to be a commit latch. We could abuse the
>> >> generation count for this: the driver writes to it to commit config
>> >> changes.
>> >
>> > I think this will work. There are a couple of things that bother me:
>> >
>> > This assumes read accesses have no side effects, and these are sometimes handy.
>> > Also the semantics for write aren't very clear to me.
>> > I guess device must buffer data until generation count write?
>> > This assumes the device has a buffer to store writes,
>> > and it must track each byte written. I kind of dislike this
>> > tracking of accessed bytes. Also, device would need to resolve conflicts
>> > if any in some device specific way.
>>
>> It should be trivial to implement: you keep a scratch copy of the config
>> space, and copy it to the master copy when they hit the latch.
>>
>> Implementation of this will show whether I've missed anything here, I
>> think.
>
> What I refer to: what happens if driver does:
> - write offset 1
> - write offset 3
> - hit commit latch
- nothing
- nothing
- effect of offset 1 and offset 3 writes
Now, since there's nothing published by the *driver* at the moment
which can't be trivially atomically written, this scheme is overkill
(sure, it means you could do a byte-at-a-time write to some 4-byte
field, but why?).
But perhaps it's overkill: no other bus has this feature, so we'd need a
feature bit for them anyway in future if we create a device which needs
such atomicity.
Cheers,
Rusty.
^ permalink raw reply
* Re: Using PCI config space to indicate config location
From: Michael S. Tsirkin @ 2012-10-12 10:02 UTC (permalink / raw)
To: Rusty Russell; +Cc: Anthony Liguori, qemu-devel, kvm, virtualization
In-Reply-To: <871uh4587d.fsf@rustcorp.com.au>
On Fri, Oct 12, 2012 at 08:21:50PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Fri, Oct 12, 2012 at 08:59:36AM +1030, Rusty Russell wrote:
> >> >> For writes, the standard seems to be a commit latch. We could abuse the
> >> >> generation count for this: the driver writes to it to commit config
> >> >> changes.
> >> >
> >> > I think this will work. There are a couple of things that bother me:
> >> >
> >> > This assumes read accesses have no side effects, and these are sometimes handy.
> >> > Also the semantics for write aren't very clear to me.
> >> > I guess device must buffer data until generation count write?
> >> > This assumes the device has a buffer to store writes,
> >> > and it must track each byte written. I kind of dislike this
> >> > tracking of accessed bytes. Also, device would need to resolve conflicts
> >> > if any in some device specific way.
> >>
> >> It should be trivial to implement: you keep a scratch copy of the config
> >> space, and copy it to the master copy when they hit the latch.
> >>
> >> Implementation of this will show whether I've missed anything here, I
> >> think.
> >
> > What I refer to: what happens if driver does:
> > - write offset 1
> > - write offset 3
> > - hit commit latch
>
> - nothing
> - nothing
> - effect of offset 1 and offset 3 writes
OK so this means that you also need to track which bytes where written
in order to know to skip byte 2.
This is what I referred to. If instead we ask driver to specify
offset/length explicitly device only needs to remember that.
Not a big deal anyway, just pointing this out.
> Now, since there's nothing published by the *driver* at the moment
> which can't be trivially atomically written, this scheme is overkill
> (sure, it means you could do a byte-at-a-time write to some 4-byte
> field, but why?).
>
> But perhaps it's overkill: no other bus has this feature, so we'd need a
> feature bit for them anyway in future if we create a device which needs
> such atomicity.
>
> Cheers,
> Rusty.
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Cornelia Huck @ 2012-10-12 11:52 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Sasha Levin, Paolo Bonzini, avi, Thomas Lendacky
In-Reply-To: <87wqyw63et.fsf@rustcorp.com.au>
On Fri, 12 Oct 2012 09:07:46 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Thu, Oct 11, 2012 at 10:33:31AM +1030, Rusty Russell wrote:
> >> OK. Well, Anthony wants qemu to be robust in this regard, so I am
> >> tempted to rework all the qemu drivers to handle arbitrary layouts.
> >> They could use a good audit anyway.
> >
> > I agree here. Still trying to understand whether we can agree to use
> > a feature bit for this, or not.
>
> I'd *like* to imply it by the new PCI layout, but if it doesn't work
> we'll add a new feature bit.
>
> I'm resisting a feature bit, since it constrains future implementations
> which could otherwise assume it.
>
> >> This would become a glaring exception, but I'm tempted to fix it to 32
> >> bytes at the same time as we get the new pci layout (ie. for the virtio
> >> 1.0 spec).
> >
> > But this isn't a virtio-pci only issue, is it?
> > qemu has s390 bus with same limmitation.
> > How can we tie it to pci layout?
>
> They can use a transport feature if they need to, of course. But
> perhaps the timing with ccw will coincide with the fix, in which they
> don't need to, but it might be a bit late.
>
> Cornelia?
My virtio-ccw host code is still going through a bit of rework, so it
might well go in after the fix.
There's also the existing (non-spec'ed) s390-virtio transport. While it
will likely be deprecated sometime in the future, it should probably
get a feature bit for consistency's sake.
>
> Cheers,
> Rusty.
>
^ permalink raw reply
* Re: [PATCH] vhost-blk: Add vhost-blk support v2
From: Asias He @ 2012-10-12 13:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization
In-Reply-To: <20121011124125.GA8074@redhat.com>
Hello Michael,
Thanks for the review!
On 10/11/2012 08:41 PM, Michael S. Tsirkin wrote:
> On Tue, Oct 09, 2012 at 04:05:18PM +0800, Asias He wrote:
>> vhost-blk is an in-kernel virito-blk device accelerator.
>>
>> Due to lack of proper in-kernel AIO interface, this version converts
>> guest's I/O request to bio and use submit_bio() to submit I/O directly.
>> So this version any supports raw block device as guest's disk image,
>> e.g. /dev/sda, /dev/ram0. We can add file based image support to
>> vhost-blk once we have in-kernel AIO interface. There are some work in
>> progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
>>
>> http://marc.info/?l=linux-fsdevel&m=133312234313122
>>
>> Performance evaluation:
>> -----------------------------
>> 1) LKVM
>> Fio with libaio ioengine on Fusion IO device using kvm tool
>> IOPS Before After Improvement
>> seq-read 107 121 +13.0%
>> seq-write 130 179 +37.6%
>> rnd-read 102 122 +19.6%
>> rnd-write 125 159 +27.0%
>>
>> 2) QEMU
>> Fio with libaio ioengine on Fusion IO device using QEMU
>> IOPS Before After Improvement
>> seq-read 76 123 +61.8%
>> seq-write 139 173 +24.4%
>> rnd-read 73 120 +64.3%
>> rnd-write 75 156 +108.0%
>>
>> Userspace bits:
>> -----------------------------
>> 1) LKVM
>> The latest vhost-blk userspace bits for kvm tool can be found here:
>> git@github.com:asias/linux-kvm.git blk.vhost-blk
>>
>> 2) QEMU
>> The latest vhost-blk userspace prototype for QEMU can be found here:
>> git@github.com:asias/qemu.git blk.vhost-blk
>>
>> Signed-off-by: Asias He <asias@redhat.com>
>> ---
>> drivers/vhost/Kconfig | 1 +
>> drivers/vhost/Kconfig.blk | 10 +
>> drivers/vhost/Makefile | 2 +
>> drivers/vhost/blk.c | 641 ++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/vhost/blk.h | 8 +
>> 5 files changed, 662 insertions(+)
>> create mode 100644 drivers/vhost/Kconfig.blk
>> create mode 100644 drivers/vhost/blk.c
>> create mode 100644 drivers/vhost/blk.h
>>
>> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
>> index 202bba6..acd8038 100644
>> --- a/drivers/vhost/Kconfig
>> +++ b/drivers/vhost/Kconfig
>> @@ -11,4 +11,5 @@ config VHOST_NET
>>
>> if STAGING
>> source "drivers/vhost/Kconfig.tcm"
>> +source "drivers/vhost/Kconfig.blk"
>> endif
>> diff --git a/drivers/vhost/Kconfig.blk b/drivers/vhost/Kconfig.blk
>> new file mode 100644
>> index 0000000..ff8ab76
>> --- /dev/null
>> +++ b/drivers/vhost/Kconfig.blk
>> @@ -0,0 +1,10 @@
>> +config VHOST_BLK
>> + tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
>> + depends on BLOCK && EXPERIMENTAL && m
>> + ---help---
>> + This kernel module can be loaded in host kernel to accelerate
>> + guest block with virtio_blk. Not to be confused with virtio_blk
>> + module itself which needs to be loaded in guest kernel.
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called vhost_blk.
>> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
>> index a27b053..1a8a4a5 100644
>> --- a/drivers/vhost/Makefile
>> +++ b/drivers/vhost/Makefile
>> @@ -2,3 +2,5 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o
>> vhost_net-y := vhost.o net.o
>>
>> obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
>> +obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
>> +vhost_blk-y := blk.o
>> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
>> new file mode 100644
>> index 0000000..6b2445a
>> --- /dev/null
>> +++ b/drivers/vhost/blk.c
>> @@ -0,0 +1,641 @@
>> +/*
>> + * Copyright (C) 2011 Taobao, Inc.
>> + * Author: Liu Yuan <tailai.ly@taobao.com>
>> + *
>> + * Copyright (C) 2012 Red Hat, Inc.
>> + * Author: Asias He <asias@redhat.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.
>> + *
>> + * virtio-blk server in host kernel.
>> + */
>> +
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/vhost.h>
>> +#include <linux/virtio_blk.h>
>> +#include <linux/mutex.h>
>> +#include <linux/file.h>
>> +#include <linux/kthread.h>
>> +#include <linux/blkdev.h>
>> +
>> +#include "vhost.c"
>> +#include "vhost.h"
>> +#include "blk.h"
>> +
>> +#define BLK_HDR 0
>
> What's this for, exactly? Please add a comment.
The block headr is in the first and separate buffer.
>> +
>> +static DEFINE_IDA(vhost_blk_index_ida);
>> +
>> +enum {
>> + VHOST_BLK_VQ_REQ = 0,
>> + VHOST_BLK_VQ_MAX = 1,
>> +};
>> +
>> +struct req_page_list {
>> + struct page **pages;
>> + int pages_nr;
>> +};
>> +
>> +struct vhost_blk_req {
>> + struct llist_node llnode;
>> + struct req_page_list *pl;
>> + struct vhost_blk *blk;
>> +
>> + struct iovec *iov;
>> + int iov_nr;
>> +
>> + struct bio **bio;
>> + atomic_t bio_nr;
>> +
>> + sector_t sector;
>> + int write;
>> + u16 head;
>> + long len;
>> +
>> + u8 *status;
>
> Is this a userspace pointer? If yes it must be tagged as such.
Will fix.
> Please run code checker - it will catch other bugs for you too.
Could you name one that you use?
>> +};
>> +
>> +struct vhost_blk {
>> + struct task_struct *host_kick;
>> + struct vhost_blk_req *reqs;
>> + struct vhost_virtqueue vq;
>> + struct llist_head llhead;
>> + struct vhost_dev dev;
>> + u16 reqs_nr;
>> + int index;
>> +};
>> +
>> +static inline int iov_num_pages(struct iovec *iov)
>> +{
>> + return (PAGE_ALIGN((unsigned long)iov->iov_base + iov->iov_len) -
>> + ((unsigned long)iov->iov_base & PAGE_MASK)) >> PAGE_SHIFT;
>> +}
>> +
>> +static int vhost_blk_setup(struct vhost_blk *blk)
>> +{
>> + blk->reqs_nr = blk->vq.num;
>> +
>> + blk->reqs = kmalloc(sizeof(struct vhost_blk_req) * blk->reqs_nr,
>> + GFP_KERNEL);
>> + if (!blk->reqs)
>> + return -ENOMEM;
>> +
>> + return 0;
>> +}
>> +
>> +static inline int vhost_blk_set_status(struct vhost_blk_req *req, u8 status)
>> +{
>> + struct vhost_blk *blk = req->blk;
>> +
>> + if (copy_to_user(req->status, &status, sizeof(status))) {
>
> Does this write into guest memory, right? This write needs to be tracked in
> log in case it's enabled.
Yes. Log is not enabled currently. I am wondering why is the log useful?
> Also, __copy_to_user should be enough here, right?
>
Hmm, why?
>> + vq_err(&blk->vq, "Failed to write status\n");
>> + vhost_discard_vq_desc(&blk->vq, 1);
>> + return -EFAULT;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void vhost_blk_enable_vq(struct vhost_blk *blk,
>> + struct vhost_virtqueue *vq)
>> +{
>> + wake_up_process(blk->host_kick);
>> +}
>> +
>> +static void vhost_blk_req_done(struct bio *bio, int err)
>> +{
>> + struct vhost_blk_req *req = bio->bi_private;
>> + struct vhost_blk *blk = req->blk;
>> +
>> + if (err)
>> + req->len = err;
>> +
>> + if (atomic_dec_and_test(&req->bio_nr)) {
>> + llist_add(&req->llnode, &blk->llhead);
>> + wake_up_process(blk->host_kick);
>> + }
>> +
>> + bio_put(bio);
>> +}
>> +
>> +static void vhost_blk_req_umap(struct vhost_blk_req *req)
>> +{
>> + struct req_page_list *pl;
>> + int i, j;
>> +
>> + if (!req->pl)
>> + return;
>> +
>> + for (i = 0; i < req->iov_nr; i++) {
>> + pl = &req->pl[i];
>> + for (j = 0; j < pl->pages_nr; j++) {
>> + if (!req->write)
>> + set_page_dirty_lock(pl->pages[j]);
>> + page_cache_release(pl->pages[j]);
>> + }
>> + }
>> +
>> + kfree(req->pl);
>> +}
>> +
>> +static int vhost_blk_bio_make(struct vhost_blk_req *req,
>> + struct block_device *bdev)
>> +{
>> + int pages_nr_total, i, j, ret;
>> + struct iovec *iov = req->iov;
>> + int iov_nr = req->iov_nr;
>> + struct page **pages, *page;
>> + struct bio *bio = NULL;
>> + int bio_nr = 0;
>> +
>> + req->len = 0;
>> + pages_nr_total = 0;
>> + for (i = 0; i < iov_nr; i++) {
>> + req->len += iov[i].iov_len;
>> + pages_nr_total += iov_num_pages(&iov[i]);
>> + }
>> +
>> + req->pl = kmalloc((iov_nr * sizeof(struct req_page_list)) +
>> + (pages_nr_total * sizeof(struct page *)) +
>> + (pages_nr_total * sizeof(struct bio *)),
>> + GFP_KERNEL);
>> + if (!req->pl)
>> + return -ENOMEM;
>> + pages = (struct page **)&req->pl[iov_nr];
>> + req->bio = (struct bio **)&pages[pages_nr_total];
>> +
>> + req->iov_nr = 0;
>> + for (i = 0; i < iov_nr; i++) {
>> + int pages_nr = iov_num_pages(&iov[i]);
>> + unsigned long iov_base, iov_len;
>> + struct req_page_list *pl;
>> +
>> + iov_base = (unsigned long)iov[i].iov_base;
>> + iov_len = (unsigned long)iov[i].iov_len;
>> +
>> + ret = get_user_pages_fast(iov_base, pages_nr,
>> + !req->write, pages);
>> + if (ret != pages_nr)
>> + goto fail;
>> +
>> + req->iov_nr++;
>> + pl = &req->pl[i];
>> + pl->pages_nr = pages_nr;
>> + pl->pages = pages;
>> +
>> + for (j = 0; j < pages_nr; j++) {
>> + unsigned int off, len;
>> + page = pages[j];
>> + off = iov_base & ~PAGE_MASK;
>> + len = PAGE_SIZE - off;
>> + if (len > iov_len)
>> + len = iov_len;
>> +
>> + while (!bio || bio_add_page(bio, page, len, off) <= 0) {
>> + bio = bio_alloc(GFP_KERNEL, pages_nr);
>> + if (!bio)
>> + goto fail;
>> + bio->bi_sector = req->sector;
>> + bio->bi_bdev = bdev;
>> + bio->bi_private = req;
>> + bio->bi_end_io = vhost_blk_req_done;
>> + req->bio[bio_nr++] = bio;
>> + }
>> + req->sector += len >> 9;
>> + iov_base += len;
>> + iov_len -= len;
>> + }
>> +
>> + pages += pages_nr;
>> + }
>> + atomic_set(&req->bio_nr, bio_nr);
>> +
>> + return 0;
>> +
>> +fail:
>> + for (i = 0; i < bio_nr; i++)
>> + bio_put(req->bio[i]);
>> + vhost_blk_req_umap(req);
>> + return -ENOMEM;
>> +}
>> +
>> +static inline void vhost_blk_bio_send(struct vhost_blk_req *req)
>> +{
>> + struct blk_plug plug;
>> + int i, bio_nr;
>> +
>> + bio_nr = atomic_read(&req->bio_nr);
>> + blk_start_plug(&plug);
>> + for (i = 0; i < bio_nr; i++)
>> + submit_bio(req->write, req->bio[i]);
>> + blk_finish_plug(&plug);
>> +}
>> +
>> +static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file *file)
>> +{
>> +
>> + struct inode *inode = file->f_mapping->host;
>> + struct block_device *bdev = inode->i_bdev;
>> + int ret;
>> +
>> + ret = vhost_blk_bio_make(req, bdev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + vhost_blk_bio_send(req);
>> +
>> + return ret;
>> +}
>> +
>> +static int vhost_blk_req_done_thread(void *data)
>> +{
>> + mm_segment_t oldfs = get_fs();
>> + struct vhost_blk *blk = data;
>> + struct vhost_virtqueue *vq;
>> + struct llist_node *llnode;
>> + struct vhost_blk_req *req;
>> + bool added;
>> + u8 status;
>> + int ret;
>> +
>> + vq = &blk->vq;
>> + set_fs(USER_DS);
>> + use_mm(blk->dev.mm);
>> + for (;;) {
>> + llnode = llist_del_all(&blk->llhead);
>
>
> Interesting, I didn't consider llist - maybe vhost.c
> could switch to that too? If we do how to handle flushing?
> If we do we can move some common code out here.
Will take a look.
>> + if (!llnode) {
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + schedule();
>> + if (unlikely(kthread_should_stop()))
>> + break;
>> + continue;
>> + }
>
> I think you need to call something like
> if (need_resched())
> schedule();
> once in a while even if the list is not empty.
Yes. We need similar stuff as commit
d550dda192c1bd039afb774b99485e88b70d7cb8 did.
I had this in the some previous versions. Somehow it's not here.
>> + added = false;
>> + while (llnode) {
>> + req = llist_entry(llnode, struct vhost_blk_req, llnode);
>> + llnode = llist_next(llnode);
>> +
>> + vhost_blk_req_umap(req);
>> +
>> + status = req->len > 0 ?
>> + VIRTIO_BLK_S_OK : VIRTIO_BLK_S_IOERR;
>> + ret = copy_to_user(req->status, &status,
>> + sizeof(status));
>
> use vhost_blk_set_status? Why not?
Okay.
>> + if (unlikely(ret)) {
>> + vq_err(&blk->vq, "Failed to write status\n");
>> + return -1;
>
> This will kill this thread. Likely not what was intended.
Yes, it kill this thread. But I am wondering when and how this
copy_to_user() of status would fail and what is the best thing to do in
this case: ignore the status and call vhost_add_used() anyway or ...
>> + }
>> + vhost_add_used(&blk->vq, req->head, req->len);
>> + added = true;
>> + }
>> + if (likely(added))
>> + vhost_signal(&blk->dev, &blk->vq);
>> +
>
> Pls dont add empty line here.
okay.
>> + }
>> + unuse_mm(blk->dev.mm);
>> + set_fs(oldfs);
>> + return 0;
>> +}
>> +
>> +static void vhost_blk_flush(struct vhost_blk *blk)
>> +{
>> + vhost_poll_flush(&blk->vq.poll);
>
> Hmm but blk kthread could still be processing requests, no?
> Need to flush these too I think?
The blk kthread does not access the *rcu* protected vq->private_data
(file). Do we still need the flush for it?
>> +}
>> +
>> +static struct file *vhost_blk_stop_vq(struct vhost_blk *blk,
>> + struct vhost_virtqueue *vq)
>> +{
>> + struct file *file;
>> +
>> + mutex_lock(&vq->mutex);
>> + file = rcu_dereference_protected(vq->private_data,
>> + lockdep_is_held(&vq->mutex));
>> + rcu_assign_pointer(vq->private_data, NULL);
>> + mutex_unlock(&vq->mutex);
>> +
>> + return file;
>> +
>> +}
>> +
>> +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
>> +{
>> +
>> + *file = vhost_blk_stop_vq(blk, &blk->vq);
>
> Is this wrapper worth it? Also maybe just return file?
Hmm. Okay. I will kill vhost_blk_stop_vq() and move it to
vhost_blk_stop(). I wanted it to be simialr with vhost_net_stop().
>> +}
>> +
>> +/* Handle guest request */
>> +static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
>> + struct virtio_blk_outhdr *hdr,
>> + u16 head, u16 out, u16 in,
>> + struct file *file)
>> +{
>> + struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
>> + struct vhost_blk_req *req;
>> + int iov_nr, ret;
>> + u8 status;
>> +
>> + if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID)
>> + iov_nr = in - 1;
>> + else
>> + iov_nr = out - 1;
>> +
>> + req = &blk->reqs[head];
>> + req->head = head;
>> + req->status = blk->vq.iov[iov_nr + 1].iov_base;
>> + req->blk = blk;
>> + req->iov = &vq->iov[BLK_HDR + 1];
>
> Lots of manual mangling of iovecs here and elsewhere is scary.
> First, there should not be so many assumptions about how buffers
> are laid out.
virtio-blk.c do set the buffer layout this way, no?
> Second, there seems to be no validation that iovec
> is large enough. It is preferable to use memcpy_toiovecend and friends
> which validate input. This applied to many places below, please
> audit code for such uses. Where you find it necessary to
> handle iovec directly, please add comments explaining where
> it's validated and why it's safe.
The vq->iov is defined as vq->iov[UIO_MAXIOV]. The iov_nr is based on
the in and out buffer number. The largest queue size I see is 256 in kvm
tool. qemu is 128. What do we need to validate here?
btw, memcpy_toiovecend() is in net/core/iovec.c.
>
>
>> + req->iov_nr = iov_nr;
>> + req->sector = hdr->sector;
>> +
>> + switch (hdr->type) {
>> + case VIRTIO_BLK_T_OUT:
>> + req->write = WRITE;
>> + ret = vhost_blk_req_submit(req, file);
>> + break;
>> + case VIRTIO_BLK_T_IN:
>> + req->write = READ;
>> + ret = vhost_blk_req_submit(req, file);
>> + break;
>> + case VIRTIO_BLK_T_FLUSH:
>> + ret = vfs_fsync(file, 1);
>> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
>> + if (!vhost_blk_set_status(req, status))
>> + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
>
> This should discard on error, no? Also return error to caller?
> r = vhost_blk_set_status(req, status);
> if (r) {
> ret = r;
> break;
> }
> vhost_add_used_and_signal(&blk->dev, vq, head, ret);
> return 0;
>
> and move discard outside switch below.
The flush code is changed in v3 already.
>> + break;
>> + case VIRTIO_BLK_T_GET_ID:
>> + ret = snprintf(vq->iov[BLK_HDR + 1].iov_base,
>> + VIRTIO_BLK_ID_BYTES, "vhost-blk%d", blk->index);
>
> snprintf into a userspace buffer? Uh oh.
Ah, will fix this *crap*.
>
>> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
>> + if (!vhost_blk_set_status(req, status))
>> + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
>> + break;
>> + default:
>> + pr_warn("Unsupported request type %d\n", hdr->type);
>
> This can be triggered by userspace so vq_err?
Okay.
>
>> + vhost_discard_vq_desc(vq, 1);
>
> Note this does not skip this descriptor - it gives userspace
> chance to correct it and retry. Is this the intended behaviour?
> Should not we fail request instead?
We should fail the request here.
>
>> + ret = -EFAULT;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/* Guest kick us for IO request */
>> +static void vhost_blk_handle_guest_kick(struct vhost_work *work)
>> +{
>> + struct virtio_blk_outhdr hdr;
>> + struct vhost_virtqueue *vq;
>> + struct vhost_blk *blk;
>> + struct blk_plug plug;
>> + struct file *f;
>> + int in, out;
>> + u16 head;
>> +
>> + vq = container_of(work, struct vhost_virtqueue, poll.work);
>> + blk = container_of(vq->dev, struct vhost_blk, dev);
>> +
>> + /* TODO: check that we are running from vhost_worker? */
>> + f = rcu_dereference_check(vq->private_data, 1);
>> + if (!f)
>> + return;
>> +
>> + vhost_disable_notify(&blk->dev, vq);
>> + blk_start_plug(&plug);
>> + for (;;) {
>> + head = vhost_get_vq_desc(&blk->dev, vq, vq->iov,
>> + ARRAY_SIZE(vq->iov),
>> + &out, &in, NULL, NULL);
>> + if (unlikely(head < 0))
>> + break;
>> +
>> + if (unlikely(head == vq->num)) {
>> + if (unlikely(vhost_enable_notify(&blk->dev, vq))) {
>> + vhost_disable_notify(&blk->dev, vq);
>> + continue;
>> + }
>> + break;
>> + }
>> +
>> + if (unlikely(copy_from_user(&hdr, vq->iov[BLK_HDR].iov_base,
>> + sizeof(hdr)))) {
>> + vq_err(vq, "Failed to get block header!\n");
>> + vhost_discard_vq_desc(vq, 1);
>> + break;
>> + }
>> +
>> + if (vhost_blk_req_handle(vq, &hdr, head, out, in, f) < 0)
>> + break;
>> + }
>> + blk_finish_plug(&plug);
>> +}
>> +
>> +static int vhost_blk_open(struct inode *inode, struct file *file)
>> +{
>> + struct vhost_blk *blk;
>> + int ret;
>> +
>> + blk = kzalloc(sizeof(*blk), GFP_KERNEL);
>> + if (!blk) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + ret = ida_simple_get(&vhost_blk_index_ida, 0, 0, GFP_KERNEL);
>> + if (ret < 0)
>> + goto out_dev;
>> + blk->index = ret;
>> +
>> + blk->vq.handle_kick = vhost_blk_handle_guest_kick;
>> +
>> + ret = vhost_dev_init(&blk->dev, &blk->vq, VHOST_BLK_VQ_MAX);
>> + if (ret < 0)
>> + goto out_dev;
>> + file->private_data = blk;
>> +
>> + blk->host_kick = kthread_create(vhost_blk_req_done_thread,
>> + blk, "vhost-blk-%d", current->pid);
>> + if (IS_ERR(blk->host_kick)) {
>> + ret = PTR_ERR(blk->host_kick);
>> + goto out_dev;
>> + }
>> +
>> + return ret;
>> +out_dev:
>> + kfree(blk);
>> +out:
>> + return ret;
>> +}
>> +
>> +static int vhost_blk_release(struct inode *inode, struct file *f)
>> +{
>> + struct vhost_blk *blk = f->private_data;
>> + struct file *file;
>> +
>> + ida_simple_remove(&vhost_blk_index_ida, blk->index);
>> + vhost_blk_stop(blk, &file);
>> + vhost_blk_flush(blk);
>> + vhost_dev_cleanup(&blk->dev, false);
>> + if (file)
>> + fput(file);
>> + kthread_stop(blk->host_kick);
>> + kfree(blk->reqs);
>> + kfree(blk);
>> +
>> + return 0;
>> +}
>> +
>> +static int vhost_blk_set_features(struct vhost_blk *blk, u64 features)
>> +{
>> + mutex_lock(&blk->dev.mutex);
>> + blk->dev.acked_features = features;
>> + mutex_unlock(&blk->dev.mutex);
>
> We also need to flush outstanding requets normally.
> If not needed here pls add a comment why.
Will add a flush here.
>> +
>> + return 0;
>> +}
>> +
>> +static long vhost_blk_set_backend(struct vhost_blk *blk, unsigned index, int fd)
>> +{
>> + struct vhost_virtqueue *vq = &blk->vq;
>> + struct file *file, *oldfile;
>> + int ret;
>> +
>> + mutex_lock(&blk->dev.mutex);
>> + ret = vhost_dev_check_owner(&blk->dev);
>> + if (ret)
>> + goto out_dev;
>> +
>> + if (index >= VHOST_BLK_VQ_MAX) {
>> + ret = -ENOBUFS;
>> + goto out_dev;
>> + }
>> +
>> + mutex_lock(&vq->mutex);
>> +
>> + if (!vhost_vq_access_ok(vq)) {
>> + ret = -EFAULT;
>> + goto out_vq;
>> + }
>> +
>> + file = fget(fd);
>> + if (IS_ERR(file)) {
>> + ret = PTR_ERR(file);
>> + goto out_vq;
>> + }
>> +
>> + oldfile = rcu_dereference_protected(vq->private_data,
>> + lockdep_is_held(&vq->mutex));
>> + if (file != oldfile) {
>> + rcu_assign_pointer(vq->private_data, file);
>> + vhost_blk_enable_vq(blk, vq);
>> +
>> + ret = vhost_init_used(vq);
>> + if (ret)
>> + goto out_vq;
>> + }
>> +
>> + mutex_unlock(&vq->mutex);
>> +
>> + if (oldfile) {
>> + vhost_blk_flush(blk);
>> + fput(oldfile);
>> + }
>> +
>> + mutex_unlock(&blk->dev.mutex);
>> + return 0;
>> +
>> +out_vq:
>> + mutex_unlock(&vq->mutex);
>> +out_dev:
>> + mutex_unlock(&blk->dev.mutex);
>> + return ret;
>> +}
>> +
>> +static long vhost_blk_reset_owner(struct vhost_blk *blk)
>> +{
>> + struct file *file = NULL;
>> + int err;
>> +
>> + mutex_lock(&blk->dev.mutex);
>> + err = vhost_dev_check_owner(&blk->dev);
>> + if (err)
>> + goto done;
>> + vhost_blk_stop(blk, &file);
>> + vhost_blk_flush(blk);
>> + err = vhost_dev_reset_owner(&blk->dev);
>> +done:
>> + mutex_unlock(&blk->dev.mutex);
>> + if (file)
>> + fput(file);
>> + return err;
>> +}
>> +
>> +static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
>> + unsigned long arg)
>> +{
>> + struct vhost_blk *blk = f->private_data;
>> + void __user *argp = (void __user *)arg;
>> + struct vhost_vring_file backend;
>> + u64 __user *featurep = argp;
>> + u64 features;
>> + int ret;
>> +
>> + switch (ioctl) {
>> + case VHOST_BLK_SET_BACKEND:
>> + if (copy_from_user(&backend, argp, sizeof(backend)))
>> + return -EFAULT;
>> + return vhost_blk_set_backend(blk, backend.index, backend.fd);
>> + case VHOST_GET_FEATURES:
>> + features = VHOST_BLK_FEATURES;
>> + if (copy_to_user(featurep, &features, sizeof(features)))
>> + return -EFAULT;
>> + return 0;
>> + case VHOST_SET_FEATURES:
>> + if (copy_from_user(&features, featurep, sizeof(features)))
>> + return -EFAULT;
>> + if (features & ~VHOST_BLK_FEATURES)
>> + return -EOPNOTSUPP;
>> + return vhost_blk_set_features(blk, features);
>> + case VHOST_RESET_OWNER:
>> + return vhost_blk_reset_owner(blk);
>> + default:
>> + mutex_lock(&blk->dev.mutex);
>> + ret = vhost_dev_ioctl(&blk->dev, ioctl, arg);
>> + if (!ret && ioctl == VHOST_SET_VRING_NUM)
>> + ret = vhost_blk_setup(blk);
>> + vhost_blk_flush(blk);
>> + mutex_unlock(&blk->dev.mutex);
>> + return ret;
>> + }
>> +}
>> +
>> +static const struct file_operations vhost_blk_fops = {
>> + .owner = THIS_MODULE,
>> + .open = vhost_blk_open,
>> + .release = vhost_blk_release,
>> + .llseek = noop_llseek,
>> + .unlocked_ioctl = vhost_blk_ioctl,
>> +};
>> +
>> +static struct miscdevice vhost_blk_misc = {
>> + MISC_DYNAMIC_MINOR,
>> + "vhost-blk",
>> + &vhost_blk_fops,
>> +};
>> +
>> +int vhost_blk_init(void)
>> +{
>> + return misc_register(&vhost_blk_misc);
>> +}
>> +
>> +void vhost_blk_exit(void)
>> +{
>> + misc_deregister(&vhost_blk_misc);
>> +}
>> +
>> +module_init(vhost_blk_init);
>> +module_exit(vhost_blk_exit);
>> +
>> +MODULE_VERSION("0.0.3");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Asias He");
>> +MODULE_DESCRIPTION("Host kernel accelerator for virtio_blk");
>> diff --git a/drivers/vhost/blk.h b/drivers/vhost/blk.h
>> new file mode 100644
>> index 0000000..2f674f0
>> --- /dev/null
>> +++ b/drivers/vhost/blk.h
>> @@ -0,0 +1,8 @@
>> +#include <linux/vhost.h>
>> +
>> +enum {
>> + VHOST_BLK_FEATURES = (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
>> + (1ULL << VIRTIO_RING_F_EVENT_IDX),
>> +};
>> +/* VHOST_BLK specific defines */
>> +#define VHOST_BLK_SET_BACKEND _IOW(VHOST_VIRTIO, 0x50, struct vhost_vring_file)
>> --
>> 1.7.11.4
Thanks.
--
Asias
^ 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