* [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() [not found] <fc78867f000b99f4c692876a77b3ea061e44a368> @ 2016-05-31 7:05 ` Lv Zheng 2016-06-01 3:03 ` Lv Zheng 1 sibling, 0 replies; 9+ messages in thread From: Lv Zheng @ 2016-05-31 7:05 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi, Boris Ostrovsky, Mike Marshall 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@oracle.com> Cc: Mike Marshall <hubcap@omnibond.com> Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() @ 2016-05-31 7:05 ` Lv Zheng 0 siblings, 0 replies; 9+ messages in thread From: Lv Zheng @ 2016-05-31 7:05 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi, Boris Ostrovsky, Mike Marshall 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@oracle.com> Cc: Mike Marshall <hubcap@omnibond.com> Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() 2016-05-31 7:05 ` Lv Zheng (?) @ 2016-05-31 7:13 ` Zheng, Lv 2016-05-31 14:36 ` Mike Marshall -1 siblings, 1 reply; 9+ messages in thread From: Zheng, Lv @ 2016-05-31 7:13 UTC (permalink / raw) To: Boris Ostrovsky, Mike Marshall Cc: Lv Zheng, linux-kernel, linux-acpi, Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Moore, Robert 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@oracle.com> > Cc: Mike Marshall <hubcap@omnibond.com> > Suggested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > --- > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() 2016-05-31 7:13 ` Zheng, Lv @ 2016-05-31 14:36 ` Mike Marshall 2016-05-31 18:03 ` Boris Ostrovsky 2016-06-01 1:30 ` Zheng, Lv 0 siblings, 2 replies; 9+ messages in thread From: Mike Marshall @ 2016-05-31 14:36 UTC (permalink / raw) To: Zheng, Lv Cc: Boris Ostrovsky, Lv Zheng, linux-kernel, linux-acpi, Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Moore, Robert 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. Acked-by: Mike Marshall <hubcap@omnibond.com> -Mike On Tue, May 31, 2016 at 3:13 AM, Zheng, Lv <lv.zheng@intel.com> 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@oracle.com> >> Cc: Mike Marshall <hubcap@omnibond.com> >> Suggested-by: Jan Beulich <jbeulich@suse.com> >> Signed-off-by: Lv Zheng <lv.zheng@intel.com> >> --- >> 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() 2016-05-31 14:36 ` Mike Marshall @ 2016-05-31 18:03 ` Boris Ostrovsky 2016-06-01 1:27 ` Zheng, Lv 2016-06-01 1:30 ` Zheng, Lv 1 sibling, 1 reply; 9+ messages in thread From: Boris Ostrovsky @ 2016-05-31 18:03 UTC (permalink / raw) To: Mike Marshall, Zheng, Lv Cc: Lv Zheng, linux-kernel, linux-acpi, Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Moore, Robert On 05/31/2016 10:36 AM, Mike Marshall wrote: > 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. > > Acked-by: Mike Marshall <hubcap@omnibond.com> > > -Mike > > On Tue, May 31, 2016 at 3:13 AM, Zheng, Lv <lv.zheng@intel.com> 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. You can add Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> although this still allows us to access bytes that we are not supposed to. You may be able to calculate access width as something like min (max_bit_width, ACPI_ROUND_UP((ACPI_ROUND_DOWN(reg->bit_offset, 8) + reg->bit_width), 8); -boris >> 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@oracle.com> >>> Cc: Mike Marshall <hubcap@omnibond.com> >>> Suggested-by: Jan Beulich <jbeulich@suse.com> >>> Signed-off-by: Lv Zheng <lv.zheng@intel.com> >>> --- >>> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() 2016-05-31 18:03 ` Boris Ostrovsky @ 2016-06-01 1:27 ` Zheng, Lv 0 siblings, 0 replies; 9+ messages in thread From: Zheng, Lv @ 2016-06-01 1:27 UTC (permalink / raw) To: Boris Ostrovsky, Mike Marshall Cc: Lv Zheng, linux-kernel, linux-acpi, Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Moore, Robert Hi, > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com] > Subject: Re: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in > acpi_hw_get_access_bit_width() > > On 05/31/2016 10:36 AM, Mike Marshall wrote: > > 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. > > > > Acked-by: Mike Marshall <hubcap@omnibond.com> > > > > -Mike > > > > On Tue, May 31, 2016 at 3:13 AM, Zheng, Lv <lv.zheng@intel.com> > 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. > > You can add > Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> [Lv Zheng] Great! > > although this still allows us to access bytes that we are not supposed to. > > You may be able to calculate access width as something like > > min (max_bit_width, > ACPI_ROUND_UP((ACPI_ROUND_DOWN(reg->bit_offset, 8) + > reg->bit_width), > 8); [Lv Zheng] This looks reasonable to me. And I actually was considering to add such kind of code before. But since we have been working with ACPICA for so many years without supporting reg->bit_offset. We probably can make sure that there is no real platform setting reg->bit_offset to the values other than 0. It is likely that the real platforms that want to utilize "access size" are using it correctly, so we actually needn't do any optimization in acpi_hw_get_access_bit_size() except returning the default one. The code I put here (which looks like an optimization) is meant to create a workaround for the bug reported against acpi_tb_init_generic_address(). Thus IMO, we could do less aggressive approach and do further changes unless we can see real cases. Cheers -Lv > > -boris > > > >> 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@oracle.com> > >>> Cc: Mike Marshall <hubcap@omnibond.com> > >>> Suggested-by: Jan Beulich <jbeulich@suse.com> > >>> Signed-off-by: Lv Zheng <lv.zheng@intel.com> > >>> --- > >>> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() 2016-05-31 14:36 ` Mike Marshall 2016-05-31 18:03 ` Boris Ostrovsky @ 2016-06-01 1:30 ` Zheng, Lv 1 sibling, 0 replies; 9+ messages in thread From: Zheng, Lv @ 2016-06-01 1:30 UTC (permalink / raw) To: Mike Marshall Cc: Boris Ostrovsky, Lv Zheng, linux-kernel, linux-acpi, Wysocki, Rafael J, Rafael J. Wysocki, Brown, Len, Moore, Robert 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 <hubcap@omnibond.com> [Lv Zheng] Thanks for the test. Best regards -Lv > > -Mike > > On Tue, May 31, 2016 at 3:13 AM, Zheng, Lv <lv.zheng@intel.com> 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@oracle.com> > >> Cc: Mike Marshall <hubcap@omnibond.com> > >> Suggested-by: Jan Beulich <jbeulich@suse.com> > >> Signed-off-by: Lv Zheng <lv.zheng@intel.com> > >> --- > >> 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() [not found] <fc78867f000b99f4c692876a77b3ea061e44a368> @ 2016-06-01 3:03 ` Lv Zheng 2016-06-01 3:03 ` Lv Zheng 1 sibling, 0 replies; 9+ messages in thread From: Lv Zheng @ 2016-06-01 3:03 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi 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()) 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 accessed using ANY access bit width). 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 registers 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 for PIO or 64 for MMIO (real GAS). Since currently, max_bit_width is 32, then: 1. BitWidth for the wrong conversion is 8,16,32; and 2. The Address of the real GAS should always be aligned to 8,16,32. The address alignment check to exclude false matched real GAS is not necessary. Thus this patch fixes the issue by removing the address alignment check. OTOH, we in fact could use a simpler check of "reg->bit_width < max_bit_width" to exclude the "BitWidth=64 PIO" case that may be issued from acpi_read()/acpi_write() in the future. Fixes: b314a172ee96 ("ACPICA: Hardware: Add optimized access bit width support") Reported-and-tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Acked-by: Mike Marshall <hubcap@omnibond.com> Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/hwregs.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c index 0f18dbc..daceb80 100644 --- a/drivers/acpi/acpica/hwregs.c +++ b/drivers/acpi/acpica/hwregs.c @@ -83,27 +83,22 @@ acpi_hw_write_multiple(u32 value, static u8 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. + * makes senses. */ - 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() @ 2016-06-01 3:03 ` Lv Zheng 0 siblings, 0 replies; 9+ messages in thread From: Lv Zheng @ 2016-06-01 3:03 UTC (permalink / raw) To: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi 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()) 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 accessed using ANY access bit width). 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 registers 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 for PIO or 64 for MMIO (real GAS). Since currently, max_bit_width is 32, then: 1. BitWidth for the wrong conversion is 8,16,32; and 2. The Address of the real GAS should always be aligned to 8,16,32. The address alignment check to exclude false matched real GAS is not necessary. Thus this patch fixes the issue by removing the address alignment check. OTOH, we in fact could use a simpler check of "reg->bit_width < max_bit_width" to exclude the "BitWidth=64 PIO" case that may be issued from acpi_read()/acpi_write() in the future. Fixes: b314a172ee96 ("ACPICA: Hardware: Add optimized access bit width support") Reported-and-tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Acked-by: Mike Marshall <hubcap@omnibond.com> Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/hwregs.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c index 0f18dbc..daceb80 100644 --- a/drivers/acpi/acpica/hwregs.c +++ b/drivers/acpi/acpica/hwregs.c @@ -83,27 +83,22 @@ acpi_hw_write_multiple(u32 value, static u8 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. + * makes senses. */ - 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-06-01 3:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <fc78867f000b99f4c692876a77b3ea061e44a368> 2016-05-31 7:05 ` [RFC PATCH v2] ACPICA / Hardware: Fix old register check in acpi_hw_get_access_bit_width() Lv Zheng 2016-05-31 7:05 ` Lv Zheng 2016-05-31 7:13 ` Zheng, Lv 2016-05-31 14:36 ` Mike Marshall 2016-05-31 18:03 ` Boris Ostrovsky 2016-06-01 1:27 ` Zheng, Lv 2016-06-01 1:30 ` Zheng, Lv 2016-06-01 3:03 ` [PATCH v3] " Lv Zheng 2016-06-01 3:03 ` Lv Zheng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.