ERST: how to avoid a dynamic memory allocation in panic case

ERST: how to avoid a dynamic memory allocation in panic case

[Seiji Aguchi]

[Issue]

Current erst driver is kicked in panic case.
On the other hand, a dynamic memory allocation seems to run in it
with a following code path.

erst_writer -> erst_write -> __erst_write_to_storage -> many of apei_exec_run -> 
ctx->ins_table[entry->instruction].run()

which are functions defined in erst_ins_type table i.e:

apei_exec_read_register -> apei_read -> acpi_os_read_memory64 -> acpi_os_ioremap -> ioremap_cache

which possibly involve IOMMU allocator.

I may cause a failure of  an erst driver if the panic happens in an interrupt context.

[Idea]

If we can remove ioremap_cache() from acpi_os_read_memory() as follows,
It is easy to fix this issue.
But I'm not sure if it is feasible because I can't see the reason of tying acpi_os_ioremap()  from a git log...

Any comment?

<snip>
@@ -918,10 +918,7 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u64 *value, u32 width)
 	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
 	if (!virt_addr) {
 		rcu_read_unlock();
-		virt_addr = acpi_os_ioremap(phys_addr, size);
-		if (!virt_addr)
-			return AE_BAD_ADDRESS;
-		unmap = true;
+		return AE_BAD_ADDRESS;
 	}
 
 	if (!value)
<snip>

Seiji

Re: ERST: how to avoid a dynamic memory allocation in panic case

[Huang Ying]

On Tue, 2013-01-22 at 23:30 +0000, Seiji Aguchi wrote:
> [Issue]
> 
> Current erst driver is kicked in panic case.
> On the other hand, a dynamic memory allocation seems to run in it
> with a following code path.
> 
> erst_writer -> erst_write -> __erst_write_to_storage -> many of apei_exec_run -> 
> ctx->ins_table[entry->instruction].run()
> 
> which are functions defined in erst_ins_type table i.e:
> 
> apei_exec_read_register -> apei_read -> acpi_os_read_memory64 -> acpi_os_ioremap -> ioremap_cache
> 
> which possibly involve IOMMU allocator.
> 
> I may cause a failure of  an erst driver if the panic happens in an interrupt context.
> 
> [Idea]
> 
> If we can remove ioremap_cache() from acpi_os_read_memory() as follows,
> It is easy to fix this issue.
> But I'm not sure if it is feasible because I can't see the reason of tying acpi_os_ioremap()  from a git log...
> 
> Any comment?
> 
> <snip>
> @@ -918,10 +918,7 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u64 *value, u32 width)
>  	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
>  	if (!virt_addr) {
>  		rcu_read_unlock();
> -		virt_addr = acpi_os_ioremap(phys_addr, size);
> -		if (!virt_addr)
> -			return AE_BAD_ADDRESS;
> -		unmap = true;
> +		return AE_BAD_ADDRESS;

No.  We can not do that.  Because some users rely on acpi_os_read_memory
to do ioremap for them.

The correct fixing should be pre-map the io-memory that may be accessed
in erst code patch with acpi_map().

Best Regards,
Huang Ying

>  	}
>  
>  	if (!value)
> <snip>
> 
> Seiji

RE: ERST: how to avoid a dynamic memory allocation in panic case

[Seiji Aguchi]

> > <snip>
> > @@ -918,10 +918,7 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u64 *value, u32 width)
> >  	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
> >  	if (!virt_addr) {
> >  		rcu_read_unlock();
> > -		virt_addr = acpi_os_ioremap(phys_addr, size);
> > -		if (!virt_addr)
> > -			return AE_BAD_ADDRESS;
> > -		unmap = true;
> > +		return AE_BAD_ADDRESS;
> 
> No.  We can not do that.  Because some users rely on acpi_os_read_memory to do ioremap for them.

Thank you for giving me the information.

> The correct fixing should be pre-map the io-memory that may be accessed in erst code patch with acpi_map().

I will take a look at the code.

Seiji

RE: ERST: how to avoid a dynamic memory allocation in panic case

[Seiji Aguchi]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 835 bytes --]



> > <snip>
> > @@ -918,10 +918,7 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u64 *value, u32 width)
> >  	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
> >  	if (!virt_addr) {
> >  		rcu_read_unlock();
> > -		virt_addr = acpi_os_ioremap(phys_addr, size);
> > -		if (!virt_addr)
> > -			return AE_BAD_ADDRESS;
> > -		unmap = true;
> > +		return AE_BAD_ADDRESS;
> 
> No.  We can not do that.  Because some users rely on acpi_os_read_memory to do ioremap for them.

Thank you for giving me the information.

> The correct fixing should be pre-map the io-memory that may be accessed in erst code patch with acpi_map().

I will take a look at the code.

Seiji
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥