Linux ACPI Development Discussion List

Xiao, Hui | 13 Jun 2012 09:39

Picon

[RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

Fix the incorrect bit width + offset check condition in apei_check_gar()
function introduced by commit v3.3-5-g15afae6.

The bug caused regression on EINJ error injection with errors:

[Firmware Bug]: APEI: Invalid bit width + offset in GAR [0x1121a5000/64/0/3/0]

on a valid address region of:
    - Register bit width: 64 bits
    - Register bit offset: 0
    - Access Size: 03 [DWord Access: 32]

Signed-off-by: Xiao, Hui <hui.xiao <at> linux.intel.com>
Signed-off-by: Chen Gong <gong.chen <at> linux.intel.com>
---
 drivers/acpi/apei/apei-base.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index 5577762..95e07b2 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
 <at>  <at>  -586,9 +586,12  <at>  <at>  static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr,
 	}
 	*access_bit_width = 1UL << (access_size_code + 2);

-	if ((bit_width + bit_offset) > *access_bit_width) {
+	/* bit_width and bit_offset must be zero when addressing a data
+	 * structure. So just check for non-zero case here */
+	if ((bit_width != 0 && *access_bit_width > bit_width) ||

(Continue reading)

Jean Delvare | 13 Jun 2012 10:46

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

Hi Xiao,

On Wed, 13 Jun 2012 15:39:44 +0800, Xiao, Hui wrote:
> Fix the incorrect bit width + offset check condition in apei_check_gar()
> function introduced by commit v3.3-5-g15afae6.
> 
> The bug caused regression on EINJ error injection with errors:
> 
> [Firmware Bug]: APEI: Invalid bit width + offset in GAR [0x1121a5000/64/0/3/0]
> 
> on a valid address region of:
>     - Register bit width: 64 bits
>     - Register bit offset: 0
>     - Access Size: 03 [DWord Access: 32]

I don't see how this is valid, sorry. If you have a 64-bit register,
you want 64-bit access, don't you?

If the access code is supposed to be able to read large registers in
smaller chunks and assemble them transparently to a larger value, then
there is no point in having any check at all, everything is valid. If
not, then the above is just as invalid as the firmware issue discussed
in bug #43282.

> 
> Signed-off-by: Xiao, Hui <hui.xiao <at> linux.intel.com>
> Signed-off-by: Chen Gong <gong.chen <at> linux.intel.com>
> ---
>  drivers/acpi/apei/apei-base.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)

(Continue reading)

Xiao, Hui | 13 Jun 2012 12:44

Picon

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

Hi Jean,

On 2012/6/13 16:46, Jean Delvare wrote:
> Hi Xiao,
> 
> On Wed, 13 Jun 2012 15:39:44 +0800, Xiao, Hui wrote:
>> Fix the incorrect bit width + offset check condition in apei_check_gar()
>> function introduced by commit v3.3-5-g15afae6.
>>
>> The bug caused regression on EINJ error injection with errors:
>>
>> [Firmware Bug]: APEI: Invalid bit width + offset in GAR [0x1121a5000/64/0/3/0]
>>
>> on a valid address region of:
>>     - Register bit width: 64 bits
>>     - Register bit offset: 0
>>     - Access Size: 03 [DWord Access: 32]
> 
> I don't see how this is valid, sorry. If you have a 64-bit register,
> you want 64-bit access, don't you?
>

Ideally yes. But I don't think if there is a 64-bit width register and only 
lower 32-bit access authority given will make this region invalid.

Assuming a 64-bit register but only lower 32-bit is writable.

> If the access code is supposed to be able to read large registers in
> smaller chunks and assemble them transparently to a larger value, then
> there is no point in having any check at all, everything is valid. If

(Continue reading)

Jean Delvare | 14 Jun 2012 09:53

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

Hi Hui,

On Wed, 13 Jun 2012 18:44:15 +0800, Xiao, Hui wrote:
> On 2012/6/13 16:46, Jean Delvare wrote:
> > If the access code is supposed to be able to read large registers in
> > smaller chunks and assemble them transparently to a larger value, then
> > there is no point in having any check at all, everything is valid. If
> > not, then the above is just as invalid as the firmware issue discussed
> > in bug #43282.
>
> Able to read large registers in smaller chunk, I think so and the register
> bit width set the access boundary.

My understanding is that Access Size, not Register Bit Width, sets the
access boundary. Thus the name.

> For "assemble them transparently to a larger value, then...", not quite 
> understand what you mean here....

I mean that:

    - Register bit width: 32 bits
    - Register bit offset: 0
    - Access Size: 01 [Byte Access: 08]

can be considered invalid (Gary's point of view) but it could also be
interpreted as: hardware can only be accessed 8-bit at a time, but we
have a logical 32-bit register, so the software ACPI layer should issue
4 8-bit reads, and assemble the read values into a 32-bit value. This
obviously raises the issue of endianess, but I guess ACPI implies

(Continue reading)

Gary Hade | 14 Jun 2012 23:49

Picon

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

On Thu, Jun 14, 2012 at 09:53:30AM +0200, Jean Delvare wrote:
> Hi Hui,
> 
> On Wed, 13 Jun 2012 18:44:15 +0800, Xiao, Hui wrote:

< snip >

> 
> > Besides if addressing a data structure, per ACPI spec bit_width and bit_offset
> > must be zero, the original condition will always end with error even valid 
> > access width is given.
> 
> I agree that the original test did not support the data structure case.
> OTOH after quickly reading the relevant page of the ACPI specification,
> I do not understand how the structure size is passed, so I have no idea
> how this case could be handled.

I wasn't able to find any references to the Generic Address
Structure (GAS) in the APEI portion of the ACPI spec implying
data structure access via an address contained in a GAS.  However,
I believe it is a good idea to cover the data structure case in the
event that the code is ever used beyond APEI where the data structure
case could become an issue.

Hui, Please check my assertion that the data structure case is
not a factor for APEI.

Thanks,
Gary

(Continue reading)

Gary Hade | 13 Jun 2012 19:45

Picon

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

On Wed, Jun 13, 2012 at 10:46:51AM +0200, Jean Delvare wrote:
> Hi Xiao,
> 
> On Wed, 13 Jun 2012 15:39:44 +0800, Xiao, Hui wrote:
> > Fix the incorrect bit width + offset check condition in apei_check_gar()
> > function introduced by commit v3.3-5-g15afae6.
> > 
> > The bug caused regression on EINJ error injection with errors:
> > 
> > [Firmware Bug]: APEI: Invalid bit width + offset in GAR [0x1121a5000/64/0/3/0]
> > 
> > on a valid address region of:
> >     - Register bit width: 64 bits
> >     - Register bit offset: 0
> >     - Access Size: 03 [DWord Access: 32]
> 
> I don't see how this is valid, sorry. If you have a 64-bit register,
> you want 64-bit access, don't you?
> 
> If the access code is supposed to be able to read large registers in
> smaller chunks and assemble them transparently to a larger value, then
> there is no point in having any check at all, everything is valid. If
> not, then the above is just as invalid as the firmware issue discussed
> in bug #43282.
> 
> > 
> > Signed-off-by: Xiao, Hui <hui.xiao <at> linux.intel.com>
> > Signed-off-by: Chen Gong <gong.chen <at> linux.intel.com>
> > ---
> >  drivers/acpi/apei/apei-base.c |    7 +++++--

(Continue reading)

Xiao, Hui | 14 Jun 2012 08:14

Picon

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

On 2012/6/14 1:45, Gary Hade wrote:
> On Wed, Jun 13, 2012 at 10:46:51AM +0200, Jean Delvare wrote:
>> Hi Xiao,
>>
>> On Wed, 13 Jun 2012 15:39:44 +0800, Xiao, Hui wrote:
>>> Fix the incorrect bit width + offset check condition in apei_check_gar()
>>> function introduced by commit v3.3-5-g15afae6.
>>>
>>> The bug caused regression on EINJ error injection with errors:
>>>
>>> [Firmware Bug]: APEI: Invalid bit width + offset in GAR [0x1121a5000/64/0/3/0]
>>>
>>> on a valid address region of:
>>>     - Register bit width: 64 bits
>>>     - Register bit offset: 0
>>>     - Access Size: 03 [DWord Access: 32]
>>
>> I don't see how this is valid, sorry. If you have a 64-bit register,
>> you want 64-bit access, don't you?
>>
>> If the access code is supposed to be able to read large registers in
>> smaller chunks and assemble them transparently to a larger value, then
>> there is no point in having any check at all, everything is valid. If
>> not, then the above is just as invalid as the firmware issue discussed
>> in bug #43282.
>>
>>>
>>> Signed-off-by: Xiao, Hui <hui.xiao <at> linux.intel.com>
>>> Signed-off-by: Chen Gong <gong.chen <at> linux.intel.com>
>>> ---

(Continue reading)

Jean Delvare | 14 Jun 2012 10:09

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

On Thu, 14 Jun 2012 14:14:30 +0800, Xiao, Hui wrote:
> From your "good example of a valid case" above. I believe we might have different
> understanding of the "Bit Width" field. 
> 
> Just to make sure, do you take "Bit Width" here(1 bit) as the bit length one should 
> got for mask /*after*/ shifting bit offset(31 bit) of the access_width(32 bit) 
> one read from the register(length unknown, or should equal to access length?) ?
> 
> That's why you think:
>        bit_width + bit_offset <= *access_bit_width
> is valid.

I am not Gary, but it is also how I read the specification.

> For me I take "Bit Width" as bits of the register for access boundary,
> so I think:
>        (*access_bit_width <= bit_width) && (bit_offset < *access_bit_width)
> is valid. 
> 
> For you above case, personally I saw you got a 1-bit register, but want to
> read 32bit from it, and want to get bit[31] by shifting 31bit and mask 0x1.
> 
> Please correct me if I am wrong. Not sure which should be the case ACPI SPEC
> expected. I also have not found any specific explanation on these assumption. 

What makes me believe Gary is right is the granularity of each field.
bit_width and bit_offset can be set with a 1-bit granularity, while
access_bit_width can only be 8, 16, 32 or 64. This clearly means that
access_bit_width (and Access Size before that) is a hardware thing,
while bit_width and bit_offset can only be software things. You've

(Continue reading)

Gary Hade | 14 Jun 2012 18:32

Picon

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

On Thu, Jun 14, 2012 at 10:09:07AM +0200, Jean Delvare wrote:
> On Thu, 14 Jun 2012 14:14:30 +0800, Xiao, Hui wrote:
> > From your "good example of a valid case" above. I believe we might have different
> > understanding of the "Bit Width" field. 
> > 
> > Just to make sure, do you take "Bit Width" here(1 bit) as the bit length one should 
> > got for mask /*after*/ shifting bit offset(31 bit) of the access_width(32 bit) 
> > one read from the register(length unknown, or should equal to access length?) ?
> > 
> > That's why you think:
> >        bit_width + bit_offset <= *access_bit_width
> > is valid.
> 
> I am not Gary, but it is also how I read the specification.

Thanks, Jean.  It seemed like the correct interpretation to me.

> 
> > For me I take "Bit Width" as bits of the register for access boundary,
> > so I think:
> >        (*access_bit_width <= bit_width) && (bit_offset < *access_bit_width)
> > is valid. 

This is not the check that the patch contains.  It also does not
verify that an access will read or write all of the register bits.

> > 
> > For you above case, personally I saw you got a 1-bit register, but want to
> > read 32bit from it, and want to get bit[31] by shifting 31bit and mask 0x1.
> >

(Continue reading)

Xiao, Hui | 15 Jun 2012 13:28

Picon

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

On 2012/6/15 0:32, Gary Hade wrote:
> On Thu, Jun 14, 2012 at 10:09:07AM +0200, Jean Delvare wrote:
>> On Thu, 14 Jun 2012 14:14:30 +0800, Xiao, Hui wrote:
>>> From your "good example of a valid case" above. I believe we might have different
>>> understanding of the "Bit Width" field. 
>>>
>>> Just to make sure, do you take "Bit Width" here(1 bit) as the bit length one should 
>>> got for mask /*after*/ shifting bit offset(31 bit) of the access_width(32 bit) 
>>> one read from the register(length unknown, or should equal to access length?) ?
>>>
>>> That's why you think:
>>>        bit_width + bit_offset <= *access_bit_width
>>> is valid.
>>
>> I am not Gary, but it is also how I read the specification.
> 
> Thanks, Jean.  It seemed like the correct interpretation to me.
> 
>>
>>> For me I take "Bit Width" as bits of the register for access boundary,
>>> so I think:
>>>        (*access_bit_width <= bit_width) && (bit_offset < *access_bit_width)
>>> is valid. 
> 
> This is not the check that the patch contains.  It also does not
> verify that an access will read or write all of the register bits.
> 
>>>
>>> For you above case, personally I saw you got a 1-bit register, but want to
>>> read 32bit from it, and want to get bit[31] by shifting 31bit and mask 0x1.

(Continue reading)

Chen Gong | 18 Jul 2012 10:24

Picon

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

于 2012/6/14 16:09, Jean Delvare 写道:
> On Thu, 14 Jun 2012 14:14:30 +0800, Xiao, Hui wrote:
>>  From your "good example of a valid case" above. I believe we might have different
>> understanding of the "Bit Width" field.
>>
>> Just to make sure, do you take "Bit Width" here(1 bit) as the bit length one should
>> got for mask /*after*/ shifting bit offset(31 bit) of the access_width(32 bit)
>> one read from the register(length unknown, or should equal to access length?) ?
>>
>> That's why you think:
>>         bit_width + bit_offset <= *access_bit_width
>> is valid.
>
> I am not Gary, but it is also how I read the specification.
>
>> For me I take "Bit Width" as bits of the register for access boundary,
>> so I think:
>>         (*access_bit_width <= bit_width) && (bit_offset < *access_bit_width)
>> is valid.
>>
>> For you above case, personally I saw you got a 1-bit register, but want to
>> read 32bit from it, and want to get bit[31] by shifting 31bit and mask 0x1.
>>
>> Please correct me if I am wrong. Not sure which should be the case ACPI SPEC
>> expected. I also have not found any specific explanation on these assumption.
>
> What makes me believe Gary is right is the granularity of each field.
> bit_width and bit_offset can be set with a 1-bit granularity, while
> access_bit_width can only be 8, 16, 32 or 64. This clearly means that
> access_bit_width (and Access Size before that) is a hardware thing,

(Continue reading)

Jean Delvare | 18 Jul 2012 16:28

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

On Wed, 18 Jul 2012 16:24:00 +0800, Chen Gong wrote:
> > On Thu, 14 Jun 2012 14:14:30 +0800, Xiao, Hui wrote:
> Now we have a final decision for this issue? Anyway, we need a patch to
> fix our BIOS issue.
> 
> Jean or Gary, if OK, would you please cook one patch to fix this issue?

Ying's patch adding resource allocation time checks is already in:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=34ddeb035d704eafdcdb3cbc781894300136c3c4
This addresses the log message flood issue.

My own patch with one BIOS bug fixup was accepted by Len Brown and
should go into kernel 3.6:
https://bugzilla.kernel.org/show_bug.cgi?id=43282#c25
(Not sure which tree this is.)

So, as far as I am concerned, the functional issue is solved. From a
performance perspective I think we could drop the run-time checks as
they are now redundant.

--

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Huang Ying | 19 Jul 2012 02:37

Picon

Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

On Wed, 2012-07-18 at 16:28 +0200, Jean Delvare wrote:
> On Wed, 18 Jul 2012 16:24:00 +0800, Chen Gong wrote:
> > > On Thu, 14 Jun 2012 14:14:30 +0800, Xiao, Hui wrote:
> > Now we have a final decision for this issue? Anyway, we need a patch to
> > fix our BIOS issue.
> > 
> > Jean or Gary, if OK, would you please cook one patch to fix this issue?
> 
> Ying's patch adding resource allocation time checks is already in:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=34ddeb035d704eafdcdb3cbc781894300136c3c4
> This addresses the log message flood issue.
> 
> My own patch with one BIOS bug fixup was accepted by Len Brown and
> should go into kernel 3.6:
> https://bugzilla.kernel.org/show_bug.cgi?id=43282#c25
> (Not sure which tree this is.)
> 
> So, as far as I am concerned, the functional issue is solved. From a
> performance perspective I think we could drop the run-time checks as
> they are now redundant.

Performance is not so important for these code.  I keep the run-time
checks as the safe guard for future potential issues.  If you don't like
it, we can add some comments to mark that it is redundant, just safe
guard and should be removed in the future.

Best Regards,
Huang Ying

--

(Continue reading)

Return

Return to gmane.linux.acpi.devel.

Project Web Page

Linux ACPI Development Discussion List

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane