On Fri, Nov 4, 2011 at 5:54 PM, Myron Stowe <myron.stowe@xxxxxxxxx> wrote:
> The snag I've hit concerns these conversions with respect to
> __apei_exec_write_register(). If __apei_exec_write_register() is called with a
> APEI flag of APEI_EXEC_PRESERVE_REGISTER the code writes the
> target register's bits of interest, specified by 'mask' and 'bit_offset', while
> maintaining the current value of all the other bits of the register.
>
> If we believe that 'bit_offset' should be handled by generic GAS/GAR related
> code and 'mask' should be handled by APEI specific code, I do not see how
> to maintain the behavior of __apei_exec_write_register()'s
> APEI_EXEC_PRESERVE_REGISTER block. With the 'bit_offset' handling
> split out into ACPI generic code as with such, we would end up losing the
> value of any bits below a 'bit_offset' value that need to be maintained.
>
> If you work through this example of __apei_exec_write_register() and assume
> 'bit_offset' has been moved into ACPI generic code I believe you will see what
> I am trying to point out.
>
> Assume val = 0x0123456789abcdef, mask = 0xff, bit_offset = 16 (0x10),
> valr = 0xfedcba987654321, and (flags & APEI_EXEC_PRESERVE_REGISTER)
> is true the the current code works as follows:
> val &= mask 0x00000000000000ef
> val <<= bit_offset 0x0000000000ef0000
> now the APEI_EXEC_PRESERVE_REGISTER block
> valr &= ~(mask << bit_offset) 0xfedcba9876003210
> val |= valr 0xfedeba9876EF3210
>
> I don't see how to maintain this behavior with a converted acpi_write() that
> itself handles 'bit_offset' shifting.
I think this analysis is incorrect because it keeps the bit_offset
shift in __apei_exec_write_register() even though it assumes
acpi_read() uses bit_offset to extract the region.
Assume a 64-bit CSR at 0x1000, with bit_offset = 16 in the GAS and an
8-bit mask, so the region of interest is bits [23:16]:
gas->address 0x1000
gas->bit_offset 16 (0x10)
entry->mask 0x00000000000000ff
Here's how we handle this in the current __apei_exec_write_register(),
with acpi_read() ignoring bit_offset:
val &= entry->mask;
val <<= entry->register_region.bit_offset;
if (entry->flags & APEI_EXEC_PRESERVE_REGISTER) {
acpi_atomic_read(&valr, &entry->register_region);
valr &= ~(entry->mask << entry->register_region.bit_offset);
val |= valr;
}
acpi_atomic_write(val, &entry->register_region);
initial value @0x1000 0xfedcba9876543210
value to write (val) 0x0123456789abcdef
val &= mask 0x00000000000000EF
val <<= bit_offset 0x0000000000EF0000
valr 0xfedcba9876543210 (from acpi_atomic_read)
valr &= ~(mask << offset) 0xfedcba9876003210
val |= valr 0xfedcba9876EF3210
final value @0x1000 0xfedcba9876EF3210 (by acpi_atomic_write)
If we make acpi_read() & acpi_write() pay attention to bit_offset,
__apei_exec_write_register() should look like this:
val &= entry->mask;
if (entry->flags & APEI_EXEC_PRESERVE_REGISTER) {
acpi_read(&valr, &entry->register_region); /* valr = bits [xx:16] */
valr &= ~entry->mask;
val |= valr;
}
acpi_write(val, &entry->register_region); /* writes only bits [xx:16] */
initial value @0x1000 0xfedcba9876543210
value to write (val) 0x0123456789abcdef
val &= mask 0x00000000000000EF
valr 0x0000fedcba987654 (from acpi_read)
valr &= ~mask 0x0000fedcba987600
val |= valr 0x0000fedcba9876EF
final value @0x1000 0xfedcba9876EF3210 (by acpi_write)
This depends on some assumptions about how acpi_read() and
acpi_write() deal with bit_offset and probably bit_width. I don't
think we have those behaviors all worked out yet, but I think it's
possible, and this is a good opportunity to do it.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
