[PATCH for-7.0] hw/intc/arm_gicv3_its: Add missing newlines to process_mapc() logging

[PATCH for-7.0] hw/intc/arm_gicv3_its: Add missing newlines to process_mapc() logging

[Peter Maydell]

In commit 84d43d2e82da we rearranged the logging of errors in
process_mapc(), and inadvertently dropped the trailing newlines
from the log messages. Restore them.

Fixes: 84d43d2e82da ("hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index b96b874afdf..d9c741f5fdd 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -524,12 +524,12 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t *cmdpkt)
     trace_gicv3_its_cmd_mapc(icid, cte.rdbase, cte.valid);
 
     if (icid >= s->ct.num_entries) {
-        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d", icid);
+        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d\n", icid);
         return CMD_CONTINUE;
     }
     if (cte.valid && cte.rdbase >= s->gicv3->num_cpu) {
         qemu_log_mask(LOG_GUEST_ERROR,
-                      "ITS MAPC: invalid RDBASE %u ", cte.rdbase);
+                      "ITS MAPC: invalid RDBASE %u\n", cte.rdbase);
         return CMD_CONTINUE;
     }
 
-- 
2.25.1

Re: [PATCH for-7.0] hw/intc/arm_gicv3_its: Add missing newlines to process_mapc() logging

[Philippe Mathieu-Daudé]

On 22/3/22 11:28, Peter Maydell wrote:
> In commit 84d43d2e82da we rearranged the logging of errors in
> process_mapc(), and inadvertently dropped the trailing newlines
> from the log messages. Restore them.
> 
> Fixes: 84d43d2e82da ("hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index b96b874afdf..d9c741f5fdd 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -524,12 +524,12 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t *cmdpkt)
>       trace_gicv3_its_cmd_mapc(icid, cte.rdbase, cte.valid);
>   
>       if (icid >= s->ct.num_entries) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d", icid);
> +        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d\n", icid);

(pre-existing: "%u" format, since icid is uint32_t)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>           return CMD_CONTINUE;
>       }
>       if (cte.valid && cte.rdbase >= s->gicv3->num_cpu) {
>           qemu_log_mask(LOG_GUEST_ERROR,
> -                      "ITS MAPC: invalid RDBASE %u ", cte.rdbase);
> +                      "ITS MAPC: invalid RDBASE %u\n", cte.rdbase);
>           return CMD_CONTINUE;
>       }
>   

Re: [PATCH for-7.0] hw/intc/arm_gicv3_its: Add missing newlines to process_mapc() logging

[Richard Henderson]

On 3/22/22 03:38, Philippe Mathieu-Daudé wrote:
> On 22/3/22 11:28, Peter Maydell wrote:
>> In commit 84d43d2e82da we rearranged the logging of errors in
>> process_mapc(), and inadvertently dropped the trailing newlines
>> from the log messages. Restore them.
>>
>> Fixes: 84d43d2e82da ("hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field")
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/intc/arm_gicv3_its.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
>> index b96b874afdf..d9c741f5fdd 100644
>> --- a/hw/intc/arm_gicv3_its.c
>> +++ b/hw/intc/arm_gicv3_its.c
>> @@ -524,12 +524,12 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t 
>> *cmdpkt)
>>       trace_gicv3_its_cmd_mapc(icid, cte.rdbase, cte.valid);
>>       if (icid >= s->ct.num_entries) {
>> -        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d", icid);
>> +        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d\n", icid);
> 
> (pre-existing: "%u" format, since icid is uint32_t)

Worse than that: 0x%d is going to be *very* misleading.
Should fix to 0x%x right away.


r~

Re: [PATCH for-7.0] hw/intc/arm_gicv3_its: Add missing newlines to process_mapc() logging

[Richard Henderson]

On 3/22/22 03:28, Peter Maydell wrote:
> In commit 84d43d2e82da we rearranged the logging of errors in
> process_mapc(), and inadvertently dropped the trailing newlines
> from the log messages. Restore them.
> 
> Fixes: 84d43d2e82da ("hw/intc/arm_gicv3_its: In MAPC with V=0, don't check rdbase field")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/intc/arm_gicv3_its.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index b96b874afdf..d9c741f5fdd 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -524,12 +524,12 @@ static ItsCmdResult process_mapc(GICv3ITSState *s, const uint64_t *cmdpkt)
>       trace_gicv3_its_cmd_mapc(icid, cte.rdbase, cte.valid);
>   
>       if (icid >= s->ct.num_entries) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d", icid);
> +        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 0x%d\n", icid);
>           return CMD_CONTINUE;
>       }
>       if (cte.valid && cte.rdbase >= s->gicv3->num_cpu) {
>           qemu_log_mask(LOG_GUEST_ERROR,
> -                      "ITS MAPC: invalid RDBASE %u ", cte.rdbase);
> +                      "ITS MAPC: invalid RDBASE %u\n", cte.rdbase);

Should fix %d as well, broken with the same patch, 84d43d2e82d.

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


r~

Re: [PATCH for-7.0] hw/intc/arm_gicv3_its: Add missing newlines to process_mapc() logging

[Philippe Mathieu-Daudé]

On 22/3/22 15:58, Richard Henderson wrote:
> On 3/22/22 03:38, Philippe Mathieu-Daudé wrote:
>> On 22/3/22 11:28, Peter Maydell wrote:
>>> In commit 84d43d2e82da we rearranged the logging of errors in
>>> process_mapc(), and inadvertently dropped the trailing newlines
>>> from the log messages. Restore them.
>>>
>>> Fixes: 84d43d2e82da ("hw/intc/arm_gicv3_its: In MAPC with V=0, don't 
>>> check rdbase field")
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>   hw/intc/arm_gicv3_its.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
>>> index b96b874afdf..d9c741f5fdd 100644
>>> --- a/hw/intc/arm_gicv3_its.c
>>> +++ b/hw/intc/arm_gicv3_its.c
>>> @@ -524,12 +524,12 @@ static ItsCmdResult process_mapc(GICv3ITSState 
>>> *s, const uint64_t *cmdpkt)
>>>       trace_gicv3_its_cmd_mapc(icid, cte.rdbase, cte.valid);
>>>       if (icid >= s->ct.num_entries) {
>>> -        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 
>>> 0x%d", icid);
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "ITS MAPC: invalid ICID 
>>> 0x%d\n", icid);
>>
>> (pre-existing: "%u" format, since icid is uint32_t)
> 
> Worse than that: 0x%d is going to be *very* misleading.
> Should fix to 0x%x right away.

Triple review is helpful :)