acpi: allow accessing acpi-cnt register by byte

[PATCH for-5.1] acpi: allow accessing acpi-cnt register by byte

Posted by Michael Tokarev 5 years, 3 months ago

From the ACPI spec (4.8.3.2.1 PM1 Control Registers):

Register Location: <PM1a_CNT_BLK / PM1b_CNT_BLK> System I/O or Memory Space
Default Value:     00h
Attribute:         Read/Write
Size:              PM1_CNT_LEN (2 bytes)
The PM1 control registers contain the fixed hardware feature control bits.
These bits can be split between two registers: PM1a_CNT or PM1b_CNT.
Each register grouping can be at a different 32-bit aligned address
and is pointed to by the PM1a_CNT_BLK or PM1b_CNT_BLK. The values
for these pointers to the register space are found in the FADT.
Accesses to PM1 control registers are accessed through byte
and word accesses.

So allow 1-byte access too, not only 2-byte.

Fixes: afafe4bbe0cf7d3318e1ac7b40925561f86a6bd4
Fixes: 5d971f9e672507210e77d020d89e0e89165c8fc9
Buglink: https://lore.kernel.org/xen-devel/20200630170913.123646-1-anthony.perard@citrix.com/T/
Buglink: https://bugs.debian.org/964793
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/acpi/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index f6d9ec4f13..2c1199d9dc 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -599,7 +599,8 @@ static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
 static const MemoryRegionOps acpi_pm_cnt_ops = {
     .read = acpi_pm_cnt_read,
     .write = acpi_pm_cnt_write,
-    .valid.min_access_size = 2,
+    .impl.min_access_size = 2,
+    .valid.min_access_size = 1,
     .valid.max_access_size = 2,
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
-- 
2.20.1

Re: [PATCH for-5.1] acpi: allow accessing acpi-cnt register by byte

Posted by Michael S. Tsirkin 5 years, 3 months ago

On Mon, Jul 20, 2020 at 06:39:15PM +0300, Michael Tokarev wrote:
> >From the ACPI spec (4.8.3.2.1 PM1 Control Registers):
> 
> Register Location: <PM1a_CNT_BLK / PM1b_CNT_BLK> System I/O or Memory Space
> Default Value:     00h
> Attribute:         Read/Write
> Size:              PM1_CNT_LEN (2 bytes)
> The PM1 control registers contain the fixed hardware feature control bits.
> These bits can be split between two registers: PM1a_CNT or PM1b_CNT.
> Each register grouping can be at a different 32-bit aligned address
> and is pointed to by the PM1a_CNT_BLK or PM1b_CNT_BLK. The values
> for these pointers to the register space are found in the FADT.
> Accesses to PM1 control registers are accessed through byte
> and word accesses.
> 
> So allow 1-byte access too, not only 2-byte.
> 
> Fixes: afafe4bbe0cf7d3318e1ac7b40925561f86a6bd4
> Fixes: 5d971f9e672507210e77d020d89e0e89165c8fc9

Fixes links are a bit off. Should be:
Fixes: <short sha> ("commit subject")

> Buglink: https://lore.kernel.org/xen-devel/20200630170913.123646-1-anthony.perard@citrix.com/T/
> Buglink: https://bugs.debian.org/964793
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/acpi/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index f6d9ec4f13..2c1199d9dc 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -599,7 +599,8 @@ static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>  static const MemoryRegionOps acpi_pm_cnt_ops = {
>      .read = acpi_pm_cnt_read,
>      .write = acpi_pm_cnt_write,
> -    .valid.min_access_size = 2,
> +    .impl.min_access_size = 2,
> +    .valid.min_access_size = 1,
>      .valid.max_access_size = 2,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> -- 
> 2.20.1

Re: [PATCH for-5.1] acpi: allow accessing acpi-cnt register by byte

Posted by Philippe Mathieu-Daudé 5 years, 3 months ago

On 7/20/20 5:58 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 20, 2020 at 06:39:15PM +0300, Michael Tokarev wrote:
>> >From the ACPI spec (4.8.3.2.1 PM1 Control Registers):
>>
>> Register Location: <PM1a_CNT_BLK / PM1b_CNT_BLK> System I/O or Memory Space
>> Default Value:     00h
>> Attribute:         Read/Write
>> Size:              PM1_CNT_LEN (2 bytes)
>> The PM1 control registers contain the fixed hardware feature control bits.
>> These bits can be split between two registers: PM1a_CNT or PM1b_CNT.
>> Each register grouping can be at a different 32-bit aligned address
>> and is pointed to by the PM1a_CNT_BLK or PM1b_CNT_BLK. The values
>> for these pointers to the register space are found in the FADT.
>> Accesses to PM1 control registers are accessed through byte
>> and word accesses.
>>
>> So allow 1-byte access too, not only 2-byte.
>>
>> Fixes: afafe4bbe0cf7d3318e1ac7b40925561f86a6bd4
>> Fixes: 5d971f9e672507210e77d020d89e0e89165c8fc9
> 
> Fixes links are a bit off. Should be:
> Fixes: <short sha> ("commit subject")

This is indeed documented:
https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message

You can use the git-alias suggested here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg711059.html

> 
>> Buglink: https://lore.kernel.org/xen-devel/20200630170913.123646-1-anthony.perard@citrix.com/T/
>> Buglink: https://bugs.debian.org/964793
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>> ---
>>  hw/acpi/core.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
>> index f6d9ec4f13..2c1199d9dc 100644
>> --- a/hw/acpi/core.c
>> +++ b/hw/acpi/core.c
>> @@ -599,7 +599,8 @@ static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>>  static const MemoryRegionOps acpi_pm_cnt_ops = {
>>      .read = acpi_pm_cnt_read,
>>      .write = acpi_pm_cnt_write,
>> -    .valid.min_access_size = 2,
>> +    .impl.min_access_size = 2,
>> +    .valid.min_access_size = 1,
>>      .valid.max_access_size = 2,
>>      .endianness = DEVICE_LITTLE_ENDIAN,
>>  };
>> -- 
>> 2.20.1
> 
>