Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support

[Boris Ostrovsky]

On 05/27/2016 03:34 AM, Zheng, Lv wrote:
> Hi,
>
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@xxxxxxxxxx]
>> Subject: Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access
>> bit width support
>>
>> On 05/26/2016 12:26 PM, Jan Beulich wrote:
>>>>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 05/25/16 9:17
>> PM >>>
>>>> On 05/05/2016 12:58 AM, Lv Zheng wrote:
>>>>> +static u8
>>>>> +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg, u8
>> max_bit_width)
>>>>> +{
>>>>> +    u64 address;
>>>>> +
>>>>> +    if (!reg->access_width) {
>>>>> +        /*
>>>>> +         * Detect old register descriptors where only the bit_width field
>>>>> +         * makes senses. The target address is copied to handle possible
>>>>> +         * alignment issues.
>>>>> +         */
>>>>> +        ACPI_MOVE_64_TO_64(&address, &reg->address);
>>>>> +        if (!reg->bit_offset && reg->bit_width &&
>>>>> +            ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
>>>>> +            ACPI_IS_ALIGNED(reg->bit_width, 8) &&
>>>>> +            ACPI_IS_ALIGNED(address, reg->bit_width)) {
>>>>> +            return (reg->bit_width);
>>>>> +        } else {
>>>>> +            if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
>>>>> +                return (32);
>>>> This (together with "... Add access_width/bit_offset support in
>>>> acpi_hw_write") breaks Xen guests using older QEMU which doesn't
>> support
>>>> 4-byte IO accesses.
>>>>
>>>> Why not return "reg->bit_width?:max_bit_width" ? This will preserve
>>>> original behavior.
>>> Did you figure out why we get here in the first place, instead of taking
>> the
>>> first "return"? I.e. isn't the issue the apparently wrong use of the second
>>> ACPI_IS_ALIGNED() above? Afaict it ought to be
>>> ACPI_IS_ALIGNED(address, reg->bit_width / 8)...
>> We are trying to access address 0x...b004 (PM1a control) so yes, fixing
>> alignment check would probably resolve the problem that we are seeing
>> now.
>>
>> However, for compatibility purposes we may consider not doing any
>> checks
>> and simply return bit_width if access_width is not available.
> [Lv Zheng] 

Your patch from earlier does resolve both issues, thanks.

> Your solution could result in problems like:
> If a GAS is defined with bit_width not a power of 2, and access_width is any (0).
>
> So the correct fix here is to make sure if bit_width is exactly 8,16,32,64, which matches old register descriptors.
> I added address check here because I want to limit this regression safe check to old register descriptors.
> As since the old bit_width can actually reflect the register's access width, the address of the register should always be aligned.
>
> There might be cases that using the new GAS register descriptor format, it is possible to define a GAS that is not aligned, and it's bit_width is exactly 8,16,32,64.
> We shouldn't make a default access_width decision using bit_width here.

True. But OTOH switching to 32-bit accesses may result in us suddenly
trying to touch bytes we haven't accessed before.

-boris


> The address check here can help to avoid applying this workaround on such cases.
>
> Thanks and best regards
> -Lv