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

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

[Xiao, Hui]

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@linux.intel.com>
Signed-off-by: Chen Gong <gong.chen@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
@@ -586,9 +586,12 @@ 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) ||
+			bit_offset > *access_bit_width) {
 		pr_warning(FW_BUG APEI_PFX
-			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",
+			   "Invalid bit width or offset in GAR [0x%llx/%u/%u/%u/%u]\n",
 			   *paddr, bit_width, bit_offset, access_size_code,
 			   space_id);
 		return -EINVAL;
-- 
1.7.3.4

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

[Jean Delvare]

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@linux.intel.com>
> Signed-off-by: Chen Gong <gong.chen@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
> @@ -586,9 +586,12 @@ 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) ||
> +			bit_offset > *access_bit_width) {

I can't make any sense of this test, sorry. And it will trigger on
valid cases, starting with the most frequent case where
*access_bit_width == bit_width. So, nack.

>  		pr_warning(FW_BUG APEI_PFX
> -			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",
> +			   "Invalid bit width or offset in GAR [0x%llx/%u/%u/%u/%u]\n",
>  			   *paddr, bit_width, bit_offset, access_size_code,
>  			   space_id);
>  		return -EINVAL;


-- 
Jean Delvare

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

[Xiao, Hui]

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

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

>>
>> Signed-off-by: Xiao, Hui <hui.xiao@linux.intel.com>
>> Signed-off-by: Chen Gong <gong.chen@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
>> @@ -586,9 +586,12 @@ 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) ||
>> +			bit_offset > *access_bit_width) {
> 
> I can't make any sense of this test, sorry. And it will trigger on
> valid cases, starting with the most frequent case where
> *access_bit_width == bit_width. So, nack.
> 
The condition here is for checking invalid GAR. When 
  *access_bit_width == bit_width
I don't think my code will trigger the error. Instead, the original condition
will trigger the error once bit_offset != 0, which doesn't make sense.

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.

>>  		pr_warning(FW_BUG APEI_PFX
>> -			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",
>> +			   "Invalid bit width or offset in GAR [0x%llx/%u/%u/%u/%u]\n",
>>  			   *paddr, bit_width, bit_offset, access_size_code,
>>  			   space_id);
>>  		return -EINVAL;
> 
> 

Thanks,
-Hui

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

[Jean Delvare]

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
little-endian anyway?

Anyway, this was really meant as a question, as I am no ACPI expert. I
don't think our ACPI code currently implements such read gathering, but
I don't know if this is by lack of time or need, or because it is
simply not supposed to be needed ever.

> > (...)
> > I can't make any sense of this test, sorry. And it will trigger on
> > valid cases, starting with the most frequent case where
> > *access_bit_width == bit_width. So, nack.
> > 
> The condition here is for checking invalid GAR. When 
>   *access_bit_width == bit_width
> I don't think my code will trigger the error.

Doh, I don't know what I was up to yesterday, but obviously I was wrong
here, sorry.

> Instead, the original condition
> will trigger the error once bit_offset != 0, which doesn't make sense.

No, it won't, depending on the value of bit_width.

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

-- 
Jean Delvare

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

[Gary Hade]

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

-- 
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

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

[Gary Hade]

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@linux.intel.com>
> > Signed-off-by: Chen Gong <gong.chen@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
> > @@ -586,9 +586,12 @@ 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) ||
> > +			bit_offset > *access_bit_width) {
> 
> I can't make any sense of this test, sorry. And it will trigger on
> valid cases, starting with the most frequent case where
> *access_bit_width == bit_width. So, nack.

I agree that the change will trigger firmware bug messages for
valid cases.  Here is a good example of a valid case from one
of our systems that confirms this.

[110h 0272   1]                       Action : 06 [Check Busy Status]
[111h 0273   1]                  Instruction : 01 [Read Register Value]
[112h 0274   1]        Flags (decoded below) : 00
                      Preserve Register Bits : 0
[113h 0275   1]                     Reserved : 00

[114h 0276  12]              Register Region : [Generic Address Structure]
[114h 0276   1]                     Space ID : 00 [SystemMemory]
[115h 0277   1]                    Bit Width : 01
[116h 0278   1]                   Bit Offset : 1F
[117h 0279   1]         Encoded Access Width : 03 [DWord Access:32]
[118h 0280   8]                      Address : 000000007F2D7038

[120h 0288   8]                        Value : 0000000000000001
[128h 0296   8]                         Mask : 0000000000000001

Gary

-- 
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

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

[Xiao, Hui]

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@linux.intel.com>
>>> Signed-off-by: Chen Gong <gong.chen@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
>>> @@ -586,9 +586,12 @@ 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) ||
>>> +			bit_offset > *access_bit_width) {
>>
>> I can't make any sense of this test, sorry. And it will trigger on
>> valid cases, starting with the most frequent case where
>> *access_bit_width == bit_width. So, nack.
> 
> I agree that the change will trigger firmware bug messages for
> valid cases.  Here is a good example of a valid case from one
> of our systems that confirms this.
> 
> [110h 0272   1]                       Action : 06 [Check Busy Status]
> [111h 0273   1]                  Instruction : 01 [Read Register Value]
> [112h 0274   1]        Flags (decoded below) : 00
>                       Preserve Register Bits : 0
> [113h 0275   1]                     Reserved : 00
> 
> [114h 0276  12]              Register Region : [Generic Address Structure]
> [114h 0276   1]                     Space ID : 00 [SystemMemory]
> [115h 0277   1]                    Bit Width : 01
> [116h 0278   1]                   Bit Offset : 1F
> [117h 0279   1]         Encoded Access Width : 03 [DWord Access:32]
> [118h 0280   8]                      Address : 000000007F2D7038
> 
> [120h 0288   8]                        Value : 0000000000000001
> [128h 0296   8]                         Mask : 0000000000000001
> 
> Gary
> 
Hi Gary,

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

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. 

Thanks,
-Hui

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

[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,
while bit_width and bit_offset can only be software things. You've
never seen I/O ports that can be read 3 or 5 bits at a time...

-- 
Jean Delvare

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

[Gary Hade]

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.
> > 
> > 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
> never seen I/O ports that can be read 3 or 5 bits at a time...

The "<= Access Size" in this diagram will hopefully clarify the
"bit_width + bit_offset <= *access_bit_width" check:

    |<------------------  <= Access Size ----------------->|
    |<-- Register Bit Width -->|<-- Register Bit Offset -->|
 |<--------------------- Access Size --------------------->|
                                                           ^
                                                           |
                                                 Address --+

The case described in the patch header looks like:

   |<-- Register Bit Width (64) -->|<-- Register Bit Offset (0) -->|
   |<-------+------>|<----------- Access Size (32)---------------->|
            |                                                      ^
            +-- **neglected register bits**                        |        
                                                         Address --+

The 1 bit width register example I provided looks like:

   |<-- Register Bit Width (1) -->|<-- Register Bit Offset (31) -->|
   |<------------------ Access Size (32)-------------------------->|
                                                                   ^
                                                                   |
                                                         Address --+

Gary

-- 
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

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

[Xiao, Hui]

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.
>>>
>>> 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
>> never seen I/O ports that can be read 3 or 5 bits at a time...
> 
> The "<= Access Size" in this diagram will hopefully clarify the
> "bit_width + bit_offset <= *access_bit_width" check:
> 
>     |<------------------  <= Access Size ----------------->|
>     |<-- Register Bit Width -->|<-- Register Bit Offset -->|
>  |<--------------------- Access Size --------------------->|
>                                                            ^
>                                                            |
>                                                  Address --+
> 
> The case described in the patch header looks like:
> 
>    |<-- Register Bit Width (64) -->|<-- Register Bit Offset (0) -->|
>    |<-------+------>|<----------- Access Size (32)---------------->|
>             |                                                      ^
>             +-- **neglected register bits**                        |        
>                                                          Address --+
> 
> The 1 bit width register example I provided looks like:
> 
>    |<-- Register Bit Width (1) -->|<-- Register Bit Offset (31) -->|
>    |<------------------ Access Size (32)-------------------------->|
>                                                                    ^
>                                                                    |
>                                                          Address --+
> 
> Gary
> 

Hi Jean and Gary,

Thanks. Got your point. Your explanation also makes sense to me.

Just an example to clarify the check of:
"(*access_bit_width <= bit_width) && (bit_offset < *access_bit_width)"

                  |<--Mask (1)-->|<--Register Bit Offset (31)-->| // final bit range got
                  |<------------Access Size (32)--------------->| // lower 32-bit accessible
|<------------------- Access Size (64)------------------------->| // all accessible
|<-------------- Register Bit Width (64) ---------------------->| // 64-bit register    
                                                                ^
                                                                |
                                                      Address --+

-Hui

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

[Chen Gong]

于 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,
> while bit_width and bit_offset can only be software things. You've
> never seen I/O ports that can be read 3 or 5 bits at a time...
>

Hi,

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jean Delvare]

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

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

[Huang Ying]

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