* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: Michael S. Tsirkin @ 2015-12-15 20:40 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stefano Stabellini, Benjamin Herrenschmidt,
linux-kernel@vger.kernel.org, Linux Virtualization, David Vrabel,
xen-devel@lists.xenproject.org
In-Reply-To: <CALCETrVEfrXNJE1Wq3piF7KPExk+4ByBP+rHHq2jWhCAURjcyA@mail.gmail.com>
On Mon, Dec 14, 2015 at 10:27:52AM -0800, Andy Lutomirski wrote:
> On Mon, Dec 14, 2015 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Dec 14, 2015 at 02:00:05PM +0000, David Vrabel wrote:
> >> On 07/12/15 16:19, Stefano Stabellini wrote:
> >> > Hi all,
> >> >
> >> > this patch series introduces support for running Linux on top of Xen
> >> > inside a virtual machine with virtio devices (nested virt scenario).
> >> > The problem is that Linux virtio drivers use virt_to_phys to get the
> >> > guest pseudo-physical addresses to pass to the backend, which doesn't
> >> > work as expected on Xen.
> >> >
> >> > Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
> >> > dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
> >> > Xen support in Linux provides an implementation of the dma API which
> >> > takes care of the additional address conversions. However using the dma
> >> > API would increase the complexity of the non-Xen case too. We would also
> >> > need to keep track of the physical or virtual address in addition to the
> >> > dma address for each vring_desc to be able to free the memory in
> >> > detach_buf (see patch #3).
> >> >
> >> > Instead this series adds few obvious checks to perform address
> >> > translations in a couple of key places, without changing non-Xen code
> >> > paths. You are welcome to suggest improvements or alternative
> >> > implementations.
> >>
> >> Andy Lutomirski also looked at this. Andy what happened to this work?
> >>
> >> David
> >
> > The approach there was to try and convert all virtio to use DMA
> > API unconditionally.
> > This is reasonable if there's a way for devices to request
> > 1:1 mappings individually.
> > As that is currently missing, that patchset can not be merged yet.
> >
>
> I still don't understand why *devices* need the ability to request
> anything in particular.
See below.
> In current kernels, devices that don't have
> an iommu work (and there's no choice about 1:1 or otherwise) and
> devices that have an iommu fail spectacularly. With the patches,
> devices that don't have an iommu continue to work as long as the DMA
> API and/or virtio correctly knows that there's no iommu. Devices that
> do have an iommu work fine, albeit slower than would be ideal. In my
> book, slower than would be ideal is strictly better than crashing.
>
> The real issue is *detecting* whether there's an iommu, and the string
> of bugs in that area (buggy QEMU for the Q35 thing and complete lack
> of a solution for PPC and SPARC is indeed a problem).
>
> I think that we could apply the series ending here:
>
> https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=virtio_dma&id=ad9d43052da44ce18363c02ea597dde01eeee11b
>
> and the only regression (performance or functionality) would be that
> the buggy Q35 iommu configuration would stop working until someone
> fixed it in QEMU. That should be okay -- it's explicitly
> experimental. (Xen works with that series applied.) (Actually,
> there might be a slight performance regression on PPC due to extra
> unused mappings being created. It would be straightforward to hack
> around that in one of several ways.)
>
> Am I missing something?
>
> --Andy
I think there's more to virtio than just QEMU.
I have no idea whether anyone implemented hypervisors with an IOMMU.
virtio bypassing iommu makes a lot of sense so it did this since
forever. I do not feel comfortable changing guest/hypervisor ABI and
waiting for people to complain.
But we do want to fix Xen.
Let's do this slowly, and whitelist the configurations that
require DMA API to work, so we know we are not breaking anything.
For example, test a device flag and use iommu if set.
Currently, set it if xen_pv_domain is enabled.
We'll add more as more platforms gain IOMMU support
for virtio and we find ways to identify them.
It would be kind of a mix of what you did and what Stefano did.
And alternative would be a quirk: make DMA API create 1:1 mappings for
virtio devices only. Then teach Xen pv to ignore this quirk. This is
what I referred to above.
For example, something like DMA_ATTR_IOMMU_BYPASS would do the trick
nicely. If there's a chance that's going to be upstream, we
could use that.
--
MST
^ permalink raw reply
* Re: [Xen-devel] kasan_map_early_shadow() on Xen
From: Luis R. Rodriguez @ 2015-12-15 20:02 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, H. Peter Anvin
Cc: Jeremy Fitzhardinge, Andrey Ryabinin,
linux-kernel@vger.kernel.org, Andy Lutomirski, Chris Wright,
xen-devel, Boris Ostrovsky, virtualization, Alok Kataria
In-Reply-To: <20150306164754.GD25035@wotan.suse.de>
On Fri, Mar 6, 2015 at 8:47 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> If I had a technical solution to this problem I'd propose it but I don't,
> I'm just flagging it right now and hope we can come up with one.
Shortly I'll be posting part of a proactive solution I believe we
could consider for these sorts of issues.
Luis
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: Andy Lutomirski @ 2015-12-15 16:45 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Michael S. Tsirkin, Benjamin Herrenschmidt,
linux-kernel@vger.kernel.org, Linux Virtualization, David Vrabel,
xen-devel@lists.xenproject.org
In-Reply-To: <alpine.DEB.2.02.1512151201190.17516@kaball.uk.xensource.com>
On Tue, Dec 15, 2015 at 4:13 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 14 Dec 2015, Andy Lutomirski wrote:
>> On Mon, Dec 14, 2015 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Dec 14, 2015 at 02:00:05PM +0000, David Vrabel wrote:
>> >> On 07/12/15 16:19, Stefano Stabellini wrote:
>> >> > Hi all,
>> >> >
>> >> > this patch series introduces support for running Linux on top of Xen
>> >> > inside a virtual machine with virtio devices (nested virt scenario).
>> >> > The problem is that Linux virtio drivers use virt_to_phys to get the
>> >> > guest pseudo-physical addresses to pass to the backend, which doesn't
>> >> > work as expected on Xen.
>> >> >
>> >> > Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
>> >> > dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
>> >> > Xen support in Linux provides an implementation of the dma API which
>> >> > takes care of the additional address conversions. However using the dma
>> >> > API would increase the complexity of the non-Xen case too. We would also
>> >> > need to keep track of the physical or virtual address in addition to the
>> >> > dma address for each vring_desc to be able to free the memory in
>> >> > detach_buf (see patch #3).
>> >> >
>> >> > Instead this series adds few obvious checks to perform address
>> >> > translations in a couple of key places, without changing non-Xen code
>> >> > paths. You are welcome to suggest improvements or alternative
>> >> > implementations.
>> >>
>> >> Andy Lutomirski also looked at this. Andy what happened to this work?
>> >>
>> >> David
>> >
>> > The approach there was to try and convert all virtio to use DMA
>> > API unconditionally.
>> > This is reasonable if there's a way for devices to request
>> > 1:1 mappings individually.
>> > As that is currently missing, that patchset can not be merged yet.
>> >
>>
>> I still don't understand why *devices* need the ability to request
>> anything in particular. In current kernels, devices that don't have
>> an iommu work (and there's no choice about 1:1 or otherwise) and
>> devices that have an iommu fail spectacularly. With the patches,
>> devices that don't have an iommu continue to work as long as the DMA
>> API and/or virtio correctly knows that there's no iommu. Devices that
>> do have an iommu work fine, albeit slower than would be ideal. In my
>> book, slower than would be ideal is strictly better than crashing.
>>
>> The real issue is *detecting* whether there's an iommu, and the string
>> of bugs in that area (buggy QEMU for the Q35 thing and complete lack
>> of a solution for PPC and SPARC is indeed a problem).
>>
>> I think that we could apply the series ending here:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=virtio_dma&id=ad9d43052da44ce18363c02ea597dde01eeee11b
>>
>> and the only regression (performance or functionality) would be that
>> the buggy Q35 iommu configuration would stop working until someone
>> fixed it in QEMU. That should be okay -- it's explicitly
>> experimental. (Xen works with that series applied.) (Actually,
>> there might be a slight performance regression on PPC due to extra
>> unused mappings being created. It would be straightforward to hack
>> around that in one of several ways.)
>>
>> Am I missing something?
>
> Your changes look plausible and if they fix Xen on virtio I am happy
> with them. I didn't choose the DMA API approach because, although it
> looks cleaner, I acknowledge that is a bit invasive.
>
> I suggest that the virtio maintainers consider one of the two approaches
> for inclusion because they fix a real issue.
>
> If you would rather avoid the DMA API, then I would be happy to work
> with you to evolve my current series in a direction of your liking.
> Please advise on how to proceed.
I would rather use the DMA API, but we need Michael's buy-in for that.
Michael, if I further modify the driver so that even the dma
allocations don't happen if the in-driver quirk is set, and we set the
quirk on sparc and ppc but clear it on x86 and presumably all the
other architectures, would that be okay with you?
--Andy
^ permalink raw reply
* [CISTI'2016]: Doctoral Symposium
From: Maria Lemos @ 2015-12-15 16:40 UTC (permalink / raw)
To: virtualization
[-- Attachment #1: Type: text/plain, Size: 3853 bytes --]
--------------------- Doctoral Symposium of CISTI'2016 -------------------------
CISTI'2016 - 11th Iberian Conference on Information Systems and Technologies
15 - 18 june, 2016, Gran Canaria Island, Span
http://www.aisti.eu/cisti2016/index.php/en/doctoral-symposium
--------------------------------------------------------------------------------
The purpose of CISTI'2016s Doctoral Symposium is to provide graduate students a setting where they can, informally, expose and discuss their work, collecting valuable expert opinions and sharing new ideas, methods and applications. The Doctoral Symposium is an excellent opportunity for PhD students to present and discuss their work in a Workshop format. Each presentation will be evaluated by a panel composed by at least three Information Systems and Technologies experts.
CONTRIBUTIONS SUBMISSION
The Doctoral Symposium is opened to PhD students whose research area includes the themes proposed for this Conference. Submissions must include an extended abstract (maximum 4 pages), following the Conference style guide. All selected contributions will be handed out along with the Conference Proceedings, in CD with an ISBN. These contributions will be available in the IEEE Xplore Digital Library and will be sent for indexing in ISI, Scopus, EI-Compendex, INSPEC and Google Scholar.
Submissions must include the field, the PhD institution and the number of months devoted to the development of the work. Additionally, they should include in a clear and succinct manner:
The problem approached and its significance or relevance
The research objectives and related investigation topics
A brief display of what is already known
A proposed solution methodology for the problem
Expected results
IMPORTANT DATES
Proposal submission deadline: 14th of February 2016
Acceptance notification: 27th of March 2016
Final submission deadline: 10th of April 2016
Registration payment, to ensure the inclusion of the accepted contribution in the Conference Proceedings: 8th of April 2016
ORGANIZING & SCIENTIFIC COMMITTEE
Manuel Pérez Cota, University of Vigo (Presidente)
A. Augusto Sousa, FEUP, University of Porto
Adolfo Lozano Tello, University of Extremadura
Alberto J. Bugarín Diz, University of Santiago de Compostela
Alma María Gómez Rodríguez, University of Vigo
Álvaro Rocha, University of Coimbra
Ana Maria Correia, Nova IMS
António Lucas Soares, FEUP, University of Porto
Arnaldo Martins, University of Aveiro
Bráulio Alturas, ISCTE-IUL
Carlos Ferrás Sexto, University of Santiago de Compostela
David Fonseca, La Salle, University Ramon Llull
Ernest Redondo, Polytechnic University of Catalonia
Eugénio Oliveira, FEUP, University of Porto
Evelio Gonzalez, University of La Laguna
Fernando Moreira, Portucalense University
Gonçalo Paiva Dias, University of Aveiro
Gonzalo Cuevas Agustín, Technical University of Madrid
Guilhermina Lobato Miranda, University of Lisbon
João Costa, University of Coimbra
Jose Antonio Calvo-Manzano Villalón, Technical University of Madrid
José Borbinha, IST, University of Lisbon
Luís Paulo Reis, University of Minho
Marco Painho, Nova IMS
María José Lado Touriño, University of Vigo
Mário Piattini, University of Castilla-La Mancha
Miguel Ramón González Castro, ENCE
Paulo Rupino da Cunha, University of Coimbra
Ramiro Gonçalves, University of Trás-os-Montes e Alto Douro
Ricardo Machado, University of Minho
Ruben González Crespo, International University de La Rioja
Tomás San Feliu, Technical University of Madrid
Doctoral Symposium webpage: http://www.aisti.eu/cisti2016/index.php/en/doctoral-symposium
Regards,
Maria Lemos
Secretariat of AISTI
http://www.aisti.eu/
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 1/3] x86/xen: Avoid fast syscall path for Xen PV guests
From: Andy Lutomirski @ 2015-12-15 15:57 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Konrad Rzeszutek Wilk, linux-kernel@vger.kernel.org,
Linux Virtualization, Ingo Molnar, David Vrabel,
Andrew Lutomirski, H. Peter Anvin, xen-devel@lists.xenproject.org,
Thomas Gleixner, Borislav Petkov
In-Reply-To: <567037BB.6080901@oracle.com>
On Tue, Dec 15, 2015 at 7:54 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 12/15/2015 10:48 AM, Borislav Petkov wrote:
>>
>> On Tue, Dec 15, 2015 at 10:21:37AM -0500, Boris Ostrovsky wrote:
>>>
>>> I know this has been in the tip tree --- when do you think this will go
>>> Linus tree? In the 4.4 timeframe?
>>
>> It is queued for 4.5 currently.
>>
>>> Xen 32-bit PV guests are broken without this.
>>
>> So this needs to go into 4.4 or even earlier? Since 5f310f739b4c went
>> into 4.4, I'd think 4.4 only for this fix too? Yes?
>
>
> Oh, I didn't realize this is scheduled for 4.5. It indeed needs to go into
> 4.4 because of 5f310f739b4c.
Agreed.
>
> Thanks.
> -boris
>
>
>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH v2 1/3] x86/xen: Avoid fast syscall path for Xen PV guests
From: Boris Ostrovsky @ 2015-12-15 15:54 UTC (permalink / raw)
To: Borislav Petkov
Cc: konrad.wilk, linux-kernel, virtualization, mingo, david.vrabel,
luto, hpa, xen-devel, tglx
In-Reply-To: <20151215154830.GF26000@pd.tnic>
On 12/15/2015 10:48 AM, Borislav Petkov wrote:
> On Tue, Dec 15, 2015 at 10:21:37AM -0500, Boris Ostrovsky wrote:
>> I know this has been in the tip tree --- when do you think this will go
>> Linus tree? In the 4.4 timeframe?
> It is queued for 4.5 currently.
>
>> Xen 32-bit PV guests are broken without this.
> So this needs to go into 4.4 or even earlier? Since 5f310f739b4c went
> into 4.4, I'd think 4.4 only for this fix too? Yes?
Oh, I didn't realize this is scheduled for 4.5. It indeed needs to go
into 4.4 because of 5f310f739b4c.
Thanks.
-boris
^ permalink raw reply
* Re: [PATCH v2 1/3] x86/xen: Avoid fast syscall path for Xen PV guests
From: Borislav Petkov @ 2015-12-15 15:48 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: konrad.wilk, linux-kernel, virtualization, mingo, david.vrabel,
luto, hpa, xen-devel, tglx
In-Reply-To: <56703001.9010205@oracle.com>
On Tue, Dec 15, 2015 at 10:21:37AM -0500, Boris Ostrovsky wrote:
> I know this has been in the tip tree --- when do you think this will go
> Linus tree? In the 4.4 timeframe?
It is queued for 4.5 currently.
> Xen 32-bit PV guests are broken without this.
So this needs to go into 4.4 or even earlier? Since 5f310f739b4c went
into 4.4, I'd think 4.4 only for this fix too? Yes?
Thanks.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 1/3] x86/xen: Avoid fast syscall path for Xen PV guests
From: Boris Ostrovsky @ 2015-12-15 15:21 UTC (permalink / raw)
To: tglx, mingo, hpa
Cc: konrad.wilk, linux-kernel, virtualization, david.vrabel, luto,
xen-devel, bp
In-Reply-To: <1447970147-1733-2-git-send-email-boris.ostrovsky@oracle.com>
On 11/19/2015 04:55 PM, Boris Ostrovsky wrote:
> After 32-bit syscall rewrite, and specifically after commit 5f310f739b4c
> ("x86/entry/32: Re-implement SYSENTER using the new C path"), the stack
> frame that is passed to xen_sysexit is no longer a "standard" one (i.e.
> it's not pt_regs).
>
> Since we end up calling xen_iret from xen_sysexit we don't need to fix
> up the stack and instead follow entry_SYSENTER_32's IRET path directly
> to xen_iret.
>
> We can do the same thing for compat mode even though stack does not need
> to be fixed. This will allow us to drop usergs_sysret32 paravirt op (in
> the subsequent patch)
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Suggested-by: Andy Lutomirski <luto@amacapital.net>
I know this has been in the tip tree --- when do you think this will go
Linus tree? In the 4.4 timeframe?
Xen 32-bit PV guests are broken without this.
Thanks.
-boris
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: Stefano Stabellini @ 2015-12-15 12:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stefano Stabellini, Benjamin Herrenschmidt, Michael S. Tsirkin,
linux-kernel@vger.kernel.org, Linux Virtualization, David Vrabel,
xen-devel@lists.xenproject.org
In-Reply-To: <CALCETrVEfrXNJE1Wq3piF7KPExk+4ByBP+rHHq2jWhCAURjcyA@mail.gmail.com>
On Mon, 14 Dec 2015, Andy Lutomirski wrote:
> On Mon, Dec 14, 2015 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Dec 14, 2015 at 02:00:05PM +0000, David Vrabel wrote:
> >> On 07/12/15 16:19, Stefano Stabellini wrote:
> >> > Hi all,
> >> >
> >> > this patch series introduces support for running Linux on top of Xen
> >> > inside a virtual machine with virtio devices (nested virt scenario).
> >> > The problem is that Linux virtio drivers use virt_to_phys to get the
> >> > guest pseudo-physical addresses to pass to the backend, which doesn't
> >> > work as expected on Xen.
> >> >
> >> > Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
> >> > dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
> >> > Xen support in Linux provides an implementation of the dma API which
> >> > takes care of the additional address conversions. However using the dma
> >> > API would increase the complexity of the non-Xen case too. We would also
> >> > need to keep track of the physical or virtual address in addition to the
> >> > dma address for each vring_desc to be able to free the memory in
> >> > detach_buf (see patch #3).
> >> >
> >> > Instead this series adds few obvious checks to perform address
> >> > translations in a couple of key places, without changing non-Xen code
> >> > paths. You are welcome to suggest improvements or alternative
> >> > implementations.
> >>
> >> Andy Lutomirski also looked at this. Andy what happened to this work?
> >>
> >> David
> >
> > The approach there was to try and convert all virtio to use DMA
> > API unconditionally.
> > This is reasonable if there's a way for devices to request
> > 1:1 mappings individually.
> > As that is currently missing, that patchset can not be merged yet.
> >
>
> I still don't understand why *devices* need the ability to request
> anything in particular. In current kernels, devices that don't have
> an iommu work (and there's no choice about 1:1 or otherwise) and
> devices that have an iommu fail spectacularly. With the patches,
> devices that don't have an iommu continue to work as long as the DMA
> API and/or virtio correctly knows that there's no iommu. Devices that
> do have an iommu work fine, albeit slower than would be ideal. In my
> book, slower than would be ideal is strictly better than crashing.
>
> The real issue is *detecting* whether there's an iommu, and the string
> of bugs in that area (buggy QEMU for the Q35 thing and complete lack
> of a solution for PPC and SPARC is indeed a problem).
>
> I think that we could apply the series ending here:
>
> https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=virtio_dma&id=ad9d43052da44ce18363c02ea597dde01eeee11b
>
> and the only regression (performance or functionality) would be that
> the buggy Q35 iommu configuration would stop working until someone
> fixed it in QEMU. That should be okay -- it's explicitly
> experimental. (Xen works with that series applied.) (Actually,
> there might be a slight performance regression on PPC due to extra
> unused mappings being created. It would be straightforward to hack
> around that in one of several ways.)
>
> Am I missing something?
Your changes look plausible and if they fix Xen on virtio I am happy
with them. I didn't choose the DMA API approach because, although it
looks cleaner, I acknowledge that is a bit invasive.
I suggest that the virtio maintainers consider one of the two approaches
for inclusion because they fix a real issue.
If you would rather avoid the DMA API, then I would be happy to work
with you to evolve my current series in a direction of your liking.
Please advise on how to proceed.
^ permalink raw reply
* Re: [PATCH v3 4/4] VSOCK: Add Makefile and Kconfig
From: Stefan Hajnoczi @ 2015-12-15 8:19 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm, Michael S. Tsirkin, netdev, virtualization, Matt Benjamin,
Asias He, Christoffer Dall, matt.ma
In-Reply-To: <87fuz8q4xf.fsf@linaro.org>
[-- Attachment #1.1: Type: text/plain, Size: 1357 bytes --]
On Fri, Dec 11, 2015 at 05:19:08PM +0000, Alex Bennée wrote:
> > +config VHOST_VSOCK
> > + tristate "vhost virtio-vsock driver"
> > + depends on VSOCKETS && EVENTFD
> > + select VIRTIO_VSOCKETS_COMMON
> > + select VHOST
> > + select VHOST_RING
> > + default n
> > + ---help---
> > + Say M here to enable the vhost-vsock for virtio-vsock guests
>
> I think checkpatch prefers a few more words for the feature but I'm
> happy with it.
I have expanded the description.
> > diff --git a/net/vmw_vsock/Kconfig b/net/vmw_vsock/Kconfig
> > index 14810ab..74e0bc8 100644
> > --- a/net/vmw_vsock/Kconfig
> > +++ b/net/vmw_vsock/Kconfig
> > @@ -26,3 +26,21 @@ config VMWARE_VMCI_VSOCKETS
> >
> > To compile this driver as a module, choose M here: the module
> > will be called vmw_vsock_vmci_transport. If unsure, say N.
> > +
> > +config VIRTIO_VSOCKETS
> > + tristate "virtio transport for Virtual Sockets"
> > + depends on VSOCKETS && VIRTIO
> > + select VIRTIO_VSOCKETS_COMMON
> > + help
> > + This module implements a virtio transport for Virtual Sockets.
> > +
> > + Enable this transport if your Virtual Machine runs on
> > Qemu/KVM.
>
> Is this better worded as:
>
> "Enable this transport if your Virtual Machine host supports vsockets
> over virtio."
Good idea. Will fix in the next revision.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 3/4] VSOCK: Introduce vhost-vsock.ko
From: Stefan Hajnoczi @ 2015-12-15 7:47 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm, Michael S. Tsirkin, netdev, virtualization, Matt Benjamin,
Asias He, Christoffer Dall, matt.ma
In-Reply-To: <87h9jpp092.fsf@linaro.org>
[-- Attachment #1.1: Type: text/plain, Size: 3986 bytes --]
On Fri, Dec 11, 2015 at 01:45:29PM +0000, Alex Bennée wrote:
> > + if (head == vq->num) {
> > + if (unlikely(vhost_enable_notify(&vsock->dev, vq))) {
> > + vhost_disable_notify(&vsock->dev, vq);
> > + continue;
>
> Why are we doing this? If we enable something we then disable it? A
> comment as to what is going on here would be useful.
This is a standard optimization to avoid vmexits that other vhost
devices and QEMU implement too.
When the host begins pulling buffers off a virtqueue it first disables
guest->host notifications. If the guest adds additional buffers while
the host is processing, the notification (vmexit) is skipped. The host
re-enables guest->host notifications when it finishes virtqueue
processing.
If the guest added buffers after vhost_get_vq_desc() but before
vhost_enable_notify(), then vhost_enable_notify() returns true and the
host must process the buffers (i.e. restart the loop). Failure to do so
could result in deadlocks because the guest didn't notify and the host
would be waiting for a notification.
I will add comments to the code.
> > + vhost_add_used(vq, head, pkt->len); /* TODO should this
> > be sizeof(pkt->hdr) + pkt->len? */
>
> TODO needs sorting our or removing.
Will fix in the next revision.
> > + /* Respect global tx buf limitation */
> > + mutex_lock(&vq->mutex);
> > + while (pkt_len + vsock->total_tx_buf >
> > VIRTIO_VSOCK_MAX_TX_BUF_SIZE) {
>
> I'm curious about the relationship between
> VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE above and VIRTIO_VSOCK_MAX_TX_BUF_SIZE
> just here. Why do we need to limit pkt_len to the smaller when really
> all that matters is pkt_len + vsock->total_tx_buf >
> VIRTIO_VSOCK_MAX_TX_BUF_SIZE?
There are two separate issues:
1. The total amount of pending data. The idea is to stop queuing
packets and make the caller wait until resources become available so
that vhost_vsock.ko memory consumption is bounded.
total_tx_buf len is an artificial limit that is lower than the actual
virtqueue maximum data size. Otherwise we could just rely on the
virtqueue to limit the size but it can be very large.
2. Splitting data into packets that fit into rx virtqueue buffers. The
guest sets up the rx virtqueue with VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE
buffers. Here, vhost_vsock.ko is assuming that the rx virtqueue
buffers are always VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE bytes so it
splits data along this boundary.
This is ugly because the guest could choose a different buffer size
and the host has VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE hardcoded. I'll
look into eliminating this assumption.
> > +static void vhost_vsock_handle_ctl_kick(struct vhost_work *work)
> > +{
> > + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> > + poll.work);
> > + struct vhost_vsock *vsock = container_of(vq->dev, struct vhost_vsock,
> > + dev);
> > +
> > + pr_debug("%s vq=%p, vsock=%p\n", __func__, vq, vsock);
> > +}
>
> This doesn't handle anything, it just prints debug stuff. Should this be
> a NOP function?
The control virtqueue is currently not used. In the next revision this
function will be dropped.
> > +static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
> > +{
> > + struct vhost_virtqueue *vq;
> > + int i;
> > +
> > + if (features & ~VHOST_VSOCK_FEATURES)
> > + return -EOPNOTSUPP;
> > +
> > + mutex_lock(&vsock->dev.mutex);
> > + if ((features & (1 << VHOST_F_LOG_ALL)) &&
> > + !vhost_log_access_ok(&vsock->dev)) {
> > + mutex_unlock(&vsock->dev.mutex);
> > + return -EFAULT;
> > + }
> > +
> > + for (i = 0; i < VSOCK_VQ_MAX; i++) {
> > + vq = &vsock->vqs[i].vq;
> > + mutex_lock(&vq->mutex);
> > + vq->acked_features = features;
>
> Is this a user supplied flag? Should it be masked to valid values?
That is already done above where VHOST_VSOCK_FEATURES is checked.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH 2/2] block: virtio-blk: support multi virt queues per virtio-blk device
From: Ming Lei @ 2015-12-15 1:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jens Axboe, Michael S. Tsirkin, linux-api, linux-kernel,
Linux Virtualization, Stefan Hajnoczi
In-Reply-To: <566E9A7E.3030203@redhat.com>
Hi Paolo,
On Mon, Dec 14, 2015 at 6:31 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 18/06/2014 06:04, Ming Lei wrote:
>> For virtio-blk, I don't think it is always better to take more queues, and
>> we need to leverage below things in host side:
>>
>> - host storage top performance, generally it reaches that with more
>> than 1 jobs with libaio(suppose it is N, so basically we can use N
>> iothread per device in qemu to try to get top performance)
>>
>> - iothreads' loading(if iothreads are at full loading, increasing
>> queues doesn't help at all)
>>
>> In my test, I only use the current per-dev iothread(x-dataplane)
>> in qemu to handle 2 vqs' notification and precess all I/O from
>> the 2 vqs, and looks it can improve IOPS by ~30%.
>>
>> For virtio-scsi, the current usage doesn't make full use of blk-mq's
>> advantage too because only one vq is active at the same time, so I
>> guess the multi vqs' benefit won't be very much and I'd like to post
>> patches to support that first, then provide test data with
>> more queues(8, 16).
>
> Hi Ming Lei,
>
> would you like to repost these patches now that MQ support is in the kernel?
>
> Also, I changed my mind about moving linux-aio to AioContext. I now
> think it's a good idea, because it limits the number of io_getevents
> syscalls. O:-) So I would be happy to review your patches for that as well.
OK, I try to figure out a new version, and it might take a while since it is
close to festival season, :-)
Thanks,
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: Andy Lutomirski @ 2015-12-14 18:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefano Stabellini, Benjamin Herrenschmidt,
linux-kernel@vger.kernel.org, Linux Virtualization, David Vrabel,
xen-devel@lists.xenproject.org
In-Reply-To: <20151214161037-mutt-send-email-mst@redhat.com>
On Mon, Dec 14, 2015 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Dec 14, 2015 at 02:00:05PM +0000, David Vrabel wrote:
>> On 07/12/15 16:19, Stefano Stabellini wrote:
>> > Hi all,
>> >
>> > this patch series introduces support for running Linux on top of Xen
>> > inside a virtual machine with virtio devices (nested virt scenario).
>> > The problem is that Linux virtio drivers use virt_to_phys to get the
>> > guest pseudo-physical addresses to pass to the backend, which doesn't
>> > work as expected on Xen.
>> >
>> > Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
>> > dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
>> > Xen support in Linux provides an implementation of the dma API which
>> > takes care of the additional address conversions. However using the dma
>> > API would increase the complexity of the non-Xen case too. We would also
>> > need to keep track of the physical or virtual address in addition to the
>> > dma address for each vring_desc to be able to free the memory in
>> > detach_buf (see patch #3).
>> >
>> > Instead this series adds few obvious checks to perform address
>> > translations in a couple of key places, without changing non-Xen code
>> > paths. You are welcome to suggest improvements or alternative
>> > implementations.
>>
>> Andy Lutomirski also looked at this. Andy what happened to this work?
>>
>> David
>
> The approach there was to try and convert all virtio to use DMA
> API unconditionally.
> This is reasonable if there's a way for devices to request
> 1:1 mappings individually.
> As that is currently missing, that patchset can not be merged yet.
>
I still don't understand why *devices* need the ability to request
anything in particular. In current kernels, devices that don't have
an iommu work (and there's no choice about 1:1 or otherwise) and
devices that have an iommu fail spectacularly. With the patches,
devices that don't have an iommu continue to work as long as the DMA
API and/or virtio correctly knows that there's no iommu. Devices that
do have an iommu work fine, albeit slower than would be ideal. In my
book, slower than would be ideal is strictly better than crashing.
The real issue is *detecting* whether there's an iommu, and the string
of bugs in that area (buggy QEMU for the Q35 thing and complete lack
of a solution for PPC and SPARC is indeed a problem).
I think that we could apply the series ending here:
https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=virtio_dma&id=ad9d43052da44ce18363c02ea597dde01eeee11b
and the only regression (performance or functionality) would be that
the buggy Q35 iommu configuration would stop working until someone
fixed it in QEMU. That should be okay -- it's explicitly
experimental. (Xen works with that series applied.) (Actually,
there might be a slight performance regression on PPC due to extra
unused mappings being created. It would be straightforward to hack
around that in one of several ways.)
Am I missing something?
--Andy
^ permalink raw reply
* Re: [PATCH 0/1] virtio/s390: one fix
From: Michael S. Tsirkin @ 2015-12-14 15:25 UTC (permalink / raw)
To: Cornelia Huck; +Cc: borntraeger, virtualization, kvm, linux-s390
In-Reply-To: <20151214160233.62f4f3c2.cornelia.huck@de.ibm.com>
On Mon, Dec 14, 2015 at 04:02:33PM +0100, Cornelia Huck wrote:
> On Thu, 3 Dec 2015 17:23:59 +0100
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
> > Michael,
> >
> > here's one fix for the virtio-ccw driver.
> >
> > Patch is against master, as your git branches on mst/vhost.git seem
> > to be quite old. Is there some branch I can base my own branch on,
> > or do you prefer to take patches directly anyway?
> >
> > Cornelia Huck (1):
> > virtio/s390: handle error values in irb
> >
> > drivers/s390/virtio/virtio_ccw.c | 62 ++++++++++++++++++++++++----------------
> > 1 file changed, 37 insertions(+), 25 deletions(-)
> >
>
> Ping?
Thanks, I'll merge this, hope it can still make 4.4.
--
MST
^ permalink raw reply
* Re: [PATCH 0/1] virtio/s390: one fix
From: Cornelia Huck @ 2015-12-14 15:02 UTC (permalink / raw)
To: mst; +Cc: borntraeger, virtualization, kvm, linux-s390
In-Reply-To: <1449159840-60270-1-git-send-email-cornelia.huck@de.ibm.com>
On Thu, 3 Dec 2015 17:23:59 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> Michael,
>
> here's one fix for the virtio-ccw driver.
>
> Patch is against master, as your git branches on mst/vhost.git seem
> to be quite old. Is there some branch I can base my own branch on,
> or do you prefer to take patches directly anyway?
>
> Cornelia Huck (1):
> virtio/s390: handle error values in irb
>
> drivers/s390/virtio/virtio_ccw.c | 62 ++++++++++++++++++++++++----------------
> 1 file changed, 37 insertions(+), 25 deletions(-)
>
Ping?
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC 1/3] xen: export xen_phys_to_bus, xen_bus_to_phys and xen_virt_to_bus
From: David Vrabel @ 2015-12-14 14:12 UTC (permalink / raw)
To: Stefano Stabellini, mst
Cc: xen-devel, boris.ostrovsky, david.vrabel, linux-kernel,
virtualization
In-Reply-To: <1449505183-29740-1-git-send-email-stefano.stabellini@eu.citrix.com>
On 07/12/15 16:19, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Can you add a brief description about why these are being moved? Then,
assuming this is needed in the end:
Acked-by: David Vrabel <david.vrabel@citrix.com>
David
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: Michael S. Tsirkin @ 2015-12-14 14:12 UTC (permalink / raw)
To: David Vrabel
Cc: xen-devel, Andy Lutomirski, virtualization, linux-kernel,
Stefano Stabellini
In-Reply-To: <566ECB65.3060509@citrix.com>
On Mon, Dec 14, 2015 at 02:00:05PM +0000, David Vrabel wrote:
> On 07/12/15 16:19, Stefano Stabellini wrote:
> > Hi all,
> >
> > this patch series introduces support for running Linux on top of Xen
> > inside a virtual machine with virtio devices (nested virt scenario).
> > The problem is that Linux virtio drivers use virt_to_phys to get the
> > guest pseudo-physical addresses to pass to the backend, which doesn't
> > work as expected on Xen.
> >
> > Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
> > dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
> > Xen support in Linux provides an implementation of the dma API which
> > takes care of the additional address conversions. However using the dma
> > API would increase the complexity of the non-Xen case too. We would also
> > need to keep track of the physical or virtual address in addition to the
> > dma address for each vring_desc to be able to free the memory in
> > detach_buf (see patch #3).
> >
> > Instead this series adds few obvious checks to perform address
> > translations in a couple of key places, without changing non-Xen code
> > paths. You are welcome to suggest improvements or alternative
> > implementations.
>
> Andy Lutomirski also looked at this. Andy what happened to this work?
>
> David
The approach there was to try and convert all virtio to use DMA
API unconditionally.
This is reasonable if there's a way for devices to request
1:1 mappings individually.
As that is currently missing, that patchset can not be merged yet.
--
MST
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: David Vrabel @ 2015-12-14 14:00 UTC (permalink / raw)
To: Stefano Stabellini, mst
Cc: xen-devel, Andy Lutomirski, linux-kernel, virtualization
In-Reply-To: <alpine.DEB.2.02.1512071555590.24652@kaball.uk.xensource.com>
On 07/12/15 16:19, Stefano Stabellini wrote:
> Hi all,
>
> this patch series introduces support for running Linux on top of Xen
> inside a virtual machine with virtio devices (nested virt scenario).
> The problem is that Linux virtio drivers use virt_to_phys to get the
> guest pseudo-physical addresses to pass to the backend, which doesn't
> work as expected on Xen.
>
> Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
> dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
> Xen support in Linux provides an implementation of the dma API which
> takes care of the additional address conversions. However using the dma
> API would increase the complexity of the non-Xen case too. We would also
> need to keep track of the physical or virtual address in addition to the
> dma address for each vring_desc to be able to free the memory in
> detach_buf (see patch #3).
>
> Instead this series adds few obvious checks to perform address
> translations in a couple of key places, without changing non-Xen code
> paths. You are welcome to suggest improvements or alternative
> implementations.
Andy Lutomirski also looked at this. Andy what happened to this work?
David
^ permalink raw reply
* Re: [RFC PATCH 2/2] block: virtio-blk: support multi virt queues per virtio-blk device
From: Paolo Bonzini @ 2015-12-14 10:31 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, Michael S. Tsirkin, linux-kernel,
Linux Virtualization, Stefan Hajnoczi, linux-api
In-Reply-To: <CACVXFVMkV5R5GUjM7Y-U54SA-ENZZ26dbbv4K2_OYPdmmiuUqg@mail.gmail.com>
On 18/06/2014 06:04, Ming Lei wrote:
> For virtio-blk, I don't think it is always better to take more queues, and
> we need to leverage below things in host side:
>
> - host storage top performance, generally it reaches that with more
> than 1 jobs with libaio(suppose it is N, so basically we can use N
> iothread per device in qemu to try to get top performance)
>
> - iothreads' loading(if iothreads are at full loading, increasing
> queues doesn't help at all)
>
> In my test, I only use the current per-dev iothread(x-dataplane)
> in qemu to handle 2 vqs' notification and precess all I/O from
> the 2 vqs, and looks it can improve IOPS by ~30%.
>
> For virtio-scsi, the current usage doesn't make full use of blk-mq's
> advantage too because only one vq is active at the same time, so I
> guess the multi vqs' benefit won't be very much and I'd like to post
> patches to support that first, then provide test data with
> more queues(8, 16).
Hi Ming Lei,
would you like to repost these patches now that MQ support is in the kernel?
Also, I changed my mind about moving linux-aio to AioContext. I now
think it's a good idea, because it limits the number of io_getevents
syscalls. O:-) So I would be happy to review your patches for that as well.
Paolo
^ permalink raw reply
* Re: [PATCH v2] vhost: replace % with & on data path
From: Andy Shevchenko @ 2015-12-13 0:01 UTC (permalink / raw)
To: Venkatesh Srinivas
Cc: netdev, virtualization, linux-kernel@vger.kernel.org, kvm,
Michael S. Tsirkin
In-Reply-To: <20151204201901.GB19430@google.com>
On Fri, Dec 4, 2015 at 10:19 PM, Venkatesh Srinivas
<venkateshs@google.com> wrote:
> On Mon, Nov 30, 2015 at 11:15:23AM +0200, Michael S. Tsirkin wrote:
>> We know vring num is a power of 2, so use &
>> to mask the high bits.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>
> The generated code switches from DIV -> masking, source is clearer as well.
First impression was why, now it seems that compiler can't predict
this for variables.
For constants it would be optimized to the same, I suppose.
>
> Tested-by: Venkatesh Srinivas <venkateshs@google.com>
>
> -- vs;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 4/4] VSOCK: Add Makefile and Kconfig
From: Alex Bennée @ 2015-12-11 17:19 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kvm, Michael S. Tsirkin, netdev, virtualization, Matt Benjamin,
Asias He, Christoffer Dall, matt.ma
In-Reply-To: <1449662633-26623-5-git-send-email-stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes:
> From: Asias He <asias@redhat.com>
>
> Enable virtio-vsock and vhost-vsock.
>
> Signed-off-by: Asias He <asias@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v3:
> * Don't put vhost vsock driver into staging
> * Add missing Kconfig dependencies (Arnd Bergmann <arnd@arndb.de>)
> ---
> drivers/vhost/Kconfig | 10 ++++++++++
> drivers/vhost/Makefile | 4 ++++
> net/vmw_vsock/Kconfig | 18 ++++++++++++++++++
> net/vmw_vsock/Makefile | 2 ++
> 4 files changed, 34 insertions(+)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 533eaf0..a1bb4c2 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -21,6 +21,16 @@ config VHOST_SCSI
> Say M here to enable the vhost_scsi TCM fabric module
> for use with virtio-scsi guests
>
> +config VHOST_VSOCK
> + tristate "vhost virtio-vsock driver"
> + depends on VSOCKETS && EVENTFD
> + select VIRTIO_VSOCKETS_COMMON
> + select VHOST
> + select VHOST_RING
> + default n
> + ---help---
> + Say M here to enable the vhost-vsock for virtio-vsock guests
I think checkpatch prefers a few more words for the feature but I'm
happy with it.
> +
> config VHOST_RING
> tristate
> ---help---
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index e0441c3..6b012b9 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -4,5 +4,9 @@ vhost_net-y := net.o
> obj-$(CONFIG_VHOST_SCSI) += vhost_scsi.o
> vhost_scsi-y := scsi.o
>
> +obj-$(CONFIG_VHOST_VSOCK) += vhost_vsock.o
> +vhost_vsock-y := vsock.o
> +
> obj-$(CONFIG_VHOST_RING) += vringh.o
> +
> obj-$(CONFIG_VHOST) += vhost.o
> diff --git a/net/vmw_vsock/Kconfig b/net/vmw_vsock/Kconfig
> index 14810ab..74e0bc8 100644
> --- a/net/vmw_vsock/Kconfig
> +++ b/net/vmw_vsock/Kconfig
> @@ -26,3 +26,21 @@ config VMWARE_VMCI_VSOCKETS
>
> To compile this driver as a module, choose M here: the module
> will be called vmw_vsock_vmci_transport. If unsure, say N.
> +
> +config VIRTIO_VSOCKETS
> + tristate "virtio transport for Virtual Sockets"
> + depends on VSOCKETS && VIRTIO
> + select VIRTIO_VSOCKETS_COMMON
> + help
> + This module implements a virtio transport for Virtual Sockets.
> +
> + Enable this transport if your Virtual Machine runs on
> Qemu/KVM.
Is this better worded as:
"Enable this transport if your Virtual Machine host supports vsockets
over virtio."
?
Otherwise:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> +
> + To compile this driver as a module, choose M here: the module
> + will be called virtio_vsock_transport. If unsure, say N.
> +
> +config VIRTIO_VSOCKETS_COMMON
> + tristate
> + ---help---
> + This option is selected by any driver which needs to access
> + the virtio_vsock.
> diff --git a/net/vmw_vsock/Makefile b/net/vmw_vsock/Makefile
> index 2ce52d7..cf4c294 100644
> --- a/net/vmw_vsock/Makefile
> +++ b/net/vmw_vsock/Makefile
> @@ -1,5 +1,7 @@
> obj-$(CONFIG_VSOCKETS) += vsock.o
> obj-$(CONFIG_VMWARE_VMCI_VSOCKETS) += vmw_vsock_vmci_transport.o
> +obj-$(CONFIG_VIRTIO_VSOCKETS) += virtio_transport.o
> +obj-$(CONFIG_VIRTIO_VSOCKETS_COMMON) += virtio_transport_common.o
>
> vsock-y += af_vsock.o vsock_addr.o
>
> --
> 2.5.0
--
Alex Bennée
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] ttm/drm: constify ttm_backend_func structures
From: Christian König @ 2015-12-11 13:59 UTC (permalink / raw)
To: Julia Lawall
Cc: Alex Deucher, kernel-janitors, linux-kernel, dri-devel,
virtualization
In-Reply-To: <1449841067-3718-1-git-send-email-Julia.Lawall@lip6.fr>
On 11.12.2015 14:37, Julia Lawall wrote:
> The ttm_backend_func structures are never modified, so declare them as
> const.
>
> Done with the help of Coccinelle.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Reviewed-by: Christian König <christian.koenig@amd.com>
>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +-
> drivers/gpu/drm/ast/ast_ttm.c | 2 +-
> drivers/gpu/drm/bochs/bochs_mm.c | 2 +-
> drivers/gpu/drm/cirrus/cirrus_ttm.c | 2 +-
> drivers/gpu/drm/mgag200/mgag200_ttm.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_sgdma.c | 4 ++--
> drivers/gpu/drm/qxl/qxl_ttm.c | 2 +-
> drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
> drivers/gpu/drm/ttm/ttm_agp_backend.c | 2 +-
> drivers/gpu/drm/virtio/virtgpu_ttm.c | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c | 2 +-
> include/drm/ttm/ttm_bo_driver.h | 2 +-
> 12 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> index 9fd924c..a323422 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> @@ -319,7 +319,7 @@ static void virtio_gpu_ttm_backend_destroy(struct ttm_tt *ttm)
> kfree(gtt);
> }
>
> -static struct ttm_backend_func virtio_gpu_backend_func = {
> +static const struct ttm_backend_func virtio_gpu_backend_func = {
> .bind = &virtio_gpu_ttm_backend_bind,
> .unbind = &virtio_gpu_ttm_backend_unbind,
> .destroy = &virtio_gpu_ttm_backend_destroy,
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index e343074..9fa758c 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -666,7 +666,7 @@ static void radeon_ttm_backend_destroy(struct ttm_tt *ttm)
> kfree(gtt);
> }
>
> -static struct ttm_backend_func radeon_backend_func = {
> +static const struct ttm_backend_func radeon_backend_func = {
> .bind = &radeon_ttm_backend_bind,
> .unbind = &radeon_ttm_backend_unbind,
> .destroy = &radeon_ttm_backend_destroy,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index d4bac5f..befa9c2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -634,7 +634,7 @@ static void amdgpu_ttm_backend_destroy(struct ttm_tt *ttm)
> kfree(gtt);
> }
>
> -static struct ttm_backend_func amdgpu_backend_func = {
> +static const struct ttm_backend_func amdgpu_backend_func = {
> .bind = &amdgpu_ttm_backend_bind,
> .unbind = &amdgpu_ttm_backend_unbind,
> .destroy = &amdgpu_ttm_backend_destroy,
> diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c
> index 08f82ea..5840785 100644
> --- a/drivers/gpu/drm/ast/ast_ttm.c
> +++ b/drivers/gpu/drm/ast/ast_ttm.c
> @@ -203,7 +203,7 @@ static void ast_ttm_backend_destroy(struct ttm_tt *tt)
> kfree(tt);
> }
>
> -static struct ttm_backend_func ast_tt_backend_func = {
> +static const struct ttm_backend_func ast_tt_backend_func = {
> .destroy = &ast_ttm_backend_destroy,
> };
>
> diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
> index d812ad0..b5b782d 100644
> --- a/drivers/gpu/drm/bochs/bochs_mm.c
> +++ b/drivers/gpu/drm/bochs/bochs_mm.c
> @@ -180,7 +180,7 @@ static void bochs_ttm_backend_destroy(struct ttm_tt *tt)
> kfree(tt);
> }
>
> -static struct ttm_backend_func bochs_tt_backend_func = {
> +static const struct ttm_backend_func bochs_tt_backend_func = {
> .destroy = &bochs_ttm_backend_destroy,
> };
>
> diff --git a/drivers/gpu/drm/cirrus/cirrus_ttm.c b/drivers/gpu/drm/cirrus/cirrus_ttm.c
> index dfffd52..da88541 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_ttm.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_ttm.c
> @@ -203,7 +203,7 @@ static void cirrus_ttm_backend_destroy(struct ttm_tt *tt)
> kfree(tt);
> }
>
> -static struct ttm_backend_func cirrus_tt_backend_func = {
> +static const struct ttm_backend_func cirrus_tt_backend_func = {
> .destroy = &cirrus_ttm_backend_destroy,
> };
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
> index 05108b5..e368562 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
> @@ -203,7 +203,7 @@ static void mgag200_ttm_backend_destroy(struct ttm_tt *tt)
> kfree(tt);
> }
>
> -static struct ttm_backend_func mgag200_tt_backend_func = {
> +static const struct ttm_backend_func mgag200_tt_backend_func = {
> .destroy = &mgag200_ttm_backend_destroy,
> };
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_sgdma.c b/drivers/gpu/drm/nouveau/nouveau_sgdma.c
> index 8c3053a..3b8f556 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_sgdma.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_sgdma.c
> @@ -51,7 +51,7 @@ nv04_sgdma_unbind(struct ttm_tt *ttm)
> return 0;
> }
>
> -static struct ttm_backend_func nv04_sgdma_backend = {
> +static const struct ttm_backend_func nv04_sgdma_backend = {
> .bind = nv04_sgdma_bind,
> .unbind = nv04_sgdma_unbind,
> .destroy = nouveau_sgdma_destroy
> @@ -82,7 +82,7 @@ nv50_sgdma_unbind(struct ttm_tt *ttm)
> return 0;
> }
>
> -static struct ttm_backend_func nv50_sgdma_backend = {
> +static const struct ttm_backend_func nv50_sgdma_backend = {
> .bind = nv50_sgdma_bind,
> .unbind = nv50_sgdma_unbind,
> .destroy = nouveau_sgdma_destroy
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index 0cbc4c9..93dc45d 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -288,7 +288,7 @@ static void qxl_ttm_backend_destroy(struct ttm_tt *ttm)
> kfree(gtt);
> }
>
> -static struct ttm_backend_func qxl_backend_func = {
> +static const struct ttm_backend_func qxl_backend_func = {
> .bind = &qxl_ttm_backend_bind,
> .unbind = &qxl_ttm_backend_unbind,
> .destroy = &qxl_ttm_backend_destroy,
> diff --git a/drivers/gpu/drm/ttm/ttm_agp_backend.c b/drivers/gpu/drm/ttm/ttm_agp_backend.c
> index 764be36..5fdaba5 100644
> --- a/drivers/gpu/drm/ttm/ttm_agp_backend.c
> +++ b/drivers/gpu/drm/ttm/ttm_agp_backend.c
> @@ -104,7 +104,7 @@ static void ttm_agp_destroy(struct ttm_tt *ttm)
> kfree(agp_be);
> }
>
> -static struct ttm_backend_func ttm_agp_func = {
> +static const struct ttm_backend_func ttm_agp_func = {
> .bind = ttm_agp_bind,
> .unbind = ttm_agp_unbind,
> .destroy = ttm_agp_destroy,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
> index 3329f62..d59572a 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
> @@ -683,7 +683,7 @@ static void vmw_ttm_unpopulate(struct ttm_tt *ttm)
> ttm_pool_unpopulate(ttm);
> }
>
> -static struct ttm_backend_func vmw_ttm_func = {
> +static const struct ttm_backend_func vmw_ttm_func = {
> .bind = vmw_ttm_bind,
> .unbind = vmw_ttm_unbind,
> .destroy = vmw_ttm_destroy,
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 813042c..c43efa4 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -113,7 +113,7 @@ enum ttm_caching_state {
>
> struct ttm_tt {
> struct ttm_bo_device *bdev;
> - struct ttm_backend_func *func;
> + const struct ttm_backend_func *func;
> struct page *dummy_read_page;
> struct page **pages;
> uint32_t page_flags;
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 3/4] VSOCK: Introduce vhost-vsock.ko
From: Alex Bennée @ 2015-12-11 13:45 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kvm, Michael S. Tsirkin, netdev, virtualization, Matt Benjamin,
Asias He, Christoffer Dall, matt.ma
In-Reply-To: <1449662633-26623-4-git-send-email-stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes:
> From: Asias He <asias@redhat.com>
>
> VM sockets vhost transport implementation. This module runs in host
> kernel.
As per previous checkpatch comments.
>
> Signed-off-by: Asias He <asias@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v3:
> * Remove unneeded variable used to store return value
> (Fengguang Wu <fengguang.wu@intel.com> and Julia Lawall
> <julia.lawall@lip6.fr>)
> v2:
> * Add missing total_tx_buf decrement
> * Support flexible rx/tx descriptor layout
> * Refuse to assign reserved CIDs
> * Refuse guest CID if already in use
> * Only accept correctly addressed packets
> ---
> drivers/vhost/vsock.c | 628 ++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/vhost/vsock.h | 4 +
> 2 files changed, 632 insertions(+)
> create mode 100644 drivers/vhost/vsock.c
> create mode 100644 drivers/vhost/vsock.h
>
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> new file mode 100644
> index 0000000..3c0034a
> --- /dev/null
> +++ b/drivers/vhost/vsock.c
> @@ -0,0 +1,628 @@
> +/*
> + * vhost transport for vsock
> + *
> + * Copyright (C) 2013-2015 Red Hat, Inc.
> + * Author: Asias He <asias@redhat.com>
> + * Stefan Hajnoczi <stefanha@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + */
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <net/sock.h>
> +#include <linux/virtio_vsock.h>
> +#include <linux/vhost.h>
> +
> +#include <net/af_vsock.h>
> +#include "vhost.h"
> +#include "vsock.h"
> +
> +#define VHOST_VSOCK_DEFAULT_HOST_CID 2
> +
> +static int vhost_transport_socket_init(struct vsock_sock *vsk,
> + struct vsock_sock *psk);
> +
> +enum {
> + VHOST_VSOCK_FEATURES = VHOST_FEATURES,
> +};
> +
> +/* Used to track all the vhost_vsock instances on the system. */
> +static LIST_HEAD(vhost_vsock_list);
> +static DEFINE_MUTEX(vhost_vsock_mutex);
> +
> +struct vhost_vsock_virtqueue {
> + struct vhost_virtqueue vq;
> +};
> +
> +struct vhost_vsock {
> + /* Vhost device */
> + struct vhost_dev dev;
> + /* Vhost vsock virtqueue*/
> + struct vhost_vsock_virtqueue vqs[VSOCK_VQ_MAX];
> + /* Link to global vhost_vsock_list*/
> + struct list_head list;
> + /* Head for pkt from host to guest */
> + struct list_head send_pkt_list;
> + /* Work item to send pkt */
> + struct vhost_work send_pkt_work;
> + /* Wait queue for send pkt */
> + wait_queue_head_t queue_wait;
> + /* Used for global tx buf limitation */
> + u32 total_tx_buf;
> + /* Guest contex id this vhost_vsock instance handles */
> + u32 guest_cid;
> +};
As with 2/4 there is a fair bit of redundancy in the comments but I
don't see any obvious grouping here that could streamline it.
> +
> +static u32 vhost_transport_get_local_cid(void)
> +{
> + return VHOST_VSOCK_DEFAULT_HOST_CID;
> +}
> +
> +static struct vhost_vsock *vhost_vsock_get(u32 guest_cid)
> +{
> + struct vhost_vsock *vsock;
> +
> + mutex_lock(&vhost_vsock_mutex);
> + list_for_each_entry(vsock, &vhost_vsock_list, list) {
> + if (vsock->guest_cid == guest_cid) {
> + mutex_unlock(&vhost_vsock_mutex);
> + return vsock;
> + }
> + }
> + mutex_unlock(&vhost_vsock_mutex);
> +
> + return NULL;
> +}
> +
> +static void
> +vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> + struct vhost_virtqueue *vq)
> +{
> + bool added = false;
> +
> + mutex_lock(&vq->mutex);
> + vhost_disable_notify(&vsock->dev, vq);
> + for (;;) {
> + struct virtio_vsock_pkt *pkt;
> + struct iov_iter iov_iter;
> + unsigned out, in;
> + struct sock *sk;
> + size_t nbytes;
> + size_t len;
> + int head;
> +
> + if (list_empty(&vsock->send_pkt_list)) {
> + vhost_enable_notify(&vsock->dev, vq);
> + break;
> + }
> +
> + head = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
> + &out, &in, NULL, NULL);
> + pr_debug("%s: head = %d\n", __func__, head);
> + if (head < 0)
> + break;
> +
> + if (head == vq->num) {
> + if (unlikely(vhost_enable_notify(&vsock->dev, vq))) {
> + vhost_disable_notify(&vsock->dev, vq);
> + continue;
Why are we doing this? If we enable something we then disable it? A
comment as to what is going on here would be useful.
> + }
> + break;
> + }
> +
> + pkt = list_first_entry(&vsock->send_pkt_list,
> + struct virtio_vsock_pkt, list);
> + list_del_init(&pkt->list);
> +
> + if (out) {
> + virtio_transport_free_pkt(pkt);
> + vq_err(vq, "Expected 0 output buffers, got %u\n", out);
> + break;
> + }
> +
> + len = iov_length(&vq->iov[out], in);
> + iov_iter_init(&iov_iter, READ, &vq->iov[out], in, len);
> +
> + nbytes = copy_to_iter(&pkt->hdr, sizeof(pkt->hdr), &iov_iter);
> + if (nbytes != sizeof(pkt->hdr)) {
> + virtio_transport_free_pkt(pkt);
> + vq_err(vq, "Faulted on copying pkt hdr\n");
> + break;
> + }
> +
> + nbytes = copy_to_iter(pkt->buf, pkt->len, &iov_iter);
> + if (nbytes != pkt->len) {
> + virtio_transport_free_pkt(pkt);
> + vq_err(vq, "Faulted on copying pkt buf\n");
> + break;
> + }
> +
> + vhost_add_used(vq, head, pkt->len); /* TODO should this
> be sizeof(pkt->hdr) + pkt->len? */
TODO needs sorting our or removing.
> + added = true;
> +
> + virtio_transport_dec_tx_pkt(pkt);
> + vsock->total_tx_buf -= pkt->len;
> +
> + sk = sk_vsock(pkt->trans->vsk);
> + /* Release refcnt taken in vhost_transport_send_pkt */
> + sock_put(sk);
> +
> + virtio_transport_free_pkt(pkt);
> + }
> + if (added)
> + vhost_signal(&vsock->dev, vq);
> + mutex_unlock(&vq->mutex);
> +
> + if (added)
> + wake_up(&vsock->queue_wait);
> +}
> +
> +static void vhost_transport_send_pkt_work(struct vhost_work *work)
> +{
> + struct vhost_virtqueue *vq;
> + struct vhost_vsock *vsock;
> +
> + vsock = container_of(work, struct vhost_vsock, send_pkt_work);
> + vq = &vsock->vqs[VSOCK_VQ_RX].vq;
> +
> + vhost_transport_do_send_pkt(vsock, vq);
> +}
> +
> +static int
> +vhost_transport_send_pkt(struct vsock_sock *vsk,
> + struct virtio_vsock_pkt_info *info)
> +{
> + u32 src_cid, src_port, dst_cid, dst_port;
> + struct virtio_transport *trans;
> + struct virtio_vsock_pkt *pkt;
> + struct vhost_virtqueue *vq;
> + struct vhost_vsock *vsock;
> + u32 pkt_len = info->pkt_len;
> + DEFINE_WAIT(wait);
> +
> + src_cid = vhost_transport_get_local_cid();
> + src_port = vsk->local_addr.svm_port;
> + if (!info->remote_cid) {
> + dst_cid = vsk->remote_addr.svm_cid;
> + dst_port = vsk->remote_addr.svm_port;
> + } else {
> + dst_cid = info->remote_cid;
> + dst_port = info->remote_port;
> + }
> +
> + /* Find the vhost_vsock according to guest context id */
> + vsock = vhost_vsock_get(dst_cid);
> + if (!vsock)
> + return -ENODEV;
> +
> + trans = vsk->trans;
> + vq = &vsock->vqs[VSOCK_VQ_RX].vq;
> +
> + /* we can send less than pkt_len bytes */
> + if (pkt_len > VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE)
> + pkt_len = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE;
> +
> + /* virtio_transport_get_credit might return less than pkt_len credit */
> + pkt_len = virtio_transport_get_credit(trans, pkt_len);
> +
> + /* Do not send zero length OP_RW pkt*/
> + if (pkt_len == 0 && info->op == VIRTIO_VSOCK_OP_RW)
> + return pkt_len;
> +
> + /* Respect global tx buf limitation */
> + mutex_lock(&vq->mutex);
> + while (pkt_len + vsock->total_tx_buf >
> VIRTIO_VSOCK_MAX_TX_BUF_SIZE) {
I'm curious about the relationship between
VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE above and VIRTIO_VSOCK_MAX_TX_BUF_SIZE
just here. Why do we need to limit pkt_len to the smaller when really
all that matters is pkt_len + vsock->total_tx_buf >
VIRTIO_VSOCK_MAX_TX_BUF_SIZE?
> + prepare_to_wait_exclusive(&vsock->queue_wait, &wait,
> + TASK_UNINTERRUPTIBLE);
> + mutex_unlock(&vq->mutex);
> + schedule();
> + mutex_lock(&vq->mutex);
> + finish_wait(&vsock->queue_wait, &wait);
> + }
> + vsock->total_tx_buf += pkt_len;
> + mutex_unlock(&vq->mutex);
> +
> + pkt = virtio_transport_alloc_pkt(vsk, info, pkt_len,
> + src_cid, src_port,
> + dst_cid, dst_port);
> + if (!pkt) {
> + mutex_lock(&vq->mutex);
> + vsock->total_tx_buf -= pkt_len;
> + mutex_unlock(&vq->mutex);
> + virtio_transport_put_credit(trans, pkt_len);
> + return -ENOMEM;
> + }
> +
> + pr_debug("%s:info->pkt_len= %d\n", __func__, pkt_len);
> + /* Released in vhost_transport_do_send_pkt */
> + sock_hold(&trans->vsk->sk);
> + virtio_transport_inc_tx_pkt(pkt);
> +
> + /* Queue it up in vhost work */
> + mutex_lock(&vq->mutex);
> + list_add_tail(&pkt->list, &vsock->send_pkt_list);
> + vhost_work_queue(&vsock->dev, &vsock->send_pkt_work);
> + mutex_unlock(&vq->mutex);
> +
> + return pkt_len;
> +}
> +
> +static struct virtio_transport_pkt_ops vhost_ops = {
> + .send_pkt = vhost_transport_send_pkt,
> +};
> +
> +static struct virtio_vsock_pkt *
> +vhost_vsock_alloc_pkt(struct vhost_virtqueue *vq,
> + unsigned int out, unsigned int in)
> +{
> + struct virtio_vsock_pkt *pkt;
> + struct iov_iter iov_iter;
> + size_t nbytes;
> + size_t len;
> +
> + if (in != 0) {
> + vq_err(vq, "Expected 0 input buffers, got %u\n", in);
> + return NULL;
> + }
> +
> + pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> + if (!pkt)
> + return NULL;
> +
> + len = iov_length(vq->iov, out);
> + iov_iter_init(&iov_iter, WRITE, vq->iov, out, len);
> +
> + nbytes = copy_from_iter(&pkt->hdr, sizeof(pkt->hdr), &iov_iter);
> + if (nbytes != sizeof(pkt->hdr)) {
> + vq_err(vq, "Expected %zu bytes for pkt->hdr, got %zu bytes\n",
> + sizeof(pkt->hdr), nbytes);
> + kfree(pkt);
> + return NULL;
> + }
> +
> + if (le16_to_cpu(pkt->hdr.type) == VIRTIO_VSOCK_TYPE_STREAM)
> + pkt->len = le32_to_cpu(pkt->hdr.len);
> +
> + /* No payload */
> + if (!pkt->len)
> + return pkt;
> +
> + /* The pkt is too big */
> + if (pkt->len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) {
> + kfree(pkt);
> + return NULL;
> + }
> +
> + pkt->buf = kmalloc(pkt->len, GFP_KERNEL);
> + if (!pkt->buf) {
> + kfree(pkt);
> + return NULL;
> + }
> +
> + nbytes = copy_from_iter(pkt->buf, pkt->len, &iov_iter);
> + if (nbytes != pkt->len) {
> + vq_err(vq, "Expected %u byte payload, got %zu bytes\n",
> + pkt->len, nbytes);
> + virtio_transport_free_pkt(pkt);
> + return NULL;
> + }
> +
> + return pkt;
> +}
> +
> +static void vhost_vsock_handle_ctl_kick(struct vhost_work *work)
> +{
> + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> + poll.work);
> + struct vhost_vsock *vsock = container_of(vq->dev, struct vhost_vsock,
> + dev);
> +
> + pr_debug("%s vq=%p, vsock=%p\n", __func__, vq, vsock);
> +}
This doesn't handle anything, it just prints debug stuff. Should this be
a NOP function?
> +
> +static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
> +{
> + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> + poll.work);
> + struct vhost_vsock *vsock = container_of(vq->dev, struct vhost_vsock,
> + dev);
> + struct virtio_vsock_pkt *pkt;
> + int head;
> + unsigned int out, in;
> + bool added = false;
> + u32 len;
> +
> + mutex_lock(&vq->mutex);
> + vhost_disable_notify(&vsock->dev, vq);
> + for (;;) {
> + head = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
> + &out, &in, NULL, NULL);
> + if (head < 0)
> + break;
> +
> + if (head == vq->num) {
> + if (unlikely(vhost_enable_notify(&vsock->dev, vq))) {
> + vhost_disable_notify(&vsock->dev, vq);
> + continue;
Same question about the enable/disable dance as above.
> + }
> + break;
> + }
> +
> + pkt = vhost_vsock_alloc_pkt(vq, out, in);
> + if (!pkt) {
> + vq_err(vq, "Faulted on pkt\n");
> + continue;
> + }
> +
> + len = pkt->len;
> +
> + /* Only accept correctly addressed packets */
> + if (le32_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid &&
> + le32_to_cpu(pkt->hdr.dst_cid) == vhost_transport_get_local_cid())
> + virtio_transport_recv_pkt(pkt);
> + else
> + virtio_transport_free_pkt(pkt);
> +
> + vhost_add_used(vq, head, len);
> + added = true;
> + }
> + if (added)
> + vhost_signal(&vsock->dev, vq);
> + mutex_unlock(&vq->mutex);
> +}
> +
> +static void vhost_vsock_handle_rx_kick(struct vhost_work *work)
> +{
> + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> + poll.work);
> + struct vhost_vsock *vsock = container_of(vq->dev, struct vhost_vsock,
> + dev);
> +
> + vhost_transport_do_send_pkt(vsock, vq);
> +}
> +
> +static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
> +{
> + struct vhost_virtqueue **vqs;
> + struct vhost_vsock *vsock;
> + int ret;
> +
> + vsock = kzalloc(sizeof(*vsock), GFP_KERNEL);
> + if (!vsock)
> + return -ENOMEM;
> +
> + pr_debug("%s:vsock=%p\n", __func__, vsock);
> +
> + vqs = kmalloc(VSOCK_VQ_MAX * sizeof(*vqs), GFP_KERNEL);
> + if (!vqs) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + vqs[VSOCK_VQ_CTRL] = &vsock->vqs[VSOCK_VQ_CTRL].vq;
> + vqs[VSOCK_VQ_TX] = &vsock->vqs[VSOCK_VQ_TX].vq;
> + vqs[VSOCK_VQ_RX] = &vsock->vqs[VSOCK_VQ_RX].vq;
> + vsock->vqs[VSOCK_VQ_CTRL].vq.handle_kick = vhost_vsock_handle_ctl_kick;
> + vsock->vqs[VSOCK_VQ_TX].vq.handle_kick = vhost_vsock_handle_tx_kick;
> + vsock->vqs[VSOCK_VQ_RX].vq.handle_kick = vhost_vsock_handle_rx_kick;
> +
> + vhost_dev_init(&vsock->dev, vqs, VSOCK_VQ_MAX);
> +
> + file->private_data = vsock;
> + init_waitqueue_head(&vsock->queue_wait);
> + INIT_LIST_HEAD(&vsock->send_pkt_list);
> + vhost_work_init(&vsock->send_pkt_work, vhost_transport_send_pkt_work);
> +
> + mutex_lock(&vhost_vsock_mutex);
> + list_add_tail(&vsock->list, &vhost_vsock_list);
> + mutex_unlock(&vhost_vsock_mutex);
> + return 0;
> +
> +out:
> + kfree(vsock);
> + return ret;
> +}
> +
> +static void vhost_vsock_flush(struct vhost_vsock *vsock)
> +{
> + int i;
> +
> + for (i = 0; i < VSOCK_VQ_MAX; i++)
> + vhost_poll_flush(&vsock->vqs[i].vq.poll);
> + vhost_work_flush(&vsock->dev, &vsock->send_pkt_work);
> +}
> +
> +static int vhost_vsock_dev_release(struct inode *inode, struct file *file)
> +{
> + struct vhost_vsock *vsock = file->private_data;
> +
> + mutex_lock(&vhost_vsock_mutex);
> + list_del(&vsock->list);
> + mutex_unlock(&vhost_vsock_mutex);
> +
> + vhost_dev_stop(&vsock->dev);
> + vhost_vsock_flush(vsock);
> + vhost_dev_cleanup(&vsock->dev, false);
> + kfree(vsock->dev.vqs);
> + kfree(vsock);
> + return 0;
> +}
> +
> +static int vhost_vsock_set_cid(struct vhost_vsock *vsock, u32 guest_cid)
> +{
> + struct vhost_vsock *other;
> +
> + /* Refuse reserved CIDs */
> + if (guest_cid <= VMADDR_CID_HOST) {
> + return -EINVAL;
> + }
> +
> + /* Refuse if CID is already in use */
> + other = vhost_vsock_get(guest_cid);
> + if (other && other != vsock) {
> + return -EADDRINUSE;
> + }
> +
> + mutex_lock(&vhost_vsock_mutex);
> + vsock->guest_cid = guest_cid;
> + pr_debug("%s:guest_cid=%d\n", __func__, guest_cid);
> + mutex_unlock(&vhost_vsock_mutex);
> +
> + return 0;
> +}
> +
> +static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
> +{
> + struct vhost_virtqueue *vq;
> + int i;
> +
> + if (features & ~VHOST_VSOCK_FEATURES)
> + return -EOPNOTSUPP;
> +
> + mutex_lock(&vsock->dev.mutex);
> + if ((features & (1 << VHOST_F_LOG_ALL)) &&
> + !vhost_log_access_ok(&vsock->dev)) {
> + mutex_unlock(&vsock->dev.mutex);
> + return -EFAULT;
> + }
> +
> + for (i = 0; i < VSOCK_VQ_MAX; i++) {
> + vq = &vsock->vqs[i].vq;
> + mutex_lock(&vq->mutex);
> + vq->acked_features = features;
Is this a user supplied flag? Should it be masked to valid values?
> + mutex_unlock(&vq->mutex);
> + }
> + mutex_unlock(&vsock->dev.mutex);
> + return 0;
> +}
> +
> +static long vhost_vsock_dev_ioctl(struct file *f, unsigned int ioctl,
> + unsigned long arg)
> +{
> + struct vhost_vsock *vsock = f->private_data;
> + void __user *argp = (void __user *)arg;
> + u64 __user *featurep = argp;
> + u32 __user *cidp = argp;
> + u32 guest_cid;
> + u64 features;
> + int r;
> +
> + switch (ioctl) {
> + case VHOST_VSOCK_SET_GUEST_CID:
> + if (get_user(guest_cid, cidp))
> + return -EFAULT;
> + return vhost_vsock_set_cid(vsock, guest_cid);
> + case VHOST_GET_FEATURES:
> + features = VHOST_VSOCK_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;
> + return vhost_vsock_set_features(vsock, features);
> + default:
> + mutex_lock(&vsock->dev.mutex);
> + r = vhost_dev_ioctl(&vsock->dev, ioctl, argp);
> + if (r == -ENOIOCTLCMD)
> + r = vhost_vring_ioctl(&vsock->dev, ioctl, argp);
> + else
> + vhost_vsock_flush(vsock);
> + mutex_unlock(&vsock->dev.mutex);
> + return r;
> + }
> +}
> +
> +static const struct file_operations vhost_vsock_fops = {
> + .owner = THIS_MODULE,
> + .open = vhost_vsock_dev_open,
> + .release = vhost_vsock_dev_release,
> + .llseek = noop_llseek,
> + .unlocked_ioctl = vhost_vsock_dev_ioctl,
> +};
> +
> +static struct miscdevice vhost_vsock_misc = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "vhost-vsock",
> + .fops = &vhost_vsock_fops,
> +};
> +
> +static int
> +vhost_transport_socket_init(struct vsock_sock *vsk, struct vsock_sock *psk)
> +{
> + struct virtio_transport *trans;
> + int ret;
> +
> + ret = virtio_transport_do_socket_init(vsk, psk);
> + if (ret)
> + return ret;
> +
> + trans = vsk->trans;
> + trans->ops = &vhost_ops;
> +
> + return ret;
> +}
> +
> +static struct vsock_transport vhost_transport = {
> + .get_local_cid = vhost_transport_get_local_cid,
> +
> + .init = vhost_transport_socket_init,
> + .destruct = virtio_transport_destruct,
> + .release = virtio_transport_release,
> + .connect = virtio_transport_connect,
> + .shutdown = virtio_transport_shutdown,
> +
> + .dgram_enqueue = virtio_transport_dgram_enqueue,
> + .dgram_dequeue = virtio_transport_dgram_dequeue,
> + .dgram_bind = virtio_transport_dgram_bind,
> + .dgram_allow = virtio_transport_dgram_allow,
> +
> + .stream_enqueue = virtio_transport_stream_enqueue,
> + .stream_dequeue = virtio_transport_stream_dequeue,
> + .stream_has_data = virtio_transport_stream_has_data,
> + .stream_has_space = virtio_transport_stream_has_space,
> + .stream_rcvhiwat = virtio_transport_stream_rcvhiwat,
> + .stream_is_active = virtio_transport_stream_is_active,
> + .stream_allow = virtio_transport_stream_allow,
> +
> + .notify_poll_in = virtio_transport_notify_poll_in,
> + .notify_poll_out = virtio_transport_notify_poll_out,
> + .notify_recv_init = virtio_transport_notify_recv_init,
> + .notify_recv_pre_block = virtio_transport_notify_recv_pre_block,
> + .notify_recv_pre_dequeue = virtio_transport_notify_recv_pre_dequeue,
> + .notify_recv_post_dequeue = virtio_transport_notify_recv_post_dequeue,
> + .notify_send_init = virtio_transport_notify_send_init,
> + .notify_send_pre_block = virtio_transport_notify_send_pre_block,
> + .notify_send_pre_enqueue = virtio_transport_notify_send_pre_enqueue,
> + .notify_send_post_enqueue = virtio_transport_notify_send_post_enqueue,
> +
> + .set_buffer_size = virtio_transport_set_buffer_size,
> + .set_min_buffer_size = virtio_transport_set_min_buffer_size,
> + .set_max_buffer_size = virtio_transport_set_max_buffer_size,
> + .get_buffer_size = virtio_transport_get_buffer_size,
> + .get_min_buffer_size = virtio_transport_get_min_buffer_size,
> + .get_max_buffer_size = virtio_transport_get_max_buffer_size,
> +};
> +
> +static int __init vhost_vsock_init(void)
> +{
> + int ret;
> +
> + ret = vsock_core_init(&vhost_transport);
> + if (ret < 0)
> + return ret;
> + return misc_register(&vhost_vsock_misc);
> +};
> +
> +static void __exit vhost_vsock_exit(void)
> +{
> + misc_deregister(&vhost_vsock_misc);
> + vsock_core_exit();
> +};
> +
> +module_init(vhost_vsock_init);
> +module_exit(vhost_vsock_exit);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Asias He");
> +MODULE_DESCRIPTION("vhost transport for vsock ");
> diff --git a/drivers/vhost/vsock.h b/drivers/vhost/vsock.h
> new file mode 100644
> index 0000000..0ddb107
> --- /dev/null
> +++ b/drivers/vhost/vsock.h
> @@ -0,0 +1,4 @@
> +#ifndef VHOST_VSOCK_H
> +#define VHOST_VSOCK_H
> +#define VHOST_VSOCK_SET_GUEST_CID _IOW(VHOST_VIRTIO, 0x60, __u32)
> +#endif
> --
> 2.5.0
--
Alex Bennée
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] ttm/drm: constify ttm_backend_func structures
From: Julia Lawall @ 2015-12-11 13:37 UTC (permalink / raw)
Cc: David Airlie, kernel-janitors, linux-kernel, dri-devel,
virtualization, Alex Deucher
The ttm_backend_func structures are never modified, so declare them as
const.
Done with the help of Coccinelle.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +-
drivers/gpu/drm/ast/ast_ttm.c | 2 +-
drivers/gpu/drm/bochs/bochs_mm.c | 2 +-
drivers/gpu/drm/cirrus/cirrus_ttm.c | 2 +-
drivers/gpu/drm/mgag200/mgag200_ttm.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_sgdma.c | 4 ++--
drivers/gpu/drm/qxl/qxl_ttm.c | 2 +-
drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
drivers/gpu/drm/ttm/ttm_agp_backend.c | 2 +-
drivers/gpu/drm/virtio/virtgpu_ttm.c | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c | 2 +-
include/drm/ttm/ttm_bo_driver.h | 2 +-
12 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index 9fd924c..a323422 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -319,7 +319,7 @@ static void virtio_gpu_ttm_backend_destroy(struct ttm_tt *ttm)
kfree(gtt);
}
-static struct ttm_backend_func virtio_gpu_backend_func = {
+static const struct ttm_backend_func virtio_gpu_backend_func = {
.bind = &virtio_gpu_ttm_backend_bind,
.unbind = &virtio_gpu_ttm_backend_unbind,
.destroy = &virtio_gpu_ttm_backend_destroy,
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index e343074..9fa758c 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -666,7 +666,7 @@ static void radeon_ttm_backend_destroy(struct ttm_tt *ttm)
kfree(gtt);
}
-static struct ttm_backend_func radeon_backend_func = {
+static const struct ttm_backend_func radeon_backend_func = {
.bind = &radeon_ttm_backend_bind,
.unbind = &radeon_ttm_backend_unbind,
.destroy = &radeon_ttm_backend_destroy,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index d4bac5f..befa9c2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -634,7 +634,7 @@ static void amdgpu_ttm_backend_destroy(struct ttm_tt *ttm)
kfree(gtt);
}
-static struct ttm_backend_func amdgpu_backend_func = {
+static const struct ttm_backend_func amdgpu_backend_func = {
.bind = &amdgpu_ttm_backend_bind,
.unbind = &amdgpu_ttm_backend_unbind,
.destroy = &amdgpu_ttm_backend_destroy,
diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c
index 08f82ea..5840785 100644
--- a/drivers/gpu/drm/ast/ast_ttm.c
+++ b/drivers/gpu/drm/ast/ast_ttm.c
@@ -203,7 +203,7 @@ static void ast_ttm_backend_destroy(struct ttm_tt *tt)
kfree(tt);
}
-static struct ttm_backend_func ast_tt_backend_func = {
+static const struct ttm_backend_func ast_tt_backend_func = {
.destroy = &ast_ttm_backend_destroy,
};
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index d812ad0..b5b782d 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -180,7 +180,7 @@ static void bochs_ttm_backend_destroy(struct ttm_tt *tt)
kfree(tt);
}
-static struct ttm_backend_func bochs_tt_backend_func = {
+static const struct ttm_backend_func bochs_tt_backend_func = {
.destroy = &bochs_ttm_backend_destroy,
};
diff --git a/drivers/gpu/drm/cirrus/cirrus_ttm.c b/drivers/gpu/drm/cirrus/cirrus_ttm.c
index dfffd52..da88541 100644
--- a/drivers/gpu/drm/cirrus/cirrus_ttm.c
+++ b/drivers/gpu/drm/cirrus/cirrus_ttm.c
@@ -203,7 +203,7 @@ static void cirrus_ttm_backend_destroy(struct ttm_tt *tt)
kfree(tt);
}
-static struct ttm_backend_func cirrus_tt_backend_func = {
+static const struct ttm_backend_func cirrus_tt_backend_func = {
.destroy = &cirrus_ttm_backend_destroy,
};
diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 05108b5..e368562 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -203,7 +203,7 @@ static void mgag200_ttm_backend_destroy(struct ttm_tt *tt)
kfree(tt);
}
-static struct ttm_backend_func mgag200_tt_backend_func = {
+static const struct ttm_backend_func mgag200_tt_backend_func = {
.destroy = &mgag200_ttm_backend_destroy,
};
diff --git a/drivers/gpu/drm/nouveau/nouveau_sgdma.c b/drivers/gpu/drm/nouveau/nouveau_sgdma.c
index 8c3053a..3b8f556 100644
--- a/drivers/gpu/drm/nouveau/nouveau_sgdma.c
+++ b/drivers/gpu/drm/nouveau/nouveau_sgdma.c
@@ -51,7 +51,7 @@ nv04_sgdma_unbind(struct ttm_tt *ttm)
return 0;
}
-static struct ttm_backend_func nv04_sgdma_backend = {
+static const struct ttm_backend_func nv04_sgdma_backend = {
.bind = nv04_sgdma_bind,
.unbind = nv04_sgdma_unbind,
.destroy = nouveau_sgdma_destroy
@@ -82,7 +82,7 @@ nv50_sgdma_unbind(struct ttm_tt *ttm)
return 0;
}
-static struct ttm_backend_func nv50_sgdma_backend = {
+static const struct ttm_backend_func nv50_sgdma_backend = {
.bind = nv50_sgdma_bind,
.unbind = nv50_sgdma_unbind,
.destroy = nouveau_sgdma_destroy
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 0cbc4c9..93dc45d 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -288,7 +288,7 @@ static void qxl_ttm_backend_destroy(struct ttm_tt *ttm)
kfree(gtt);
}
-static struct ttm_backend_func qxl_backend_func = {
+static const struct ttm_backend_func qxl_backend_func = {
.bind = &qxl_ttm_backend_bind,
.unbind = &qxl_ttm_backend_unbind,
.destroy = &qxl_ttm_backend_destroy,
diff --git a/drivers/gpu/drm/ttm/ttm_agp_backend.c b/drivers/gpu/drm/ttm/ttm_agp_backend.c
index 764be36..5fdaba5 100644
--- a/drivers/gpu/drm/ttm/ttm_agp_backend.c
+++ b/drivers/gpu/drm/ttm/ttm_agp_backend.c
@@ -104,7 +104,7 @@ static void ttm_agp_destroy(struct ttm_tt *ttm)
kfree(agp_be);
}
-static struct ttm_backend_func ttm_agp_func = {
+static const struct ttm_backend_func ttm_agp_func = {
.bind = ttm_agp_bind,
.unbind = ttm_agp_unbind,
.destroy = ttm_agp_destroy,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
index 3329f62..d59572a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
@@ -683,7 +683,7 @@ static void vmw_ttm_unpopulate(struct ttm_tt *ttm)
ttm_pool_unpopulate(ttm);
}
-static struct ttm_backend_func vmw_ttm_func = {
+static const struct ttm_backend_func vmw_ttm_func = {
.bind = vmw_ttm_bind,
.unbind = vmw_ttm_unbind,
.destroy = vmw_ttm_destroy,
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 813042c..c43efa4 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -113,7 +113,7 @@ enum ttm_caching_state {
struct ttm_tt {
struct ttm_bo_device *bdev;
- struct ttm_backend_func *func;
+ const struct ttm_backend_func *func;
struct page *dummy_read_page;
struct page **pages;
uint32_t page_flags;
^ permalink raw reply related
* Re: [PATCH v3 2/4] VSOCK: Introduce virtio-vsock.ko
From: Stefan Hajnoczi @ 2015-12-11 3:00 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm, Michael S. Tsirkin, netdev, virtualization, Matt Benjamin,
Asias He, Christoffer Dall, matt.ma
In-Reply-To: <87io46ov5e.fsf@linaro.org>
[-- Attachment #1.1: Type: text/plain, Size: 5135 bytes --]
On Thu, Dec 10, 2015 at 09:23:25PM +0000, Alex Bennée wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > From: Asias He <asias@redhat.com>
> >
> > VM sockets virtio transport implementation. This module runs in guest
> > kernel.
>
> checkpatch warns on a bunch of whitespace/tab issues.
Will fix in the next version.
> > +struct virtio_vsock {
> > + /* Virtio device */
> > + struct virtio_device *vdev;
> > + /* Virtio virtqueue */
> > + struct virtqueue *vqs[VSOCK_VQ_MAX];
> > + /* Wait queue for send pkt */
> > + wait_queue_head_t queue_wait;
> > + /* Work item to send pkt */
> > + struct work_struct tx_work;
> > + /* Work item to recv pkt */
> > + struct work_struct rx_work;
> > + /* Mutex to protect send pkt*/
> > + struct mutex tx_lock;
> > + /* Mutex to protect recv pkt*/
> > + struct mutex rx_lock;
>
> Further down I got confused by what lock was what and exactly what was
> being protected. If the receive and transmit paths touch separate things
> it might be worth re-arranging the structure to make it clearer, eg:
>
> /* The transmit path is protected by tx_lock */
> struct mutex tx_lock;
> struct work_struct tx_work;
> ..
> ..
>
> /* The receive path is protected by rx_lock */
> wait_queue_head_t queue_wait;
> ..
> ..
>
> Which might make things a little clearer. Then all the redundant
> information in the comments can be removed. I don't need to know what
> is a Virtio device, virtqueue or wait_queue etc as they are implicit in
> the structure name.
Thanks, that is a nice idea.
> > + mutex_lock(&vsock->tx_lock);
> > + while ((ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, pkt,
> > + GFP_KERNEL)) < 0) {
> > + prepare_to_wait_exclusive(&vsock->queue_wait, &wait,
> > + TASK_UNINTERRUPTIBLE);
> > + mutex_unlock(&vsock->tx_lock);
> > + schedule();
> > + mutex_lock(&vsock->tx_lock);
> > + finish_wait(&vsock->queue_wait, &wait);
> > + }
> > + virtqueue_kick(vq);
> > + mutex_unlock(&vsock->tx_lock);
>
> What are we protecting with tx_lock here? See comments above about
> making the lock usage semantics clearer.
vq (vsock->vqs[VSOCK_VQ_TX]) is being protected. Concurrent calls to
virtqueue_add_sgs() are not allowed.
> > +
> > + return pkt_len;
> > +}
> > +
> > +static struct virtio_transport_pkt_ops virtio_ops = {
> > + .send_pkt = virtio_transport_send_pkt,
> > +};
> > +
> > +static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
> > +{
> > + int buf_len = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE;
> > + struct virtio_vsock_pkt *pkt;
> > + struct scatterlist hdr, buf, *sgs[2];
> > + struct virtqueue *vq;
> > + int ret;
> > +
> > + vq = vsock->vqs[VSOCK_VQ_RX];
> > +
> > + do {
> > + pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> > + if (!pkt) {
> > + pr_debug("%s: fail to allocate pkt\n", __func__);
> > + goto out;
> > + }
> > +
> > + /* TODO: use mergeable rx buffer */
>
> TODO's should end up in merged code.
Will fix in next revision.
> > + pkt->buf = kmalloc(buf_len, GFP_KERNEL);
> > + if (!pkt->buf) {
> > + pr_debug("%s: fail to allocate pkt->buf\n", __func__);
> > + goto err;
> > + }
> > +
> > + sg_init_one(&hdr, &pkt->hdr, sizeof(pkt->hdr));
> > + sgs[0] = &hdr;
> > +
> > + sg_init_one(&buf, pkt->buf, buf_len);
> > + sgs[1] = &buf;
> > + ret = virtqueue_add_sgs(vq, sgs, 0, 2, pkt, GFP_KERNEL);
> > + if (ret)
> > + goto err;
> > + vsock->rx_buf_nr++;
> > + } while (vq->num_free);
> > + if (vsock->rx_buf_nr > vsock->rx_buf_max_nr)
> > + vsock->rx_buf_max_nr = vsock->rx_buf_nr;
> > +out:
> > + virtqueue_kick(vq);
> > + return;
> > +err:
> > + virtqueue_kick(vq);
> > + virtio_transport_free_pkt(pkt);
>
> You could free the pkt memory at the fail site and just have one exit path.
Okay, I agree the err label is of marginal use. Let's get rid of it.
>
> > + return;
> > +}
> > +
> > +static void virtio_transport_send_pkt_work(struct work_struct *work)
> > +{
> > + struct virtio_vsock *vsock =
> > + container_of(work, struct virtio_vsock, tx_work);
> > + struct virtio_vsock_pkt *pkt;
> > + bool added = false;
> > + struct virtqueue *vq;
> > + unsigned int len;
> > + struct sock *sk;
> > +
> > + vq = vsock->vqs[VSOCK_VQ_TX];
> > + mutex_lock(&vsock->tx_lock);
> > + do {
>
> You can move the declarations of pkt/len into the do block.
Okay.
>
> > + virtqueue_disable_cb(vq);
> > + while ((pkt = virtqueue_get_buf(vq, &len)) != NULL) {
>
> And the sk declaration here
Okay.
> > +static void virtio_transport_recv_pkt_work(struct work_struct *work)
> > +{
> > + struct virtio_vsock *vsock =
> > + container_of(work, struct virtio_vsock, rx_work);
> > + struct virtio_vsock_pkt *pkt;
> > + struct virtqueue *vq;
> > + unsigned int len;
>
> Same as above for pkt, len.
Okay.
> > + vsock = kzalloc(sizeof(*vsock), GFP_KERNEL);
> > + if (!vsock) {
> > + ret = -ENOMEM;
> > + goto out;
>
> Won't this attempt to kfree a NULL vsock?
kfree(NULL) is a nop so this is safe.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ 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