[Qemu-devel] [PATCH 0/2] Remove hw_error from target-arm

[Qemu-devel] [PATCH 0/2] Remove hw_error from target-arm

[Peter Crosthwaite]

Hi Peter,

This removes the last of the hw_errors from target-arm.

Regards,
Peter

Peter Crosthwaite (2):
  arm: cpu: Soften no-EL2 virt IRQ error condition.
  arm: Remove hw_error() usages.

 target-arm/cpu.c    | 10 ++++++----
 target-arm/helper.c |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] arm: cpu: Soften no-EL2 virt IRQ error condition.

[Peter Crosthwaite]

Replace the hw_error for no-EL2 VIRQ with a LOG_UNIMP. This is more
accurate and handles the corner case where the user defeatures EL2
using overrides in a system that would otherwise have EL2
connectivity.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
I'm primarily interested in getting rid of the hw_error() more than
anything. If my use-case seems invalid and a stronger error is
preffered, it can be redone as an assert and the code will still work
for me.
---
 target-arm/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 29b7eca..c4b8448 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -310,8 +310,10 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level)
     case ARM_CPU_VIRQ:
     case ARM_CPU_VFIQ:
         if (!arm_feature(env, ARM_FEATURE_EL2)) {
-            hw_error("%s: Virtual interrupt line %d with no EL2 support\n",
-                     __func__, irq);
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: Virtual interrupt line %d with no EL2 support\n",
+                          __func__, irq);
+            return;
         }
         /* fall through */
     case ARM_CPU_IRQ:
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] arm: Remove hw_error() usages.

[Peter Crosthwaite]

All of these hw_errors are fatal and indicate something wrong with
QEMU implementation.

Convert to g_assert_not_reached.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Dropped the error messages as they are not hugely useful outside of
a debug flow.
---
 target-arm/cpu.c    | 4 ++--
 target-arm/helper.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index c4b8448..8b5471a 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -325,7 +325,7 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level)
         }
         break;
     default:
-        hw_error("arm_cpu_set_irq: Bad interrupt line %d\n", irq);
+        g_assert_not_reached();
     }
 }
 
@@ -344,7 +344,7 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
         kvm_irq |= KVM_ARM_IRQ_CPU_FIQ;
         break;
     default:
-        hw_error("arm_cpu_kvm_set_irq: Bad interrupt line %d\n", irq);
+        g_assert_not_reached();
     }
     kvm_irq |= cs->cpu_index << KVM_ARM_IRQ_VCPU_SHIFT;
     kvm_set_irq(kvm_state, kvm_irq, level ? 1 : 0);
diff --git a/target-arm/helper.c b/target-arm/helper.c
index b2f3db9..662e2bb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3997,7 +3997,7 @@ int bank_number(int mode)
     case ARM_CPU_MODE_MON:
         return 7;
     }
-    hw_error("bank number requested for bad CPSR mode value 0x%x\n", mode);
+    g_assert_not_reached();
 }
 
 void switch_mode(CPUARMState *env, int mode)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] arm: cpu: Soften no-EL2 virt IRQ error condition.

[Peter Maydell]

On 18 July 2015 at 07:00, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> Replace the hw_error for no-EL2 VIRQ with a LOG_UNIMP. This is more
> accurate and handles the corner case where the user defeatures EL2
> using overrides in a system that would otherwise have EL2
> connectivity.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> I'm primarily interested in getting rid of the hw_error() more than
> anything. If my use-case seems invalid and a stronger error is
> preffered, it can be redone as an assert and the code will still work
> for me.
> ---
>  target-arm/cpu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 29b7eca..c4b8448 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -310,8 +310,10 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level)
>      case ARM_CPU_VIRQ:
>      case ARM_CPU_VFIQ:
>          if (!arm_feature(env, ARM_FEATURE_EL2)) {
> -            hw_error("%s: Virtual interrupt line %d with no EL2 support\n",
> -                     __func__, irq);
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: Virtual interrupt line %d with no EL2 support\n",
> +                          __func__, irq);
> +            return;

My inclination is to say we should assert. If the board
model has put together a system where it creates a CPU
with no EL2 support and then wires up the VIRQ/VFIQ
lines to something, then that sounds like a board model
bug to me. LOG_UNIMP is for "this is legitimate guest
behaviour but we haven't written the code to support it".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] arm: Remove hw_error() usages.

[Peter Maydell]

On 18 July 2015 at 07:00, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> All of these hw_errors are fatal and indicate something wrong with
> QEMU implementation.
>
> Convert to g_assert_not_reached.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> Dropped the error messages as they are not hugely useful outside of
> a debug flow.
> ---
>  target-arm/cpu.c    | 4 ++--
>  target-arm/helper.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index c4b8448..8b5471a 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -325,7 +325,7 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level)
>          }
>          break;
>      default:
> -        hw_error("arm_cpu_set_irq: Bad interrupt line %d\n", irq);
> +        g_assert_not_reached();
>      }
>  }
>
> @@ -344,7 +344,7 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
>          kvm_irq |= KVM_ARM_IRQ_CPU_FIQ;
>          break;
>      default:
> -        hw_error("arm_cpu_kvm_set_irq: Bad interrupt line %d\n", irq);
> +        g_assert_not_reached();

We should be consistent between this and arm_cpu_set_irq()
about whether we assert or LOG_UNIMP the VIRQ/VFIQ cases.
I'm happy for them all to assert.

>      }
>      kvm_irq |= cs->cpu_index << KVM_ARM_IRQ_VCPU_SHIFT;
>      kvm_set_irq(kvm_state, kvm_irq, level ? 1 : 0);
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b2f3db9..662e2bb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3997,7 +3997,7 @@ int bank_number(int mode)
>      case ARM_CPU_MODE_MON:
>          return 7;
>      }
> -    hw_error("bank number requested for bad CPSR mode value 0x%x\n", mode);
> +    g_assert_not_reached();
>  }
>
>  void switch_mode(CPUARMState *env, int mode)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM