From: Xianting Tian <xianting.tian@linux.alibaba.com> To: gregkh@linuxfoundation.org, jirislaby@kernel.org, amit@kernel.org, arnd@arndb.de, osandov@fb.com Cc: shile.zhang@linux.alibaba.com, linuxppc-dev@lists.ozlabs.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Xianting Tian <xianting.tian@linux.alibaba.com> Subject: [PATCH v8 2/3] tty: hvc: pass DMA capable memory to put_chars() Date: Wed, 18 Aug 2021 16:21:21 +0800 [thread overview] Message-ID: <20210818082122.166881-3-xianting.tian@linux.alibaba.com> (raw) In-Reply-To: <20210818082122.166881-1-xianting.tian@linux.alibaba.com> 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): 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> --- drivers/tty/hvc/hvc_console.c | 27 ++++++++++++--------------- drivers/tty/hvc/hvc_console.h | 16 ++++++++++++++-- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index 5bb8c4e44..300e9c037 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__(L1_CACHE_BYTES))) - 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 char *cons_outbufs[MAX_NR_HVC_CONSOLES]; /* * Console APIs, NOT TTY. These APIs are available immediately when @@ -151,7 +142,7 @@ 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; @@ -163,6 +154,10 @@ static void hvc_console_print(struct console *co, const char *b, if (vtermnos[index] == -1) return; + c = cons_outbufs[index]; + if (!c) + return; + while (count > 0 || i > 0) { if (count > 0 && i < sizeof(c)) { if (b[n] == '\n' && !donecr) { @@ -879,8 +874,10 @@ static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch) struct hvc_struct *hp = tty->driver_data; int n; + hp->out_ch = ch; + do { - n = hp->ops->put_chars(hp->vtermno, &ch, 1); + n = hp->ops->put_chars(hp->vtermno, hp->out_ch, 1); } while (n <= 0); } #endif @@ -922,8 +919,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,7 +927,6 @@ 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; @@ -964,6 +959,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_outbufs[i] = hp->out_buf; } list_add_tail(&(hp->next), &hvc_structs); @@ -988,6 +984,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_outbufs[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..b94576d55 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; + char out_ch; + char out_buf[N_OUTBUF] __ALIGNED__; + int outbuf_size; + char outbuf[0] __ALIGNED__; }; /* implemented by a low level driver */ -- 2.17.1
WARNING: multiple messages have this Message-ID (diff)
From: Xianting Tian <xianting.tian@linux.alibaba.com> To: gregkh@linuxfoundation.org, jirislaby@kernel.org, amit@kernel.org, arnd@arndb.de, osandov@fb.com Cc: Xianting Tian <xianting.tian@linux.alibaba.com>, shile.zhang@linux.alibaba.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: [PATCH v8 2/3] tty: hvc: pass DMA capable memory to put_chars() Date: Wed, 18 Aug 2021 16:21:21 +0800 [thread overview] Message-ID: <20210818082122.166881-3-xianting.tian@linux.alibaba.com> (raw) In-Reply-To: <20210818082122.166881-1-xianting.tian@linux.alibaba.com> 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): 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> --- drivers/tty/hvc/hvc_console.c | 27 ++++++++++++--------------- drivers/tty/hvc/hvc_console.h | 16 ++++++++++++++-- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index 5bb8c4e44..300e9c037 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__(L1_CACHE_BYTES))) - 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 char *cons_outbufs[MAX_NR_HVC_CONSOLES]; /* * Console APIs, NOT TTY. These APIs are available immediately when @@ -151,7 +142,7 @@ 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; @@ -163,6 +154,10 @@ static void hvc_console_print(struct console *co, const char *b, if (vtermnos[index] == -1) return; + c = cons_outbufs[index]; + if (!c) + return; + while (count > 0 || i > 0) { if (count > 0 && i < sizeof(c)) { if (b[n] == '\n' && !donecr) { @@ -879,8 +874,10 @@ static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch) struct hvc_struct *hp = tty->driver_data; int n; + hp->out_ch = ch; + do { - n = hp->ops->put_chars(hp->vtermno, &ch, 1); + n = hp->ops->put_chars(hp->vtermno, hp->out_ch, 1); } while (n <= 0); } #endif @@ -922,8 +919,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,7 +927,6 @@ 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; @@ -964,6 +959,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_outbufs[i] = hp->out_buf; } list_add_tail(&(hp->next), &hvc_structs); @@ -988,6 +984,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_outbufs[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..b94576d55 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; + char out_ch; + char out_buf[N_OUTBUF] __ALIGNED__; + int outbuf_size; + char outbuf[0] __ALIGNED__; }; /* implemented by a low level driver */ -- 2.17.1
next prev parent reply other threads:[~2021-08-18 8:21 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-18 8:21 [PATCH v8 0/3] make hvc pass dma capable memory to its backend Xianting Tian 2021-08-18 8:21 ` Xianting Tian 2021-08-18 8:21 ` [PATCH v8 1/3] tty: hvc: use correct dma alignment size Xianting Tian 2021-08-18 8:21 ` Xianting Tian 2021-08-18 8:21 ` Xianting Tian [this message] 2021-08-18 8:21 ` [PATCH v8 2/3] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian 2021-08-18 11:03 ` kernel test robot 2021-08-18 11:03 ` kernel test robot 2021-08-18 11:03 ` kernel test robot 2021-08-18 11:03 ` kernel test robot 2021-08-18 17:41 ` kernel test robot 2021-08-18 17:41 ` kernel test robot 2021-08-18 17:41 ` kernel test robot 2021-08-18 17:41 ` kernel test robot 2021-08-20 6:49 ` Daniel Axtens 2021-08-20 8:43 ` Xianting TIan 2021-09-18 12:32 ` Xianting Tian 2021-09-18 12:40 ` Greg KH 2021-09-18 12:40 ` Greg KH 2021-09-18 12:40 ` Greg KH 2021-09-18 12:47 ` Xianting Tian 2021-09-18 12:47 ` Xianting Tian 2021-08-20 12:34 ` Michael Ellerman 2021-08-20 12:34 ` Michael Ellerman 2021-08-18 8:21 ` [PATCH v8 3/3] virtio-console: remove unnecessary kmemdup() Xianting Tian 2021-08-18 8:21 ` 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=20210818082122.166881-3-xianting.tian@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=shile.zhang@linux.alibaba.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.