[PATCH v6 0/4] tests/qtest: Update tests using PL011 UART

[PATCH v6 0/4] tests/qtest: Update tests using PL011 UART

[Philippe Mathieu-Daudé]

This series split a patch [*] which was previously posted
too compact in multiple trivial steps. The last patch
initializes the PL011 Control register, enabling the
UART and its transmit channel, before using it to send
the 'T' character in the boot-serial-test.

[*] https://lore.kernel.org/qemu-devel/20240719181041.49545-10-philmd@linaro.org/

Supersedes: <20240719181041.49545-10-philmd@linaro.org>

Philippe Mathieu-Daudé (4):
  tests/qtest/boot-serial-test: Improve ASM comments of PL011 tests
  tests/qtest/boot-serial-test: Reduce for() loop in PL011 tests
  tests/qtest/boot-serial-test: Reorder pair of instructions in PL011
    test
  tests/qtest/boot-serial-test: Initialize PL011 Control register

 tests/qtest/boot-serial-test.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

-- 
2.47.1

[PATCH v6 1/4] tests/qtest/boot-serial-test: Improve ASM comments of PL011 tests

[Philippe Mathieu-Daudé]

Re-indent ASM comments adding the 'loop:' label.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 3b92fa5d506..a71d2857807 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -70,18 +70,18 @@ static const uint8_t kernel_plml605[] = {
 };
 
 static const uint8_t bios_raspi2[] = {
-    0x08, 0x30, 0x9f, 0xe5,                 /* ldr   r3,[pc,#8]    Get base */
-    0x54, 0x20, 0xa0, 0xe3,                 /* mov     r2,#'T' */
-    0x00, 0x20, 0xc3, 0xe5,                 /* strb    r2,[r3] */
-    0xfb, 0xff, 0xff, 0xea,                 /* b       loop */
-    0x00, 0x10, 0x20, 0x3f,                 /* 0x3f201000 = UART0 base addr */
+    0x08, 0x30, 0x9f, 0xe5,                 /* loop:  ldr     r3, [pc, #8]   Get &UART0 */
+    0x54, 0x20, 0xa0, 0xe3,                 /*        mov     r2, #'T' */
+    0x00, 0x20, 0xc3, 0xe5,                 /*        strb    r2, [r3]       *TXDAT = 'T' */
+    0xfb, 0xff, 0xff, 0xea,                 /*        b       -12            (loop) */
+    0x00, 0x10, 0x20, 0x3f,                 /* UART0: 0x3f201000 */
 };
 
 static const uint8_t kernel_aarch64[] = {
-    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
-    0x02, 0x20, 0xa1, 0xd2,                 /* mov     x2, #0x9000000 */
-    0x41, 0x00, 0x00, 0x39,                 /* strb    w1, [x2] */
-    0xfd, 0xff, 0xff, 0x17,                 /* b       -12 (loop) */
+    0x81, 0x0a, 0x80, 0x52,                 /* loop:  mov    w1, #'T' */
+    0x02, 0x20, 0xa1, 0xd2,                 /*        mov    x2, #0x9000000  Load UART0 */
+    0x41, 0x00, 0x00, 0x39,                 /*        strb   w1, [x2]        *TXDAT = 'T' */
+    0xfd, 0xff, 0xff, 0x17,                 /*        b      -12             (loop) */
 };
 
 static const uint8_t kernel_nrf51[] = {
-- 
2.47.1

[PATCH v6 2/4] tests/qtest/boot-serial-test: Reduce for() loop in PL011 tests

[Philippe Mathieu-Daudé]

Since registers are not modified, we don't need
to refill their values. Directly jump to the previous
store instruction to keep filling the TXDAT register.

The equivalent C code remains:

  while (true) {
      *UART_DATA = 'T';
  }

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index a71d2857807..553575ca75c 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -70,18 +70,18 @@ static const uint8_t kernel_plml605[] = {
 };
 
 static const uint8_t bios_raspi2[] = {
-    0x08, 0x30, 0x9f, 0xe5,                 /* loop:  ldr     r3, [pc, #8]   Get &UART0 */
+    0x08, 0x30, 0x9f, 0xe5,                 /*        ldr     r3, [pc, #8]   Get &UART0 */
     0x54, 0x20, 0xa0, 0xe3,                 /*        mov     r2, #'T' */
-    0x00, 0x20, 0xc3, 0xe5,                 /*        strb    r2, [r3]       *TXDAT = 'T' */
-    0xfb, 0xff, 0xff, 0xea,                 /*        b       -12            (loop) */
+    0x00, 0x20, 0xc3, 0xe5,                 /* loop:  strb    r2, [r3]       *TXDAT = 'T' */
+    0xff, 0xff, 0xff, 0xea,                 /*        b       -4             (loop) */
     0x00, 0x10, 0x20, 0x3f,                 /* UART0: 0x3f201000 */
 };
 
 static const uint8_t kernel_aarch64[] = {
-    0x81, 0x0a, 0x80, 0x52,                 /* loop:  mov    w1, #'T' */
+    0x81, 0x0a, 0x80, 0x52,                 /*        mov    w1, #'T' */
     0x02, 0x20, 0xa1, 0xd2,                 /*        mov    x2, #0x9000000  Load UART0 */
-    0x41, 0x00, 0x00, 0x39,                 /*        strb   w1, [x2]        *TXDAT = 'T' */
-    0xfd, 0xff, 0xff, 0x17,                 /*        b      -12             (loop) */
+    0x41, 0x00, 0x00, 0x39,                 /* loop:  strb   w1, [x2]        *TXDAT = 'T' */
+    0xff, 0xff, 0xff, 0x17,                 /*        b      -4              (loop) */
 };
 
 static const uint8_t kernel_nrf51[] = {
-- 
2.47.1

[PATCH v6 3/4] tests/qtest/boot-serial-test: Reorder pair of instructions in PL011 test

[Philippe Mathieu-Daudé]

In the next commit we are going to use a different value
for the $w1 register, maintaining the same $x2 value. In
order to keep the next commit trivial to review, set $x2
before $w1.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 553575ca75c..bcfa504826c 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -78,8 +78,8 @@ static const uint8_t bios_raspi2[] = {
 };
 
 static const uint8_t kernel_aarch64[] = {
-    0x81, 0x0a, 0x80, 0x52,                 /*        mov    w1, #'T' */
     0x02, 0x20, 0xa1, 0xd2,                 /*        mov    x2, #0x9000000  Load UART0 */
+    0x81, 0x0a, 0x80, 0x52,                 /*        mov    w1, #'T' */
     0x41, 0x00, 0x00, 0x39,                 /* loop:  strb   w1, [x2]        *TXDAT = 'T' */
     0xff, 0xff, 0xff, 0x17,                 /*        b      -4              (loop) */
 };
-- 
2.47.1

[PATCH v6 4/4] tests/qtest/boot-serial-test: Initialize PL011 Control register

[Philippe Mathieu-Daudé]

The tests using the PL011 UART of the virt and raspi machines
weren't properly enabling the UART and its transmitter previous
to sending characters. Follow the PL011 manual initialization
recommendation by setting the proper bits of the control register.

Update the ASM code prefixing:

  *UART_CTRL = UART_ENABLE | TX_ENABLE;

to:

  while (true) {
      *UART_DATA = 'T';
  }

Note, since commit 51b61dd4d56 ("hw/char/pl011: Warn when using
disabled transmitter") incomplete PL011 initialization can be
logged using the '-d guest_errors' command line option.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index bcfa504826c..6ea7e62e076 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -70,15 +70,20 @@ static const uint8_t kernel_plml605[] = {
 };
 
 static const uint8_t bios_raspi2[] = {
-    0x08, 0x30, 0x9f, 0xe5,                 /*        ldr     r3, [pc, #8]   Get &UART0 */
+    0x10, 0x30, 0x9f, 0xe5,                 /*        ldr     r3, [pc, #8]   Get &UART0 */
+    0x10, 0x20, 0x9f, 0xe5,                 /*        ldr     r2, [pc, #8]   Get &CR */
+    0xb0, 0x23, 0xc3, 0xe1,                 /*        strh    r2, [r3, #48]  Set CR */
     0x54, 0x20, 0xa0, 0xe3,                 /*        mov     r2, #'T' */
     0x00, 0x20, 0xc3, 0xe5,                 /* loop:  strb    r2, [r3]       *TXDAT = 'T' */
     0xff, 0xff, 0xff, 0xea,                 /*        b       -4             (loop) */
     0x00, 0x10, 0x20, 0x3f,                 /* UART0: 0x3f201000 */
+    0x01, 0x01, 0x00, 0x00,                 /* CR:    0x101 = UARTEN|TXE */
 };
 
 static const uint8_t kernel_aarch64[] = {
     0x02, 0x20, 0xa1, 0xd2,                 /*        mov    x2, #0x9000000  Load UART0 */
+    0x21, 0x20, 0x80, 0x52,                 /*        mov    w1, 0x101       CR = UARTEN|TXE */
+    0x41, 0x60, 0x00, 0x79,                 /*        strh   w1, [x2, #48]   Set CR */
     0x81, 0x0a, 0x80, 0x52,                 /*        mov    w1, #'T' */
     0x41, 0x00, 0x00, 0x39,                 /* loop:  strb   w1, [x2]        *TXDAT = 'T' */
     0xff, 0xff, 0xff, 0x17,                 /*        b      -4              (loop) */
-- 
2.47.1

Re: [PATCH v6 0/4] tests/qtest: Update tests using PL011 UART

[Richard Henderson]

On 1/2/25 02:58, Philippe Mathieu-Daudé wrote:
> This series split a patch [*] which was previously posted
> too compact in multiple trivial steps. The last patch
> initializes the PL011 Control register, enabling the
> UART and its transmit channel, before using it to send
> the 'T' character in the boot-serial-test.
> 
> [*] https://lore.kernel.org/qemu-devel/20240719181041.49545-10-philmd@linaro.org/
> 
> Supersedes: <20240719181041.49545-10-philmd@linaro.org>
> 
> Philippe Mathieu-Daudé (4):
>    tests/qtest/boot-serial-test: Improve ASM comments of PL011 tests
>    tests/qtest/boot-serial-test: Reduce for() loop in PL011 tests
>    tests/qtest/boot-serial-test: Reorder pair of instructions in PL011
>      test
>    tests/qtest/boot-serial-test: Initialize PL011 Control register
> 
>   tests/qtest/boot-serial-test.c | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
> 

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v6 1/4] tests/qtest/boot-serial-test: Improve ASM comments of PL011 tests

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Re-indent ASM comments adding the 'loop:' label.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v6 2/4] tests/qtest/boot-serial-test: Reduce for() loop in PL011 tests

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Since registers are not modified, we don't need
> to refill their values. Directly jump to the previous
> store instruction to keep filling the TXDAT register.
>
> The equivalent C code remains:
>
>   while (true) {
>       *UART_DATA = 'T';
>   }
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v6 3/4] tests/qtest/boot-serial-test: Reorder pair of instructions in PL011 test

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> In the next commit we are going to use a different value
> for the $w1 register, maintaining the same $x2 value. In
> order to keep the next commit trivial to review, set $x2
> before $w1.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v6 4/4] tests/qtest/boot-serial-test: Initialize PL011 Control register

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> The tests using the PL011 UART of the virt and raspi machines
> weren't properly enabling the UART and its transmitter previous
> to sending characters. Follow the PL011 manual initialization
> recommendation by setting the proper bits of the control register.
>
> Update the ASM code prefixing:
>
>   *UART_CTRL = UART_ENABLE | TX_ENABLE;
>
> to:
>
>   while (true) {
>       *UART_DATA = 'T';
>   }
>
> Note, since commit 51b61dd4d56 ("hw/char/pl011: Warn when using
> disabled transmitter") incomplete PL011 initialization can be
> logged using the '-d guest_errors' command line option.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  tests/qtest/boot-serial-test.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
> index bcfa504826c..6ea7e62e076 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -70,15 +70,20 @@ static const uint8_t kernel_plml605[] = {
>  };
>  
>  static const uint8_t bios_raspi2[] = {
> -    0x08, 0x30, 0x9f, 0xe5,                 /*        ldr     r3, [pc, #8]   Get &UART0 */
> +    0x10, 0x30, 0x9f, 0xe5,                 /*        ldr     r3, [pc, #8]   Get &UART0 */

The comment needs updating, no?

> +    0x10, 0x20, 0x9f, 0xe5,                 /*        ldr     r2, [pc, #8]   Get &CR */
> +    0xb0, 0x23, 0xc3, 0xe1,                 /*        strh    r2, [r3, #48]  Set CR */
>      0x54, 0x20, 0xa0, 0xe3,                 /*        mov     r2, #'T' */
>      0x00, 0x20, 0xc3, 0xe5,                 /* loop:  strb    r2, [r3]       *TXDAT = 'T' */
>      0xff, 0xff, 0xff, 0xea,                 /*        b       -4             (loop) */
>      0x00, 0x10, 0x20, 0x3f,                 /* UART0: 0x3f201000 */
> +    0x01, 0x01, 0x00, 0x00,                 /* CR:    0x101 = UARTEN|TXE */
>  };
>  
>  static const uint8_t kernel_aarch64[] = {
>      0x02, 0x20, 0xa1, 0xd2,                 /*        mov    x2, #0x9000000  Load UART0 */
> +    0x21, 0x20, 0x80, 0x52,                 /*        mov    w1, 0x101       CR = UARTEN|TXE */
> +    0x41, 0x60, 0x00, 0x79,                 /*        strh   w1, [x2, #48]   Set CR */
>      0x81, 0x0a, 0x80, 0x52,                 /*        mov    w1, #'T' */
>      0x41, 0x00, 0x00, 0x39,                 /* loop:  strb   w1, [x2]        *TXDAT = 'T' */
>      0xff, 0xff, 0xff, 0x17,                 /*        b      -4              (loop) */

Re: [PATCH v6 4/4] tests/qtest/boot-serial-test: Initialize PL011 Control register

[Philippe Mathieu-Daudé]

On 3/1/25 16:00, Fabiano Rosas wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> The tests using the PL011 UART of the virt and raspi machines
>> weren't properly enabling the UART and its transmitter previous
>> to sending characters. Follow the PL011 manual initialization
>> recommendation by setting the proper bits of the control register.
>>
>> Update the ASM code prefixing:
>>
>>    *UART_CTRL = UART_ENABLE | TX_ENABLE;
>>
>> to:
>>
>>    while (true) {
>>        *UART_DATA = 'T';
>>    }
>>
>> Note, since commit 51b61dd4d56 ("hw/char/pl011: Warn when using
>> disabled transmitter") incomplete PL011 initialization can be
>> logged using the '-d guest_errors' command line option.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/boot-serial-test.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
>> index bcfa504826c..6ea7e62e076 100644
>> --- a/tests/qtest/boot-serial-test.c
>> +++ b/tests/qtest/boot-serial-test.c
>> @@ -70,15 +70,20 @@ static const uint8_t kernel_plml605[] = {
>>   };
>>   
>>   static const uint8_t bios_raspi2[] = {
>> -    0x08, 0x30, 0x9f, 0xe5,                 /*        ldr     r3, [pc, #8]   Get &UART0 */
>> +    0x10, 0x30, 0x9f, 0xe5,                 /*        ldr     r3, [pc, #8]   Get &UART0 */
> 
> The comment needs updating, no?

Oops indeed, good catch :)

    "ldr     r3, [pc, #16]"

> 
>> +    0x10, 0x20, 0x9f, 0xe5,                 /*        ldr     r2, [pc, #8]   Get &CR */
>> +    0xb0, 0x23, 0xc3, 0xe1,                 /*        strh    r2, [r3, #48]  Set CR */
>>       0x54, 0x20, 0xa0, 0xe3,                 /*        mov     r2, #'T' */
>>       0x00, 0x20, 0xc3, 0xe5,                 /* loop:  strb    r2, [r3]       *TXDAT = 'T' */
>>       0xff, 0xff, 0xff, 0xea,                 /*        b       -4             (loop) */
>>       0x00, 0x10, 0x20, 0x3f,                 /* UART0: 0x3f201000 */
>> +    0x01, 0x01, 0x00, 0x00,                 /* CR:    0x101 = UARTEN|TXE */
>>   };
>>   
>>   static const uint8_t kernel_aarch64[] = {
>>       0x02, 0x20, 0xa1, 0xd2,                 /*        mov    x2, #0x9000000  Load UART0 */
>> +    0x21, 0x20, 0x80, 0x52,                 /*        mov    w1, 0x101       CR = UARTEN|TXE */
>> +    0x41, 0x60, 0x00, 0x79,                 /*        strh   w1, [x2, #48]   Set CR */
>>       0x81, 0x0a, 0x80, 0x52,                 /*        mov    w1, #'T' */
>>       0x41, 0x00, 0x00, 0x39,                 /* loop:  strb   w1, [x2]        *TXDAT = 'T' */
>>       0xff, 0xff, 0xff, 0x17,                 /*        b      -4              (loop) */