[Qemu-devel] [PATCH-for-4.1] target/arm: Add missing break statement for Hypervisor Trap Exception

[Qemu-devel] [PATCH-for-4.1] target/arm: Add missing break statement for Hypervisor Trap Exception

[Philippe Mathieu-Daudé]

Reported by GCC9 when building with  -Wimplicit-fallthrough=2:

  target/arm/helper.c: In function ‘arm_cpu_do_interrupt_aarch32_hyp’:
  target/arm/helper.c:7958:14: error: this statement may fall through [-Werror=implicit-fallthrough=]
   7958 |         addr = 0x14;
        |         ~~~~~^~~~~~
  target/arm/helper.c:7959:5: note: here
   7959 |     default:
        |     ^~~~~~~
  cc1: all warnings being treated as errors

Fixes: b9bc21ff9f9
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/arm/helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 20f8728be1..b74c23a9bc 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7956,6 +7956,7 @@ static void arm_cpu_do_interrupt_aarch32_hyp(CPUState *cs)
         break;
     case EXCP_HYP_TRAP:
         addr = 0x14;
+        break;
     default:
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH-for-4.1] target/arm: Add missing break statement for Hypervisor Trap Exception

[Peter Maydell]

On Fri, 19 Jul 2019 at 12:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Reported by GCC9 when building with  -Wimplicit-fallthrough=2:
>
>   target/arm/helper.c: In function ‘arm_cpu_do_interrupt_aarch32_hyp’:
>   target/arm/helper.c:7958:14: error: this statement may fall through [-Werror=implicit-fallthrough=]
>    7958 |         addr = 0x14;
>         |         ~~~~~^~~~~~
>   target/arm/helper.c:7959:5: note: here
>    7959 |     default:
>         |     ^~~~~~~
>   cc1: all warnings being treated as errors
>
> Fixes: b9bc21ff9f9
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/arm/helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 20f8728be1..b74c23a9bc 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7956,6 +7956,7 @@ static void arm_cpu_do_interrupt_aarch32_hyp(CPUState *cs)
>          break;
>      case EXCP_HYP_TRAP:
>          addr = 0x14;
> +        break;
>      default:
>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>      }

I think this is right, but EXCP_HYP_TRAP is a bit odd -- we appear
to use this only for the case of "SMC instruction is trapped from
NS EL1 to EL2 by HCR.TSC". I was expecting more traps-to-EL2
to use this EXCP_ variable... Mostly we seem to use EXCP_UDEF,
eg for CP_ACCESS_TRAP_UNCATEGORIZED_EL2 coprocessor/sysreg accesses:
this has the same behaviour as EXCP_HYP_TRAP as long as we know
we are going from an EL below 2 to EL2. Which I think we could
also use in the one place we use EXCP_HYP_TRAP; or we could make
wider use of EXCP_HYP_TRAP, since feeding everything through
EXCP_UDEF is rather confusing.

Anyway, for 4.1 we should do this.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH-for-4.1] target/arm: Add missing break statement for Hypervisor Trap Exception

[Philippe Mathieu-Daudé]

On 7/19/19 1:47 PM, Peter Maydell wrote:
> On Fri, 19 Jul 2019 at 12:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Reported by GCC9 when building with  -Wimplicit-fallthrough=2:
>>
>>   target/arm/helper.c: In function ‘arm_cpu_do_interrupt_aarch32_hyp’:
>>   target/arm/helper.c:7958:14: error: this statement may fall through [-Werror=implicit-fallthrough=]
>>    7958 |         addr = 0x14;
>>         |         ~~~~~^~~~~~
>>   target/arm/helper.c:7959:5: note: here
>>    7959 |     default:
>>         |     ^~~~~~~
>>   cc1: all warnings being treated as errors
>>
>> Fixes: b9bc21ff9f9
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  target/arm/helper.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 20f8728be1..b74c23a9bc 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -7956,6 +7956,7 @@ static void arm_cpu_do_interrupt_aarch32_hyp(CPUState *cs)
>>          break;
>>      case EXCP_HYP_TRAP:
>>          addr = 0x14;
>> +        break;
>>      default:
>>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>>      }
> 
> I think this is right, but EXCP_HYP_TRAP is a bit odd -- we appear
> to use this only for the case of "SMC instruction is trapped from
> NS EL1 to EL2 by HCR.TSC". I was expecting more traps-to-EL2
> to use this EXCP_ variable... Mostly we seem to use EXCP_UDEF,
> eg for CP_ACCESS_TRAP_UNCATEGORIZED_EL2 coprocessor/sysreg accesses:
> this has the same behaviour as EXCP_HYP_TRAP as long as we know
> we are going from an EL below 2 to EL2. Which I think we could
> also use in the one place we use EXCP_HYP_TRAP; or we could make
> wider use of EXCP_HYP_TRAP, since feeding everything through
> EXCP_UDEF is rather confusing.
> 
> Anyway, for 4.1 we should do this.
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks, if you take this I forgot to mention:
Reported-by: Stefan Weil <sw@weilnetz.de>

Regards,

Phil.