[PATCH 1/2] tty: hvc: pass DMA capable memory to put_chars()

[PATCH 1/2] tty: hvc: pass DMA capable memory to put_chars()

[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 smae issue if
a new hvc backend using dma added in the furture.

Considering lock competition of hp->outbuf and the complicated logic in
hvc_console_print(), I didn’t use hp->outbuf, just allocate additional
memory(length N_OUTBUF) and append it to hp->outbuf.
For the issue in hvc_poll_put_char(), I use a static char to replace
the char in stack.

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>
---
 drivers/tty/hvc/hvc_console.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 5bb8c4e44..f7a7fa083 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -151,9 +151,10 @@ 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;
+	struct hvc_struct *hp;
 
 	/* Console access attempt outside of acceptable console range. */
 	if (index >= MAX_NR_HVC_CONSOLES)
@@ -163,8 +164,14 @@ static void hvc_console_print(struct console *co, const char *b,
 	if (vtermnos[index] == -1)
 		return;
 
+	list_for_each_entry(hp, &hvc_structs, next)
+		if (hp->vtermno == vtermnos[index])
+			break;
+
+	c = &hp->outbuf[hp->outbuf_size];
+
 	while (count > 0 || i > 0) {
-		if (count > 0 && i < sizeof(c)) {
+		if (count > 0 && i < N_OUTBUF) {
 			if (b[n] == '\n' && !donecr) {
 				c[i++] = '\r';
 				donecr = 1;
@@ -878,6 +885,7 @@ 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;
+	static char ch = ch;
 
 	do {
 		n = hp->ops->put_chars(hp->vtermno, &ch, 1);
@@ -922,7 +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,
+	hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size + N_OUTBUF,
 			GFP_KERNEL);
 	if (!hp)
 		return ERR_PTR(-ENOMEM);
-- 
2.17.1

Re: [PATCH 1/2] tty: hvc: pass DMA capable memory to put_chars()

[Arnd Bergmann]

On Sun, Aug 1, 2021 at 7:16 AM Xianting Tian
<xianting.tian@linux.alibaba.com> wrote:

> Considering lock competition of hp->outbuf and the complicated logic in
> hvc_console_print(), I didn’t use hp->outbuf, just allocate additional
> memory(length N_OUTBUF) and append it to hp->outbuf.
> For the issue in hvc_poll_put_char(), I use a static char to replace
> the char in stack.

While this may work, it sounds rather obscure to me, I don't think
it's a good idea
to append the buffer at the back.

If you need a separate field besides hp->outbuf, I would make that part of the
structure itself, and give it the correct alignment constraints to ensure it is
in a cache line by itself. The size of this field is a compile-time
constant, so I
don't see a need to play tricks with pointer arithmetic.

I'm not sure about the locking either: Is it possible for two CPUs to enter
hvc_console_print() at the same time, or is there locking at a higher level
already? It would be good to document this in the structure definition next
to the field.

> @@ -878,6 +885,7 @@ 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;
> +       static char ch = ch;
>
>         do {
>                 n = hp->ops->put_chars(hp->vtermno, &ch, 1);

This does not compile, and it's neither thread-safe nor dma safe if you get it
to build by renaming the variable.

        Arnd

Re: [PATCH 1/2] tty: hvc: pass DMA capable memory to put_chars()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]

Hi Xianting,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on char-misc/char-misc-testing soc/for-next linus/master v5.14-rc3 next-20210730]
[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/tty-hvc-pass-DMA-capable-memory-to-put_chars/20210801-131800
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 10.3.0
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/b33d1f04db78e1bfa5d798b676bd1062e2d54afc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Xianting-Tian/tty-hvc-pass-DMA-capable-memory-to-put_chars/20210801-131800
        git checkout b33d1f04db78e1bfa5d798b676bd1062e2d54afc
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=sparc64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/tty/hvc/hvc_console.c: In function 'hvc_poll_put_char':
>> drivers/tty/hvc/hvc_console.c:888:14: error: 'ch' redeclared as different kind of symbol
     888 |  static char ch = ch;
         |              ^~
   drivers/tty/hvc/hvc_console.c:883:73: note: previous definition of 'ch' was here
     883 | static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
         |                                                                    ~~~~~^~
>> drivers/tty/hvc/hvc_console.c:888:19: error: initializer element is not constant
     888 |  static char ch = ch;
         |                   ^~


vim +/ch +888 drivers/tty/hvc/hvc_console.c

   882	
   883	static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
   884	{
   885		struct tty_struct *tty = driver->ttys[0];
   886		struct hvc_struct *hp = tty->driver_data;
   887		int n;
 > 888		static char ch = ch;
   889	
   890		do {
   891			n = hp->ops->put_chars(hp->vtermno, &ch, 1);
   892		} while (n <= 0);
   893	}
   894	#endif
   895	

---
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: 69625 bytes --]