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

[Zheng, Lv]

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 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.
The address check here can help to avoid applying this workaround on such cases.

Thanks and best regards
-Lv