Messages in this thread | | | Subject | Re: [PATCH v8 2/3] tty: hvc: pass DMA capable memory to put_chars() | From | Xianting Tian <> | Date | Sat, 18 Sep 2021 20:32:01 +0800 |
| |
hi
Will you consider to continue the disscussion of this patch? thanks
在 2021/8/20 下午4:43, Xianting TIan 写道: > > 在 2021/8/20 下午2:49, Daniel Axtens 写道: >> Xianting Tian <xianting.tian@linux.alibaba.com> writes: >> >>> As well known, hvc backend driver(eg, virtio-console) can register its >>> operations to hvc framework. The operations can contain put_chars(), >>> get_chars() and so on. >>> >>> Some hvc backend may do dma in its operations. eg, put_chars() of >>> virtio-console. But in the code of hvc framework, it may pass DMA >>> incapable memory to put_chars() under a specific configuration, which >>> is explained in commit c4baad5029(virtio-console: avoid DMA from >>> stack): >> We could also run into issues on powerpc where Andrew is working on >> adding vmap-stack but the opal hvc driver assumes that it is passed a >> buffer which is not in vmalloc space but in the linear mapping. So it >> would be good to fix this (or more clearly document what drivers can >> expect). >> >>> 1, c[] is on stack, >>> hvc_console_print(): >>> char c[N_OUTBUF] __ALIGNED__; >>> cons_ops[index]->put_chars(vtermnos[index], c, i); >>> 2, ch is on stack, >>> static void hvc_poll_put_char(,,char ch) >>> { >>> struct tty_struct *tty = driver->ttys[0]; >>> struct hvc_struct *hp = tty->driver_data; >>> int n; >>> >>> do { >>> n = hp->ops->put_chars(hp->vtermno, &ch, 1); >>> } while (n <= 0); >>> } >>> >>> Commit c4baad5029 is just the fix to avoid DMA from stack memory, which >>> is passed to virtio-console by hvc framework in above code. But I think >>> the fix is aggressive, it directly uses kmemdup() to alloc new buffer >>> from kmalloc area and do memcpy no matter the memory is in kmalloc area >>> or not. But most importantly, it should better be fixed in the hvc >>> framework, by changing it to never pass stack memory to the put_chars() >>> function in the first place. Otherwise, we still face the same issue if >>> a new hvc backend using dma added in the future. >>> >>> In this patch, we make 'char out_buf[N_OUTBUF]' and 'chat out_ch' part >>> of 'struct hvc_struct', so both two buf are no longer the stack memory. >>> we can use it in above two cases separately. >>> >>> Introduce another array(cons_outbufs[]) for buffer pointers next to >>> the cons_ops[] and vtermnos[] arrays. With the array, we can easily >>> find >>> the buffer, instead of traversing hp list. >>> >>> With the patch, we can remove the fix c4baad5029. >>> >>> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com> >>> Reviewed-by: Shile Zhang <shile.zhang@linux.alibaba.com> >>> struct hvc_struct { >>> struct tty_port port; >>> spinlock_t lock; >>> int index; >>> int do_wakeup; >>> - char *outbuf; >>> - int outbuf_size; >>> int n_outbuf; >>> uint32_t vtermno; >>> const struct hv_ops *ops; >>> @@ -48,6 +56,10 @@ struct hvc_struct { >>> struct work_struct tty_resize; >>> struct list_head next; >>> unsigned long flags; >>> + char out_ch; >>> + char out_buf[N_OUTBUF] __ALIGNED__; >>> + int outbuf_size; >>> + char outbuf[0] __ALIGNED__; >> I'm trying to understand this patch but I am finding it very difficult >> to understand what the difference between `out_buf` and `outbuf` >> (without the underscore) is supposed to be. `out_buf` is statically >> sized and the size of `outbuf` is supposed to depend on the arguments to >> hvc_alloc(), but I can't quite figure out what the roles of each one are >> and their names are confusingly similiar! >> >> I looked briefly at the older revisions of the series but it didn't make >> things much clearer. >> >> Could you give them clearer names? > > thanks for the comments, > > It is indeed not easy to understand by the name. I will change it to a > proper name if we have next version patch. > > Jiri Slaby is worring about the performance, because we need add two > locks to protect 'out_ch' and 'out_buf' separately, the origin > on-stack buffer is lockless. > > I don't know whether this solution can be accepted, just waiting for > Jiri's further commtents. > >> >> Also, looking at Documentation/process/deprecated.rst, it looks like >> maybe we want to use a 'flexible array member' instead: >> >> .. note:: If you are using struct_size() on a structure containing a >> zero-length >> or a one-element array as a trailing array member, please >> refactor such >> array usage and switch to a `flexible array member >> <#zero-length-and-one-element-arrays>`_ instead. >> >> I think we want: > thanks, we should use [], not [0]. >> >>> + char outbuf[] __ALIGNED__; >> Kind regards, >> Daniel
| |