[PATCH] Fix incorrect return value of pre_map_gar_callback

[PATCH] Fix incorrect return value of pre_map_gar_callback

[yaoaili126]

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

RE: [PATCH] Fix incorrect return value of pre_map_gar_callback

[Luck, Tony]

> -	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

Re: [PATCH] Fix incorrect return value of pre_map_gar_callback

[James Morse]

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;

Re:Re: [PATCH] Fix incorrect return value of pre_map_gar_callback

[yaoaili126]

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

Re: [PATCH] Fix incorrect return value of pre_map_gar_callback

[James Morse]

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