Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1407173 > unrolled thread
| Started by | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| First post | 2016-05-25 21:20 +0200 |
| Last post | 2016-05-27 05:20 +0200 |
| Articles | 8 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-05-25 21:20 +0200
Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support "Jan Beulich" <jbeulich@suse.com> - 2016-05-26 18:30 +0200
Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-05-26 19:00 +0200
Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-05-27 00:10 +0200
RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-27 05:30 +0200
RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-27 09:40 +0200
Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-05-27 19:40 +0200
RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-27 05:20 +0200
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2016-05-25 21:20 +0200 |
| Subject | Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support |
| Message-ID | <rCMvv-4LK-9@gated-at.bofh.it> |
On 05/05/2016 12:58 AM, Lv Zheng wrote:
> ACPICA commit c49a751b4dae7baec1790748a2b4b6e8ab599f51
>
> For Access Size = 0, it actually can use user expected access bit width.
> This patch implements this.
>
> Besides of the ACPICA upstream commit, this patch also includes a fix fixing
> the issue reported by the FreeBSD community.
> The old register descriptors are translated in acpi_tb_init_generic_address()
> with access_width being filled with 0. This breaks code in
> acpi_hw_get_access_bit_width() when the registers are 16-bit IO ports and their
> bit_width fields are filled with 16. The rapid fix is meant to make code
> written for acpi_hw_get_access_bit_width() regression safer before the issue is
> correctly fixed from acpi_tb_init_generic_address(). Reported by
> John Baldwin <jhb@freebsd.org>, fixed by Lv Zheng <lv.zheng@intel.com>, tested
> by Jung-uk Kim <jkim@freebsd.org>.
>
> Link: https://github.com/acpica/acpica/commit/c49a751b
> Reported-by: John Baldwin <jhb@freebsd.org>
> Tested-by Jung-uk Kim <jkim@freebsd.org>.
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Signed-off-by: Bob Moore <robert.moore@intel.com>
> ---
> drivers/acpi/acpica/hwregs.c | 49 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> index 035fb52..892e677 100644
> --- a/drivers/acpi/acpica/hwregs.c
> +++ b/drivers/acpi/acpica/hwregs.c
> @@ -51,6 +51,10 @@ ACPI_MODULE_NAME("hwregs")
>
> #if (!ACPI_REDUCED_HARDWARE)
> /* Local Prototypes */
> +static u8
> +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg,
> + u8 max_bit_width);
> +
> static acpi_status
> acpi_hw_read_multiple(u32 *value,
> struct acpi_generic_address *register_a,
> @@ -65,6 +69,48 @@ acpi_hw_write_multiple(u32 value,
>
> /******************************************************************************
> *
> + * FUNCTION: acpi_hw_get_access_bit_width
> + *
> + * PARAMETERS: reg - GAS register structure
> + * max_bit_width - Max bit_width supported (32 or 64)
> + *
> + * RETURN: Status
> + *
> + * DESCRIPTION: Obtain optimal access bit width
> + *
> + ******************************************************************************/
> +
> +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, ®->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.
-boris
> + } else {
> + return (max_bit_width);
> + }
> + }
> + } else {
> + return (1 << (reg->access_width + 2));
> + }
> +}
> +
> +/******************************************************************************
> + *
> * FUNCTION: acpi_hw_validate_register
> *
> * PARAMETERS: reg - GAS register structure
> @@ -122,8 +168,7 @@ acpi_hw_validate_register(struct acpi_generic_address *reg,
>
> /* Validate the bit_width, convert access_width into number of bits */
>
> - access_width = reg->access_width ? reg->access_width : 1;
> - access_width = 1 << (access_width + 2);
> + access_width = acpi_hw_get_access_bit_width(reg, max_bit_width);
> bit_width =
> ACPI_ROUND_UP(reg->bit_offset + reg->bit_width, access_width);
> if (max_bit_width < bit_width) {
[toc] | [next] | [standalone]
| From | "Jan Beulich" <jbeulich@suse.com> |
|---|---|
| Date | 2016-05-26 18:30 +0200 |
| Subject | Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support |
| Message-ID | <rD6kx-dV-11@gated-at.bofh.it> |
| In reply to | #1407173 |
>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 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, ®->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)...
Jan
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2016-05-26 19:00 +0200 |
| Message-ID | <rD6Nz-p8-21@gated-at.bofh.it> |
| In reply to | #1407624 |
On 05/26/2016 12:26 PM, Jan Beulich wrote:
>>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 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, ®->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.
-boris
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2016-05-27 00:10 +0200 |
| Message-ID | <rDbDA-3vK-13@gated-at.bofh.it> |
| In reply to | #1407639 |
On 05/26/2016 12:55 PM, Boris Ostrovsky wrote:
> On 05/26/2016 12:26 PM, Jan Beulich wrote:
>>>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 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, ®->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.
Interestingly enough I bisected what I thought was a completely
different problem to this patch as well.
Something is messed up with 32-bit dom0 booting (so no QEMU) on one
machine in my pool. First error that I see is
pcieport 0000:00:02.0: can't find IRQ for PCI INT A; probably buggy
MP table
I'll look at this tomorrow, hopefully.
-boris
[toc] | [prev] | [next] | [standalone]
| From | "Zheng, Lv" <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-27 05:30 +0200 |
| Subject | RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support |
| Message-ID | <rDgDf-6xT-1@gated-at.bofh.it> |
| In reply to | #1407639 |
Hi,
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Friday, May 27, 2016 12:56 AM
> 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@oracle.com> 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, ®->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]
There might be GAS defined with AccessWidth = 0.
And its BitOffset can still make senses.
That's why I put the check here, making it a tuning rather than a regression safe check.
Thanks for the information, I'll submit the fix of the address check.
To convert it to:
ACPI_IS_ALIGNED(address, reg->bit_width / 8)
This is my fault.
Thanks
-Lv
>
> -boris
>
[toc] | [prev] | [next] | [standalone]
| From | "Zheng, Lv" <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-27 09:40 +0200 |
| Subject | RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support |
| Message-ID | <rDkxc-uS-17@gated-at.bofh.it> |
| In reply to | #1407639 |
Hi,
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> 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@oracle.com> 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, ®->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
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2016-05-27 19:40 +0200 |
| Message-ID | <rDtTP-6nl-3@gated-at.bofh.it> |
| In reply to | #1407878 |
On 05/27/2016 03:34 AM, Zheng, Lv wrote:
> Hi,
>
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> 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@oracle.com> 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, ®->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 patch from earlier does resolve both issues, thanks.
> 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.
True. But OTOH switching to 32-bit accesses may result in us suddenly
trying to touch bytes we haven't accessed before.
-boris
> The address check here can help to avoid applying this workaround on such cases.
>
> Thanks and best regards
> -Lv
[toc] | [prev] | [next] | [standalone]
| From | "Zheng, Lv" <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-27 05:20 +0200 |
| Subject | RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support |
| Message-ID | <rDgtz-6uI-1@gated-at.bofh.it> |
| In reply to | #1407173 |
Hi,
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Boris Ostrovsky
> Sent: Thursday, May 26, 2016 3:17 AM
> Subject: Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit
> width support
>
> On 05/05/2016 12:58 AM, Lv Zheng wrote:
> > ACPICA commit c49a751b4dae7baec1790748a2b4b6e8ab599f51
> >
> > For Access Size = 0, it actually can use user expected access bit width.
> > This patch implements this.
> >
> > Besides of the ACPICA upstream commit, this patch also includes a fix fixing
> > the issue reported by the FreeBSD community.
> > The old register descriptors are translated in acpi_tb_init_generic_address()
> > with access_width being filled with 0. This breaks code in
> > acpi_hw_get_access_bit_width() when the registers are 16-bit IO ports and
> their
> > bit_width fields are filled with 16. The rapid fix is meant to make code
> > written for acpi_hw_get_access_bit_width() regression safer before the issue
> is
> > correctly fixed from acpi_tb_init_generic_address(). Reported by
> > John Baldwin <jhb@freebsd.org>, fixed by Lv Zheng <lv.zheng@intel.com>,
> tested
> > by Jung-uk Kim <jkim@freebsd.org>.
> >
> > Link: https://github.com/acpica/acpica/commit/c49a751b
> > Reported-by: John Baldwin <jhb@freebsd.org>
> > Tested-by Jung-uk Kim <jkim@freebsd.org>.
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Signed-off-by: Bob Moore <robert.moore@intel.com>
> > ---
> > drivers/acpi/acpica/hwregs.c | 49
> ++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> > index 035fb52..892e677 100644
> > --- a/drivers/acpi/acpica/hwregs.c
> > +++ b/drivers/acpi/acpica/hwregs.c
> > @@ -51,6 +51,10 @@ ACPI_MODULE_NAME("hwregs")
> >
> > #if (!ACPI_REDUCED_HARDWARE)
> > /* Local Prototypes */
> > +static u8
> > +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg,
> > + u8 max_bit_width);
> > +
> > static acpi_status
> > acpi_hw_read_multiple(u32 *value,
> > struct acpi_generic_address *register_a,
> > @@ -65,6 +69,48 @@ acpi_hw_write_multiple(u32 value,
> >
> >
> /***************************************************************
> ***************
> > *
> > + * FUNCTION: acpi_hw_get_access_bit_width
> > + *
> > + * PARAMETERS: reg - GAS register structure
> > + * max_bit_width - Max bit_width supported (32 or 64)
> > + *
> > + * RETURN: Status
> > + *
> > + * DESCRIPTION: Obtain optimal access bit width
> > + *
> > +
> ****************************************************************
> **************/
> > +
> > +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, ®->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.
[Lv Zheng]
Let me ask 2 questions:
A. Was this a bug of the older qemu?
If so, you only need a quirk mechanism for old qemu to work.
And it would be better to provide the quirk inside of the older qemu.
B. Could you provide the detailed register description?
The case I saw from the freebsd community around the qemu is a bit_width = 16 case.
Which is a conversion result of acpi_tb_init_generic_address().
Thus:
1. reg->access_width is always 0
2. reg->bit_offset is always 0
3. reg->bit_width is either 8 or 16
So they can be covered by this check:
if (reg->access_width) {
if (!reg->bit_offset && reg->bit_width &&
ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
ACPI_IS_ALIGNED(reg->bit_width, 8))
I just enhanced it with the address check: ACPI_IS_ALIGNED(address, reg->bit_width)
For your case, what the values of "address/access_width" are?
Can it be fixed by:
1. Either removing ACPI_IS_ALIGNED(address, reg->bit_width), or
2. moving the entire check out of if (!reg->access_width) to form:
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->access_width) {
return (1 << (reg->access_width + 2));
} else {
if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
return (32);
} else {
return (max_bit_width);
}
}
Thanks
-Lv
>
> -boris
>
> > + } else {
> > + return (max_bit_width);
> > + }
> > + }
> > + } else {
> > + return (1 << (reg->access_width + 2));
> > + }
> > +}
> > +
> >
> +/**************************************************************
> ****************
> > + *
> > * FUNCTION: acpi_hw_validate_register
> > *
> > * PARAMETERS: reg - GAS register structure
> > @@ -122,8 +168,7 @@ acpi_hw_validate_register(struct
> acpi_generic_address *reg,
> >
> > /* Validate the bit_width, convert access_width into number of bits */
> >
> > - access_width = reg->access_width ? reg->access_width : 1;
> > - access_width = 1 << (access_width + 2);
> > + access_width = acpi_hw_get_access_bit_width(reg, max_bit_width);
> > bit_width =
> > ACPI_ROUND_UP(reg->bit_offset + reg->bit_width, access_width);
> > if (max_bit_width < bit_width) {
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web