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.
drivers/char/virtio_console.c | 12 ++----------
drivers/tty/hvc/hvc_console.c | 40 +++++++++++++++++++++--------------
drivers/tty/hvc/hvc_console.h | 16 ++++++++++++--
3 file changed
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 <redacted>
Tested-by: Xianting Tian <redacted>
---
drivers/tty/hvc/hvc_console.c | 40 +++++++++++++++++++++--------------
drivers/tty/hvc/hvc_console.h | 16 ++++++++++++--
2 files changed, 38 insertions(+), 18 deletions(-)
@@ -41,16 +41,6 @@*/#define HVC_CLOSE_WAIT (HZ/100) /* 1/10 of a second */-/*-*Thesesizesaremostefficientforvio,becausetheyarethe-*nativetransfersize.Wecouldmakethemselectableinthe-*futuretobetterdealwithbackendsthatwantotherbuffersizes.-*/-#define N_OUTBUF 16-#define N_INBUF 16--#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))-staticstructtty_driver*hvc_driver;staticstructtask_struct*hvc_task;
@@ -142,6 +132,7 @@ static int hvc_flush(struct hvc_struct *hp)staticconststructhv_ops*cons_ops[MAX_NR_HVC_CONSOLES];staticuint32_tvtermnos[MAX_NR_HVC_CONSOLES]={[0...MAX_NR_HVC_CONSOLES-1]=-1};+staticchar*cons_outbuf[MAX_NR_HVC_CONSOLES];/**ConsoleAPIs,NOTTTY.TheseAPIsareavailableimmediatelywhen
@@ -151,18 +142,23 @@ static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =staticvoidhvc_console_print(structconsole*co,constchar*b,unsignedcount){-charc[N_OUTBUF]__ALIGNED__;+char*c;unsignedi=0,n=0;intr,donecr=0,index=co->index;+unsignedlongflags;+structhvc_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){
@@ -922,8 +929,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,returnERR_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)returnERR_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. */
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 <redacted>
---
drivers/char/virtio_console.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
Hi,
On 12. 08. 21, 14:26, kernel test robot wrote:
quoted
quoted
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,
--
js
suse labs
Hi,
On 12. 08. 21, 14:26, kernel test robot wrote:
quoted
quoted
quoted
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.
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 <redacted>
Tested-by: Xianting Tian <redacted>
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
On Thu, Aug 12, 2021 at 05:45:31PM +0800, Xianting Tian wrote:
quoted
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 <redacted>
Tested-by: Xianting Tian <redacted>
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.
Hi,
On 12. 08. 21, 14:26, kernel test robot wrote:
quoted
quoted
quoted
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