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

["Xiao, Hui" <hui.xiao@linux.intel.com>]

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