Re: [PATCH v5 00/20] Share TTM code among DRM framebuffer drivers
Hi Am 15.05.19 um 16:22 schrieb Gerd Hoffmann: > On Wed, May 15, 2019 at 09:05:54AM +0200, Thomas Zimmermann wrote: >> Hi, >> >> most of this patch set still needs reviews. >> >> If it's too large for merging or reviewing at once, I could move the >> driver changes into separate patch sets. The vbox driver's changes have >> been accepted by Hans already. So only keeping the core changes plus >> vbox would be an option. > > Looks all good to me. bochs survived my testing, vbox is reviewed and > IIRC you've tested two of the other three drivers. So all but one > driver is covered. > > I'll go push this to drm-misc-next in a moment. Thank you so much! Best regards Thomas > >>> Future directions: with these changes, the respective drivers can also >>> share some of their mode-setting or fbdev code. GEM VRAM's PRIME helpers >>> allow for using the generic fbcon emulation. > > Using generic fbcon should be easy now. > > cheers, > Gerd > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) signature.asc Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vsock/virtio: Initialize core virtio vsock before registering the driver
On Tue, May 07, 2019 at 02:25:43PM +0200, Stefano Garzarella wrote:
> Hi Jorge,
>
> On Mon, May 06, 2019 at 01:19:55PM -0700, Jorge Moreira Broche wrote:
> > > On Wed, May 01, 2019 at 03:08:31PM -0400, Stefan Hajnoczi wrote:
> > > > On Tue, Apr 30, 2019 at 05:30:01PM -0700, Jorge E. Moreira wrote:
> > > > > Avoid a race in which static variables in net/vmw_vsock/af_vsock.c are
> > > > > accessed (while handling interrupts) before they are initialized.
> > > > >
> > > > >
> > > > > [4.201410] BUG: unable to handle kernel paging request at
> > > > > ffe8
> > > > > [4.207829] IP: vsock_addr_equals_addr+0x3/0x20
> > > > > [4.211379] PGD 28210067 P4D 28210067 PUD 28212067 PMD 0
> > > > > [4.211379] Oops: [#1] PREEMPT SMP PTI
> > > > > [4.211379] Modules linked in:
> > > > > [4.211379] CPU: 1 PID: 30 Comm: kworker/1:1 Not tainted
> > > > > 4.14.106-419297-gd7e28cc1f241 #1
> > > > > [4.211379] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > > > > BIOS 1.10.2-1 04/01/2014
> > > > > [4.211379] Workqueue: virtio_vsock virtio_transport_rx_work
> > > > > [4.211379] task: a3273d175280 task.stack: aea1800e8000
> > > > > [4.211379] RIP: 0010:vsock_addr_equals_addr+0x3/0x20
> > > > > [4.211379] RSP: :aea1800ebd28 EFLAGS: 00010286
> > > > > [4.211379] RAX: 0002 RBX: RCX:
> > > > > b94e42f0
> > > > > [4.211379] RDX: 0400 RSI: ffe0 RDI:
> > > > > aea1800ebdd0
> > > > > [4.211379] RBP: aea1800ebd58 R08: 0001 R09:
> > > > > 0001
> > > > > [4.211379] R10: R11: b89d5d60 R12:
> > > > > aea1800ebdd0
> > > > > [4.211379] R13: 828cbfbf R14: R15:
> > > > > aea1800ebdc0
> > > > > [4.211379] FS: () GS:a3273fd0()
> > > > > knlGS:
> > > > > [4.211379] CS: 0010 DS: ES: CR0: 80050033
> > > > > [4.211379] CR2: ffe8 CR3: 2820e001 CR4:
> > > > > 001606e0
> > > > > [4.211379] DR0: DR1: DR2:
> > > > >
> > > > > [4.211379] DR3: DR6: fffe0ff0 DR7:
> > > > > 0400
> > > > > [4.211379] Call Trace:
> > > > > [4.211379] ? vsock_find_connected_socket+0x6c/0xe0
> > > > > [4.211379] virtio_transport_recv_pkt+0x15f/0x740
> > > > > [4.211379] ? detach_buf+0x1b5/0x210
> > > > > [4.211379] virtio_transport_rx_work+0xb7/0x140
> > > > > [4.211379] process_one_work+0x1ef/0x480
> > > > > [4.211379] worker_thread+0x312/0x460
> > > > > [4.211379] kthread+0x132/0x140
> > > > > [4.211379] ? process_one_work+0x480/0x480
> > > > > [4.211379] ? kthread_destroy_worker+0xd0/0xd0
> > > > > [4.211379] ret_from_fork+0x35/0x40
> > > > > [4.211379] Code: c7 47 08 00 00 00 00 66 c7 07 28 00 c7 47 08 ff
> > > > > ff ff ff c7 47 04 ff ff ff ff c3 0f 1f 00 66 2e 0f 1f 84 00 00 00 00
> > > > > 00 8b 47 08 <3b> 46 08 75 0a 8b 47 04 3b 46 04 0f 94 c0 c3 31 c0 c3
> > > > > 90 66 2e
> > > > > [4.211379] RIP: vsock_addr_equals_addr+0x3/0x20 RSP:
> > > > > aea1800ebd28
> > > > > [4.211379] CR2: ffe8
> > > > > [4.211379] ---[ end trace f31cc4a2e6df3689 ]---
> > > > > [4.211379] Kernel panic - not syncing: Fatal exception in
> > > > > interrupt
> > > > > [4.211379] Kernel Offset: 0x3700 from 0x8100
> > > > > (relocation range: 0x8000-0xbfff)
> > > > > [4.211379] Rebooting in 5 seconds..
> > > > >
> > > > > Fixes: 22b5c0b63f32 ("vsock/virtio: fix kernel panic after device
> > > > > hot-unplug")
> > > > > Cc: Stefan Hajnoczi
> > > > > Cc: "David S. Miller"
> > > > > Cc: k...@vger.kernel.org
> > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > Cc: net...@vger.kernel.org
> > > > > Cc: kernel-t...@android.com
> > > > > Cc: sta...@vger.kernel.org [4.9+]
> > > > > Signed-off-by: Jorge E. Moreira
> > > > > ---
> > > > > net/vmw_vsock/virtio_transport.c | 13 ++---
> > > > > 1 file changed, 6 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/net/vmw_vsock/virtio_transport.c
> > > > > b/net/vmw_vsock/virtio_transport.c
> > > > > index 15eb5d3d4750..96ab344f17bb 100644
> > > > > --- a/net/vmw_vsock/virtio_transport.c
> > > > > +++ b/net/vmw_vsock/virtio_transport.c
> > > > > @@ -702,28 +702,27 @@ static int __init virtio_vsock_init(void)
> > > > > if (!virtio_vsock_workqueue)
> > > > > return -ENOMEM;
> > > > >
> > > > > - ret = register_virtio_driver(_vsock_driver);
> > > > > + ret = vsock_core_init(_transport.transport);
> > > >
> > > > Have you checked that all transport callbacks are safe even if another
> > > > CPU calls them while virtio_vsock_probe() is executing on another CPU?
> > > >
> > >
> > > I have the same doubt.
> >
Re: [PATCH v5 00/20] Share TTM code among DRM framebuffer drivers
On Wed, May 15, 2019 at 09:05:54AM +0200, Thomas Zimmermann wrote: > Hi, > > most of this patch set still needs reviews. > > If it's too large for merging or reviewing at once, I could move the > driver changes into separate patch sets. The vbox driver's changes have > been accepted by Hans already. So only keeping the core changes plus > vbox would be an option. Looks all good to me. bochs survived my testing, vbox is reviewed and IIRC you've tested two of the other three drivers. So all but one driver is covered. I'll go push this to drm-misc-next in a moment. > > Future directions: with these changes, the respective drivers can also > > share some of their mode-setting or fbdev code. GEM VRAM's PRIME helpers > > allow for using the generic fbcon emulation. Using generic fbcon should be easy now. cheers, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 10/10] virtio/s390: make airq summary indicators DMA
On Wed, 15 May 2019 15:43:23 +0200 Michael Mueller wrote: > > Hm, where is airq_areas_lock defined? If it was introduced in one of > > the previous patches, I have missed it. > > There is no airq_areas_lock defined currently. My assumption is that > this will be used in context with the likely race condition this > part of the patch is talking about. Right! I first started resolving the race, but then decided to discuss the issue first, because if I were to just have hallucinated that race, it would be a lots of wasted effort. Unfortunately I forgot to get rid of this comment. Regards, Halil ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 05/10] s390/cio: introduce DMA pools to cio
On Mon, 13 May 2019 15:29:24 +0200 Cornelia Huck wrote: > On Sun, 12 May 2019 20:22:56 +0200 > Halil Pasic wrote: > > > On Fri, 10 May 2019 16:10:13 +0200 > > Cornelia Huck wrote: > > > > > On Fri, 10 May 2019 00:11:12 +0200 > > > Halil Pasic wrote: > > > > > > > On Thu, 9 May 2019 12:11:06 +0200 > > > > Cornelia Huck wrote: > > > > > > > > > On Wed, 8 May 2019 23:22:10 +0200 > > > > > Halil Pasic wrote: > > > > > > > > > > > On Wed, 8 May 2019 15:18:10 +0200 (CEST) > > > > > > Sebastian Ott wrote: > > > > > > > > > > > > > @@ -1063,6 +1163,7 @@ static int __init css_bus_init(void) > > > > > > > > > > > > > > > > unregister_reboot_notifier(_reboot_notifier); > > > > > > > > goto out_unregister; > > > > > > > > } > > > > > > > > + cio_dma_pool_init(); > > > > > > > > > > > > > > This is too late for early devices (ccw console!). > > > > > > > > > > > > You have already raised concern about this last time (thanks). I > > > > > > think, > > > > > > I've addressed this issue: the cio_dma_pool is only used by the airq > > > > > > stuff. I don't think the ccw console needs it. Please have an other > > > > > > look > > > > > > at patch #6, and explain your concern in more detail if it > > > > > > persists. > > > > > > > > > > What about changing the naming/adding comments here, so that (1) folks > > > > > aren't confused by the same thing in the future and (2) folks don't > > > > > try > > > > > to use that pool for something needed for the early ccw consoles? > > > > > > > > > > > > > I'm all for clarity! Suggestions for better names? > > > > > > css_aiv_dma_pool, maybe? Or is there other cross-device stuff that may > > > need it? > > > > > > > Ouch! I was considering to use cio_dma_zalloc() for the adapter > > interruption vectors but I ended up between the two chairs in the end. > > So with this series there are no uses for cio_dma pool. > > > > I don't feel strongly about this going one way the other. > > > > Against getting rid of the cio_dma_pool and sticking with the speaks > > dma_alloc_coherent() that we waste a DMA page per vector, which is a > > non obvious side effect. > > That would basically mean one DMA page per virtio-ccw device, right? Not quite: virtio-ccw shares airq vectors among multiple devices. We alloc 64 bytes a time and use that as long as we don't run out of bits. > For single queue devices, this seems like quite a bit of overhead. > Nod. > Are we expecting many devices in use per guest? This is affect linux in general, not only guest 2 stuff (i.e. we also waste in vanilla lpar mode). And zpci seems to do at least one airq_iv_create() per pci function. > > > > > What speaks against cio_dma_pool is that it is slightly more code, and > > this currently can not be used for very early stuff, which I don't > > think is relevant. > > Unless properly documented, it feels like something you can easily trip > over, however. > > I assume that the "very early stuff" is basically only ccw consoles. > Not sure if we can use virtio-serial as an early ccw console -- IIRC > that was only about 3215/3270? While QEMU guests are able to use a 3270 > console, this is experimental and I would not count that as a use case. > Anyway, 3215/3270 don't use adapter interrupts, and probably not > anything cross-device, either; so unless early virtio-serial is a > thing, this restriction is fine if properly documented. > Mimu can you dig into this a bit? We could also aim for getting rid of this limitation. One idea would be some sort of lazy initialization (i.e. triggered by first usage). Another idea would be simply doing the initialization earlier. Unfortunately I'm not all that familiar with the early stuff. Is there some doc you can recommend? Anyway before investing much more into this, I think we should have a fair idea which option do we prefer... > > What also used to speak against it is that > > allocations asking for more than a page would just fail, but I addressed > > that in the patch I've hacked up on top of the series, and I'm going to > > paste below. While at it I addressed some other issues as well. > > Hm, which "other issues"? > The kfree() I've forgotten to get rid of, and this 'document does not work early' (pun intended) business. > > > > I've also got code that deals with AIRQ_IV_CACHELINE by turning the > > kmem_cache into a dma_pool. > > Isn't that percolating to other airq users again? Or maybe I just don't > understand what you're proposing here... Absolutely. > > > > > Cornelia, Sebastian which approach do you prefer: > > 1) get rid of cio_dma_pool and AIRQ_IV_CACHELINE, and waste a page per > > vector, or > > 2) go with the approach taken by the patch below? > > I'm not sure that I properly understand this (yeah, you probably > guessed); so I'm not sure I can make a good call here. > I hope you, I managed to clarify some of the
Re: [PATCH 10/10] virtio/s390: make airq summary indicators DMA
On Wed, 15 May 2019 15:33:02 +0200
Michael Mueller wrote:
> >> @@ -273,8 +281,9 @@ static unsigned long get_airq_indicator(struct
> >> virtqueue *vqs[], int nvqs,
> >> unsigned long bit, flags;
> >> for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
> >> + /* TODO: this seems to be racy */
> >
> > yes, my opinions too, was already racy, in my opinion, we need another
> > patch in another series to fix this.
> >
> > However, not sure about the comment.
>
> I will drop this comment for v2 of this patch series.
> We shall fix the race with a separate patch.
Unless there is somebody eager to address this real soon, I would prefer
keeping the comment as a reminder.
Thanks for shouldering the v2!
Regards,
Halil
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v9 4/7] dm: enable synchronous dax
[ add Mike and dm-devel ]
Mike, any concerns with the below addition to the device-mapper-dax
implementation?
On Tue, May 14, 2019 at 7:58 AM Pankaj Gupta wrote:
>
> This patch sets dax device 'DAXDEV_SYNC' flag if all the target
> devices of device mapper support synchrononous DAX. If device
> mapper consists of both synchronous and asynchronous dax devices,
> we don't set 'DAXDEV_SYNC' flag.
>
> Signed-off-by: Pankaj Gupta
> ---
> drivers/md/dm-table.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index cde3b49b2a91..1cce626ff576 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -886,10 +886,17 @@ static int device_supports_dax(struct dm_target *ti,
> struct dm_dev *dev,
> return bdev_dax_supported(dev->bdev, PAGE_SIZE);
> }
>
> +static int device_synchronous(struct dm_target *ti, struct dm_dev *dev,
> + sector_t start, sector_t len, void *data)
> +{
> + return dax_synchronous(dev->dax_dev);
> +}
> +
> static bool dm_table_supports_dax(struct dm_table *t)
> {
> struct dm_target *ti;
> unsigned i;
> + bool dax_sync = true;
>
> /* Ensure that all targets support DAX. */
> for (i = 0; i < dm_table_get_num_targets(t); i++) {
> @@ -901,7 +908,14 @@ static bool dm_table_supports_dax(struct dm_table *t)
> if (!ti->type->iterate_devices ||
> !ti->type->iterate_devices(ti, device_supports_dax, NULL))
> return false;
> +
> + /* Check devices support synchronous DAX */
> + if (dax_sync &&
> + !ti->type->iterate_devices(ti, device_synchronous, NULL))
> + dax_sync = false;
> }
> + if (dax_sync)
> + set_dax_synchronous(t->md->dax_dev);
>
> return true;
> }
> --
> 2.20.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
> + vpmem->vdev = vdev;
> + vdev->priv = vpmem;
> + err = init_vq(vpmem);
> + if (err) {
> + dev_err(>dev, "failed to initialize virtio pmem vq's\n");
> + goto out_err;
> + }
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, >start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, >size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
nit: " - 1;"
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus_register(>dev,
> + >nd_desc);
> + if (!vpmem->nvdimm_bus) {
> + dev_err(>dev, "failed to register device with
> nvdimm_bus\n");
> + err = -ENXIO;
> + goto out_vq;
> + }
> +
> + dev_set_drvdata(>dev, vpmem->nvdimm_bus);
> +
> + ndr_desc.res =
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = async_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, _desc.flags);
> + set_bit(ND_REGION_ASYNC, _desc.flags);
> + nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, _desc);
> + if (!nd_region) {
> + dev_err(>dev, "failed to create nvdimm region\n");
> + err = -ENXIO;
> + goto out_nd;
> + }
> + nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
> + return 0;
> +out_nd:
> + nvdimm_bus_unregister(vpmem->nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + return err;
> +}
> +
> +static void virtio_pmem_remove(struct virtio_device *vdev)
> +{
> + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(>dev);
> +
> + nvdimm_bus_unregister(nvdimm_bus);
> + vdev->config->del_vqs(vdev);
> + vdev->config->reset(vdev);
> +}
> +
> +static struct virtio_driver virtio_pmem_driver = {
> + .driver.name= KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = id_table,
> + .probe = virtio_pmem_probe,
> + .remove = virtio_pmem_remove,
> +};
> +
> +module_virtio_driver(virtio_pmem_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio pmem driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
> new file mode 100644
> index ..ab1da877575d
> --- /dev/null
> +++ b/drivers/nvdimm/virtio_pmem.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * virtio_pmem.h: virtio pmem Driver
> + *
> + * Discovers persistent memory range information
> + * from host and provides a virtio based flushing
> + * interface.
> + **/
> +
> +#ifndef _LINUX_VIRTIO_PMEM_H
> +#define _LINUX_VIRTIO_PMEM_H
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct virtio_pmem_request {
> + /* Host return status corresponding to flush request */
> + int ret;
> +
> + /* command name*/
> + char name[16];
So ... why are we sending string commands and expect native-endianess
integers and don't define a proper request/response structure + request
types in include/uapi/linux/virtio_pmem.h like
struct virtio_pmem_resp {
__virtio32 ret;
}
#define VIRTIO_PMEM_REQ_TYPE_FLUSH 1
struct virtio_pmem_req {
__virtio16 type;
}
... and this way we also define a proper endianess format for exchange
and keep it extensible
@MST, what's your take on this?
--
Thanks,
David / dhildenb
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Qemu-devel] [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
On Tue, May 14, 2019 at 8:25 AM Pankaj Gupta wrote: > > > > On 5/14/19 7:54 AM, Pankaj Gupta wrote: > > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig > > > index 35897649c24f..94bad084ebab 100644 > > > --- a/drivers/virtio/Kconfig > > > +++ b/drivers/virtio/Kconfig > > > @@ -42,6 +42,17 @@ config VIRTIO_PCI_LEGACY > > > > > > If unsure, say Y. > > > > > > +config VIRTIO_PMEM > > > + tristate "Support for virtio pmem driver" > > > + depends on VIRTIO > > > + depends on LIBNVDIMM > > > + help > > > + This driver provides access to virtio-pmem devices, storage devices > > > + that are mapped into the physical address space - similar to NVDIMMs > > > +- with a virtio-based flushing interface. > > > + > > > + If unsure, say M. > > > > > > from Documentation/process/coding-style.rst: > > "Lines under a ``config`` definition > > are indented with one tab, while help text is indented an additional two > > spaces." > > ah... I changed help text and 'checkpatch' did not say anything :( . > > Will wait for Dan, If its possible to add two spaces to help text while > applying > the series. I'm inclined to handle this with a fixup appended to the end of the series just so the patchwork-bot does not get confused by the content changing from what was sent to the list. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/10] s390/cio: add basic protected virtualization support
On Tue, 14 May 2019 10:47:34 -0400
"Jason J. Herne" wrote:
> On 5/13/19 5:41 AM, Cornelia Huck wrote:
> > On Fri, 26 Apr 2019 20:32:41 +0200
> > Halil Pasic wrote:
> >
> >> As virtio-ccw devices are channel devices, we need to use the dma area
> >> for any communication with the hypervisor.
> >>
> >> This patch addresses the most basic stuff (mostly what is required for
> >> virtio-ccw), and does take care of QDIO or any devices.
> >
> > "does not take care of QDIO", surely? (Also, what does "any devices"
> > mean? Do you mean "every arbitrary device", perhaps?)
> >
> >>
> >> An interesting side effect is that virtio structures are now going to
> >> get allocated in 31 bit addressable storage.
> >
> > Hm...
> >
> >>
> >> Signed-off-by: Halil Pasic
> >> ---
> >> arch/s390/include/asm/ccwdev.h | 4 +++
> >> drivers/s390/cio/ccwreq.c| 8 ++---
> >> drivers/s390/cio/device.c| 65
> >> +---
> >> drivers/s390/cio/device_fsm.c| 40 -
> >> drivers/s390/cio/device_id.c | 18 +--
> >> drivers/s390/cio/device_ops.c| 21 +++--
> >> drivers/s390/cio/device_pgid.c | 20 ++---
> >> drivers/s390/cio/device_status.c | 24 +++
> >> drivers/s390/cio/io_sch.h| 21 +
> >> drivers/s390/virtio/virtio_ccw.c | 10 ---
> >> 10 files changed, 148 insertions(+), 83 deletions(-)
> >
> > (...)
> >
> >> diff --git a/drivers/s390/virtio/virtio_ccw.c
> >> b/drivers/s390/virtio/virtio_ccw.c
> >> index 6d989c360f38..bb7a92316fc8 100644
> >> --- a/drivers/s390/virtio/virtio_ccw.c
> >> +++ b/drivers/s390/virtio/virtio_ccw.c
> >> @@ -66,7 +66,6 @@ struct virtio_ccw_device {
> >>bool device_lost;
> >>unsigned int config_ready;
> >>void *airq_info;
> >> - u64 dma_mask;
> >> };
> >>
> >> struct vq_info_block_legacy {
> >> @@ -1255,16 +1254,7 @@ static int virtio_ccw_online(struct ccw_device
> >> *cdev)
> >>ret = -ENOMEM;
> >>goto out_free;
> >>}
> >> -
> >>vcdev->vdev.dev.parent = >dev;
> >> - cdev->dev.dma_mask = >dma_mask;
> >> - /* we are fine with common virtio infrastructure using 64 bit DMA */
> >> - ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(64));
> >> - if (ret) {
> >> - dev_warn(>dev, "Failed to enable 64-bit DMA.\n");
> >> - goto out_free;
> >> - }
> >
> > This means that vring structures now need to fit into 31 bits as well,
> > I think? Is there any way to reserve the 31 bit restriction for channel
> > subsystem structures and keep vring in the full 64 bit range? (Or am I
> > fundamentally misunderstanding something?)
> >
>
> I hope I've understood everything... I'm new to virtio. But from what I'm
> understanding,
> the vring structure (a.k.a. the VirtQueue) needs to be accessed and modified
> by both host
> and guest. Therefore the page(s) holding that data need to be marked shared
> if using
> protected virtualization. This patch set makes use of DMA pages by way of
> swiotlb (always
> below 32-bit line right?) for shared memory.
The last sentence is wrong. You have to differentiate between stuff that
is mapped as DMA and that is allocated as DMA. The mapped stuff is
handled via swiotlb and bouncing. But that can not work for vring stuff
which needs to be allocated as DMA.
> Therefore, a side effect is that all shared
> memory, including VirtQueue data will be in the DMA zone and in 32-bit memory.
>
Consequently wrong. The reason I explained in a reply to Connie (see
there).
> I don't see any restrictions on sharing pages above the 32-bit line. So it
> seems possible.
> I'm not sure how much more work it would be. I wonder if Halil has considered
> this?
I did consider this, the RFC was doing this (again see other mail).
> Are we
> worried that virtio data structures are going to be a burden on the 31-bit
> address space?
>
>
That is a good question I can not answer. Since it is currently at least
a page per queue (because we use dma direct, right Mimu?), I am concerned
about this.
Connie, what is your opinion?
Regards,
Halil
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 06/10] s390/cio: add basic protected virtualization support
On Mon, 13 May 2019 11:41:36 +0200
Cornelia Huck wrote:
> On Fri, 26 Apr 2019 20:32:41 +0200
> Halil Pasic wrote:
>
> > As virtio-ccw devices are channel devices, we need to use the dma area
> > for any communication with the hypervisor.
> >
> > This patch addresses the most basic stuff (mostly what is required for
> > virtio-ccw), and does take care of QDIO or any devices.
>
> "does not take care of QDIO", surely?
I did not bother making the QDIO library code use dma memory for
anything that is conceptually dma memory. AFAIK QDIO is out of scope for
prot virt for now. If one were to do some emulated qdio with prot virt
guests, one wound need to make a bunch of things shared.
> (Also, what does "any devices"
> mean? Do you mean "every arbitrary device", perhaps?)
What I mean is: this patch takes care of the core stuff, but any
particular device is likely to have to do more -- that is it ain't all
the cio devices support prot virt with this patch. For example
virtio-ccw needs to make sure that the ccws constituting the channel
programs, as well as the data pointed by the ccws is shared. If one
would want to make vfio-ccw DASD pass-through work under prot virt, one
would need to make sure, that everything that needs to be shared is
shared (data buffers, channel programs).
Does is clarify things?
>
> >
> > An interesting side effect is that virtio structures are now going to
> > get allocated in 31 bit addressable storage.
>
> Hm...
>
> >
> > Signed-off-by: Halil Pasic
> > ---
> > arch/s390/include/asm/ccwdev.h | 4 +++
> > drivers/s390/cio/ccwreq.c| 8 ++---
> > drivers/s390/cio/device.c| 65
> > +---
> > drivers/s390/cio/device_fsm.c| 40 -
> > drivers/s390/cio/device_id.c | 18 +--
> > drivers/s390/cio/device_ops.c| 21 +++--
> > drivers/s390/cio/device_pgid.c | 20 ++---
> > drivers/s390/cio/device_status.c | 24 +++
> > drivers/s390/cio/io_sch.h| 21 +
> > drivers/s390/virtio/virtio_ccw.c | 10 ---
> > 10 files changed, 148 insertions(+), 83 deletions(-)
>
> (...)
>
> > diff --git a/drivers/s390/virtio/virtio_ccw.c
> > b/drivers/s390/virtio/virtio_ccw.c
> > index 6d989c360f38..bb7a92316fc8 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
> > @@ -66,7 +66,6 @@ struct virtio_ccw_device {
> > bool device_lost;
> > unsigned int config_ready;
> > void *airq_info;
> > - u64 dma_mask;
> > };
> >
> > struct vq_info_block_legacy {
> > @@ -1255,16 +1254,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
> > ret = -ENOMEM;
> > goto out_free;
> > }
> > -
> > vcdev->vdev.dev.parent = >dev;
> > - cdev->dev.dma_mask = >dma_mask;
> > - /* we are fine with common virtio infrastructure using 64 bit DMA */
> > - ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(64));
> > - if (ret) {
> > - dev_warn(>dev, "Failed to enable 64-bit DMA.\n");
> > - goto out_free;
> > - }
>
> This means that vring structures now need to fit into 31 bits as well,
> I think?
Nod.
> Is there any way to reserve the 31 bit restriction for channel
> subsystem structures and keep vring in the full 64 bit range? (Or am I
> fundamentally misunderstanding something?)
>
At the root of this problem is that the DMA API basically says devices
may have addressing limitations expressed by the dma_mask, while our
addressing limitations are not coming from the device but from the IO
arch: e.g. orb.cpa and ccw.cda are 31 bit addresses. In our case it
depends on how and for what is the device going to use the memory (e.g.
buffers addressed by MIDA vs IDA vs direct).
Virtio uses the DMA properties of the parent, that is in our case the
struct device embedded in struct ccw_device.
The previous version (RFC) used to allocate all the cio DMA stuff from
this global cio_dma_pool using the css0.dev for the DMA API
interactions. And we set *css0.dev.dma_mask == DMA_BIT_MASK(31) so
e.g. the allocated ccws are 31 bit addressable.
But I was asked to change this so that when I allocate DMA memory for a
channel program of particular ccw device, a struct device of that ccw
device is used as the first argument of dma_alloc_coherent().
Considering
void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
gfp_t flag, unsigned long attrs)
{
const struct dma_map_ops *ops = get_dma_ops(dev);
void *cpu_addr;
WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
if (dma_alloc_from_dev_coherent(dev, size, dma_handle, _addr))
return cpu_addr;
/* let the implementation decide on the zone to allocate from: */
flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM);
that is the GFP flags dropped that implies that we really want
cdev->dev restricted to 31 bit addressable memory because
Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
On 15.05.19 22:46, David Hildenbrand wrote:
>> +vpmem->vdev = vdev;
>> +vdev->priv = vpmem;
>> +err = init_vq(vpmem);
>> +if (err) {
>> +dev_err(>dev, "failed to initialize virtio pmem vq's\n");
>> +goto out_err;
>> +}
>> +
>> +virtio_cread(vpmem->vdev, struct virtio_pmem_config,
>> +start, >start);
>> +virtio_cread(vpmem->vdev, struct virtio_pmem_config,
>> +size, >size);
>> +
>> +res.start = vpmem->start;
>> +res.end = vpmem->start + vpmem->size-1;
>
> nit: " - 1;"
>
>> +vpmem->nd_desc.provider_name = "virtio-pmem";
>> +vpmem->nd_desc.module = THIS_MODULE;
>> +
>> +vpmem->nvdimm_bus = nvdimm_bus_register(>dev,
>> +>nd_desc);
>> +if (!vpmem->nvdimm_bus) {
>> +dev_err(>dev, "failed to register device with
>> nvdimm_bus\n");
>> +err = -ENXIO;
>> +goto out_vq;
>> +}
>> +
>> +dev_set_drvdata(>dev, vpmem->nvdimm_bus);
>> +
>> +ndr_desc.res =
>> +ndr_desc.numa_node = nid;
>> +ndr_desc.flush = async_pmem_flush;
>> +set_bit(ND_REGION_PAGEMAP, _desc.flags);
>> +set_bit(ND_REGION_ASYNC, _desc.flags);
>> +nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, _desc);
>> +if (!nd_region) {
>> +dev_err(>dev, "failed to create nvdimm region\n");
>> +err = -ENXIO;
>> +goto out_nd;
>> +}
>> +nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
>> +return 0;
>> +out_nd:
>> +nvdimm_bus_unregister(vpmem->nvdimm_bus);
>> +out_vq:
>> +vdev->config->del_vqs(vdev);
>> +out_err:
>> +return err;
>> +}
>> +
>> +static void virtio_pmem_remove(struct virtio_device *vdev)
>> +{
>> +struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(>dev);
>> +
>> +nvdimm_bus_unregister(nvdimm_bus);
>> +vdev->config->del_vqs(vdev);
>> +vdev->config->reset(vdev);
>> +}
>> +
>> +static struct virtio_driver virtio_pmem_driver = {
>> +.driver.name= KBUILD_MODNAME,
>> +.driver.owner = THIS_MODULE,
>> +.id_table = id_table,
>> +.probe = virtio_pmem_probe,
>> +.remove = virtio_pmem_remove,
>> +};
>> +
>> +module_virtio_driver(virtio_pmem_driver);
>> +MODULE_DEVICE_TABLE(virtio, id_table);
>> +MODULE_DESCRIPTION("Virtio pmem driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
>> new file mode 100644
>> index ..ab1da877575d
>> --- /dev/null
>> +++ b/drivers/nvdimm/virtio_pmem.h
>> @@ -0,0 +1,60 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * virtio_pmem.h: virtio pmem Driver
>> + *
>> + * Discovers persistent memory range information
>> + * from host and provides a virtio based flushing
>> + * interface.
>> + **/
>> +
>> +#ifndef _LINUX_VIRTIO_PMEM_H
>> +#define _LINUX_VIRTIO_PMEM_H
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +struct virtio_pmem_request {
>> +/* Host return status corresponding to flush request */
>> +int ret;
>> +
>> +/* command name*/
>> +char name[16];
>
> So ... why are we sending string commands and expect native-endianess
> integers and don't define a proper request/response structure + request
> types in include/uapi/linux/virtio_pmem.h like
>
> struct virtio_pmem_resp {
> __virtio32 ret;
> }
FWIW, I wonder if we should even properly translate return values and
define types like
VIRTIO_PMEM_RESP_TYPE_OK0
VIRTIO_PMEM_RESP_TYPE_EIO 1
..
>
> #define VIRTIO_PMEM_REQ_TYPE_FLUSH1
> struct virtio_pmem_req {
> __virtio16 type;
> }
>
> ... and this way we also define a proper endianess format for exchange
> and keep it extensible
>
> @MST, what's your take on this?
>
>
--
Thanks,
David / dhildenb
Re: [PATCH v9 1/7] libnvdimm: nd_region flush callback support
On Tue, May 14, 2019 at 7:55 AM Pankaj Gupta wrote:
>
> This patch adds functionality to perform flush from guest
> to host over VIRTIO. We are registering a callback based
> on 'nd_region' type. virtio_pmem driver requires this special
> flush function. For rest of the region types we are registering
> existing flush function. Report error returned by host fsync
> failure to userspace.
>
> Signed-off-by: Pankaj Gupta
> ---
> drivers/acpi/nfit/core.c | 4 ++--
> drivers/nvdimm/claim.c | 6 --
> drivers/nvdimm/nd.h | 1 +
> drivers/nvdimm/pmem.c| 13 -
> drivers/nvdimm/region_devs.c | 26 --
> include/linux/libnvdimm.h| 8 +++-
> 6 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 5a389a4f4f65..08dde76cf459 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2434,7 +2434,7 @@ static void write_blk_ctl(struct nfit_blk *nfit_blk,
> unsigned int bw,
> offset = to_interleave_offset(offset, mmio);
>
> writeq(cmd, mmio->addr.base + offset);
> - nvdimm_flush(nfit_blk->nd_region);
> + nvdimm_flush(nfit_blk->nd_region, NULL);
>
> if (nfit_blk->dimm_flags & NFIT_BLK_DCR_LATCH)
> readq(mmio->addr.base + offset);
> @@ -2483,7 +2483,7 @@ static int acpi_nfit_blk_single_io(struct nfit_blk
> *nfit_blk,
> }
>
> if (rw)
> - nvdimm_flush(nfit_blk->nd_region);
> + nvdimm_flush(nfit_blk->nd_region, NULL);
>
> rc = read_blk_stat(nfit_blk, lane) ? -EIO : 0;
> return rc;
> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
> index fb667bf469c7..13510bae1e6f 100644
> --- a/drivers/nvdimm/claim.c
> +++ b/drivers/nvdimm/claim.c
> @@ -263,7 +263,7 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns,
> struct nd_namespace_io *nsio = to_nd_namespace_io(>dev);
> unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
> sector_t sector = offset >> 9;
> - int rc = 0;
> + int rc = 0, ret = 0;
>
> if (unlikely(!size))
> return 0;
> @@ -301,7 +301,9 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns,
> }
>
> memcpy_flushcache(nsio->addr + offset, buf, size);
> - nvdimm_flush(to_nd_region(ndns->dev.parent));
> + ret = nvdimm_flush(to_nd_region(ndns->dev.parent), NULL);
> + if (ret)
> + rc = ret;
>
> return rc;
> }
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index a5ac3b240293..0c74d2428bd7 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -159,6 +159,7 @@ struct nd_region {
> struct badblocks bb;
> struct nd_interleave_set *nd_set;
> struct nd_percpu_lane __percpu *lane;
> + int (*flush)(struct nd_region *nd_region, struct bio *bio);
So this triggers:
In file included from drivers/nvdimm/e820.c:7:
./include/linux/libnvdimm.h:140:51: warning: ‘struct bio’ declared
inside parameter list will not be visible outside of this definition
or declaration
int (*flush)(struct nd_region *nd_region, struct bio *bio);
^~~
I was already feeling uneasy about trying to squeeze this into v5.2,
but this warning and the continued drip of comments leads me to
conclude that this driver would do well to wait one more development
cycle. Lets close out the final fixups and let this driver soak in
-next. Then for the v5.3 cycle I'll redouble my efforts towards the
goal of closing patch acceptance at the -rc6 / -rc7 development
milestone.
Re: [PATCH v5 00/20] Share TTM code among DRM framebuffer drivers
Hi,
most of this patch set still needs reviews.
If it's too large for merging or reviewing at once, I could move the
driver changes into separate patch sets. The vbox driver's changes have
been accepted by Hans already. So only keeping the core changes plus
vbox would be an option.
Best regards
Thomas
Am 08.05.19 um 10:26 schrieb Thomas Zimmermann:
> Several simple framebuffer drivers copy most of the TTM code from each
> other. The implementation is always the same; except for the name of
> some data structures.
>
> As recently discussed, this patch set provides generic memory-management
> code for simple framebuffers with dedicated video memory. It further
> converts the respective drivers to the generic code. The shared code
> is basically the same implementation as the one copied among individual
> drivers.
>
> The patch set contains two major changes: first, it introduces
> |struct drm_gem_vram_object| and helpers (GEM VRAM). It's a GEM object
> that is backed by VRAM. The type's purpose is somewhat similar to
> |struct drm_gem_{cma, shmem}_object|: it provides an commom implementation
> that handles all the basic cases. Second, the patch set introduces
> |struct drm_vram_mm| and helpers (VRAM MM). It's an implementation of a
> basic memory manager for VRAM.
>
> Both, GEM VRAM and VRAM MM, support buffer placement in VRAM and system
> memory. Both can be used independedly from each other if desired by the
> DRM driver.
>
> Currently ast, bochs, mgag200, vboxvideo and hisilicon/hibmc can use
> these helpers.
>
> Future directions: with these changes, the respective drivers can also
> share some of their mode-setting or fbdev code. GEM VRAM's PRIME helpers
> allow for using the generic fbcon emulation.
>
> The patch set is against a recent drm-tip.
>
> v5:
> * move bochs PRIME functions to GEM VRAM helpers
> * always set struct file_operations.llseek to no_llseek()
> * add WARN_ON_ONCE for pin-count mismatches
> * only allocate 2 entries in placements array
> v4:
> * cleanups from checkpatch.pl
> * add more documentation for VRAM helpers
> * remove several fixed-size types from interfaces
> * don't make drivers depend on DRM_TTM; auto-selected if necessary
> * use single config optiom DRM_VRAM_HELPER
> v3:
> * share VRAM MM callback structure among drivers
> * move VRAM MM instances to drm_device and share rsp. code
> v2:
> * rename |struct drm_gem_ttm_object| to |struct drm_gem_vram_object|
> * rename |struct drm_simple_ttm| to |struct drm_vram_mm|
> * make drm_is_gem_ttm() an internal helper
> * add drm_gem_vram_kmap_at()
> * return is_iomem from kmap functions
> * redefine TTM placement flags for public interface
> * add drm_vram_mm_mmap() helper
> * replace almost all of driver's TTM code with these helpers
> * documentation fixes
>
> Thomas Zimmermann (20):
> drm: Add |struct drm_gem_vram_object| and helpers
> drm: Add |struct drm_gem_vram_object| callbacks for |struct
> ttm_bo_driver|
> drm: Add |struct drm_gem_vram_object| callbacks for |struct
> drm_driver|
> drm: Add drm_gem_vram_fill_create_dumb() to create dumb buffers
> drm: Add simple PRIME helpers for GEM VRAM
> drm: Add VRAM MM, a simple memory manager for dedicated VRAM
> drm: Add default instance for VRAM MM callback functions
> drm: Integrate VRAM MM into struct drm_device
> drm/ast: Convert AST driver to |struct drm_gem_vram_object|
> drm/ast: Convert AST driver to VRAM MM
> drm/ast: Replace mapping code with drm_gem_vram_{kmap/kunmap}()
> drm/bochs: Convert bochs driver to |struct drm_gem_vram_object|
> drm/bochs: Convert bochs driver to VRAM MM
> drm/mgag200: Convert mgag200 driver to |struct drm_gem_vram_object|
> drm/mgag200: Convert mgag200 driver to VRAM MM
> drm/mgag200: Replace mapping code with drm_gem_vram_{kmap/kunmap}()
> drm/vboxvideo: Convert vboxvideo driver to |struct
> drm_gem_vram_object|
> drm/vboxvideo: Convert vboxvideo driver to VRAM MM
> drm/hisilicon: Convert hibmc-drm driver to |struct
> drm_gem_vram_object|
> drm/hisilicon: Convert hibmc-drm driver to VRAM MM
>
> Documentation/gpu/drm-mm.rst | 34 +-
> drivers/gpu/drm/Kconfig | 7 +
> drivers/gpu/drm/Makefile | 5 +
> drivers/gpu/drm/ast/Kconfig | 3 +-
> drivers/gpu/drm/ast/ast_drv.c | 13 +-
> drivers/gpu/drm/ast/ast_drv.h | 71 +-
> drivers/gpu/drm/ast/ast_fb.c | 34 +-
> drivers/gpu/drm/ast/ast_main.c| 77 +-
> drivers/gpu/drm/ast/ast_mode.c| 124 +--
> drivers/gpu/drm/ast/ast_ttm.c | 302 +---
> drivers/gpu/drm/bochs/Kconfig | 2 +-
> drivers/gpu/drm/bochs/bochs.h | 54 +-
> drivers/gpu/drm/bochs/bochs_drv.c | 22 +-
>
Re: [PATCH v2 7/8] vsock/virtio: increase RX buffer size to 64 KiB
On Wed, May 15, 2019 at 10:50:43AM +0800, Jason Wang wrote:
>
> On 2019/5/15 上午12:20, Stefano Garzarella wrote:
> > On Tue, May 14, 2019 at 11:38:05AM +0800, Jason Wang wrote:
> > > On 2019/5/14 上午1:51, Stefano Garzarella wrote:
> > > > On Mon, May 13, 2019 at 06:01:52PM +0800, Jason Wang wrote:
> > > > > On 2019/5/10 下午8:58, Stefano Garzarella wrote:
> > > > > > In order to increase host -> guest throughput with large packets,
> > > > > > we can use 64 KiB RX buffers.
> > > > > >
> > > > > > Signed-off-by: Stefano Garzarella
> > > > > > ---
> > > > > > include/linux/virtio_vsock.h | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/include/linux/virtio_vsock.h
> > > > > > b/include/linux/virtio_vsock.h
> > > > > > index 84b72026d327..5a9d25be72df 100644
> > > > > > --- a/include/linux/virtio_vsock.h
> > > > > > +++ b/include/linux/virtio_vsock.h
> > > > > > @@ -10,7 +10,7 @@
> > > > > > #define VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE 128
> > > > > > #define VIRTIO_VSOCK_DEFAULT_BUF_SIZE (1024 * 256)
> > > > > > #define VIRTIO_VSOCK_DEFAULT_MAX_BUF_SIZE (1024 * 256)
> > > > > > -#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4)
> > > > > > +#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 64)
> > > > > > #define VIRTIO_VSOCK_MAX_BUF_SIZE 0xUL
> > > > > > #define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64)
> > > > > We probably don't want such high order allocation. It's better to
> > > > > switch to
> > > > > use order 0 pages in this case. See add_recvbuf_big() for virtio-net.
> > > > > If we
> > > > > get datapath unified, we will get more stuffs set.
> > > > IIUC, you are suggesting to allocate only pages and put them in a
> > > > scatterlist, then add them to the virtqueue.
> > > >
> > > > Is it correct?
> > >
> > > Yes since you are using:
> > >
> > > pkt->buf = kmalloc(buf_len, GFP_KERNEL);
> > > if (!pkt->buf) {
> > > virtio_transport_free_pkt(pkt);
> > > break;
> > > }
> > >
> > > This is likely to fail when the memory is fragmented which is kind of
> > > fragile.
> > >
> > >
> > Thanks for pointing that out.
> >
> > > > The issue that I have here, is that the virtio-vsock guest driver, see
> > > > virtio_vsock_rx_fill(), allocates a struct virtio_vsock_pkt that
> > > > contains the room for the header, then allocates the buffer for the
> > > > payload.
> > > > At this point it fills the scatterlist with the _vsock_pkt.hdr
> > > > and the
> > > > buffer for the payload.
> > >
> > > This part should be fine since what is needed is just adding more pages to
> > > sg[] and call virtuqeueu_add_sg().
> > >
> > >
> > Yes, I agree.
> >
> > > > Changing this will require several modifications, and if we get datapath
> > > > unified, I'm not sure it's worth it.
> > > > Of course, if we leave the datapaths separated, I'd like to do that
> > > > later.
> > > >
> > > > What do you think?
> > >
> > > For the driver it self, it should not be hard. But I think you mean the
> > > issue of e.g virtio_vsock_pkt itself which doesn't support sg. For short
> > > time, maybe we can use kvec instead.
> > I'll try to use kvec in the virtio_vsock_pkt.
> >
> > Since this struct is shared also with the host driver (vhost-vsock),
> > I hope the changes could be limited, otherwise we can remove the last 2
> > patches of the series for now, leaving the RX buffer size to 4KB.
>
>
> Yes and if it introduces too much changes, maybe we can do the 64KB buffer
> in the future with the conversion of using skb where supports page frag
> natively.
Yes, I completely agree!
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 10/10] virtio/s390: make airq summary indicators DMA
On Wed, 15 May 2019 15:43:23 +0200
Michael Mueller wrote:
> On 13.05.19 14:20, Cornelia Huck wrote:
> > On Fri, 26 Apr 2019 20:32:45 +0200
> > Halil Pasic wrote:
> >
> >> Hypervisor needs to interact with the summary indicators, so these
> >> need to be DMA memory as well (at least for protected virtualization
> >> guests).
> >>
> >> Signed-off-by: Halil Pasic
> >> ---
> >> drivers/s390/virtio/virtio_ccw.c | 24 +---
> >> 1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > (...)
> >
> >> @@ -237,7 +243,8 @@ static void virtio_airq_handler(struct airq_struct
> >> *airq)
> >>read_unlock(>lock);
> >> }
> >>
> >> -static struct airq_info *new_airq_info(void)
> >> +/* call with drivers/s390/virtio/virtio_ccw.cheld */
> >
> > Hm, where is airq_areas_lock defined? If it was introduced in one of
> > the previous patches, I have missed it.
>
> There is no airq_areas_lock defined currently. My assumption is that
> this will be used in context with the likely race condition this
> part of the patch is talking about.
>
> @@ -273,8 +281,9 @@ static unsigned long get_airq_indicator(struct
> virtqueue *vqs[], int nvqs,
> unsigned long bit, flags;
>
> for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
> + /* TODO: this seems to be racy */
> if (!airq_areas[i])
> - airq_areas[i] = new_airq_info();
> + airq_areas[i] = new_airq_info(i);
>
>
> As this shall be handled by a separate patch I will drop the comment
> in regard to airq_areas_lock from this patch as well for v2.
Ok, that makes sense.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
