1. Patchew
  2. QEMU
  3. View series

[PATCH] rust/pl011: Fix range checks for device ID accesses

Junjie Mao posted 1 patch 2 weeks, 2 days ago 
rust/hw/char/pl011/src/device.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] rust/pl011: Fix range checks for device ID accesses
Posted by Junjie Mao 2 weeks, 2 days ago 
The peripheral and PrimeCell identification registers of pl011 are located at
offset 0xFE0 - 0xFFC. To check if a read falls to such registers, the C
implementation checks if the offset-shifted-by-2 (not the offset itself) is in
the range 0x3F8 - 0x3FF.

Use the same check in the Rust implementation.

This fixes the timeout of the following avocado tests:

  * tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt
  * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_virt
  * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_vexpressa9

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
---
 rust/hw/char/pl011/src/device.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2a85960b81..476cacc844 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -182,7 +182,7 @@ pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u
         use RegisterOffset::*;
 
         std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
-            Err(v) if (0x3f8..0x400).contains(&v) => {
+            Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
                 u64::from(self.device_id[(offset - 0xfe0) >> 2])
             }
             Err(_) => {
-- 
2.34.1
Re: [PATCH] rust/pl011: Fix range checks for device ID accesses
Posted by Alex Bennée 2 days, 2 hours ago 
Junjie Mao <junjie.mao@hotmail.com> writes:

> The peripheral and PrimeCell identification registers of pl011 are located at
> offset 0xFE0 - 0xFFC. To check if a read falls to such registers, the C
> implementation checks if the offset-shifted-by-2 (not the offset itself) is in
> the range 0x3F8 - 0x3FF.
>
> Use the same check in the Rust implementation.
>
> This fixes the timeout of the following avocado tests:
>
>   * tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt
>   * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_virt
>   * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_vexpressa9
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>

As this is the simple fix I'm grabbing this for rc2. We can revisit more
Rusty solutions after the release.

Queued to maintainer/for-9.2-rc2, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH] rust/pl011: Fix range checks for device ID accesses
Posted by Alex Bennée 5 days, 9 hours ago 
Junjie Mao <junjie.mao@hotmail.com> writes:

> The peripheral and PrimeCell identification registers of pl011 are located at
> offset 0xFE0 - 0xFFC. To check if a read falls to such registers, the C
> implementation checks if the offset-shifted-by-2 (not the offset itself) is in
> the range 0x3F8 - 0x3FF.
>
> Use the same check in the Rust implementation.
>
> This fixes the timeout of the following avocado tests:
>
>   * tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt
>   * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_virt
>   * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_vexpressa9
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>

This certainly fixes the avocado failures.

Tested-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  rust/hw/char/pl011/src/device.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 2a85960b81..476cacc844 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -182,7 +182,7 @@ pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u
>          use RegisterOffset::*;
>  
>          std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
> -            Err(v) if (0x3f8..0x400).contains(&v) => {
> +            Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
>                  u64::from(self.device_id[(offset - 0xfe0) >> 2])
>              }
>              Err(_) => {

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH] rust/pl011: Fix range checks for device ID accesses
Posted by Junjie Mao 5 days, 9 hours ago 
Alex Bennée <alex.bennee@linaro.org> writes:

> Junjie Mao <junjie.mao@hotmail.com> writes:
>
>> The peripheral and PrimeCell identification registers of pl011 are located at
>> offset 0xFE0 - 0xFFC. To check if a read falls to such registers, the C
>> implementation checks if the offset-shifted-by-2 (not the offset itself) is in
>> the range 0x3F8 - 0x3FF.
>>
>> Use the same check in the Rust implementation.
>>
>> This fixes the timeout of the following avocado tests:
>>
>>   * tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt
>>   * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_virt
>>   * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_vexpressa9
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>
> This certainly fixes the avocado failures.
>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>

Thanks for reviewing and testing, Alex!

Meanwhile, Manos has submitted another fix [1] which also replaces
arrays of constant register values with more explicit register
getters. His change may supercedes mine.

[1] https://lore.kernel.org/qemu-devel/20241117161039.3758840-1-manos.pitsidianakis@linaro.org

--
Best Regards
Junjie Mao
Re: [PATCH] rust/pl011: Fix range checks for device ID accesses
Posted by Alex Bennée 5 days, 8 hours ago 
Junjie Mao <junjie.mao@hotmail.com> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> Junjie Mao <junjie.mao@hotmail.com> writes:
>>
>>> The peripheral and PrimeCell identification registers of pl011 are located at
>>> offset 0xFE0 - 0xFFC. To check if a read falls to such registers, the C
>>> implementation checks if the offset-shifted-by-2 (not the offset itself) is in
>>> the range 0x3F8 - 0x3FF.
>>>
>>> Use the same check in the Rust implementation.
>>>
>>> This fixes the timeout of the following avocado tests:
>>>
>>>   * tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt
>>>   * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_virt
>>>   * tests/avocado/replay_kernel.py:ReplayKernelNormal.test_arm_vexpressa9
>>>
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
>>
>> This certainly fixes the avocado failures.
>>
>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>
>
> Thanks for reviewing and testing, Alex!
>
> Meanwhile, Manos has submitted another fix [1] which also replaces
> arrays of constant register values with more explicit register
> getters. His change may supercedes mine.
>
> [1]
> https://lore.kernel.org/qemu-devel/20241117161039.3758840-1-manos.pitsidianakis@linaro.org

Yes I'm looking at that series now. I just wanted to note this simple
fix might be more relevant to 9.2 as its concise and easy to see what
its doing.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.