* [Qemu-devel] [PATCH v5 0/2] Fix ARM KVM GICv3 get/put data shift bug
@ 2018-05-31 3:15 Shannon Zhao
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR Shannon Zhao
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR Shannon Zhao
0 siblings, 2 replies; 10+ messages in thread
From: Shannon Zhao @ 2018-05-31 3:15 UTC (permalink / raw)
To: qemu-arm
Cc: peter.maydell, eric.auger, qemu-devel, shannon.zhaosl, Shannon Zhao
These two patches fix ARM KVM GICv3 get/put data shift bug and add
compatibility fro migration from old qemu to new one.
Major Changes in V5:
* Add detailed comment as suggested by Peter
* Use memmove instead of memcpy
* Delete .need callback
* set gicd_no_migration_shift_bug to true after move the data
Shannon Zhao (2):
arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by
GICR_IPRIORITYR
arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR
hw/intc/arm_gicv3_common.c | 79 ++++++++++++++++++++++++++++++++++++++
hw/intc/arm_gicv3_kvm.c | 56 ++++++++++++++++++++++++++-
include/hw/intc/arm_gicv3_common.h | 1 +
3 files changed, 134 insertions(+), 2 deletions(-)
--
2.0.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR
2018-05-31 3:15 [Qemu-devel] [PATCH v5 0/2] Fix ARM KVM GICv3 get/put data shift bug Shannon Zhao
@ 2018-05-31 3:15 ` Shannon Zhao
2018-05-31 11:01 ` Auger Eric
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR Shannon Zhao
1 sibling, 1 reply; 10+ messages in thread
From: Shannon Zhao @ 2018-05-31 3:15 UTC (permalink / raw)
To: qemu-arm
Cc: peter.maydell, eric.auger, qemu-devel, shannon.zhaosl,
Shannon Zhao, qemu-stable
While for_each_dist_irq_reg loop starts from GIC_INTERNAL, it forgot to
offset the date array and index. This will overlap the GICR registers
value and leave the last GIC_INTERNAL irq's registers out of update.
Fixes: 367b9f527becdd20ddf116e17a3c0c2bbc486920
Cc: qemu-stable@nongnu.org
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
---
hw/intc/arm_gicv3_kvm.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 3536795..147e691 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -135,7 +135,14 @@ static void kvm_dist_get_priority(GICv3State *s, uint32_t offset, uint8_t *bmp)
uint32_t reg, *field;
int irq;
- field = (uint32_t *)bmp;
+ /* For the KVM GICv3, affinity routing is always enabled, and the first 8
+ * GICD_IPRIORITYR<n> registers are always RAZ/WI. The corresponding
+ * functionality is replaced by GICR_IPRIORITYR<n>. It doesn't need to
+ * sync them. So it needs to skip the field of GIC_INTERNAL irqs in bmp and
+ * offset.
+ */
+ field = (uint32_t *)(bmp + GIC_INTERNAL);
+ offset += (GIC_INTERNAL * 8) / 8;
for_each_dist_irq_reg(irq, s->num_irq, 8) {
kvm_gicd_access(s, offset, ®, false);
*field = reg;
@@ -149,7 +156,14 @@ static void kvm_dist_put_priority(GICv3State *s, uint32_t offset, uint8_t *bmp)
uint32_t reg, *field;
int irq;
- field = (uint32_t *)bmp;
+ /* For the KVM GICv3, affinity routing is always enabled, and the first 8
+ * GICD_IPRIORITYR<n> registers are always RAZ/WI. The corresponding
+ * functionality is replaced by GICR_IPRIORITYR<n>. It doesn't need to
+ * sync them. So it needs to skip the field of GIC_INTERNAL irqs in bmp and
+ * offset.
+ */
+ field = (uint32_t *)(bmp + GIC_INTERNAL);
+ offset += (GIC_INTERNAL * 8) / 8;
for_each_dist_irq_reg(irq, s->num_irq, 8) {
reg = *field;
kvm_gicd_access(s, offset, ®, true);
--
2.0.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR
2018-05-31 3:15 [Qemu-devel] [PATCH v5 0/2] Fix ARM KVM GICv3 get/put data shift bug Shannon Zhao
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR Shannon Zhao
@ 2018-05-31 3:15 ` Shannon Zhao
2018-05-31 11:32 ` Auger Eric
2018-05-31 13:50 ` Peter Maydell
1 sibling, 2 replies; 10+ messages in thread
From: Shannon Zhao @ 2018-05-31 3:15 UTC (permalink / raw)
To: qemu-arm
Cc: peter.maydell, eric.auger, qemu-devel, shannon.zhaosl,
Shannon Zhao, qemu-stable
While we skip the GIC_INTERNAL irqs, we don't change the register offset
accordingly. This will overlap the GICR registers value and leave the
last GIC_INTERNAL irq's registers out of update.
Fix this by skipping the registers banked by GICR.
Also for migration compatibility if the migration source (old version
qemu) doesn't send gicd_no_migration_shift_bug = 1 to destination, then
we shift the data of PPI to get the right data for SPI.
Fixes: 367b9f527becdd20ddf116e17a3c0c2bbc486920
Cc: qemu-stable@nongnu.org
Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
---
hw/intc/arm_gicv3_common.c | 79 ++++++++++++++++++++++++++++++++++++++
hw/intc/arm_gicv3_kvm.c | 38 ++++++++++++++++++
include/hw/intc/arm_gicv3_common.h | 1 +
3 files changed, 118 insertions(+)
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 7b54d52..68211a2 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -27,6 +27,7 @@
#include "hw/intc/arm_gicv3_common.h"
#include "gicv3_internal.h"
#include "hw/arm/linux-boot-if.h"
+#include "sysemu/kvm.h"
static int gicv3_pre_save(void *opaque)
{
@@ -141,6 +142,79 @@ static const VMStateDescription vmstate_gicv3_cpu = {
}
};
+static int gicv3_gicd_no_migration_shift_bug_pre_load(void *opaque)
+{
+ GICv3State *cs = opaque;
+
+ /*
+ * The gicd_no_migration_shift_bug flag is used for migration compatibilty
+ * for old version QEMU which may have the GICD bmp shift bug under KVM mode.
+ * Strictly, what we want to know is whether the migration source is using
+ * KVM. Since we don't have any way to determine that, we look at whether the
+ * destination is using KVM; this is close enough because for the older QEMU
+ * versions with this bug KVM -> TCG migration didn't work anyway. If the
+ * source is a newer QEMU without this bug it will transmit the migration
+ * subsection which sets the flag to true; otherwise it will remain set to
+ * the value we select here.
+ */
+ if (kvm_enabled()) {
+ cs->gicd_no_migration_shift_bug = false;
+ }
+
+ return 0;
+}
+
+static int gicv3_gicd_no_migration_shift_bug_post_load(void *opaque,
+ int version_id)
+{
+ GICv3State *cs = opaque;
+
+ if (cs->gicd_no_migration_shift_bug) {
+ return 0;
+ }
+
+ /* Older versions of QEMU had a bug in the handling of state save/restore
+ * to the KVM GICv3: they got the offset in the bitmap arrays wrong,
+ * so that instead of the data for external interrupts 32 and up
+ * starting at bit position 32 in the bitmap, it started at bit
+ * position 64. If we're receiving data from a QEMU with that bug,
+ * we must move the data down into the right place.
+ */
+ memmove(cs->group, (uint8_t *)cs->group + GIC_INTERNAL / 8,
+ sizeof(cs->group) - GIC_INTERNAL / 8);
+ memmove(cs->grpmod, (uint8_t *)cs->grpmod + GIC_INTERNAL / 8,
+ sizeof(cs->grpmod) - GIC_INTERNAL / 8);
+ memmove(cs->enabled, (uint8_t *)cs->enabled + GIC_INTERNAL / 8,
+ sizeof(cs->enabled) - GIC_INTERNAL / 8);
+ memmove(cs->pending, (uint8_t *)cs->pending + GIC_INTERNAL / 8,
+ sizeof(cs->pending) - GIC_INTERNAL / 8);
+ memmove(cs->active, (uint8_t *)cs->active + GIC_INTERNAL / 8,
+ sizeof(cs->active) - GIC_INTERNAL / 8);
+ memmove(cs->edge_trigger, (uint8_t *)cs->edge_trigger + GIC_INTERNAL / 8,
+ sizeof(cs->edge_trigger) - GIC_INTERNAL / 8);
+
+ /*
+ * While this new version QEMU doesn't have this kind of bug as we fix it,
+ * so it's need to set the flag to true to indicate that and it's neccessary
+ * for next migration to work from this new version QEMU.
+ */
+ cs->gicd_no_migration_shift_bug = true;
+
+ return 0;
+}
+
+const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = {
+ .name = "arm_gicv3/gicd_no_migration_shift_bug",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .pre_load = gicv3_gicd_no_migration_shift_bug_pre_load,
+ .post_load = gicv3_gicd_no_migration_shift_bug_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_BOOL(gicd_no_migration_shift_bug, GICv3State),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_gicv3 = {
.name = "arm_gicv3",
.version_id = 1,
@@ -165,6 +239,10 @@ static const VMStateDescription vmstate_gicv3 = {
VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, GICv3State, num_cpu,
vmstate_gicv3_cpu, GICv3CPUState),
VMSTATE_END_OF_LIST()
+ },
+ .subsections = (const VMStateDescription * []) {
+ &vmstate_gicv3_gicd_no_migration_shift_bug,
+ NULL
}
};
@@ -364,6 +442,7 @@ static void arm_gicv3_common_reset(DeviceState *dev)
gicv3_gicd_group_set(s, i);
}
}
+ s->gicd_no_migration_shift_bug = true;
}
static void arm_gic_common_linux_init(ARMLinuxBootIf *obj,
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 147e691..001d82b 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -178,6 +178,14 @@ static void kvm_dist_get_edge_trigger(GICv3State *s, uint32_t offset,
uint32_t reg;
int irq;
+ /* For the KVM GICv3, affinity routing is always enabled, and the first 2
+ * GICD_ICFGR<n> registers are always RAZ/WI. The corresponding
+ * functionality is replaced by GICR_ICFGR<n>. It doesn't need to sync
+ * them. So it should increase the offset to skip GIC_INTERNAL irqs.
+ * This matches the for_each_dist_irq_reg() macro which also skips the
+ * first GIC_INTERNAL irqs.
+ */
+ offset += (GIC_INTERNAL * 2) / 8;
for_each_dist_irq_reg(irq, s->num_irq, 2) {
kvm_gicd_access(s, offset, ®, false);
reg = half_unshuffle32(reg >> 1);
@@ -195,6 +203,14 @@ static void kvm_dist_put_edge_trigger(GICv3State *s, uint32_t offset,
uint32_t reg;
int irq;
+ /* For the KVM GICv3, affinity routing is always enabled, and the first 2
+ * GICD_ICFGR<n> registers are always RAZ/WI. The corresponding
+ * functionality is replaced by GICR_ICFGR<n>. It doesn't need to sync
+ * them. So it should increase the offset to skip GIC_INTERNAL irqs.
+ * This matches the for_each_dist_irq_reg() macro which also skips the
+ * first GIC_INTERNAL irqs.
+ */
+ offset += (GIC_INTERNAL * 2) / 8;
for_each_dist_irq_reg(irq, s->num_irq, 2) {
reg = *gic_bmp_ptr32(bmp, irq);
if (irq % 32 != 0) {
@@ -236,6 +252,15 @@ static void kvm_dist_getbmp(GICv3State *s, uint32_t offset, uint32_t *bmp)
uint32_t reg;
int irq;
+ /* For the KVM GICv3, affinity routing is always enabled, and the
+ * GICD_IGROUPR0/GICD_IGRPMODR0/GICD_ISENABLER0/GICD_ISPENDR0/
+ * GICD_ISACTIVER0 registers are always RAZ/WI. The corresponding
+ * functionality is replaced by the GICR registers. It doesn't need to sync
+ * them. So it should increase the offset to skip GIC_INTERNAL irqs.
+ * This matches the for_each_dist_irq_reg() macro which also skips the
+ * first GIC_INTERNAL irqs.
+ */
+ offset += (GIC_INTERNAL * 1) / 8;
for_each_dist_irq_reg(irq, s->num_irq, 1) {
kvm_gicd_access(s, offset, ®, false);
*gic_bmp_ptr32(bmp, irq) = reg;
@@ -249,6 +274,19 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
uint32_t reg;
int irq;
+ /* For the KVM GICv3, affinity routing is always enabled, and the
+ * GICD_IGROUPR0/GICD_IGRPMODR0/GICD_ISENABLER0/GICD_ISPENDR0/
+ * GICD_ISACTIVER0 registers are always RAZ/WI. The corresponding
+ * functionality is replaced by the GICR registers. It doesn't need to sync
+ * them. So it should increase the offset to skip GIC_INTERNAL irqs.
+ * This matches the for_each_dist_irq_reg() macro which also skips the
+ * first GIC_INTERNAL irqs.
+ */
+ offset += (GIC_INTERNAL * 1) / 8;
+ if (clroffset != 0) {
+ clroffset += (1 * sizeof(uint32_t));
+ }
+
for_each_dist_irq_reg(irq, s->num_irq, 1) {
/* If this bitmap is a set/clear register pair, first write to the
* clear-reg to clear all bits before using the set-reg to write
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index bccdfe1..d75b49d 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -217,6 +217,7 @@ struct GICv3State {
uint32_t revision;
bool security_extn;
bool irq_reset_nonsecure;
+ bool gicd_no_migration_shift_bug;
int dev_fd; /* kvm device fd if backed by kvm vgic support */
Error *migration_blocker;
--
2.0.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR Shannon Zhao
@ 2018-05-31 11:01 ` Auger Eric
2018-06-11 8:32 ` Shannon Zhao
0 siblings, 1 reply; 10+ messages in thread
From: Auger Eric @ 2018-05-31 11:01 UTC (permalink / raw)
To: Shannon Zhao, qemu-arm
Cc: peter.maydell, qemu-devel, shannon.zhaosl, qemu-stable
Hi,
On 05/31/2018 05:15 AM, Shannon Zhao wrote:
> While for_each_dist_irq_reg loop starts from GIC_INTERNAL, it forgot to
> offset the date array and index. This will overlap the GICR registers
> value and leave the last GIC_INTERNAL irq's registers out of update.
>
> Fixes: 367b9f527becdd20ddf116e17a3c0c2bbc486920
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> ---
> hw/intc/arm_gicv3_kvm.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 3536795..147e691 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -135,7 +135,14 @@ static void kvm_dist_get_priority(GICv3State *s, uint32_t offset, uint8_t *bmp)
> uint32_t reg, *field;
> int irq;
>
> - field = (uint32_t *)bmp;
> + /* For the KVM GICv3, affinity routing is always enabled, and the first 8
> + * GICD_IPRIORITYR<n> registers are always RAZ/WI. The corresponding
> + * functionality is replaced by GICR_IPRIORITYR<n>. It doesn't need to
> + * sync them. So it needs to skip the field of GIC_INTERNAL irqs in bmp and
> + * offset.
> + */
> + field = (uint32_t *)(bmp + GIC_INTERNAL);
> + offset += (GIC_INTERNAL * 8) / 8;
> for_each_dist_irq_reg(irq, s->num_irq, 8) {
> kvm_gicd_access(s, offset, ®, false);
> *field = reg;
> @@ -149,7 +156,14 @@ static void kvm_dist_put_priority(GICv3State *s, uint32_t offset, uint8_t *bmp)
> uint32_t reg, *field;
> int irq;
>
> - field = (uint32_t *)bmp;
> + /* For the KVM GICv3, affinity routing is always enabled, and the first 8
> + * GICD_IPRIORITYR<n> registers are always RAZ/WI. The corresponding
> + * functionality is replaced by GICR_IPRIORITYR<n>. It doesn't need to
> + * sync them. So it needs to skip the field of GIC_INTERNAL irqs in bmp and
> + * offset.
> + */
> + field = (uint32_t *)(bmp + GIC_INTERNAL);
> + offset += (GIC_INTERNAL * 8) / 8;
> for_each_dist_irq_reg(irq, s->num_irq, 8) {
> reg = *field;
> kvm_gicd_access(s, offset, ®, true);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR Shannon Zhao
@ 2018-05-31 11:32 ` Auger Eric
2018-05-31 13:50 ` Peter Maydell
1 sibling, 0 replies; 10+ messages in thread
From: Auger Eric @ 2018-05-31 11:32 UTC (permalink / raw)
To: Shannon Zhao, qemu-arm
Cc: peter.maydell, qemu-stable, qemu-devel, shannon.zhaosl
Hi,
On 05/31/2018 05:15 AM, Shannon Zhao wrote:
> While we skip the GIC_INTERNAL irqs, we don't change the register offset
> accordingly. This will overlap the GICR registers value and leave the
> last GIC_INTERNAL irq's registers out of update.
>
> Fix this by skipping the registers banked by GICR.
>
> Also for migration compatibility if the migration source (old version
> qemu) doesn't send gicd_no_migration_shift_bug = 1 to destination, then
> we shift the data of PPI to get the right data for SPI.
>
> Fixes: 367b9f527becdd20ddf116e17a3c0c2bbc486920
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> ---
> hw/intc/arm_gicv3_common.c | 79 ++++++++++++++++++++++++++++++++++++++
> hw/intc/arm_gicv3_kvm.c | 38 ++++++++++++++++++
> include/hw/intc/arm_gicv3_common.h | 1 +
> 3 files changed, 118 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 7b54d52..68211a2 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -27,6 +27,7 @@
> #include "hw/intc/arm_gicv3_common.h"
> #include "gicv3_internal.h"
> #include "hw/arm/linux-boot-if.h"
> +#include "sysemu/kvm.h"
>
> static int gicv3_pre_save(void *opaque)
> {
> @@ -141,6 +142,79 @@ static const VMStateDescription vmstate_gicv3_cpu = {
> }
> };
>
> +static int gicv3_gicd_no_migration_shift_bug_pre_load(void *opaque)
> +{
> + GICv3State *cs = opaque;
> +
> + /*
> + * The gicd_no_migration_shift_bug flag is used for migration compatibilty
nit: compatibility
> + * for old version QEMU which may have the GICD bmp shift bug under KVM mode.
> + * Strictly, what we want to know is whether the migration source is using
> + * KVM. Since we don't have any way to determine that, we look at whether the
> + * destination is using KVM; this is close enough because for the older QEMU
> + * versions with this bug KVM -> TCG migration didn't work anyway. If the
> + * source is a newer QEMU without this bug it will transmit the migration
> + * subsection which sets the flag to true; otherwise it will remain set to
> + * the value we select here.
> + */
> + if (kvm_enabled()) {
> + cs->gicd_no_migration_shift_bug = false;
> + }
> +
> + return 0;
> +}
> +
> +static int gicv3_gicd_no_migration_shift_bug_post_load(void *opaque,
> + int version_id)
> +{
> + GICv3State *cs = opaque;
> +
> + if (cs->gicd_no_migration_shift_bug) {
> + return 0;
> + }
> +
> + /* Older versions of QEMU had a bug in the handling of state save/restore
> + * to the KVM GICv3: they got the offset in the bitmap arrays wrong,
> + * so that instead of the data for external interrupts 32 and up
> + * starting at bit position 32 in the bitmap, it started at bit
> + * position 64. If we're receiving data from a QEMU with that bug,
> + * we must move the data down into the right place.
> + */
> + memmove(cs->group, (uint8_t *)cs->group + GIC_INTERNAL / 8,
> + sizeof(cs->group) - GIC_INTERNAL / 8);
> + memmove(cs->grpmod, (uint8_t *)cs->grpmod + GIC_INTERNAL / 8,
> + sizeof(cs->grpmod) - GIC_INTERNAL / 8);
> + memmove(cs->enabled, (uint8_t *)cs->enabled + GIC_INTERNAL / 8,
> + sizeof(cs->enabled) - GIC_INTERNAL / 8);
> + memmove(cs->pending, (uint8_t *)cs->pending + GIC_INTERNAL / 8,
> + sizeof(cs->pending) - GIC_INTERNAL / 8);
> + memmove(cs->active, (uint8_t *)cs->active + GIC_INTERNAL / 8,
> + sizeof(cs->active) - GIC_INTERNAL / 8);
> + memmove(cs->edge_trigger, (uint8_t *)cs->edge_trigger + GIC_INTERNAL / 8,
> + sizeof(cs->edge_trigger) - GIC_INTERNAL / 8);
> +
> + /*
> + * While this new version QEMU doesn't have this kind of bug as we fix it,
> + * so it's need to set the flag to true to indicate that and it's neccessary
nit: it needs, necessary
> + * for next migration to work from this new version QEMU.
> + */
> + cs->gicd_no_migration_shift_bug = true;
> +
> + return 0;
> +}
> +
> +const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = {
> + .name = "arm_gicv3/gicd_no_migration_shift_bug",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .pre_load = gicv3_gicd_no_migration_shift_bug_pre_load,
> + .post_load = gicv3_gicd_no_migration_shift_bug_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_BOOL(gicd_no_migration_shift_bug, GICv3State),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_gicv3 = {
> .name = "arm_gicv3",
> .version_id = 1,
> @@ -165,6 +239,10 @@ static const VMStateDescription vmstate_gicv3 = {
> VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, GICv3State, num_cpu,
> vmstate_gicv3_cpu, GICv3CPUState),
> VMSTATE_END_OF_LIST()
> + },
> + .subsections = (const VMStateDescription * []) {
> + &vmstate_gicv3_gicd_no_migration_shift_bug,
> + NULL
> }
> };
>
> @@ -364,6 +442,7 @@ static void arm_gicv3_common_reset(DeviceState *dev)
> gicv3_gicd_group_set(s, i);
> }
> }
> + s->gicd_no_migration_shift_bug = true;
> }
>
> static void arm_gic_common_linux_init(ARMLinuxBootIf *obj,
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 147e691..001d82b 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -178,6 +178,14 @@ static void kvm_dist_get_edge_trigger(GICv3State *s, uint32_t offset,
> uint32_t reg;
> int irq;
>
> + /* For the KVM GICv3, affinity routing is always enabled, and the first 2
> + * GICD_ICFGR<n> registers are always RAZ/WI. The corresponding
> + * functionality is replaced by GICR_ICFGR<n>. It doesn't need to sync
> + * them. So it should increase the offset to skip GIC_INTERNAL irqs.
> + * This matches the for_each_dist_irq_reg() macro which also skips the
> + * first GIC_INTERNAL irqs.
> + */
> + offset += (GIC_INTERNAL * 2) / 8;
> for_each_dist_irq_reg(irq, s->num_irq, 2) {
> kvm_gicd_access(s, offset, ®, false);
> reg = half_unshuffle32(reg >> 1);
> @@ -195,6 +203,14 @@ static void kvm_dist_put_edge_trigger(GICv3State *s, uint32_t offset,
> uint32_t reg;
> int irq;
>
> + /* For the KVM GICv3, affinity routing is always enabled, and the first 2
> + * GICD_ICFGR<n> registers are always RAZ/WI. The corresponding
> + * functionality is replaced by GICR_ICFGR<n>. It doesn't need to sync
> + * them. So it should increase the offset to skip GIC_INTERNAL irqs.
> + * This matches the for_each_dist_irq_reg() macro which also skips the
> + * first GIC_INTERNAL irqs.
> + */
> + offset += (GIC_INTERNAL * 2) / 8;
> for_each_dist_irq_reg(irq, s->num_irq, 2) {
> reg = *gic_bmp_ptr32(bmp, irq);
> if (irq % 32 != 0) {
> @@ -236,6 +252,15 @@ static void kvm_dist_getbmp(GICv3State *s, uint32_t offset, uint32_t *bmp)
> uint32_t reg;
> int irq;
>
> + /* For the KVM GICv3, affinity routing is always enabled, and the
> + * GICD_IGROUPR0/GICD_IGRPMODR0/GICD_ISENABLER0/GICD_ISPENDR0/
> + * GICD_ISACTIVER0 registers are always RAZ/WI. The corresponding
> + * functionality is replaced by the GICR registers. It doesn't need to sync
> + * them. So it should increase the offset to skip GIC_INTERNAL irqs.
> + * This matches the for_each_dist_irq_reg() macro which also skips the
> + * first GIC_INTERNAL irqs.
> + */
> + offset += (GIC_INTERNAL * 1) / 8;
> for_each_dist_irq_reg(irq, s->num_irq, 1) {
> kvm_gicd_access(s, offset, ®, false);
> *gic_bmp_ptr32(bmp, irq) = reg;
> @@ -249,6 +274,19 @@ static void kvm_dist_putbmp(GICv3State *s, uint32_t offset,
> uint32_t reg;
> int irq;
>
> + /* For the KVM GICv3, affinity routing is always enabled, and the
> + * GICD_IGROUPR0/GICD_IGRPMODR0/GICD_ISENABLER0/GICD_ISPENDR0/
> + * GICD_ISACTIVER0 registers are always RAZ/WI. The corresponding
> + * functionality is replaced by the GICR registers. It doesn't need to sync
> + * them. So it should increase the offset to skip GIC_INTERNAL irqs.
> + * This matches the for_each_dist_irq_reg() macro which also skips the
> + * first GIC_INTERNAL irqs.
> + */
> + offset += (GIC_INTERNAL * 1) / 8;
> + if (clroffset != 0) {
> + clroffset += (1 * sizeof(uint32_t));
> + }
> +
> for_each_dist_irq_reg(irq, s->num_irq, 1) {
> /* If this bitmap is a set/clear register pair, first write to the
> * clear-reg to clear all bits before using the set-reg to write
> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
> index bccdfe1..d75b49d 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -217,6 +217,7 @@ struct GICv3State {
> uint32_t revision;
> bool security_extn;
> bool irq_reset_nonsecure;
> + bool gicd_no_migration_shift_bug;
>
> int dev_fd; /* kvm device fd if backed by kvm vgic support */
> Error *migration_blocker;
>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR Shannon Zhao
2018-05-31 11:32 ` Auger Eric
@ 2018-05-31 13:50 ` Peter Maydell
2018-05-31 14:36 ` Shannon Zhao
1 sibling, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2018-05-31 13:50 UTC (permalink / raw)
To: Shannon Zhao
Cc: qemu-arm, Eric Auger, QEMU Developers, Shannon Zhao, qemu-stable
On 31 May 2018 at 04:15, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> While we skip the GIC_INTERNAL irqs, we don't change the register offset
> accordingly. This will overlap the GICR registers value and leave the
> last GIC_INTERNAL irq's registers out of update.
>
> Fix this by skipping the registers banked by GICR.
>
> Also for migration compatibility if the migration source (old version
> qemu) doesn't send gicd_no_migration_shift_bug = 1 to destination, then
> we shift the data of PPI to get the right data for SPI.
>
> Fixes: 367b9f527becdd20ddf116e17a3c0c2bbc486920
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> + /*
> + * While this new version QEMU doesn't have this kind of bug as we fix it,
> + * so it's need to set the flag to true to indicate that and it's neccessary
> + * for next migration to work from this new version QEMU.
> + */
> + cs->gicd_no_migration_shift_bug = true;
Nice catch; I would have forgotten that we needed to do this...
>
> + /* For the KVM GICv3, affinity routing is always enabled, and the
> + * GICD_IGROUPR0/GICD_IGRPMODR0/GICD_ISENABLER0/GICD_ISPENDR0/
> + * GICD_ISACTIVER0 registers are always RAZ/WI. The corresponding
> + * functionality is replaced by the GICR registers. It doesn't need to sync
> + * them. So it should increase the offset to skip GIC_INTERNAL irqs.
> + * This matches the for_each_dist_irq_reg() macro which also skips the
> + * first GIC_INTERNAL irqs.
> + */
> + offset += (GIC_INTERNAL * 1) / 8;
> + if (clroffset != 0) {
> + clroffset += (1 * sizeof(uint32_t));
> + }
> +
Shouldn't we be adding the same thing to clroffset that we add to offset ?
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR
2018-05-31 13:50 ` Peter Maydell
@ 2018-05-31 14:36 ` Shannon Zhao
2018-05-31 14:56 ` Peter Maydell
0 siblings, 1 reply; 10+ messages in thread
From: Shannon Zhao @ 2018-05-31 14:36 UTC (permalink / raw)
To: Peter Maydell
Cc: Shannon Zhao, qemu-arm, Eric Auger, QEMU Developers, qemu-stable
2018-05-31 21:50 GMT+08:00 Peter Maydell <peter.maydell@linaro.org>:
> On 31 May 2018 at 04:15, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> > While we skip the GIC_INTERNAL irqs, we don't change the register offset
> > accordingly. This will overlap the GICR registers value and leave the
> > last GIC_INTERNAL irq's registers out of update.
> >
> > Fix this by skipping the registers banked by GICR.
> >
> > Also for migration compatibility if the migration source (old version
> > qemu) doesn't send gicd_no_migration_shift_bug = 1 to destination, then
> > we shift the data of PPI to get the right data for SPI.
> >
> > Fixes: 367b9f527becdd20ddf116e17a3c0c2bbc486920
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>
> > + /*
> > + * While this new version QEMU doesn't have this kind of bug as we
> fix it,
> > + * so it's need to set the flag to true to indicate that and it's
> neccessary
> > + * for next migration to work from this new version QEMU.
> > + */
> > + cs->gicd_no_migration_shift_bug = true;
>
> Nice catch; I would have forgotten that we needed to do this...
>
>
> >
> > + /* For the KVM GICv3, affinity routing is always enabled, and the
> > + * GICD_IGROUPR0/GICD_IGRPMODR0/GICD_ISENABLER0/GICD_ISPENDR0/
> > + * GICD_ISACTIVER0 registers are always RAZ/WI. The corresponding
> > + * functionality is replaced by the GICR registers. It doesn't need
> to sync
> > + * them. So it should increase the offset to skip GIC_INTERNAL irqs.
> > + * This matches the for_each_dist_irq_reg() macro which also skips
> the
> > + * first GIC_INTERNAL irqs.
> > + */
> > + offset += (GIC_INTERNAL * 1) / 8;
> > + if (clroffset != 0) {
> > + clroffset += (1 * sizeof(uint32_t));
> > + }
> > +
>
> Shouldn't we be adding the same thing to clroffset that we add to offset ?
>
> Yeah, right. It should like this
clroffset += (GIC_INTERNAL * 1) / 8;
I don't remember why I didn't write this way. Anyway, Could you please fix
this when you apply this patch or I send a new version?
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR
2018-05-31 14:36 ` Shannon Zhao
@ 2018-05-31 14:56 ` Peter Maydell
0 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2018-05-31 14:56 UTC (permalink / raw)
To: Shannon Zhao
Cc: Shannon Zhao, qemu-arm, Eric Auger, QEMU Developers, qemu-stable
On 31 May 2018 at 15:36, Shannon Zhao <shannon.zhaosl@gmail.com> wrote:
>
>
> 2018-05-31 21:50 GMT+08:00 Peter Maydell <peter.maydell@linaro.org>:
>> Shouldn't we be adding the same thing to clroffset that we add to offset ?
>>
>> Yeah, right. It should like this
>
> clroffset += (GIC_INTERNAL * 1) / 8;
>
> I don't remember why I didn't write this way. Anyway, Could you please fix
> this when you apply this patch or I send a new version?
It's probably easiest if you resend; you have Eric's comment
typos to fix too.
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR
2018-05-31 11:01 ` Auger Eric
@ 2018-06-11 8:32 ` Shannon Zhao
2018-06-11 13:08 ` Peter Maydell
0 siblings, 1 reply; 10+ messages in thread
From: Shannon Zhao @ 2018-06-11 8:32 UTC (permalink / raw)
To: Auger Eric, qemu-arm
Cc: peter.maydell, qemu-devel, shannon.zhaosl, qemu-stable
On 2018/5/31 19:01, Auger Eric wrote:
> Hi,
>
> On 05/31/2018 05:15 AM, Shannon Zhao wrote:
>> While for_each_dist_irq_reg loop starts from GIC_INTERNAL, it forgot to
>> offset the date array and index. This will overlap the GICR registers
>> value and leave the last GIC_INTERNAL irq's registers out of update.
>>
>> Fixes: 367b9f527becdd20ddf116e17a3c0c2bbc486920
>> Cc: qemu-stable@nongnu.org
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
Hi Peter,
Looks like we missed picking up this patch. I didn't include this in v6
since you and Eric both reviewed it.
Could you please pick it up?
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR
2018-06-11 8:32 ` Shannon Zhao
@ 2018-06-11 13:08 ` Peter Maydell
0 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2018-06-11 13:08 UTC (permalink / raw)
To: Shannon Zhao
Cc: Auger Eric, qemu-arm, QEMU Developers, Shannon Zhao, qemu-stable
On 11 June 2018 at 09:32, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> Hi Peter,
>
> Looks like we missed picking up this patch. I didn't include this in v6
> since you and Eric both reviewed it.
>
> Could you please pick it up?
Oops; added to target-arm.next. (I generally forget entirely about vN
of a patchset as soon as vN+1 appears on the list, so better not to
drop patches from a set unless I've specifically said I've put
them in target-arm.next or they're on the list as their own patchset.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-06-11 13:09 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-31 3:15 [Qemu-devel] [PATCH v5 0/2] Fix ARM KVM GICv3 get/put data shift bug Shannon Zhao
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR Shannon Zhao
2018-05-31 11:01 ` Auger Eric
2018-06-11 8:32 ` Shannon Zhao
2018-06-11 13:08 ` Peter Maydell
2018-05-31 3:15 ` [Qemu-devel] [PATCH v5 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR Shannon Zhao
2018-05-31 11:32 ` Auger Eric
2018-05-31 13:50 ` Peter Maydell
2018-05-31 14:36 ` Shannon Zhao
2018-05-31 14:56 ` Peter Maydell
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.