* [PATCH v7 0/2] make hvc pass dma capable memory to its backend @ 2021-08-17 13:22 Xianting Tian 2021-08-17 13:22 ` [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian 2021-08-17 13:23 ` [PATCH v7 2/2] virtio-console: remove unnecessary kmemdup() Xianting Tian 0 siblings, 2 replies; 9+ messages in thread From: Xianting Tian @ 2021-08-17 13:22 UTC (permalink / raw) To: gregkh, jirislaby, amit, arnd, osandov Cc: linuxppc-dev, virtualization, linux-kernel, Xianting Tian Dear all, This patch series make hvc framework pass DMA capable memory to put_chars() of hvc backend(eg, virtio-console), and revert commit c4baad5029 ("virtio-console: avoid DMA from stack”) V1 virtio-console: avoid DMA from vmalloc area https://lkml.org/lkml/2021/7/27/494 For v1 patch, Arnd Bergmann suggests to fix the issue in the first place: Make hvc pass DMA capable memory to put_chars() The fix suggestion is included in v2. V2 [PATCH 1/2] tty: hvc: pass DMA capable memory to put_chars() https://lkml.org/lkml/2021/8/1/8 [PATCH 2/2] virtio-console: remove unnecessary kmemdup() https://lkml.org/lkml/2021/8/1/9 For v2 patch, Arnd Bergmann suggests to make new buf part of the hvc_struct structure, and fix the compile issue. The fix suggestion is included in v3. V3 [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars() https://lkml.org/lkml/2021/8/3/1347 [PATCH v3 2/2] virtio-console: remove unnecessary kmemdup() https://lkml.org/lkml/2021/8/3/1348 For v3 patch, Jiri Slaby suggests to make 'char c[N_OUTBUF]' part of hvc_struct, and make 'hp->outbuf' aligned and use struct_size() to calculate the size of hvc_struct. The fix suggestion is included in v4. V4 [PATCH v4 0/2] make hvc pass dma capable memory to its backend https://lkml.org/lkml/2021/8/5/1350 [PATCH v4 1/2] tty: hvc: pass DMA capable memory to put_chars() https://lkml.org/lkml/2021/8/5/1351 [PATCH v4 2/2] virtio-console: remove unnecessary kmemdup() https://lkml.org/lkml/2021/8/5/1352 For v4 patch, Arnd Bergmann suggests to introduce another array(cons_outbuf[]) for the buffer pointers next to the cons_ops[] and vtermnos[] arrays. This fix included in this v5 patch. V5 Arnd Bergmann suggests to use "L1_CACHE_BYTES" as dma alignment, use 'sizeof(long)' as dma alignment is wrong. fix it in v6. V6 It contains coding error, fix it in v7 and it worked normally according to test result. drivers/tty/hvc/hvc_console.c | 39 +++++++++++++++++++++-------------- drivers/tty/hvc/hvc_console.h | 16 ++++++++++++-- drivers/tty/hvc/hvc_console.h | 16 ++++++++++++-- 3 file changed ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() 2021-08-17 13:22 [PATCH v7 0/2] make hvc pass dma capable memory to its backend Xianting Tian @ 2021-08-17 13:22 ` Xianting Tian 2021-08-17 15:48 ` Greg KH ` (2 more replies) 2021-08-17 13:23 ` [PATCH v7 2/2] virtio-console: remove unnecessary kmemdup() Xianting Tian 1 sibling, 3 replies; 9+ messages in thread From: Xianting Tian @ 2021-08-17 13:22 UTC (permalink / raw) To: gregkh, jirislaby, amit, arnd, osandov Cc: linuxppc-dev, virtualization, linux-kernel, Xianting Tian As well known, hvc backend can register its opertions to hvc backend. the opertions contain put_chars(), get_chars() and so on. Some hvc backend may do dma in its opertions. 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): 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 furture. We make 'char c[N_OUTBUF]' part of 'struct hvc_struct', so hp->c is no longer the stack memory. we can use it in above two cases. Other fix is use L1_CACHE_BYTES as the alignment, use 'sizeof(long)' as dma alignment is wrong. And use struct_size() to calculate size of hvc_struct. Introduce another array(cons_hvcs[]) for hvc_struct pointers next to the cons_ops[] and vtermnos[] arrays. With the array, we can easily find the hp, instead of traversing hp list. With the patch, we can remove the fix c4baad5029. We tested the patch, it worked normally. Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com> --- drivers/tty/hvc/hvc_console.c | 39 +++++++++++++++++++++-------------- drivers/tty/hvc/hvc_console.h | 16 ++++++++++++-- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index 5bb8c4e44..b882ceb5f 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -41,16 +41,6 @@ */ #define HVC_CLOSE_WAIT (HZ/100) /* 1/10 of a second */ -/* - * These sizes are most efficient for vio, because they are the - * native transfer size. We could make them selectable in the - * future to better deal with backends that want other buffer sizes. - */ -#define N_OUTBUF 16 -#define N_INBUF 16 - -#define __ALIGNED__ __attribute__((__aligned__(sizeof(long)))) - static struct tty_driver *hvc_driver; static struct task_struct *hvc_task; @@ -142,6 +132,7 @@ static int hvc_flush(struct hvc_struct *hp) static const struct hv_ops *cons_ops[MAX_NR_HVC_CONSOLES]; static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] = {[0 ... MAX_NR_HVC_CONSOLES - 1] = -1}; +static struct hvc_struct *cons_hvcs[MAX_NR_HVC_CONSOLES]; /* * Console APIs, NOT TTY. These APIs are available immediately when @@ -151,9 +142,11 @@ static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] = static void hvc_console_print(struct console *co, const char *b, unsigned count) { - char c[N_OUTBUF] __ALIGNED__; + char *c; unsigned i = 0, n = 0; int r, donecr = 0, index = co->index; + unsigned long flags; + struct hvc_struct *hp; /* Console access attempt outside of acceptable console range. */ if (index >= MAX_NR_HVC_CONSOLES) @@ -163,6 +156,13 @@ static void hvc_console_print(struct console *co, const char *b, if (vtermnos[index] == -1) return; + hp = cons_hvcs[index]; + if (!hp || !hp->c) + return; + + c = hp->c; + + spin_lock_irqsave(&hp->c_lock, flags); while (count > 0 || i > 0) { if (count > 0 && i < sizeof(c)) { if (b[n] == '\n' && !donecr) { @@ -191,6 +191,7 @@ static void hvc_console_print(struct console *co, const char *b, } } } + spin_unlock_irqrestore(&hp->c_lock, flags); hvc_console_flush(cons_ops[index], vtermnos[index]); } @@ -878,9 +879,16 @@ static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch) struct tty_struct *tty = driver->ttys[0]; struct hvc_struct *hp = tty->driver_data; int n; + unsigned long flags; + + if (!hp || !hp->c) + return; do { - n = hp->ops->put_chars(hp->vtermno, &ch, 1); + spin_lock_irqsave(&hp->c_lock, flags); + hp->c[0] = ch; + n = hp->ops->put_chars(hp->vtermno, hp->c, 1); + spin_unlock_irqrestore(&hp->c_lock, flags); } while (n <= 0); } #endif @@ -922,8 +930,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data, return ERR_PTR(err); } - hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size, - GFP_KERNEL); + hp = kzalloc(struct_size(hp, outbuf, outbuf_size), GFP_KERNEL); if (!hp) return ERR_PTR(-ENOMEM); @@ -931,13 +938,13 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data, hp->data = data; hp->ops = ops; hp->outbuf_size = outbuf_size; - hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))]; tty_port_init(&hp->port); hp->port.ops = &hvc_port_ops; INIT_WORK(&hp->tty_resize, hvc_set_winsz); spin_lock_init(&hp->lock); + spin_lock_init(&hp->c_lock); mutex_lock(&hvc_structs_mutex); /* @@ -964,6 +971,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data, if (i < MAX_NR_HVC_CONSOLES) { cons_ops[i] = ops; vtermnos[i] = vtermno; + cons_hvcs[i] = hp; } list_add_tail(&(hp->next), &hvc_structs); @@ -988,6 +996,7 @@ int hvc_remove(struct hvc_struct *hp) if (hp->index < MAX_NR_HVC_CONSOLES) { vtermnos[hp->index] = -1; cons_ops[hp->index] = NULL; + cons_hvcs[hp->index] = NULL; } /* Don't whack hp->irq because tty_hangup() will need to free the irq. */ diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h index 18d005814..97a5f1e0f 100644 --- a/drivers/tty/hvc/hvc_console.h +++ b/drivers/tty/hvc/hvc_console.h @@ -32,13 +32,21 @@ */ #define HVC_ALLOC_TTY_ADAPTERS 8 +/* + * These sizes are most efficient for vio, because they are the + * native transfer size. We could make them selectable in the + * future to better deal with backends that want other buffer sizes. + */ +#define N_OUTBUF 16 +#define N_INBUF 16 + +#define __ALIGNED__ __attribute__((__aligned__(L1_CACHE_BYTES))) + 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; + spinlock_t c_lock; + char c[N_OUTBUF] __ALIGNED__; + int outbuf_size; + char outbuf[0] __ALIGNED__; }; /* implemented by a low level driver */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() 2021-08-17 13:22 ` [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian @ 2021-08-17 15:48 ` Greg KH 2021-08-18 1:49 ` Xianting TIan 2021-08-17 18:24 ` kernel test robot 2021-08-18 3:17 ` Jiri Slaby 2 siblings, 1 reply; 9+ messages in thread From: Greg KH @ 2021-08-17 15:48 UTC (permalink / raw) To: Xianting Tian Cc: jirislaby, amit, arnd, osandov, linuxppc-dev, virtualization, linux-kernel On Tue, Aug 17, 2021 at 09:22:59PM +0800, Xianting Tian wrote: > We tested the patch, it worked normally. Who is "we"? > Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com> Like I said before, I need another developer from your company to review and sign-off on this patch (and the other one), before I am willing to look at it, based on the previous mistakes that have happened here. thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() 2021-08-17 15:48 ` Greg KH @ 2021-08-18 1:49 ` Xianting TIan 0 siblings, 0 replies; 9+ messages in thread From: Xianting TIan @ 2021-08-18 1:49 UTC (permalink / raw) To: Greg KH Cc: jirislaby, amit, arnd, osandov, linuxppc-dev, virtualization, linux-kernel 在 2021/8/17 下午11:48, Greg KH 写道: > On Tue, Aug 17, 2021 at 09:22:59PM +0800, Xianting Tian wrote: >> We tested the patch, it worked normally. > Who is "we"? > >> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com> > Like I said before, I need another developer from your company to review > and sign-off on this patch (and the other one), before I am willing to > look at it, based on the previous mistakes that have happened here. thanks, I will add the developer in v8 and also with fix a build warning, which I don't meet in my build process. > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() 2021-08-17 13:22 ` [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian 2021-08-17 15:48 ` Greg KH @ 2021-08-17 18:24 ` kernel test robot 2021-08-18 3:17 ` Jiri Slaby 2 siblings, 0 replies; 9+ messages in thread From: kernel test robot @ 2021-08-17 18:24 UTC (permalink / raw) To: Xianting Tian, gregkh, jirislaby, amit, arnd, osandov Cc: clang-built-linux, kbuild-all, linuxppc-dev, virtualization, linux-kernel, Xianting Tian [-- Attachment #1: Type: text/plain, Size: 4844 bytes --] Hi Xianting, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on tty/tty-testing] [also build test WARNING on char-misc/char-misc-testing soc/for-next v5.14-rc6 next-20210817] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Xianting-Tian/make-hvc-pass-dma-capable-memory-to-its-backend/20210817-212556 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing config: i386-randconfig-r021-20210816 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 2c6448cdc2f68f8c28fd0bd9404182b81306e6e6) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/f12c3bee9f2413ed7643d858b40ce2337329fdae git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Xianting-Tian/make-hvc-pass-dma-capable-memory-to-its-backend/20210817-212556 git checkout f12c3bee9f2413ed7643d858b40ce2337329fdae # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): clang-14: warning: optimization flag '-falign-jumps=0' is not supported [-Wignored-optimization-argument] In file included from drivers/tty/hvc/hvc_console.c:15: In file included from include/linux/kbd_kern.h:5: In file included from include/linux/tty.h:5: In file included from include/linux/fs.h:6: In file included from include/linux/wait_bit.h:8: In file included from include/linux/wait.h:9: In file included from include/linux/spinlock.h:51: In file included from include/linux/preempt.h:78: In file included from arch/x86/include/asm/preempt.h:7: In file included from include/linux/thread_info.h:60: arch/x86/include/asm/thread_info.h:172:13: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe [-Wframe-address] oldframe = __builtin_frame_address(1); ^~~~~~~~~~~~~~~~~~~~~~~~~~ arch/x86/include/asm/thread_info.h:174:11: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe [-Wframe-address] frame = __builtin_frame_address(2); ^~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/tty/hvc/hvc_console.c:160:18: warning: address of array 'hp->c' will always evaluate to 'true' [-Wpointer-bool-conversion] if (!hp || !hp->c) ~~~~~^ 3 warnings generated. vim +160 drivers/tty/hvc/hvc_console.c 136 137 /* 138 * Console APIs, NOT TTY. These APIs are available immediately when 139 * hvc_console_setup() finds adapters. 140 */ 141 142 static void hvc_console_print(struct console *co, const char *b, 143 unsigned count) 144 { 145 char *c; 146 unsigned i = 0, n = 0; 147 int r, donecr = 0, index = co->index; 148 unsigned long flags; 149 struct hvc_struct *hp; 150 151 /* Console access attempt outside of acceptable console range. */ 152 if (index >= MAX_NR_HVC_CONSOLES) 153 return; 154 155 /* This console adapter was removed so it is not usable. */ 156 if (vtermnos[index] == -1) 157 return; 158 159 hp = cons_hvcs[index]; > 160 if (!hp || !hp->c) 161 return; 162 163 c = hp->c; 164 165 spin_lock_irqsave(&hp->c_lock, flags); 166 while (count > 0 || i > 0) { 167 if (count > 0 && i < sizeof(c)) { 168 if (b[n] == '\n' && !donecr) { 169 c[i++] = '\r'; 170 donecr = 1; 171 } else { 172 c[i++] = b[n++]; 173 donecr = 0; 174 --count; 175 } 176 } else { 177 r = cons_ops[index]->put_chars(vtermnos[index], c, i); 178 if (r <= 0) { 179 /* throw away characters on error 180 * but spin in case of -EAGAIN */ 181 if (r != -EAGAIN) { 182 i = 0; 183 } else { 184 hvc_console_flush(cons_ops[index], 185 vtermnos[index]); 186 } 187 } else if (r > 0) { 188 i -= r; 189 if (i > 0) 190 memmove(c, c+r, i); 191 } 192 } 193 } 194 spin_unlock_irqrestore(&hp->c_lock, flags); 195 hvc_console_flush(cons_ops[index], vtermnos[index]); 196 } 197 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 36668 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() 2021-08-17 13:22 ` [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian 2021-08-17 15:48 ` Greg KH 2021-08-17 18:24 ` kernel test robot @ 2021-08-18 3:17 ` Jiri Slaby 2021-08-18 3:35 ` Xianting TIan 2021-08-19 3:00 ` Xianting TIan 2 siblings, 2 replies; 9+ messages in thread From: Jiri Slaby @ 2021-08-18 3:17 UTC (permalink / raw) To: Xianting Tian, gregkh, amit, arnd, osandov Cc: linuxppc-dev, virtualization, linux-kernel Hi, On 17. 08. 21, 15:22, Xianting Tian wrote: > As well known, hvc backend can register its opertions to hvc backend. > the opertions contain put_chars(), get_chars() and so on. "operations". And there too: > Some hvc backend may do dma in its opertions. 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): > 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 furture. > > We make 'char c[N_OUTBUF]' part of 'struct hvc_struct', so hp->c is no > longer the stack memory. we can use it in above two cases. In fact, you need only a single char for the poll case (hvc_poll_put_char), so hvc_struct needs to contain only c, not an array. OTOH, you need c[N_OUTBUF] in the console case (hvc_console_print), but not whole hvc_struct. So cons_hvcs should be an array of structs composed of only the lock and the buffer. Hum. Or maybe rethink and take care of the console case by kmemdup and be done with that case? What problem do you have with allocating 16 bytes? It should be quite easy and really fast (lockless) in most cases. On the contrary, your solution has to take a spinlock to access the global buffer. > Other fix is use L1_CACHE_BYTES as the alignment, use 'sizeof(long)' as > dma alignment is wrong. And use struct_size() to calculate size of > hvc_struct. This ought to be in separate patches. thanks, -- js suse labs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() 2021-08-18 3:17 ` Jiri Slaby @ 2021-08-18 3:35 ` Xianting TIan 2021-08-19 3:00 ` Xianting TIan 1 sibling, 0 replies; 9+ messages in thread From: Xianting TIan @ 2021-08-18 3:35 UTC (permalink / raw) To: Jiri Slaby, gregkh, amit, arnd, osandov Cc: linuxppc-dev, virtualization, linux-kernel 在 2021/8/18 上午11:17, Jiri Slaby 写道: > Hi, > > On 17. 08. 21, 15:22, Xianting Tian wrote: >> As well known, hvc backend can register its opertions to hvc backend. >> the opertions contain put_chars(), get_chars() and so on. > > "operations". And there too: > >> Some hvc backend may do dma in its opertions. 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): >> 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 furture. >> >> We make 'char c[N_OUTBUF]' part of 'struct hvc_struct', so hp->c is no >> longer the stack memory. we can use it in above two cases. > > In fact, you need only a single char for the poll case > (hvc_poll_put_char), so hvc_struct needs to contain only c, not an array. > > OTOH, you need c[N_OUTBUF] in the console case (hvc_console_print), > but not whole hvc_struct. So cons_hvcs should be an array of structs > composed of only the lock and the buffer. > > Hum. > > Or maybe rethink and take care of the console case by kmemdup and be > done with that case? What problem do you have with allocating 16 > bytes? It should be quite easy and really fast (lockless) in most > cases. On the contrary, your solution has to take a spinlock to access > the global buffer. May be we can change hvc_struct as below, struct hvc_struct { ... char out_ch; char c[N_OUTBUF] __ALIGNED__; int outbuf_size; char outbuf[0] __ALIGNED__; }; c[N_OUTBUF] is only used for hvc_console_print(); out_ch is only used for hvc_poll_put_char(). Thus no competition exits, the spinlock can be removed. Then cons_hvcs array can only contains the buffer. Is it OK for you? thanks > >> Other fix is use L1_CACHE_BYTES as the alignment, use 'sizeof(long)' as >> dma alignment is wrong. And use struct_size() to calculate size of >> hvc_struct. > > This ought to be in separate patches. OK, thanks > > thanks, ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() 2021-08-18 3:17 ` Jiri Slaby 2021-08-18 3:35 ` Xianting TIan @ 2021-08-19 3:00 ` Xianting TIan 1 sibling, 0 replies; 9+ messages in thread From: Xianting TIan @ 2021-08-19 3:00 UTC (permalink / raw) To: Jiri Slaby, gregkh, amit, arnd, osandov Cc: linuxppc-dev, virtualization, linux-kernel 在 2021/8/18 上午11:17, Jiri Slaby 写道: > Hi, > > On 17. 08. 21, 15:22, Xianting Tian wrote: >> As well known, hvc backend can register its opertions to hvc backend. >> the opertions contain put_chars(), get_chars() and so on. > > "operations". And there too: > >> Some hvc backend may do dma in its opertions. 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): >> 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 furture. >> >> We make 'char c[N_OUTBUF]' part of 'struct hvc_struct', so hp->c is no >> longer the stack memory. we can use it in above two cases. > > In fact, you need only a single char for the poll case > (hvc_poll_put_char), so hvc_struct needs to contain only c, not an array. > > OTOH, you need c[N_OUTBUF] in the console case (hvc_console_print), > but not whole hvc_struct. So cons_hvcs should be an array of structs > composed of only the lock and the buffer. > > Hum. > > Or maybe rethink and take care of the console case by kmemdup and be > done with that case? What problem do you have with allocating 16 > bytes? It should be quite easy and really fast (lockless) in most > cases. On the contrary, your solution has to take a spinlock to access > the global buffer. As I replyed before, this issue may can be solved just by adjust the alignment to L1_CACHE_BYTES or at least 16: #define __ALIGNED__ __attribute__((__aligned__(L1_CACHE_BYTES))) Then, c[16] won't cross the pages, that is to say c[16]'s physical address is continuous. Could you comment this? I submitted v8, I found it still can't solve ths issue, even we create 'char out_buf[N_OUTBUF]' and 'chat out_ch' be part of 'struct hvc_struct', and use it separately, we still need lock to protect each buf. When we invloced lock, it will impact the hvc performance. So we can back to the original intention of this solution, just fix the kmemdup issue in virtio_console driver? > >> Other fix is use L1_CACHE_BYTES as the alignment, use 'sizeof(long)' as >> dma alignment is wrong. And use struct_size() to calculate size of >> hvc_struct. > > This ought to be in separate patches. > > thanks, ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v7 2/2] virtio-console: remove unnecessary kmemdup() 2021-08-17 13:22 [PATCH v7 0/2] make hvc pass dma capable memory to its backend Xianting Tian 2021-08-17 13:22 ` [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian @ 2021-08-17 13:23 ` Xianting Tian 1 sibling, 0 replies; 9+ messages in thread From: Xianting Tian @ 2021-08-17 13:23 UTC (permalink / raw) To: gregkh, jirislaby, amit, arnd, osandov Cc: linuxppc-dev, virtualization, linux-kernel, Xianting Tian hvc framework will never pass stack memory to the put_chars() function, So the calling of kmemdup() is unnecessary, we can remove it. This revert commit c4baad5029 ("virtio-console: avoid DMA from stack") Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com> --- drivers/char/virtio_console.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 7eaf303a7..4ed3ffb1d 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -1117,8 +1117,6 @@ static int put_chars(u32 vtermno, const char *buf, int count) { struct port *port; struct scatterlist sg[1]; - void *data; - int ret; if (unlikely(early_put_chars)) return early_put_chars(vtermno, buf, count); @@ -1127,14 +1125,8 @@ static int put_chars(u32 vtermno, const char *buf, int count) if (!port) return -EPIPE; - data = kmemdup(buf, count, GFP_ATOMIC); - if (!data) - return -ENOMEM; - - sg_init_one(sg, data, count); - ret = __send_to_port(port, sg, 1, count, data, false); - kfree(data); - return ret; + sg_init_one(sg, buf, count); + return __send_to_port(port, sg, 1, count, (void *)buf, false); } /* -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-19 3:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-17 13:22 [PATCH v7 0/2] make hvc pass dma capable memory to its backend Xianting Tian 2021-08-17 13:22 ` [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian 2021-08-17 15:48 ` Greg KH 2021-08-18 1:49 ` Xianting TIan 2021-08-17 18:24 ` kernel test robot 2021-08-18 3:17 ` Jiri Slaby 2021-08-18 3:35 ` Xianting TIan 2021-08-19 3:00 ` Xianting TIan 2021-08-17 13:23 ` [PATCH v7 2/2] virtio-console: remove unnecessary kmemdup() Xianting Tian
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).