[Qemu-devel] [PATCH 0/3] GICv3 bugfixes (preliminary for virt)

[Qemu-devel] [PATCH 0/3] GICv3 bugfixes (preliminary for virt)

[Peter Maydell]

This patchset has a couple of fixes for bugs in the GICv3
emulation. I only noticed these in the course of adding the
virtualizations support, but they're bugs in the existing
non-virtualized code.

Patch 1 is strictly speaking not a GIC patch, but it's
an improvement to the debug logging that I found helpful
while trying to track down the bugs fixed in patches
2 and 3...

(I do now have the virtualization support to a point
where it will boot a nested Linux guest. Still a bunch
of loose ends to clean up but I hope to get that patchset
out to the list before the holidays...)

Peter Maydell (3):
  target-arm: Log AArch64 exception returns
  hw/intc/arm_gicv3: Remove incorrect usage of fieldoffset
  hw/intc/arm_gicv3: Don't signal Pending+Active interrupts to CPU

 hw/intc/arm_gicv3.c       |  5 +++++
 hw/intc/arm_gicv3_cpuif.c | 13 ++++++-------
 target-arm/op_helper.c    |  9 +++++++++
 3 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] target-arm: Log AArch64 exception returns

[Peter Maydell]

We already log exception entry; add logging of the AArch64 exception
return path as well.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/op_helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index cd94216..ba796d8 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "internals.h"
@@ -972,6 +973,9 @@ void HELPER(exception_return)(CPUARMState *env)
         } else {
             env->regs[15] = env->elr_el[cur_el] & ~0x3;
         }
+        qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
+                      "AArch32 EL%d PC 0x%" PRIx32 "\n",
+                      cur_el, new_el, env->regs[15]);
     } else {
         env->aarch64 = 1;
         pstate_write(env, spsr);
@@ -980,6 +984,9 @@ void HELPER(exception_return)(CPUARMState *env)
         }
         aarch64_restore_sp(env, new_el);
         env->pc = env->elr_el[cur_el];
+        qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
+                      "AArch64 EL%d PC 0x%" PRIx64 "\n",
+                      cur_el, new_el, env->pc);
     }
 
     arm_call_el_change_hook(arm_env_get_cpu(env));
@@ -1002,6 +1009,8 @@ illegal_return:
     if (!arm_singlestep_active(env)) {
         env->pstate &= ~PSTATE_SS;
     }
+    qemu_log_mask(LOG_GUEST_ERROR, "Illegal exception return at EL%d: "
+                  "resuming execution at 0x%" PRIx64 "\n", cur_el, env->pc);
 }
 
 /* Return true if the linked breakpoint entry lbn passes its checks */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] target-arm: Log AArch64 exception returns

[Edgar E. Iglesias]

On Tue, Dec 06, 2016 at 05:46:17PM +0000, Peter Maydell wrote:
> We already log exception entry; add logging of the AArch64 exception
> return path as well.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---
>  target-arm/op_helper.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index cd94216..ba796d8 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -17,6 +17,7 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "cpu.h"
>  #include "exec/helper-proto.h"
>  #include "internals.h"
> @@ -972,6 +973,9 @@ void HELPER(exception_return)(CPUARMState *env)
>          } else {
>              env->regs[15] = env->elr_el[cur_el] & ~0x3;
>          }
> +        qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
> +                      "AArch32 EL%d PC 0x%" PRIx32 "\n",
> +                      cur_el, new_el, env->regs[15]);
>      } else {
>          env->aarch64 = 1;
>          pstate_write(env, spsr);
> @@ -980,6 +984,9 @@ void HELPER(exception_return)(CPUARMState *env)
>          }
>          aarch64_restore_sp(env, new_el);
>          env->pc = env->elr_el[cur_el];
> +        qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
> +                      "AArch64 EL%d PC 0x%" PRIx64 "\n",
> +                      cur_el, new_el, env->pc);
>      }
>  
>      arm_call_el_change_hook(arm_env_get_cpu(env));
> @@ -1002,6 +1009,8 @@ illegal_return:
>      if (!arm_singlestep_active(env)) {
>          env->pstate &= ~PSTATE_SS;
>      }
> +    qemu_log_mask(LOG_GUEST_ERROR, "Illegal exception return at EL%d: "
> +                  "resuming execution at 0x%" PRIx64 "\n", cur_el, env->pc);
>  }
>  
>  /* Return true if the linked breakpoint entry lbn passes its checks */
> -- 
> 2.7.4
> 

[Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3: Remove incorrect usage of fieldoffset

[Peter Maydell]

In the ARMCPRegInfo definitions for the GICv3 CPU interface
registers, we were trying to use .fieldoffset to specify
the locations of data fields within the GICv3CPUState struct.
This is completely broken, because .fieldoffset is for offsets
into the CPUARMState struct. We didn't notice because we
were only using this for reads to BPR0, AP0R<n>, IGRPEN0
and CTLR_EL3, and Linux doesn't use these registers.

Replace the .fieldoffset uses with explicit read functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index bca30c4..35e8eb3 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -1118,35 +1118,35 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] = {
       .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 3,
       .type = ARM_CP_IO | ARM_CP_NO_RAW,
       .access = PL1_RW, .accessfn = gicv3_fiq_access,
-      .fieldoffset = offsetof(GICv3CPUState, icc_bpr[GICV3_G0]),
+      .readfn = icc_bpr_read,
       .writefn = icc_bpr_write,
     },
     { .name = "ICC_AP0R0_EL1", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 4,
       .type = ARM_CP_IO | ARM_CP_NO_RAW,
       .access = PL1_RW, .accessfn = gicv3_fiq_access,
-      .fieldoffset = offsetof(GICv3CPUState, icc_apr[GICV3_G0][0]),
+      .readfn = icc_ap_read,
       .writefn = icc_ap_write,
     },
     { .name = "ICC_AP0R1_EL1", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 5,
       .type = ARM_CP_IO | ARM_CP_NO_RAW,
       .access = PL1_RW, .accessfn = gicv3_fiq_access,
-      .fieldoffset = offsetof(GICv3CPUState, icc_apr[GICV3_G0][1]),
+      .readfn = icc_ap_read,
       .writefn = icc_ap_write,
     },
     { .name = "ICC_AP0R2_EL1", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 6,
       .type = ARM_CP_IO | ARM_CP_NO_RAW,
       .access = PL1_RW, .accessfn = gicv3_fiq_access,
-      .fieldoffset = offsetof(GICv3CPUState, icc_apr[GICV3_G0][2]),
+      .readfn = icc_ap_read,
       .writefn = icc_ap_write,
     },
     { .name = "ICC_AP0R3_EL1", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 7,
       .type = ARM_CP_IO | ARM_CP_NO_RAW,
       .access = PL1_RW, .accessfn = gicv3_fiq_access,
-      .fieldoffset = offsetof(GICv3CPUState, icc_apr[GICV3_G0][3]),
+      .readfn = icc_ap_read,
       .writefn = icc_ap_write,
     },
     /* All the ICC_AP1R*_EL1 registers are banked */
@@ -1275,7 +1275,7 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] = {
       .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 12, .opc2 = 6,
       .type = ARM_CP_IO | ARM_CP_NO_RAW,
       .access = PL1_RW, .accessfn = gicv3_fiq_access,
-      .fieldoffset = offsetof(GICv3CPUState, icc_igrpen[GICV3_G0]),
+      .readfn = icc_igrpen_read,
       .writefn = icc_igrpen_write,
     },
     /* This register is banked */
@@ -1299,7 +1299,6 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] = {
       .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 12, .opc2 = 4,
       .type = ARM_CP_IO | ARM_CP_NO_RAW,
       .access = PL3_RW,
-      .fieldoffset = offsetof(GICv3CPUState, icc_ctlr_el3),
       .readfn = icc_ctlr_el3_read,
       .writefn = icc_ctlr_el3_write,
     },
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3: Remove incorrect usage of fieldoffset

[Edgar E. Iglesias]

On Tue, Dec 06, 2016 at 05:46:18PM +0000, Peter Maydell wrote:
> In the ARMCPRegInfo definitions for the GICv3 CPU interface
> registers, we were trying to use .fieldoffset to specify
> the locations of data fields within the GICv3CPUState struct.
> This is completely broken, because .fieldoffset is for offsets
> into the CPUARMState struct. We didn't notice because we
> were only using this for reads to BPR0, AP0R<n>, IGRPEN0
> and CTLR_EL3, and Linux doesn't use these registers.
> 
> Replace the .fieldoffset uses with explicit read functions.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv3_cpuif.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index bca30c4..35e8eb3 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -1118,35 +1118,35 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] = {
>        .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 3,
>        .type = ARM_CP_IO | ARM_CP_NO_RAW,
>        .access = PL1_RW, .accessfn = gicv3_fiq_access,
> -      .fieldoffset = offsetof(GICv3CPUState, icc_bpr[GICV3_G0]),
> +      .readfn = icc_bpr_read,
>        .writefn = icc_bpr_write,
>      },
>      { .name = "ICC_AP0R0_EL1", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 4,
>        .type = ARM_CP_IO | ARM_CP_NO_RAW,
>        .access = PL1_RW, .accessfn = gicv3_fiq_access,
> -      .fieldoffset = offsetof(GICv3CPUState, icc_apr[GICV3_G0][0]),
> +      .readfn = icc_ap_read,
>        .writefn = icc_ap_write,
>      },
>      { .name = "ICC_AP0R1_EL1", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 5,
>        .type = ARM_CP_IO | ARM_CP_NO_RAW,
>        .access = PL1_RW, .accessfn = gicv3_fiq_access,
> -      .fieldoffset = offsetof(GICv3CPUState, icc_apr[GICV3_G0][1]),
> +      .readfn = icc_ap_read,
>        .writefn = icc_ap_write,
>      },
>      { .name = "ICC_AP0R2_EL1", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 6,
>        .type = ARM_CP_IO | ARM_CP_NO_RAW,
>        .access = PL1_RW, .accessfn = gicv3_fiq_access,
> -      .fieldoffset = offsetof(GICv3CPUState, icc_apr[GICV3_G0][2]),
> +      .readfn = icc_ap_read,
>        .writefn = icc_ap_write,
>      },
>      { .name = "ICC_AP0R3_EL1", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 8, .opc2 = 7,
>        .type = ARM_CP_IO | ARM_CP_NO_RAW,
>        .access = PL1_RW, .accessfn = gicv3_fiq_access,
> -      .fieldoffset = offsetof(GICv3CPUState, icc_apr[GICV3_G0][3]),
> +      .readfn = icc_ap_read,
>        .writefn = icc_ap_write,
>      },
>      /* All the ICC_AP1R*_EL1 registers are banked */
> @@ -1275,7 +1275,7 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] = {
>        .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 12, .opc2 = 6,
>        .type = ARM_CP_IO | ARM_CP_NO_RAW,
>        .access = PL1_RW, .accessfn = gicv3_fiq_access,
> -      .fieldoffset = offsetof(GICv3CPUState, icc_igrpen[GICV3_G0]),
> +      .readfn = icc_igrpen_read,
>        .writefn = icc_igrpen_write,
>      },
>      /* This register is banked */
> @@ -1299,7 +1299,6 @@ static const ARMCPRegInfo gicv3_cpuif_reginfo[] = {
>        .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 12, .opc2 = 4,
>        .type = ARM_CP_IO | ARM_CP_NO_RAW,
>        .access = PL3_RW,
> -      .fieldoffset = offsetof(GICv3CPUState, icc_ctlr_el3),
>        .readfn = icc_ctlr_el3_read,
>        .writefn = icc_ctlr_el3_write,
>      },
> -- 
> 2.7.4
> 

[Qemu-devel] [PATCH 3/3] hw/intc/arm_gicv3: Don't signal Pending+Active interrupts to CPU

[Peter Maydell]

The GICv3 requires that we only signal Pending interrupts to
the CPU. This category does not include Pending+Active interrupts,
which means we need to check whether the interrupt is Active in
the gicr_int_pending() and gicd_int_pending() functions.

Interrupts are rarely in the Active+Pending state, but KVM
uses this as part of its handling of the virtual timer, so
this bug was causing KVM to go into an infinite loop of
taking the vtimer interrupt when the guest first triggered it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
index 8a6c647..f0c967b 100644
--- a/hw/intc/arm_gicv3.c
+++ b/hw/intc/arm_gicv3.c
@@ -54,6 +54,7 @@ static uint32_t gicd_int_pending(GICv3State *s, int irq)
      *  + the PENDING latch is set OR it is level triggered and the input is 1
      *  + its ENABLE bit is set
      *  + the GICD enable bit for its group is set
+     *  + its ACTIVE bit is not set (otherwise it would be Active+Pending)
      * Conveniently we can bulk-calculate this with bitwise operations.
      */
     uint32_t pend, grpmask;
@@ -63,9 +64,11 @@ static uint32_t gicd_int_pending(GICv3State *s, int irq)
     uint32_t group = *gic_bmp_ptr32(s->group, irq);
     uint32_t grpmod = *gic_bmp_ptr32(s->grpmod, irq);
     uint32_t enable = *gic_bmp_ptr32(s->enabled, irq);
+    uint32_t active = *gic_bmp_ptr32(s->active, irq);
 
     pend = pending | (~edge_trigger & level);
     pend &= enable;
+    pend &= ~active;
 
     if (s->gicd_ctlr & GICD_CTLR_DS) {
         grpmod = 0;
@@ -96,12 +99,14 @@ static uint32_t gicr_int_pending(GICv3CPUState *cs)
      *  + the PENDING latch is set OR it is level triggered and the input is 1
      *  + its ENABLE bit is set
      *  + the GICD enable bit for its group is set
+     *  + its ACTIVE bit is not set (otherwise it would be Active+Pending)
      * Conveniently we can bulk-calculate this with bitwise operations.
      */
     uint32_t pend, grpmask, grpmod;
 
     pend = cs->gicr_ipendr0 | (~cs->edge_trigger & cs->level);
     pend &= cs->gicr_ienabler0;
+    pend &= ~cs->gicr_iactiver0;
 
     if (cs->gic->gicd_ctlr & GICD_CTLR_DS) {
         grpmod = 0;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] hw/intc/arm_gicv3: Don't signal Pending+Active interrupts to CPU

[Edgar E. Iglesias]

On Tue, Dec 06, 2016 at 05:46:19PM +0000, Peter Maydell wrote:
> The GICv3 requires that we only signal Pending interrupts to
> the CPU. This category does not include Pending+Active interrupts,
> which means we need to check whether the interrupt is Active in
> the gicr_int_pending() and gicd_int_pending() functions.
> 
> Interrupts are rarely in the Active+Pending state, but KVM
> uses this as part of its handling of the virtual timer, so
> this bug was causing KVM to go into an infinite loop of
> taking the vtimer interrupt when the guest first triggered it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gicv3.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> index 8a6c647..f0c967b 100644
> --- a/hw/intc/arm_gicv3.c
> +++ b/hw/intc/arm_gicv3.c
> @@ -54,6 +54,7 @@ static uint32_t gicd_int_pending(GICv3State *s, int irq)
>       *  + the PENDING latch is set OR it is level triggered and the input is 1
>       *  + its ENABLE bit is set
>       *  + the GICD enable bit for its group is set
> +     *  + its ACTIVE bit is not set (otherwise it would be Active+Pending)
>       * Conveniently we can bulk-calculate this with bitwise operations.
>       */
>      uint32_t pend, grpmask;
> @@ -63,9 +64,11 @@ static uint32_t gicd_int_pending(GICv3State *s, int irq)
>      uint32_t group = *gic_bmp_ptr32(s->group, irq);
>      uint32_t grpmod = *gic_bmp_ptr32(s->grpmod, irq);
>      uint32_t enable = *gic_bmp_ptr32(s->enabled, irq);
> +    uint32_t active = *gic_bmp_ptr32(s->active, irq);
>  
>      pend = pending | (~edge_trigger & level);
>      pend &= enable;
> +    pend &= ~active;
>  
>      if (s->gicd_ctlr & GICD_CTLR_DS) {
>          grpmod = 0;
> @@ -96,12 +99,14 @@ static uint32_t gicr_int_pending(GICv3CPUState *cs)
>       *  + the PENDING latch is set OR it is level triggered and the input is 1
>       *  + its ENABLE bit is set
>       *  + the GICD enable bit for its group is set
> +     *  + its ACTIVE bit is not set (otherwise it would be Active+Pending)
>       * Conveniently we can bulk-calculate this with bitwise operations.
>       */
>      uint32_t pend, grpmask, grpmod;
>  
>      pend = cs->gicr_ipendr0 | (~cs->edge_trigger & cs->level);
>      pend &= cs->gicr_ienabler0;
> +    pend &= ~cs->gicr_iactiver0;
>  
>      if (cs->gic->gicd_ctlr & GICD_CTLR_DS) {
>          grpmod = 0;
> -- 
> 2.7.4
>