Hi, > From: Mike Marshall [mailto:hubcap@xxxxxxxxxxxx] > Subject: Re: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in > acpi_hw_get_access_bit_width() > > Hi Lv... > > I was dead in the water before this patch, qemu-kvm would crash > right away, now everything seems to work great again, thanks! From > my perspective this fixes the c3bc26d problem. [Lv Zheng] Great. The bisection result is c3bc26d, but the code is actually upstreamed in b314a172. c3bc26d is the first commit enabled the bug. :) > > Acked-by: Mike Marshall <hubcap@xxxxxxxxxxxx> [Lv Zheng] Thanks for the test. Best regards -Lv > > -Mike > > On Tue, May 31, 2016 at 3:13 AM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote: > > Hi, Boris and Mike > > > > Please help to validate if this version can also fix your issues. > > After enumerating the possible cases, I realized that the address check > might not be necessary. > > But we need a max_bit_width check in this function to make it prepared > for a future usage in acpi_read()/acpi_write(). > > Thanks in advance. > > > > Best regards > > -Lv > > > >> From: Zheng, Lv > >> Subject: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in > >> acpi_hw_get_access_bit_width() > >> > >> The address check in acpi_hw_get_access_bit_width() should be byte > >> width > >> based, not bit width based. This patch fixes this mistake. > >> > >> For those who want to review acpi_hw_access_bit_width(), here is the > >> concerns and the design details of the function: > >> > >> It is supposed that the GAS Address field should be aligned to the byte > >> width indicated by the GAS AccessSize field. Similarly, for the old non > >> GAS register, it is supposed that its Address should be aligned to its > >> Length. > >> For the "AccessSize = 0 (meaning ANY)" case, we try to return the > >> maximum > >> instruction width (64 for MMIO or 32 for PIO) or the user expected > access > >> bit width (64 for acpi_read()/acpi_write() or 32 for acpi_hw_read()/ > >> acpi_hw_write()) for futher operation and it is supposed that the GAS > >> Address field should always be aligned to the maximum expected > access > >> bit > >> width (otherwise it can't be ANY). > >> > >> The problem is in acpi_tb_init_generic_address(), where the non GAS > >> register's Length is converted into the GAS BitWidth field, its Address is > >> converted into the GAS Address field, and the GAS AccessSize field is > left > >> 0 but most of the register actually cannot be accessed using "ANY" > >> accesses. > >> > >> As a conclusion, when AccessSize = 0 (ANY), the Address should either > be > >> aligned to the BitWidth (wrong conversion) or aligned to 32 (PIO) or 64 > >> (MMIO). Since BitWidth for the wrong conversion is 8,16,32, the > Address > >> of the real GAS should always be aligned to 8,16,32, the address > alignment > >> check is not necessary. But we in fact could enhance the check for a > future > >> case where max_bit_width could be 64 for a PIO access issued from > >> acpi_read()/acpi_write(). > >> > >> Fixes: b314a172ee96 ("ACPICA: Hardware: Add optimized access bit > width > >> support") > >> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> > >> Cc: Mike Marshall <hubcap@xxxxxxxxxxxx> > >> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx> > >> Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> > >> --- > >> drivers/acpi/acpica/hwregs.c | 16 +++++++--------- > >> 1 file changed, 7 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c > >> index 0f18dbc..0553c0b 100644 > >> --- a/drivers/acpi/acpica/hwregs.c > >> +++ b/drivers/acpi/acpica/hwregs.c > >> @@ -86,24 +86,22 @@ acpi_hw_get_access_bit_width(struct > >> acpi_generic_address *reg, u8 max_bit_width) > >> u64 address; > >> > >> if (!reg->access_width) { > >> + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > >> + max_bit_width = 32; > >> + } > >> /* > >> * 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, ®->address); > >> - if (!reg->bit_offset && reg->bit_width && > >> + if (reg->bit_width < max_bit_width && > >> + !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)) { > >> + ACPI_IS_ALIGNED(reg->bit_width, 8)) { > >> return (reg->bit_width); > >> - } else { > >> - if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) > >> { > >> - return (32); > >> - } else { > >> - return (max_bit_width); > >> - } > >> } > >> + return (max_bit_width); > >> } else { > >> return (1 << (reg->access_width + 2)); > >> } > >> -- > >> 1.7.10 > > ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
