From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932435AbcFABbB (ORCPT ); Tue, 31 May 2016 21:31:01 -0400 Received: from mga14.intel.com ([192.55.52.115]:28827 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756887AbcFABbA (ORCPT ); Tue, 31 May 2016 21:31:00 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,398,1459839600"; d="scan'208";a="818833507" From: "Zheng, Lv" To: Mike Marshall CC: Boris Ostrovsky , Lv Zheng , "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "Wysocki, Rafael J" , "Rafael J. Wysocki" , "Brown, Len" , "Moore, Robert" Subject: RE: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() Thread-Topic: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() Thread-Index: AQHRuwrziyxVelhW00CGtH0TIGS6yZ/SoJSg///22wCAATxFwA== Date: Wed, 1 Jun 2016 01:30:47 +0000 Message-ID: <1AE640813FDE7649BE1B193DEA596E883BBB6FD4@SHSMSX101.ccr.corp.intel.com> References: <1AE640813FDE7649BE1B193DEA596E883BBB6D12@SHSMSX101.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2Q0NzNhMTktNjdlOC00NzE1LTg1MTYtODBlM2Y1YmRhMGJjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IkFmald5WHQ2Y0FUVG03UE1IbW5kYU5EMDFxcHpyMGJhYUExVmlpZ0hRMzA9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u511V8Gv006409 Hi, > From: Mike Marshall [mailto:hubcap@omnibond.com] > 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 [Lv Zheng] Thanks for the test. Best regards -Lv > > -Mike > > On Tue, May 31, 2016 at 3:13 AM, Zheng, Lv 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 > >> Cc: Mike Marshall > >> Suggested-by: Jan Beulich > >> Signed-off-by: Lv Zheng > >> --- > >> 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 > >