* [Qemu-arm] [PATCH 0/3] GICv3 bugfixes (preliminary for virt) @ 2016-12-06 17:46 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2016-12-06 17:46 UTC (permalink / raw) To: qemu-arm, qemu-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 0/3] GICv3 bugfixes (preliminary for virt) @ 2016-12-06 17:46 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2016-12-06 17:46 UTC (permalink / raw) To: qemu-arm, qemu-devel; +Cc: Edgar E. Iglesias 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-arm] [PATCH 1/3] target-arm: Log AArch64 exception returns 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell @ 2016-12-06 17:46 ` Peter Maydell -1 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2016-12-06 17:46 UTC (permalink / raw) To: qemu-arm, qemu-devel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 1/3] target-arm: Log AArch64 exception returns @ 2016-12-06 17:46 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2016-12-06 17:46 UTC (permalink / raw) To: qemu-arm, qemu-devel; +Cc: Edgar E. Iglesias 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-arm] [PATCH 1/3] target-arm: Log AArch64 exception returns 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell @ 2016-12-07 21:01 ` Edgar E. Iglesias -1 siblings, 0 replies; 14+ messages in thread From: Edgar E. Iglesias @ 2016-12-07 21:01 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] target-arm: Log AArch64 exception returns @ 2016-12-07 21:01 ` Edgar E. Iglesias 0 siblings, 0 replies; 14+ messages in thread From: Edgar E. Iglesias @ 2016-12-07 21:01 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-arm] [PATCH 2/3] hw/intc/arm_gicv3: Remove incorrect usage of fieldoffset 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell @ 2016-12-06 17:46 ` Peter Maydell -1 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2016-12-06 17:46 UTC (permalink / raw) To: qemu-arm, qemu-devel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3: Remove incorrect usage of fieldoffset @ 2016-12-06 17:46 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2016-12-06 17:46 UTC (permalink / raw) To: qemu-arm, qemu-devel; +Cc: Edgar E. Iglesias 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-arm] [PATCH 2/3] hw/intc/arm_gicv3: Remove incorrect usage of fieldoffset 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell @ 2016-12-07 21:02 ` Edgar E. Iglesias -1 siblings, 0 replies; 14+ messages in thread From: Edgar E. Iglesias @ 2016-12-07 21:02 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/intc/arm_gicv3: Remove incorrect usage of fieldoffset @ 2016-12-07 21:02 ` Edgar E. Iglesias 0 siblings, 0 replies; 14+ messages in thread From: Edgar E. Iglesias @ 2016-12-07 21:02 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-arm] [PATCH 3/3] hw/intc/arm_gicv3: Don't signal Pending+Active interrupts to CPU 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell @ 2016-12-06 17:46 ` Peter Maydell -1 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2016-12-06 17:46 UTC (permalink / raw) To: qemu-arm, qemu-devel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 3/3] hw/intc/arm_gicv3: Don't signal Pending+Active interrupts to CPU @ 2016-12-06 17:46 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2016-12-06 17:46 UTC (permalink / raw) To: qemu-arm, qemu-devel; +Cc: Edgar E. Iglesias 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-arm] [PATCH 3/3] hw/intc/arm_gicv3: Don't signal Pending+Active interrupts to CPU 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell @ 2016-12-07 22:46 ` Edgar E. Iglesias -1 siblings, 0 replies; 14+ messages in thread From: Edgar E. Iglesias @ 2016-12-07 22:46 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] hw/intc/arm_gicv3: Don't signal Pending+Active interrupts to CPU @ 2016-12-07 22:46 ` Edgar E. Iglesias 0 siblings, 0 replies; 14+ messages in thread From: Edgar E. Iglesias @ 2016-12-07 22:46 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-arm, qemu-devel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-12-07 22:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-06 17:46 [Qemu-arm] [PATCH 0/3] GICv3 bugfixes (preliminary for virt) Peter Maydell 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell 2016-12-06 17:46 ` [Qemu-arm] [PATCH 1/3] target-arm: Log AArch64 exception returns Peter Maydell 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell 2016-12-07 21:01 ` [Qemu-arm] " Edgar E. Iglesias 2016-12-07 21:01 ` [Qemu-devel] " Edgar E. Iglesias 2016-12-06 17:46 ` [Qemu-arm] [PATCH 2/3] hw/intc/arm_gicv3: Remove incorrect usage of fieldoffset Peter Maydell 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell 2016-12-07 21:02 ` [Qemu-arm] " Edgar E. Iglesias 2016-12-07 21:02 ` [Qemu-devel] " Edgar E. Iglesias 2016-12-06 17:46 ` [Qemu-arm] [PATCH 3/3] hw/intc/arm_gicv3: Don't signal Pending+Active interrupts to CPU Peter Maydell 2016-12-06 17:46 ` [Qemu-devel] " Peter Maydell 2016-12-07 22:46 ` [Qemu-arm] " Edgar E. Iglesias 2016-12-07 22:46 ` [Qemu-devel] " Edgar E. Iglesias
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.