[PATCH v1 1/2] semihosting: defer connect_chardevs a little more to use serialx

[PATCH v1 1/2] semihosting: defer connect_chardevs a little more to use serialx

[konrad]

From: KONRAD Frederic <frederic.konrad@adacore.com>

With that we can just use chardev=serial0.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 softmmu/vl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index f669c06..9b8b48a 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4123,8 +4123,6 @@ void qemu_init(int argc, char **argv, char **envp)
 
     qemu_opts_foreach(qemu_find_opts("chardev"),
                       chardev_init_func, NULL, &error_fatal);
-    /* now chardevs have been created we may have semihosting to connect */
-    qemu_semihosting_connect_chardevs();
 
 #ifdef CONFIG_VIRTFS
     qemu_opts_foreach(qemu_find_opts("fsdev"),
@@ -4271,6 +4269,9 @@ void qemu_init(int argc, char **argv, char **envp)
     if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
         exit(1);
 
+    /* now chardevs have been created we may have semihosting to connect */
+    qemu_semihosting_connect_chardevs();
+
     /* If no default VGA is requested, the default is "none".  */
     if (default_vga) {
         vga_model = get_default_vga_model(machine_class);
-- 
1.8.3.1

[PATCH v1 2/2] semihosting: don't send the trailing '\0'

[konrad]

From: KONRAD Frederic <frederic.konrad@adacore.com>

Don't send the trailing 0 from the string.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 hw/semihosting/console.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index 22e7827..9b4fee9 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -52,7 +52,9 @@ static GString *copy_user_string(CPUArchState *env, target_ulong addr)
 
     do {
         if (cpu_memory_rw_debug(cpu, addr++, &c, 1, 0) == 0) {
-            s = g_string_append_c(s, c);
+            if (c) {
+                s = g_string_append_c(s, c);
+            }
         } else {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "%s: passed inaccessible address " TARGET_FMT_lx,
-- 
1.8.3.1

Re: [PATCH v1 2/2] semihosting: don't send the trailing '\0'

[Philippe Mathieu-Daudé]

On 6/15/20 12:00 PM, konrad@adacore.com wrote:
> From: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> Don't send the trailing 0 from the string.
> 

Fixes: a331c6d7741
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>  hw/semihosting/console.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
> index 22e7827..9b4fee9 100644
> --- a/hw/semihosting/console.c
> +++ b/hw/semihosting/console.c
> @@ -52,7 +52,9 @@ static GString *copy_user_string(CPUArchState *env, target_ulong addr)
>  
>      do {
>          if (cpu_memory_rw_debug(cpu, addr++, &c, 1, 0) == 0) {
> -            s = g_string_append_c(s, c);
> +            if (c) {
> +                s = g_string_append_c(s, c);
> +            }
>          } else {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "%s: passed inaccessible address " TARGET_FMT_lx,
> 

Re: [PATCH v1 1/2] semihosting: defer connect_chardevs a little more to use serialx

[Alex Bennée]

konrad@adacore.com writes:

> From: KONRAD Frederic <frederic.konrad@adacore.com>
>
> With that we can just use chardev=serial0.

I don't quite follow what this means.

./aarch64-softmmu/qemu-system-aarch64 -cpu max -monitor none -chardev=serial0 -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
qemu-system-aarch64: -chardev=serial0: invalid option

./aarch64-softmmu/qemu-system-aarch64 -cpu max -monitor none -chardev id=serial0 -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
qemu-system-aarch64: -chardev id=serial0: chardev: "serial0" missing backend

The run:

./aarch64-softmmu/qemu-system-aarch64 -cpu max -serial mon:stdio -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory

works fine without this patch.

>
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>  softmmu/vl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f669c06..9b8b48a 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -4123,8 +4123,6 @@ void qemu_init(int argc, char **argv, char **envp)
>  
>      qemu_opts_foreach(qemu_find_opts("chardev"),
>                        chardev_init_func, NULL, &error_fatal);
> -    /* now chardevs have been created we may have semihosting to connect */
> -    qemu_semihosting_connect_chardevs();
>  
>  #ifdef CONFIG_VIRTFS
>      qemu_opts_foreach(qemu_find_opts("fsdev"),
> @@ -4271,6 +4269,9 @@ void qemu_init(int argc, char **argv, char **envp)
>      if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
>          exit(1);
>  
> +    /* now chardevs have been created we may have semihosting to connect */
> +    qemu_semihosting_connect_chardevs();
> +
>      /* If no default VGA is requested, the default is "none".  */
>      if (default_vga) {
>          vga_model = get_default_vga_model(machine_class);


-- 
Alex Bennée

Re: [PATCH v1 2/2] semihosting: don't send the trailing '\0'

[Alex Bennée]

konrad@adacore.com writes:

> From: KONRAD Frederic <frederic.konrad@adacore.com>
>
> Don't send the trailing 0 from the string.
>
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>

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

-- 
Alex Bennée

Re: [PATCH v1 1/2] semihosting: defer connect_chardevs a little more to use serialx

[Fred Konrad]

Le 6/16/20 à 4:52 PM, Alex Bennée a écrit :
> 
> konrad@adacore.com writes:
> 
>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>
>> With that we can just use chardev=serial0.
> 
> I don't quite follow what this means.
> 
> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -monitor none -chardev=serial0 -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
> qemu-system-aarch64: -chardev=serial0: invalid option
> 
> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -monitor none -chardev id=serial0 -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
> qemu-system-aarch64: -chardev id=serial0: chardev: "serial0" missing backend
> 
> The run:
> 
> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -serial mon:stdio -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
> 
> works fine without this patch.

Hi Alex, and sorry for the delay,

I meant `-semihosting-config chardev=serial0`.  I suspect your last command-line
will print any string sent to semihosting to stderr by default.

> 
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>> ---
>>   softmmu/vl.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index f669c06..9b8b48a 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -4123,8 +4123,6 @@ void qemu_init(int argc, char **argv, char **envp)
>>   
>>       qemu_opts_foreach(qemu_find_opts("chardev"),
>>                         chardev_init_func, NULL, &error_fatal);
>> -    /* now chardevs have been created we may have semihosting to connect */
>> -    qemu_semihosting_connect_chardevs();
>>   
>>   #ifdef CONFIG_VIRTFS
>>       qemu_opts_foreach(qemu_find_opts("fsdev"),
>> @@ -4271,6 +4269,9 @@ void qemu_init(int argc, char **argv, char **envp)
>>       if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
>>           exit(1);
>>   
>> +    /* now chardevs have been created we may have semihosting to connect */
>> +    qemu_semihosting_connect_chardevs();
>> +
>>       /* If no default VGA is requested, the default is "none".  */
>>       if (default_vga) {
>>           vga_model = get_default_vga_model(machine_class);
> 
> 

Re: [PATCH v1 1/2] semihosting: defer connect_chardevs a little more to use serialx

[Fred Konrad]

Le 6/26/20 à 12:06 PM, Fred Konrad a écrit :
> 
> 
> Le 6/16/20 à 4:52 PM, Alex Bennée a écrit :
>>
>> konrad@adacore.com writes:
>>
>>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>>
>>> With that we can just use chardev=serial0.
>>
>> I don't quite follow what this means.
>>
>> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -monitor none -chardev=serial0 
>> -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
>> qemu-system-aarch64: -chardev=serial0: invalid option
>>
>> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -monitor none -chardev 
>> id=serial0 -M virt -display none -semihosting -kernel 
>> ./tests/tcg/aarch64-softmmu/memory
>> qemu-system-aarch64: -chardev id=serial0: chardev: "serial0" missing backend
>>
>> The run:
>>
>> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -serial mon:stdio -M virt 
>> -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
>>
>> works fine without this patch.
> 
> Hi Alex, and sorry for the delay,
> 
> I meant `-semihosting-config chardev=serial0`.  I suspect your last command-line
> will print any string sent to semihosting to stderr by default.

Does that make sense?  BTW the second patch fixes a bug, it might be interesting
to have it in 5.1.

Cheers,
Fred

> 
>>
>>>
>>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>>> ---
>>>   softmmu/vl.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>>> index f669c06..9b8b48a 100644
>>> --- a/softmmu/vl.c
>>> +++ b/softmmu/vl.c
>>> @@ -4123,8 +4123,6 @@ void qemu_init(int argc, char **argv, char **envp)
>>>       qemu_opts_foreach(qemu_find_opts("chardev"),
>>>                         chardev_init_func, NULL, &error_fatal);
>>> -    /* now chardevs have been created we may have semihosting to connect */
>>> -    qemu_semihosting_connect_chardevs();
>>>   #ifdef CONFIG_VIRTFS
>>>       qemu_opts_foreach(qemu_find_opts("fsdev"),
>>> @@ -4271,6 +4269,9 @@ void qemu_init(int argc, char **argv, char **envp)
>>>       if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
>>>           exit(1);
>>> +    /* now chardevs have been created we may have semihosting to connect */
>>> +    qemu_semihosting_connect_chardevs();
>>> +
>>>       /* If no default VGA is requested, the default is "none".  */
>>>       if (default_vga) {
>>>           vga_model = get_default_vga_model(machine_class);
>>
>>

Re: [PATCH v1 1/2] semihosting: defer connect_chardevs a little more to use serialx

[Alex Bennée]

Fred Konrad <konrad@adacore.com> writes:

> Le 6/26/20 à 12:06 PM, Fred Konrad a écrit :
>> 
>> 
>> Le 6/16/20 à 4:52 PM, Alex Bennée a écrit :
>>>
>>> konrad@adacore.com writes:
>>>
>>>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>>>
>>>> With that we can just use chardev=serial0.
>>>
>>> I don't quite follow what this means.
>>>
>>> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -monitor none -chardev=serial0 
>>> -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
>>> qemu-system-aarch64: -chardev=serial0: invalid option
>>>
>>> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -monitor none -chardev 
>>> id=serial0 -M virt -display none -semihosting -kernel 
>>> ./tests/tcg/aarch64-softmmu/memory
>>> qemu-system-aarch64: -chardev id=serial0: chardev: "serial0" missing backend
>>>
>>> The run:
>>>
>>> ./aarch64-softmmu/qemu-system-aarch64 -cpu max -serial mon:stdio -M virt 
>>> -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory
>>>
>>> works fine without this patch.
>> 
>> Hi Alex, and sorry for the delay,
>> 
>> I meant `-semihosting-config chardev=serial0`.  I suspect your last command-line
>> will print any string sent to semihosting to stderr by default.
>
> Does that make sense?  BTW the second patch fixes a bug, it might be interesting
> to have it in 5.1.

Right - can confirm the difference between:

  ./aarch64-softmmu/qemu-system-aarch64 -cpu max -serial mon:stdio -M virt -display none -semihosting -kernel ./tests/tcg/aarch64-softmmu/memory

and

  ./aarch64-softmmu/qemu-system-aarch64 -cpu max -serial mon:stdio -M virt -display none -semihosting-config chardev=serial0 -kernel ./tests/tcg/aarch64-softmmu/memory

is where the data ends up. I've slightly amended the commit message for
the 1/1 to make it clearer.

Queued to for-5.1/fixes-for-rc1, thanks.

>
> Cheers,
> Fred
>
>> 
>>>
>>>>
>>>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>>>> ---
>>>>   softmmu/vl.c | 5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>>>> index f669c06..9b8b48a 100644
>>>> --- a/softmmu/vl.c
>>>> +++ b/softmmu/vl.c
>>>> @@ -4123,8 +4123,6 @@ void qemu_init(int argc, char **argv, char **envp)
>>>>       qemu_opts_foreach(qemu_find_opts("chardev"),
>>>>                         chardev_init_func, NULL, &error_fatal);
>>>> -    /* now chardevs have been created we may have semihosting to connect */
>>>> -    qemu_semihosting_connect_chardevs();
>>>>   #ifdef CONFIG_VIRTFS
>>>>       qemu_opts_foreach(qemu_find_opts("fsdev"),
>>>> @@ -4271,6 +4269,9 @@ void qemu_init(int argc, char **argv, char **envp)
>>>>       if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
>>>>           exit(1);
>>>> +    /* now chardevs have been created we may have semihosting to connect */
>>>> +    qemu_semihosting_connect_chardevs();
>>>> +
>>>>       /* If no default VGA is requested, the default is "none".  */
>>>>       if (default_vga) {
>>>>           vga_model = get_default_vga_model(machine_class);
>>>
>>>


-- 
Alex Bennée