Re: [PATCH v5 09/16] tests/qtest: Update tests using PL011 UART

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 29/7/24 17:47, Peter Maydell wrote:
> On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> We weren't enabling the PL011 TX UART before using it
>> on the raspi and virt machines. Update the ASM code
>> prefixing:
>>
>>    *UART_CTRL = UART_ENABLE | TX_ENABLE;
>>
>> to:
>>
>>    while (true) {
>>        *UART_DATA = 'T';
>>    }
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/boot-serial-test.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
>> index 3b92fa5d50..5cb309ccf0 100644
>> --- a/tests/qtest/boot-serial-test.c
>> +++ b/tests/qtest/boot-serial-test.c
>> @@ -70,18 +70,23 @@ static const uint8_t kernel_plml605[] = {
>>   };
>>
>>   static const uint8_t bios_raspi2[] = {
>> -    0x08, 0x30, 0x9f, 0xe5,                 /* ldr   r3,[pc,#8]    Get base */
>> +    0x10, 0x30, 0x9f, 0xe5,                 /* ldr     r3,[pc,#8]    Get base */
> 
> The instruction bytes have changed but the disassembly comment has not...
> 
>> +    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,                 /* strb    r2,[r3] */
>> -    0xfb, 0xff, 0xff, 0xea,                 /* b       loop */
>> +    0x00, 0x20, 0xc3, 0xe5,                 /* strb    r2,[r3]       loop: */
> 
> This placement of the "loop:" label is odd -- usually the label
> goes before the instruction that a branch to the label will
> start executing at.
> 
>> +    0xfd, 0xff, 0xff, 0xea,                 /* b       loop */
> 
> Here also the bytes changed but not the disassembly. Since
> 'b' is a relative branch, why does the offset change?

It felt simpler while single-stepping to just fill the TXDATA register
with the byte to send, and not reset the other registers with unchanged
values. Anyway you are right, I'll split the changes for clarity.

> 
>>       0x00, 0x10, 0x20, 0x3f,                 /* 0x3f201000 = UART0 base addr */
>> +    0x01, 0x01, 0x00, 0x00,                 /* 0x101      = CR UARTEN|TXE */
>>   };
>>
>>   static const uint8_t kernel_aarch64[] = {
>> -    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
>>       0x02, 0x20, 0xa1, 0xd2,                 /* mov     x2, #0x9000000 */
>> +    0x21, 0x20, 0x80, 0x52,                 /* mov     w1, #0x101 */
>> +    0x41, 0x60, 0x00, 0x79,                 /* strh    w1, [x2, #48] */
>> +    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
>>       0x41, 0x00, 0x00, 0x39,                 /* strb    w1, [x2] */
>> -    0xfd, 0xff, 0xff, 0x17,                 /* b       -12 (loop) */
>> +    0xff, 0xff, 0xff, 0x17,                 /* b       -4 (loop) */
> 
> Another unexplained offset change here.
> 
>>   };
>>
>>   static const uint8_t kernel_nrf51[] = {
>> --
>> 2.41.0
> 
> thanks
> -- PMM