From: Xianting TIan <xianting.tian@linux.alibaba.com> To: Greg KH <gregkh@linuxfoundation.org> Cc: jirislaby@kernel.org, amit@kernel.org, arnd@arndb.de, osandov@fb.com, linuxppc-dev@lists.ozlabs.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 1/2] tty: hvc: pass DMA capable memory to put_chars() Date: Fri, 13 Aug 2021 19:27:09 +0800 [thread overview] Message-ID: <aca8a77c-f608-ef2f-5bcb-9411cec3e99d@linux.alibaba.com> (raw) In-Reply-To: <YRYeyeZ/22rR26u7@kroah.com> 在 2021/8/13 下午3:27, Greg KH 写道: > On Thu, Aug 12, 2021 at 05:45:31PM +0800, 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. >> >> 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_outbuf[]) for the hp->c pointers next to >> the cons_ops[] and vtermnos[] arrays. >> >> With the patch, we can remove the fix c4baad5029. >> >> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com> >> Tested-by: Xianting Tian <xianting.tian@linux.alibaba.com> > As the build shows, you obviously did not test this code :( > > Also, no need to add a tested-by line as that should be implicit if you > wrote and signed off on it. > > I am going to ask you to get some help from some other developers at > your company, and get them to test and sign off on this series before > sending it out again, as there seems to be a bit of a disconnect as to > what is actually needed to do when sending a patch for us to review. > > That is now a requirement for us to be able to take your changes here. > > thanks, Sorry for this. I tested V1-V4, But for V6, I take it for granted that there is no problem when I just switch to use array(cons_outbuf[]). I indeed didn't test it:( I will test it and find virtualization test expert to test again before sending next patch. > > greg k-h
WARNING: multiple messages have this Message-ID (diff)
From: Xianting TIan <xianting.tian@linux.alibaba.com> To: Greg KH <gregkh@linuxfoundation.org> Cc: arnd@arndb.de, amit@kernel.org, jirislaby@kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, linuxppc-dev@lists.ozlabs.org, osandov@fb.com Subject: Re: [PATCH v6 1/2] tty: hvc: pass DMA capable memory to put_chars() Date: Fri, 13 Aug 2021 19:27:09 +0800 [thread overview] Message-ID: <aca8a77c-f608-ef2f-5bcb-9411cec3e99d@linux.alibaba.com> (raw) In-Reply-To: <YRYeyeZ/22rR26u7@kroah.com> 在 2021/8/13 下午3:27, Greg KH 写道: > On Thu, Aug 12, 2021 at 05:45:31PM +0800, 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. >> >> 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_outbuf[]) for the hp->c pointers next to >> the cons_ops[] and vtermnos[] arrays. >> >> With the patch, we can remove the fix c4baad5029. >> >> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com> >> Tested-by: Xianting Tian <xianting.tian@linux.alibaba.com> > As the build shows, you obviously did not test this code :( > > Also, no need to add a tested-by line as that should be implicit if you > wrote and signed off on it. > > I am going to ask you to get some help from some other developers at > your company, and get them to test and sign off on this series before > sending it out again, as there seems to be a bit of a disconnect as to > what is actually needed to do when sending a patch for us to review. > > That is now a requirement for us to be able to take your changes here. > > thanks, Sorry for this. I tested V1-V4, But for V6, I take it for granted that there is no problem when I just switch to use array(cons_outbuf[]). I indeed didn't test it:( I will test it and find virtualization test expert to test again before sending next patch. > > greg k-h
next prev parent reply other threads:[~2021-08-13 11:27 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-12 9:45 [PATCH v6 0/2] make hvc pass dma capable memory to its backend Xianting Tian 2021-08-12 9:45 ` Xianting Tian 2021-08-12 9:45 ` [PATCH v6 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian 2021-08-12 9:45 ` Xianting Tian 2021-08-12 12:26 ` kernel test robot 2021-08-12 12:26 ` kernel test robot 2021-08-12 12:26 ` kernel test robot 2021-08-12 12:26 ` kernel test robot 2021-08-13 5:53 ` Jiri Slaby 2021-08-13 5:53 ` Jiri Slaby 2021-08-13 5:53 ` Jiri Slaby 2021-08-13 6:04 ` Xianting TIan 2021-08-13 6:04 ` Xianting TIan 2021-08-13 6:04 ` Xianting TIan 2021-08-13 16:07 ` Xianting TIan 2021-08-13 16:07 ` Xianting TIan 2021-08-13 16:07 ` Xianting TIan 2021-08-13 7:27 ` Greg KH 2021-08-13 7:27 ` Greg KH 2021-08-13 7:27 ` Greg KH 2021-08-13 11:27 ` Xianting TIan [this message] 2021-08-13 11:27 ` Xianting TIan 2021-08-12 9:45 ` [PATCH v6 2/2] virtio-console: remove unnecessary kmemdup() Xianting Tian 2021-08-12 9:45 ` Xianting Tian
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=aca8a77c-f608-ef2f-5bcb-9411cec3e99d@linux.alibaba.com \ --to=xianting.tian@linux.alibaba.com \ --cc=amit@kernel.org \ --cc=arnd@arndb.de \ --cc=gregkh@linuxfoundation.org \ --cc=jirislaby@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=osandov@fb.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.