* Re:Re: [PATCH] Fix incorrect return value of pre_map_gar_callback
[not found] <6b71b5b3-b423-6768-15f4-44f7aa7dc12d () arm ! com>
@ 2020-10-29 11:30 ` yaoaili126
2020-10-30 14:03 ` James Morse
0 siblings, 1 reply; 5+ messages in thread
From: yaoaili126 @ 2020-10-29 11:30 UTC (permalink / raw)
To: james.morse; +Cc: rjw, lenb, tony.luck, bp, linux-acpi, YANGFENG1, yaoaili
From: Aili Yao <yaoaili@kingsoft.com>
Hi, Thanks for your comments!
> -----Original Message-----
> From: James Morse [mailto:james.morse@arm.com]
> Sent: Thursday, October 29, 2020 3:19 AM
> To: yaoaili126@163.com
> Cc: rjw@rjwysocki.net; lenb@kernel.org; tony.luck@intel.com; bp@alien8.de;
> linux-acpi@vger.kernel.org; YANGFENG1 [杨峰]
> <YANGFENG1@kingsoft.com>; yaoaili [么爱利] <yaoaili@kingsoft.com>
> Subject: Re: [PATCH] Fix incorrect return value of pre_map_gar_callback
>
> Hi!
>
> On 26/10/2020 06:15, yaoaili126@163.com wrote:
> > From: Aili Yao <yaoaili@kingsoft.com>
> >
> > From commit 6915564dc5a8 ("ACPI: OSL: Change the type of
> > acpi_os_map_generic_address() return
> > value"),acpi_os_map_generic_address
> > will return logical address or NULL for error, but
> > pre_map_gar_callback,for ACPI_ADR_SPACE_SYSTEM_IO case, it should
>
> 'it should' refers to pre_map_gar_callback(), not
> acpi_os_map_generic_address()?
>
yes
>
> > be also return 0,as it's a
> > normal case, but now it will return -ENXIO. so check it out for such
> > case to avoid einj module initialization fail.
>
> apei_map_generic_address() calls acpi_os_map_generic_address() which
> returns NULL for any address space that isn't
> ACPI_ADR_SPACE_SYSTEM_MEMORY.
> That commit now maps this to an error code, where-as before: this code was
> getting away with it.
>
> The bug is it tries to map a GAR that doesn't need mapping.
>
>
> Could we avoid this problem more clearly by returning 0 from
> apei_map_generic_address() for address spaces that don't need mapping?
> (e.g. IO)
>
> This would also fix any other callers lurking in apei.
>
yes, you are right, I think we can move the ACPI_ADR_SPACE_SYSTEM_MEMORY check to:
626 int apei_map_generic_address(struct acpi_generic_address *reg)
627 {
628 int rc;
629 u32 access_bit_width;
630 u64 address;
631
632 rc = apei_check_gar(reg, &address, &access_bit_width);
633 if (rc)
634 return rc;
635
636 if (!acpi_os_map_generic_address(reg) &&
637 reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
638 return -ENXIO;
639
640 return 0;
641 }
642 EXPORT_SYMBOL_GPL(apei_map_generic_address);
>
> Thanks,
>
> James
>
>
> > diff --git a/drivers/acpi/apei/apei-base.c
> > b/drivers/acpi/apei/apei-base.c index 552fd9ffaca4..042d2dbdb855
> > 100644
> > --- a/drivers/acpi/apei/apei-base.c
> > +++ b/drivers/acpi/apei/apei-base.c
> > @@ -230,7 +230,8 @@ static int pre_map_gar_callback(struct
> > apei_exec_context *ctx, {
> > u8 ins = entry->instruction;
> >
> > - if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
> > + if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER &&
> > + entry->register_region.space_id ==
> ACPI_ADR_SPACE_SYSTEM_MEMORY)
> > return apei_map_generic_address(&entry->register_region);
> >
> > return 0;
>
Thanks
Aili Yao
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Fix incorrect return value of pre_map_gar_callback
2020-10-29 11:30 ` Re:Re: [PATCH] Fix incorrect return value of pre_map_gar_callback yaoaili126
@ 2020-10-30 14:03 ` James Morse
0 siblings, 0 replies; 5+ messages in thread
From: James Morse @ 2020-10-30 14:03 UTC (permalink / raw)
To: yaoaili126; +Cc: rjw, lenb, tony.luck, bp, linux-acpi, YANGFENG1, yaoaili
Hello,
On 29/10/2020 11:30, yaoaili126@163.com wrote:
> From: Aili Yao <yaoaili@kingsoft.com>
>> From: James Morse [mailto:james.morse
>> The bug is it tries to map a GAR that doesn't need mapping.
>>
>>
>> Could we avoid this problem more clearly by returning 0 from
>> apei_map_generic_address() for address spaces that don't need mapping?
>> (e.g. IO)
>>
>> This would also fix any other callers lurking in apei.
> yes, you are right, I think we can move the ACPI_ADR_SPACE_SYSTEM_MEMORY check to:
>
> 626 int apei_map_generic_address(struct acpi_generic_address *reg)
> 627 {
> 628 int rc;
> 629 u32 access_bit_width;
> 630 u64 address;
> 631
> 632 rc = apei_check_gar(reg, &address, &access_bit_width);
> 633 if (rc)
> 634 return rc;
> 635
> 636 if (!acpi_os_map_generic_address(reg) &&
> 637 reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> 638 return -ENXIO;
> 639
> 640 return 0;
> 641 }
> 642 EXPORT_SYMBOL_GPL(apei_map_generic_address);
This swallows the error code for other address spaces too, which would be nasty to debug
in the future!
Could you do something like:
| rc = apei_check_gar(reg, &address, &access_bit_width);
| if (rc)
| return rc;
|
| /* IO space doesn't need mapping */
| if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO)
| return 0;
|
| return acpi_os_map_generic_address(reg);
(and it continues to get-away with the unmap call as that doesn't return anything, which I
think is fine)
If you post this, please follow the most popular style of previous patches for the
subject. See: git log --oneline drivers/acpi/apei/apei-base.c
(This is to let the maintainer spot which patches they need to do something with)
Thanks,
James
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Fix incorrect return value of pre_map_gar_callback
@ 2020-10-26 6:15 yaoaili126
2020-10-26 16:59 ` Luck, Tony
2020-10-28 19:19 ` James Morse
0 siblings, 2 replies; 5+ messages in thread
From: yaoaili126 @ 2020-10-26 6:15 UTC (permalink / raw)
To: rjw, lenb; +Cc: james.morse, tony.luck, bp, linux-acpi, yangfeng1, yaoaili
From: Aili Yao <yaoaili@kingsoft.com>
From commit 6915564dc5a8 ("ACPI: OSL: Change the type of
acpi_os_map_generic_address() return value"),acpi_os_map_generic_address
will return logical address or NULL for error, but pre_map_gar_callback,for
ACPI_ADR_SPACE_SYSTEM_IO case, it should be also return 0,as it's a
normal case, but now it will return -ENXIO. so check it out for such case
to avoid einj module initialization fail.
Signed-off-by: Aili Yao <yaoaili@kingsoft.com>
---
drivers/acpi/apei/apei-base.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 552fd9ffaca4..042d2dbdb855 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -230,7 +230,8 @@ static int pre_map_gar_callback(struct apei_exec_context *ctx,
{
u8 ins = entry->instruction;
- if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
+ if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER &&
+ entry->register_region.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
return apei_map_generic_address(&entry->register_region);
return 0;
--
2.18.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* RE: [PATCH] Fix incorrect return value of pre_map_gar_callback
2020-10-26 6:15 yaoaili126
@ 2020-10-26 16:59 ` Luck, Tony
2020-10-28 19:19 ` James Morse
1 sibling, 0 replies; 5+ messages in thread
From: Luck, Tony @ 2020-10-26 16:59 UTC (permalink / raw)
To: yaoaili126, rjw, lenb; +Cc: james.morse, bp, linux-acpi, yangfeng1, yaoaili
> - if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
> + if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER &&
> + entry->register_region.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
I don't know (too long since I looked at APEI code) if this is the right fix, but I tried it out and it does allow einj.ko module to load.
Tested-by: Tony Luck <tony.luck@intel.com>
-Tony
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix incorrect return value of pre_map_gar_callback
2020-10-26 6:15 yaoaili126
2020-10-26 16:59 ` Luck, Tony
@ 2020-10-28 19:19 ` James Morse
1 sibling, 0 replies; 5+ messages in thread
From: James Morse @ 2020-10-28 19:19 UTC (permalink / raw)
To: yaoaili126; +Cc: rjw, lenb, tony.luck, bp, linux-acpi, yangfeng1, yaoaili
Hi!
On 26/10/2020 06:15, yaoaili126@163.com wrote:
> From: Aili Yao <yaoaili@kingsoft.com>
>
> From commit 6915564dc5a8 ("ACPI: OSL: Change the type of
> acpi_os_map_generic_address() return value"),acpi_os_map_generic_address
> will return logical address or NULL for error, but pre_map_gar_callback,for
> ACPI_ADR_SPACE_SYSTEM_IO case, it should
'it should' refers to pre_map_gar_callback(), not acpi_os_map_generic_address()?
> be also return 0,as it's a
> normal case, but now it will return -ENXIO. so check it out for such case
> to avoid einj module initialization fail.
apei_map_generic_address() calls acpi_os_map_generic_address() which returns NULL for any
address space that isn't ACPI_ADR_SPACE_SYSTEM_MEMORY.
That commit now maps this to an error code, where-as before: this code was getting away
with it.
The bug is it tries to map a GAR that doesn't need mapping.
Could we avoid this problem more clearly by returning 0 from apei_map_generic_address()
for address spaces that don't need mapping? (e.g. IO)
This would also fix any other callers lurking in apei.
Thanks,
James
> diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> index 552fd9ffaca4..042d2dbdb855 100644
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -230,7 +230,8 @@ static int pre_map_gar_callback(struct apei_exec_context *ctx,
> {
> u8 ins = entry->instruction;
>
> - if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER)
> + if (ctx->ins_table[ins].flags & APEI_EXEC_INS_ACCESS_REGISTER &&
> + entry->register_region.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> return apei_map_generic_address(&entry->register_region);
>
> return 0;
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-30 14:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <6b71b5b3-b423-6768-15f4-44f7aa7dc12d () arm ! com>
2020-10-29 11:30 ` Re:Re: [PATCH] Fix incorrect return value of pre_map_gar_callback yaoaili126
2020-10-30 14:03 ` James Morse
2020-10-26 6:15 yaoaili126
2020-10-26 16:59 ` Luck, Tony
2020-10-28 19:19 ` James Morse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).