| Message ID | 20210812094532.145497-2-xianting.tian@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | make hvc pass dma capable memory to its backend | expand |
| Related | show |
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-rc5 next-20210812]
[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/20210812-174847
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: hexagon-randconfig-r041-20210812 (attached as .config)
compiler: clang version 12.0.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/9f2925b5429149ceb0ea6eeaa8c81d422c3124fc
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/20210812-174847
git checkout 9f2925b5429149ceb0ea6eeaa8c81d422c3124fc
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=hexagon
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 >>):
>> drivers/tty/hvc/hvc_console.c:190:26: warning: variable 'hp' is uninitialized when used here [-Wuninitialized]
spin_unlock_irqrestore(&hp->c_lock, flags);
^~
drivers/tty/hvc/hvc_console.c:149:23: note: initialize the variable 'hp' to silence this warning
struct hvc_struct *hp;
^
= NULL
1 warning generated.
vim +/hp +190 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 || !cons_outbuf[index])
157 return;
158
159 c = cons_outbuf[index];
160
161 spin_lock_irqsave(&hp->c_lock, flags);
162 while (count > 0 || i > 0) {
163 if (count > 0 && i < sizeof(c)) {
164 if (b[n] == '\n' && !donecr) {
165 c[i++] = '\r';
166 donecr = 1;
167 } else {
168 c[i++] = b[n++];
169 donecr = 0;
170 --count;
171 }
172 } else {
173 r = cons_ops[index]->put_chars(vtermnos[index], c, i);
174 if (r <= 0) {
175 /* throw away characters on error
176 * but spin in case of -EAGAIN */
177 if (r != -EAGAIN) {
178 i = 0;
179 } else {
180 hvc_console_flush(cons_ops[index],
181 vtermnos[index]);
182 }
183 } else if (r > 0) {
184 i -= r;
185 if (i > 0)
186 memmove(c, c+r, i);
187 }
188 }
189 }
> 190 spin_unlock_irqrestore(&hp->c_lock, flags);
191 hvc_console_flush(cons_ops[index], vtermnos[index]);
192 }
193
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi, On 12. 08. 21, 14:26, kernel test robot wrote: >>> drivers/tty/hvc/hvc_console.c:190:26: warning: variable 'hp' is uninitialized when used here [-Wuninitialized] > spin_unlock_irqrestore(&hp->c_lock, flags); > ^~ > drivers/tty/hvc/hvc_console.c:149:23: note: initialize the variable 'hp' to silence this warning > struct hvc_struct *hp; > ^ > = NULL So you clearly didn't test your change as it would crash quite instantly. I wonder, where do you intend to get hp from in the console::print() hook? thanks,
在 2021/8/13 下午1:53, Jiri Slaby 写道: > Hi, > > On 12. 08. 21, 14:26, kernel test robot wrote: >>>> drivers/tty/hvc/hvc_console.c:190:26: warning: variable 'hp' is >>>> uninitialized when used here [-Wuninitialized] >> spin_unlock_irqrestore(&hp->c_lock, flags); >> ^~ >> drivers/tty/hvc/hvc_console.c:149:23: note: initialize the >> variable 'hp' to silence this warning >> struct hvc_struct *hp; >> ^ >> = NULL > > So you clearly didn't test your change as it would crash quite > instantly. I wonder, where do you intend to get hp from in the > console::print() hook? I am very sorry for the inconvenience caused. This is caused by my carelessness:( I take it for granted that there is no problem when I just switch to use array(cons_outbuf[]). sorry agin. > > thanks,
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, greg k-h
在 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
在 2021/8/13 下午1:53, Jiri Slaby 写道: > Hi, > > On 12. 08. 21, 14:26, kernel test robot wrote: >>>> drivers/tty/hvc/hvc_console.c:190:26: warning: variable 'hp' is >>>> uninitialized when used here [-Wuninitialized] >> spin_unlock_irqrestore(&hp->c_lock, flags); >> ^~ >> drivers/tty/hvc/hvc_console.c:149:23: note: initialize the >> variable 'hp' to silence this warning >> struct hvc_struct *hp; >> ^ >> = NULL > > So you clearly didn't test your change as it would crash quite > instantly. I wonder, where do you intend to get hp from in the > console::print() hook? > > thanks, According to analysis, this issue may can be solved just by adjust the alignment to L1_CACHE_BYTES: #define __ALIGNED__ __attribute__((__aligned__(L1_CACHE_BYTES))) Our analysis as below, the original __ALIGNED__ is sizeof(long) which is 8 for 64bit cpu. char c[N_OUTBUF] __ALIGNED__; //c[16] __ALIGNED__; For 4K page, c[16] may cross the page when alignemnt is 8. In the case the physical address of c[16] is noncontiguous. |------8----|..........|-----8-----| PAGE_1 ..........................|-----------16----------| c[16] .........................................|-----8-----|.............|-----8-----| PAGE_2 But when the alignment is L1_CACHE_BYTES(eg, 64), or at least N_OUTBUF(16), we have no dma issue as c[16] won't cross the page, the physical address of c[16] is contiguous. |--------64--------|.........|--------64---------| PAGE_3 ..................................|--c[16]--| Could you help comments this? thanks
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index 5bb8c4e44..c56564eb7 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 char *cons_outbuf[MAX_NR_HVC_CONSOLES]; /* * Console APIs, NOT TTY. These APIs are available immediately when @@ -151,18 +142,23 @@ 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) return; /* This console adapter was removed so it is not usable. */ - if (vtermnos[index] == -1) + if (vtermnos[index] == -1 || !cons_outbuf[index]) return; + c = cons_outbuf[index]; + + 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 +187,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 +875,19 @@ 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; + char *c; + + if (!hp || !cons_outbuf[hp->index]) + return; + + c = cons_outbuf[hp->index]; do { - n = hp->ops->put_chars(hp->vtermno, &ch, 1); + spin_lock_irqsave(&hp->c_lock, flags); + c[0] = ch; + n = hp->ops->put_chars(hp->vtermno, c, 1); + spin_unlock_irqrestore(&hp->c_lock, flags); } while (n <= 0); } #endif @@ -922,8 +929,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 +937,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 +970,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_outbuf[i] = hp->c; } list_add_tail(&(hp->next), &hvc_structs); @@ -988,6 +995,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_outbuf[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..52374e2da 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 */
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> --- drivers/tty/hvc/hvc_console.c | 40 +++++++++++++++++++++-------------- drivers/tty/hvc/hvc_console.h | 16 ++++++++++++-- 2 files changed, 38 insertions(+), 18 deletions(-)