[PATCH 0/7] vgic fixes for 4.7-rc1

[PATCH 0/7] vgic fixes for 4.7-rc1

[Marc Zyngier]

The dust has not yet settled on the new vgic, but here's the first
batch of fixes anyway!

The first two patches are courtesy of Christoffer, who noticed that we
fail to clear LRs that generate a maintenance interrupt, leading to
potential screaming interrupts. This bug already exists in the current
mainline.

The next two address a bug where we fail to properly resample the line
level on exit, which could result in spurious interrupts being
injected. This is specific to the new vgic.

The following two patches tighten our GICv3 emulation by preventing
the guest from changing the SRE setting. This bug already exists in
mainline.

The last patch is actually a performance optimization: if the guest is
using GICv3, we can drop a number of barriers (since we don't need to
change SRE, and there is no memory-mapped view to synchronize
with). This results in a world switch that is 2.5% faster on my LS2085
box when running GICv3 guests.

These patches have been tested on top of the kvmarm/next branch.

Thanks,

	M.

Christoffer Dall (2):
  KVM: arm/arm64: vgic-v2: Clear all dirty LRs
  KVM: arm/arm64: vgic-v3: Clear all dirty LRs

Marc Zyngier (5):
  KVM: arm/arm64: vgic-v2: Always resample level interrupts
  KVM: arm/arm64: vgic-v3: Always resample level interrupts
  arm64: KVM: Make ICC_SRE_EL1 access return the configured SRE value
  arm64: KVM: vgic-v3: Prevent the guest from messing with ICC_SRE_EL1
  arm64: KVM: vgic-v3: Relax synchronization when SRE==1

 arch/arm64/kvm/hyp/vgic-v3-sr.c | 36 +++++++++++++++++++++---------------
 arch/arm64/kvm/sys_regs.c       | 13 ++++++++++++-
 virt/kvm/arm/hyp/vgic-v2-sr.c   |  7 +++----
 virt/kvm/arm/vgic/vgic-v2.c     | 11 +++++++----
 virt/kvm/arm/vgic/vgic-v3.c     | 11 +++++++----
 5 files changed, 50 insertions(+), 28 deletions(-)

-- 
2.1.4

[PATCH 0/7] vgic fixes for 4.7-rc1

[Marc Zyngier]

The dust has not yet settled on the new vgic, but here's the first
batch of fixes anyway!

The first two patches are courtesy of Christoffer, who noticed that we
fail to clear LRs that generate a maintenance interrupt, leading to
potential screaming interrupts. This bug already exists in the current
mainline.

The next two address a bug where we fail to properly resample the line
level on exit, which could result in spurious interrupts being
injected. This is specific to the new vgic.

The following two patches tighten our GICv3 emulation by preventing
the guest from changing the SRE setting. This bug already exists in
mainline.

The last patch is actually a performance optimization: if the guest is
using GICv3, we can drop a number of barriers (since we don't need to
change SRE, and there is no memory-mapped view to synchronize
with). This results in a world switch that is 2.5% faster on my LS2085
box when running GICv3 guests.

These patches have been tested on top of the kvmarm/next branch.

Thanks,

	M.

Christoffer Dall (2):
  KVM: arm/arm64: vgic-v2: Clear all dirty LRs
  KVM: arm/arm64: vgic-v3: Clear all dirty LRs

Marc Zyngier (5):
  KVM: arm/arm64: vgic-v2: Always resample level interrupts
  KVM: arm/arm64: vgic-v3: Always resample level interrupts
  arm64: KVM: Make ICC_SRE_EL1 access return the configured SRE value
  arm64: KVM: vgic-v3: Prevent the guest from messing with ICC_SRE_EL1
  arm64: KVM: vgic-v3: Relax synchronization when SRE==1

 arch/arm64/kvm/hyp/vgic-v3-sr.c | 36 +++++++++++++++++++++---------------
 arch/arm64/kvm/sys_regs.c       | 13 ++++++++++++-
 virt/kvm/arm/hyp/vgic-v2-sr.c   |  7 +++----
 virt/kvm/arm/vgic/vgic-v2.c     | 11 +++++++----
 virt/kvm/arm/vgic/vgic-v3.c     | 11 +++++++----
 5 files changed, 50 insertions(+), 28 deletions(-)

-- 
2.1.4

[PATCH 1/7] KVM: arm/arm64: vgic-v2: Clear all dirty LRs

[Marc Zyngier]

From: Christoffer Dall <christoffer.dall@linaro.org>

When saving the state of the list registers, it is critical to
reset them zero, as we could otherwise leave unexpected EOI
interrupts pending for virtual level interrupts.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/hyp/vgic-v2-sr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/hyp/vgic-v2-sr.c b/virt/kvm/arm/hyp/vgic-v2-sr.c
index a3f12b3..3a3a699 100644
--- a/virt/kvm/arm/hyp/vgic-v2-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v2-sr.c
@@ -100,12 +100,11 @@ static void __hyp_text save_lrs(struct kvm_vcpu *vcpu, void __iomem *base)
 		if (!(vcpu->arch.vgic_cpu.live_lrs & (1UL << i)))
 			continue;
 
-		if (cpu_if->vgic_elrsr & (1UL << i)) {
+		if (cpu_if->vgic_elrsr & (1UL << i))
 			cpu_if->vgic_lr[i] &= ~GICH_LR_STATE;
-			continue;
-		}
+		else
+			cpu_if->vgic_lr[i] = readl_relaxed(base + GICH_LR0 + (i * 4));
 
-		cpu_if->vgic_lr[i] = readl_relaxed(base + GICH_LR0 + (i * 4));
 		writel_relaxed(0, base + GICH_LR0 + (i * 4));
 	}
 }
-- 
2.1.4

[PATCH 1/7] KVM: arm/arm64: vgic-v2: Clear all dirty LRs

[Marc Zyngier]

From: Christoffer Dall <christoffer.dall@linaro.org>

When saving the state of the list registers, it is critical to
reset them zero, as we could otherwise leave unexpected EOI
interrupts pending for virtual level interrupts.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/hyp/vgic-v2-sr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/hyp/vgic-v2-sr.c b/virt/kvm/arm/hyp/vgic-v2-sr.c
index a3f12b3..3a3a699 100644
--- a/virt/kvm/arm/hyp/vgic-v2-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v2-sr.c
@@ -100,12 +100,11 @@ static void __hyp_text save_lrs(struct kvm_vcpu *vcpu, void __iomem *base)
 		if (!(vcpu->arch.vgic_cpu.live_lrs & (1UL << i)))
 			continue;
 
-		if (cpu_if->vgic_elrsr & (1UL << i)) {
+		if (cpu_if->vgic_elrsr & (1UL << i))
 			cpu_if->vgic_lr[i] &= ~GICH_LR_STATE;
-			continue;
-		}
+		else
+			cpu_if->vgic_lr[i] = readl_relaxed(base + GICH_LR0 + (i * 4));
 
-		cpu_if->vgic_lr[i] = readl_relaxed(base + GICH_LR0 + (i * 4));
 		writel_relaxed(0, base + GICH_LR0 + (i * 4));
 	}
 }
-- 
2.1.4

[PATCH 2/7] KVM: arm/arm64: vgic-v3: Clear all dirty LRs

[Marc Zyngier]

From: Christoffer Dall <christoffer.dall@linaro.org>

When saving the state of the list registers, it is critical to
reset them zero, as we could otherwise leave unexpected EOI
interrupts pending for virtual level interrupts.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index fff7cd4..3129df9 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -190,12 +190,11 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 			if (!(vcpu->arch.vgic_cpu.live_lrs & (1UL << i)))
 				continue;
 
-			if (cpu_if->vgic_elrsr & (1 << i)) {
+			if (cpu_if->vgic_elrsr & (1 << i))
 				cpu_if->vgic_lr[i] &= ~ICH_LR_STATE;
-				continue;
-			}
+			else
+				cpu_if->vgic_lr[i] = __gic_v3_get_lr(i);
 
-			cpu_if->vgic_lr[i] = __gic_v3_get_lr(i);
 			__gic_v3_set_lr(0, i);
 		}
 
-- 
2.1.4

[PATCH 2/7] KVM: arm/arm64: vgic-v3: Clear all dirty LRs

[Marc Zyngier]

From: Christoffer Dall <christoffer.dall@linaro.org>

When saving the state of the list registers, it is critical to
reset them zero, as we could otherwise leave unexpected EOI
interrupts pending for virtual level interrupts.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index fff7cd4..3129df9 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -190,12 +190,11 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 			if (!(vcpu->arch.vgic_cpu.live_lrs & (1UL << i)))
 				continue;
 
-			if (cpu_if->vgic_elrsr & (1 << i)) {
+			if (cpu_if->vgic_elrsr & (1 << i))
 				cpu_if->vgic_lr[i] &= ~ICH_LR_STATE;
-				continue;
-			}
+			else
+				cpu_if->vgic_lr[i] = __gic_v3_get_lr(i);
 
-			cpu_if->vgic_lr[i] = __gic_v3_get_lr(i);
 			__gic_v3_set_lr(0, i);
 		}
 
-- 
2.1.4

[PATCH 3/7] KVM: arm/arm64: vgic-v2: Always resample level interrupts

[Marc Zyngier]

When reading back from the list registers, we need to perform
two actions for level interrupts:
1) clear the soft-pending bit if the interrupt is not pending
   anymore *in the list register*
2) resample the line level and propagate it to the pending state

But these two actions linked, and we should *always* resample
the line level, no matter what state is in the list register.
Otherwise, we may end-up injecting spurious interrupts that
have been already retired.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v2.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 8ad42c2..f659af0 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -112,10 +112,13 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 			}
 		}
 
-		/* Clear soft pending state when level IRQs have been acked */
-		if (irq->config == VGIC_CONFIG_LEVEL &&
-		    !(val & GICH_LR_PENDING_BIT)) {
-			irq->soft_pending = false;
+		/*
+		 * Clear soft pending state when level irqs have been acked.
+		 * Always resample the line level.
+		 */
+		if (irq->config == VGIC_CONFIG_LEVEL) {
+			if (!(val & GICH_LR_PENDING_BIT))
+				irq->soft_pending = false;
 			irq->pending = irq->line_level;
 		}
 
-- 
2.1.4

[PATCH 3/7] KVM: arm/arm64: vgic-v2: Always resample level interrupts

[Marc Zyngier]

When reading back from the list registers, we need to perform
two actions for level interrupts:
1) clear the soft-pending bit if the interrupt is not pending
   anymore *in the list register*
2) resample the line level and propagate it to the pending state

But these two actions linked, and we should *always* resample
the line level, no matter what state is in the list register.
Otherwise, we may end-up injecting spurious interrupts that
have been already retired.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v2.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 8ad42c2..f659af0 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -112,10 +112,13 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 			}
 		}
 
-		/* Clear soft pending state when level IRQs have been acked */
-		if (irq->config == VGIC_CONFIG_LEVEL &&
-		    !(val & GICH_LR_PENDING_BIT)) {
-			irq->soft_pending = false;
+		/*
+		 * Clear soft pending state when level irqs have been acked.
+		 * Always resample the line level.
+		 */
+		if (irq->config == VGIC_CONFIG_LEVEL) {
+			if (!(val & GICH_LR_PENDING_BIT))
+				irq->soft_pending = false;
 			irq->pending = irq->line_level;
 		}
 
-- 
2.1.4

[PATCH 4/7] KVM: arm/arm64: vgic-v3: Always resample level interrupts

[Marc Zyngier]

When reading back from the list registers, we need to perform
two actions for level interrupts:
1) clear the soft-pending bit if the interrupt is not pending
   anymore *in the list register*
2) resample the line level and propagate it to the pending state

But these two actions linked, and we should *always* resample
the line level, no matter what state is in the list register.
Otherwise, we may end-up injecting spurious interrupts that
have been already retired.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v3.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 336a461..63b8bae 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -101,10 +101,13 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 			}
 		}
 
-		/* Clear soft pending state when level irqs have been acked */
-		if (irq->config == VGIC_CONFIG_LEVEL &&
-		    !(val & ICH_LR_PENDING_BIT)) {
-			irq->soft_pending = false;
+		/*
+		 * Clear soft pending state when level irqs have been acked.
+		 * Always resample the line level.
+		 */
+		if (irq->config == VGIC_CONFIG_LEVEL) {
+			if (!(val & ICH_LR_PENDING_BIT))
+				irq->soft_pending = false;
 			irq->pending = irq->line_level;
 		}
 
-- 
2.1.4

[PATCH 4/7] KVM: arm/arm64: vgic-v3: Always resample level interrupts

[Marc Zyngier]

When reading back from the list registers, we need to perform
two actions for level interrupts:
1) clear the soft-pending bit if the interrupt is not pending
   anymore *in the list register*
2) resample the line level and propagate it to the pending state

But these two actions linked, and we should *always* resample
the line level, no matter what state is in the list register.
Otherwise, we may end-up injecting spurious interrupts that
have been already retired.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v3.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 336a461..63b8bae 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -101,10 +101,13 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 			}
 		}
 
-		/* Clear soft pending state when level irqs have been acked */
-		if (irq->config == VGIC_CONFIG_LEVEL &&
-		    !(val & ICH_LR_PENDING_BIT)) {
-			irq->soft_pending = false;
+		/*
+		 * Clear soft pending state when level irqs have been acked.
+		 * Always resample the line level.
+		 */
+		if (irq->config == VGIC_CONFIG_LEVEL) {
+			if (!(val & ICH_LR_PENDING_BIT))
+				irq->soft_pending = false;
 			irq->pending = irq->line_level;
 		}
 
-- 
2.1.4

[PATCH 5/7] arm64: KVM: Make ICC_SRE_EL1 access return the configured SRE value

[Marc Zyngier]

When we trap ICC_SRE_EL1, we handle it as RAZ/WI. It would be
more correct to actual make it RO, and return the configured
value when read.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7bbe3ff..a57d650 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -134,6 +134,17 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool access_gic_sre(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	if (p->is_write)
+		return ignore_write(vcpu, p);
+
+	p->regval = vcpu->arch.vgic_cpu.vgic_v3.vgic_sre;
+	return true;
+}
+
 static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 			struct sys_reg_params *p,
 			const struct sys_reg_desc *r)
@@ -958,7 +969,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  access_gic_sgi },
 	/* ICC_SRE_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b101),
-	  trap_raz_wi },
+	  access_gic_sre },
 
 	/* CONTEXTIDR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b001),
-- 
2.1.4

[PATCH 5/7] arm64: KVM: Make ICC_SRE_EL1 access return the configured SRE value

[Marc Zyngier]

When we trap ICC_SRE_EL1, we handle it as RAZ/WI. It would be
more correct to actual make it RO, and return the configured
value when read.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7bbe3ff..a57d650 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -134,6 +134,17 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool access_gic_sre(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	if (p->is_write)
+		return ignore_write(vcpu, p);
+
+	p->regval = vcpu->arch.vgic_cpu.vgic_v3.vgic_sre;
+	return true;
+}
+
 static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 			struct sys_reg_params *p,
 			const struct sys_reg_desc *r)
@@ -958,7 +969,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  access_gic_sgi },
 	/* ICC_SRE_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b101),
-	  trap_raz_wi },
+	  access_gic_sre },
 
 	/* CONTEXTIDR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b001),
-- 
2.1.4

[PATCH 6/7] arm64: KVM: vgic-v3: Prevent the guest from messing with ICC_SRE_EL1

[Marc Zyngier]

Both our GIC emulations are "strict", in the sense that we either
emulate a GICv2 or a GICv3, and not a GICv3 with GICv2 legacy
support.

But when running on a GICv3 host, we still allow the guest to
tinker with the ICC_SRE_EL1 register during its time slice:
it can switch SRE off, observe that it is off, and yet on the
next world switch, find the SRE bit to be set again. Not very
nice.

An obvious solution is to always trap accesses to ICC_SRE_EL1
(by clearing ICC_SRE_EL2.Enable), and to let the handler return
the programmed value on a read, or ignore the write.

That way, the guest can always observe that our GICv3 is SRE==1
only.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 3129df9..40c3b4c 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -313,10 +313,8 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	 * Prevent the guest from touching the GIC system registers if
 	 * SRE isn't enabled for GICv3 emulation.
 	 */
-	if (!cpu_if->vgic_sre) {
-		write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE,
-			     ICC_SRE_EL2);
-	}
+	write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE,
+		     ICC_SRE_EL2);
 }
 
 void __hyp_text __vgic_v3_init_lrs(void)
-- 
2.1.4

[PATCH 6/7] arm64: KVM: vgic-v3: Prevent the guest from messing with ICC_SRE_EL1

[Marc Zyngier]

Both our GIC emulations are "strict", in the sense that we either
emulate a GICv2 or a GICv3, and not a GICv3 with GICv2 legacy
support.

But when running on a GICv3 host, we still allow the guest to
tinker with the ICC_SRE_EL1 register during its time slice:
it can switch SRE off, observe that it is off, and yet on the
next world switch, find the SRE bit to be set again. Not very
nice.

An obvious solution is to always trap accesses to ICC_SRE_EL1
(by clearing ICC_SRE_EL2.Enable), and to let the handler return
the programmed value on a read, or ignore the write.

That way, the guest can always observe that our GICv3 is SRE==1
only.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 3129df9..40c3b4c 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -313,10 +313,8 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	 * Prevent the guest from touching the GIC system registers if
 	 * SRE isn't enabled for GICv3 emulation.
 	 */
-	if (!cpu_if->vgic_sre) {
-		write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE,
-			     ICC_SRE_EL2);
-	}
+	write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE,
+		     ICC_SRE_EL2);
 }
 
 void __hyp_text __vgic_v3_init_lrs(void)
-- 
2.1.4

[PATCH 7/7] arm64: KVM: vgic-v3: Relax synchronization when SRE==1

[Marc Zyngier]

The GICv3 backend of the vgic is quite barrier heavy, in order
to ensure synchronization of the system registers and the
memory mapped view for a potential GICv2 guest.

But when the guest is using a GICv3 model, there is absolutely
no need to execute all these heavy barriers, and it is actually
beneficial to avoid them altogether.

This patch makes the synchonization conditional, and ensures
that we do not change the EL1 SRE settings if we do not need to.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 40c3b4c..5f8f80b 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -169,7 +169,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 	 * Make sure stores to the GIC via the memory mapped interface
 	 * are now visible to the system register interface.
 	 */
-	dsb(st);
+	if (!cpu_if->vgic_sre)
+		dsb(st);
 
 	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
 
@@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 
 	val = read_gicreg(ICC_SRE_EL2);
 	write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
-	isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
-	write_gicreg(1, ICC_SRE_EL1);
+
+	if (!cpu_if->vgic_sre) {
+		/* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
+		isb();
+		write_gicreg(1, ICC_SRE_EL1);
+	}
 }
 
 void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
@@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	 * been actually programmed with the value we want before
 	 * starting to mess with the rest of the GIC.
 	 */
-	write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
-	isb();
+	if (!cpu_if->vgic_sre) {
+		write_gicreg(0, ICC_SRE_EL1);
+		isb();
+	}
 
 	val = read_gicreg(ICH_VTR_EL2);
 	max_lr_idx = vtr_to_max_lr_idx(val);
@@ -305,8 +312,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	 * (re)distributors. This ensure the guest will read the
 	 * correct values from the memory-mapped interface.
 	 */
-	isb();
-	dsb(sy);
+	if (!cpu_if->vgic_sre) {
+		isb();
+		dsb(sy);
+	}
 	vcpu->arch.vgic_cpu.live_lrs = live_lrs;
 
 	/*
-- 
2.1.4

[PATCH 7/7] arm64: KVM: vgic-v3: Relax synchronization when SRE==1

[Marc Zyngier]

The GICv3 backend of the vgic is quite barrier heavy, in order
to ensure synchronization of the system registers and the
memory mapped view for a potential GICv2 guest.

But when the guest is using a GICv3 model, there is absolutely
no need to execute all these heavy barriers, and it is actually
beneficial to avoid them altogether.

This patch makes the synchonization conditional, and ensures
that we do not change the EL1 SRE settings if we do not need to.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 40c3b4c..5f8f80b 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -169,7 +169,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 	 * Make sure stores to the GIC via the memory mapped interface
 	 * are now visible to the system register interface.
 	 */
-	dsb(st);
+	if (!cpu_if->vgic_sre)
+		dsb(st);
 
 	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
 
@@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 
 	val = read_gicreg(ICC_SRE_EL2);
 	write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
-	isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
-	write_gicreg(1, ICC_SRE_EL1);
+
+	if (!cpu_if->vgic_sre) {
+		/* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
+		isb();
+		write_gicreg(1, ICC_SRE_EL1);
+	}
 }
 
 void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
@@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	 * been actually programmed with the value we want before
 	 * starting to mess with the rest of the GIC.
 	 */
-	write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
-	isb();
+	if (!cpu_if->vgic_sre) {
+		write_gicreg(0, ICC_SRE_EL1);
+		isb();
+	}
 
 	val = read_gicreg(ICH_VTR_EL2);
 	max_lr_idx = vtr_to_max_lr_idx(val);
@@ -305,8 +312,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	 * (re)distributors. This ensure the guest will read the
 	 * correct values from the memory-mapped interface.
 	 */
-	isb();
-	dsb(sy);
+	if (!cpu_if->vgic_sre) {
+		isb();
+		dsb(sy);
+	}
 	vcpu->arch.vgic_cpu.live_lrs = live_lrs;
 
 	/*
-- 
2.1.4

Re: [PATCH 3/7] KVM: arm/arm64: vgic-v2: Always resample level interrupts

[Christoffer Dall]

On Mon, May 23, 2016 at 01:36:59PM +0100, Marc Zyngier wrote:
> When reading back from the list registers, we need to perform
> two actions for level interrupts:
> 1) clear the soft-pending bit if the interrupt is not pending
>    anymore *in the list register*
> 2) resample the line level and propagate it to the pending state
> 
> But these two actions linked, and we should *always* resample

were linked?

> the line level, no matter what state is in the list register.
> Otherwise, we may end-up injecting spurious interrupts that
> have been already retired.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-v2.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index 8ad42c2..f659af0 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -112,10 +112,13 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
>  			}
>  		}
>  
> -		/* Clear soft pending state when level IRQs have been acked */
> -		if (irq->config == VGIC_CONFIG_LEVEL &&
> -		    !(val & GICH_LR_PENDING_BIT)) {
> -			irq->soft_pending = false;
> +		/*
> +		 * Clear soft pending state when level irqs have been acked.
> +		 * Always resample the line level.
> +		 */
> +		if (irq->config == VGIC_CONFIG_LEVEL) {
> +			if (!(val & GICH_LR_PENDING_BIT))
> +				irq->soft_pending = false;
>  			irq->pending = irq->line_level;

shouldn't this context line then be:
	irq->pending = irq->line_level || irq->soft_pending;  ??

Thanks,
-Christoffer

[PATCH 3/7] KVM: arm/arm64: vgic-v2: Always resample level interrupts

[Christoffer Dall]

On Mon, May 23, 2016 at 01:36:59PM +0100, Marc Zyngier wrote:
> When reading back from the list registers, we need to perform
> two actions for level interrupts:
> 1) clear the soft-pending bit if the interrupt is not pending
>    anymore *in the list register*
> 2) resample the line level and propagate it to the pending state
> 
> But these two actions linked, and we should *always* resample

were linked?

> the line level, no matter what state is in the list register.
> Otherwise, we may end-up injecting spurious interrupts that
> have been already retired.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-v2.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index 8ad42c2..f659af0 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -112,10 +112,13 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
>  			}
>  		}
>  
> -		/* Clear soft pending state when level IRQs have been acked */
> -		if (irq->config == VGIC_CONFIG_LEVEL &&
> -		    !(val & GICH_LR_PENDING_BIT)) {
> -			irq->soft_pending = false;
> +		/*
> +		 * Clear soft pending state when level irqs have been acked.
> +		 * Always resample the line level.
> +		 */
> +		if (irq->config == VGIC_CONFIG_LEVEL) {
> +			if (!(val & GICH_LR_PENDING_BIT))
> +				irq->soft_pending = false;
>  			irq->pending = irq->line_level;

shouldn't this context line then be:
	irq->pending = irq->line_level || irq->soft_pending;  ??

Thanks,
-Christoffer

Re: [PATCH 4/7] KVM: arm/arm64: vgic-v3: Always resample level interrupts

[Christoffer Dall]

On Mon, May 23, 2016 at 01:37:00PM +0100, Marc Zyngier wrote:
> When reading back from the list registers, we need to perform
> two actions for level interrupts:
> 1) clear the soft-pending bit if the interrupt is not pending
>    anymore *in the list register*
> 2) resample the line level and propagate it to the pending state
> 
> But these two actions linked, and we should *always* resample
> the line level, no matter what state is in the list register.
> Otherwise, we may end-up injecting spurious interrupts that
> have been already retired.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 336a461..63b8bae 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -101,10 +101,13 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
>  			}
>  		}
>  
> -		/* Clear soft pending state when level irqs have been acked */
> -		if (irq->config == VGIC_CONFIG_LEVEL &&
> -		    !(val & ICH_LR_PENDING_BIT)) {
> -			irq->soft_pending = false;
> +		/*
> +		 * Clear soft pending state when level irqs have been acked.
> +		 * Always resample the line level.
> +		 */
> +		if (irq->config == VGIC_CONFIG_LEVEL) {
> +			if (!(val & ICH_LR_PENDING_BIT))
> +				irq->soft_pending = false;
>  			irq->pending = irq->line_level;

same question as previous patch

[PATCH 4/7] KVM: arm/arm64: vgic-v3: Always resample level interrupts

[Christoffer Dall]

On Mon, May 23, 2016 at 01:37:00PM +0100, Marc Zyngier wrote:
> When reading back from the list registers, we need to perform
> two actions for level interrupts:
> 1) clear the soft-pending bit if the interrupt is not pending
>    anymore *in the list register*
> 2) resample the line level and propagate it to the pending state
> 
> But these two actions linked, and we should *always* resample
> the line level, no matter what state is in the list register.
> Otherwise, we may end-up injecting spurious interrupts that
> have been already retired.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 336a461..63b8bae 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -101,10 +101,13 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
>  			}
>  		}
>  
> -		/* Clear soft pending state when level irqs have been acked */
> -		if (irq->config == VGIC_CONFIG_LEVEL &&
> -		    !(val & ICH_LR_PENDING_BIT)) {
> -			irq->soft_pending = false;
> +		/*
> +		 * Clear soft pending state when level irqs have been acked.
> +		 * Always resample the line level.
> +		 */
> +		if (irq->config == VGIC_CONFIG_LEVEL) {
> +			if (!(val & ICH_LR_PENDING_BIT))
> +				irq->soft_pending = false;
>  			irq->pending = irq->line_level;

same question as previous patch

Re: [PATCH 3/7] KVM: arm/arm64: vgic-v2: Always resample level interrupts

[Marc Zyngier]

On 23/05/16 15:19, Christoffer Dall wrote:
> On Mon, May 23, 2016 at 01:36:59PM +0100, Marc Zyngier wrote:
>> When reading back from the list registers, we need to perform
>> two actions for level interrupts:
>> 1) clear the soft-pending bit if the interrupt is not pending
>>    anymore *in the list register*
>> 2) resample the line level and propagate it to the pending state
>>
>> But these two actions linked, and we should *always* resample
> 
> were linked?

Indeed.

> 
>> the line level, no matter what state is in the list register.
>> Otherwise, we may end-up injecting spurious interrupts that
>> have been already retired.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-v2.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
>> index 8ad42c2..f659af0 100644
>> --- a/virt/kvm/arm/vgic/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
>> @@ -112,10 +112,13 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
>>  			}
>>  		}
>>  
>> -		/* Clear soft pending state when level IRQs have been acked */
>> -		if (irq->config == VGIC_CONFIG_LEVEL &&
>> -		    !(val & GICH_LR_PENDING_BIT)) {
>> -			irq->soft_pending = false;
>> +		/*
>> +		 * Clear soft pending state when level irqs have been acked.
>> +		 * Always resample the line level.
>> +		 */
>> +		if (irq->config == VGIC_CONFIG_LEVEL) {
>> +			if (!(val & GICH_LR_PENDING_BIT))
>> +				irq->soft_pending = false;
>>  			irq->pending = irq->line_level;
> 
> shouldn't this context line then be:
> 	irq->pending = irq->line_level || irq->soft_pending;  ??

Yes, you're right. The only case we discard the soft-pending bit is when
we know for sure that the interrupt has transitioned from pending to active.

I'll update the two patches and repost once the rest of the series has
been reviewed.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 3/7] KVM: arm/arm64: vgic-v2: Always resample level interrupts

[Marc Zyngier]

On 23/05/16 15:19, Christoffer Dall wrote:
> On Mon, May 23, 2016 at 01:36:59PM +0100, Marc Zyngier wrote:
>> When reading back from the list registers, we need to perform
>> two actions for level interrupts:
>> 1) clear the soft-pending bit if the interrupt is not pending
>>    anymore *in the list register*
>> 2) resample the line level and propagate it to the pending state
>>
>> But these two actions linked, and we should *always* resample
> 
> were linked?

Indeed.

> 
>> the line level, no matter what state is in the list register.
>> Otherwise, we may end-up injecting spurious interrupts that
>> have been already retired.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-v2.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
>> index 8ad42c2..f659af0 100644
>> --- a/virt/kvm/arm/vgic/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
>> @@ -112,10 +112,13 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
>>  			}
>>  		}
>>  
>> -		/* Clear soft pending state when level IRQs have been acked */
>> -		if (irq->config == VGIC_CONFIG_LEVEL &&
>> -		    !(val & GICH_LR_PENDING_BIT)) {
>> -			irq->soft_pending = false;
>> +		/*
>> +		 * Clear soft pending state when level irqs have been acked.
>> +		 * Always resample the line level.
>> +		 */
>> +		if (irq->config == VGIC_CONFIG_LEVEL) {
>> +			if (!(val & GICH_LR_PENDING_BIT))
>> +				irq->soft_pending = false;
>>  			irq->pending = irq->line_level;
> 
> shouldn't this context line then be:
> 	irq->pending = irq->line_level || irq->soft_pending;  ??

Yes, you're right. The only case we discard the soft-pending bit is when
we know for sure that the interrupt has transitioned from pending to active.

I'll update the two patches and repost once the rest of the series has
been reviewed.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 5/7] arm64: KVM: Make ICC_SRE_EL1 access return the configured SRE value

[Christoffer Dall]

On Mon, May 23, 2016 at 01:37:01PM +0100, Marc Zyngier wrote:
> When we trap ICC_SRE_EL1, we handle it as RAZ/WI. It would be
> more correct to actual make it RO, and return the configured
> value when read.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

>  arch/arm64/kvm/sys_regs.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7bbe3ff..a57d650 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -134,6 +134,17 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static bool access_gic_sre(struct kvm_vcpu *vcpu,
> +			   struct sys_reg_params *p,
> +			   const struct sys_reg_desc *r)
> +{
> +	if (p->is_write)
> +		return ignore_write(vcpu, p);
> +
> +	p->regval = vcpu->arch.vgic_cpu.vgic_v3.vgic_sre;
> +	return true;
> +}
> +
>  static bool trap_raz_wi(struct kvm_vcpu *vcpu,
>  			struct sys_reg_params *p,
>  			const struct sys_reg_desc *r)
> @@ -958,7 +969,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	  access_gic_sgi },
>  	/* ICC_SRE_EL1 */
>  	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b101),
> -	  trap_raz_wi },
> +	  access_gic_sre },
>  
>  	/* CONTEXTIDR_EL1 */
>  	{ Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b001),
> -- 
> 2.1.4
> 

[PATCH 5/7] arm64: KVM: Make ICC_SRE_EL1 access return the configured SRE value

[Christoffer Dall]

On Mon, May 23, 2016 at 01:37:01PM +0100, Marc Zyngier wrote:
> When we trap ICC_SRE_EL1, we handle it as RAZ/WI. It would be
> more correct to actual make it RO, and return the configured
> value when read.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

>  arch/arm64/kvm/sys_regs.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7bbe3ff..a57d650 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -134,6 +134,17 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static bool access_gic_sre(struct kvm_vcpu *vcpu,
> +			   struct sys_reg_params *p,
> +			   const struct sys_reg_desc *r)
> +{
> +	if (p->is_write)
> +		return ignore_write(vcpu, p);
> +
> +	p->regval = vcpu->arch.vgic_cpu.vgic_v3.vgic_sre;
> +	return true;
> +}
> +
>  static bool trap_raz_wi(struct kvm_vcpu *vcpu,
>  			struct sys_reg_params *p,
>  			const struct sys_reg_desc *r)
> @@ -958,7 +969,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	  access_gic_sgi },
>  	/* ICC_SRE_EL1 */
>  	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b101),
> -	  trap_raz_wi },
> +	  access_gic_sre },
>  
>  	/* CONTEXTIDR_EL1 */
>  	{ Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b001),
> -- 
> 2.1.4
> 

Re: [PATCH 6/7] arm64: KVM: vgic-v3: Prevent the guest from messing with ICC_SRE_EL1

[Christoffer Dall]

On Mon, May 23, 2016 at 01:37:02PM +0100, Marc Zyngier wrote:
> Both our GIC emulations are "strict", in the sense that we either
> emulate a GICv2 or a GICv3, and not a GICv3 with GICv2 legacy
> support.
> 
> But when running on a GICv3 host, we still allow the guest to
> tinker with the ICC_SRE_EL1 register during its time slice:
> it can switch SRE off, observe that it is off, and yet on the
> next world switch, find the SRE bit to be set again. Not very
> nice.
> 
> An obvious solution is to always trap accesses to ICC_SRE_EL1
> (by clearing ICC_SRE_EL2.Enable), and to let the handler return
> the programmed value on a read, or ignore the write.
> 
> That way, the guest can always observe that our GICv3 is SRE==1
> only.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

[PATCH 6/7] arm64: KVM: vgic-v3: Prevent the guest from messing with ICC_SRE_EL1

[Christoffer Dall]

On Mon, May 23, 2016 at 01:37:02PM +0100, Marc Zyngier wrote:
> Both our GIC emulations are "strict", in the sense that we either
> emulate a GICv2 or a GICv3, and not a GICv3 with GICv2 legacy
> support.
> 
> But when running on a GICv3 host, we still allow the guest to
> tinker with the ICC_SRE_EL1 register during its time slice:
> it can switch SRE off, observe that it is off, and yet on the
> next world switch, find the SRE bit to be set again. Not very
> nice.
> 
> An obvious solution is to always trap accesses to ICC_SRE_EL1
> (by clearing ICC_SRE_EL2.Enable), and to let the handler return
> the programmed value on a read, or ignore the write.
> 
> That way, the guest can always observe that our GICv3 is SRE==1
> only.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Re: [PATCH 7/7] arm64: KVM: vgic-v3: Relax synchronization when SRE==1

[Christoffer Dall]

On Mon, May 23, 2016 at 01:37:03PM +0100, Marc Zyngier wrote:
> The GICv3 backend of the vgic is quite barrier heavy, in order
> to ensure synchronization of the system registers and the
> memory mapped view for a potential GICv2 guest.
> 
> But when the guest is using a GICv3 model, there is absolutely
> no need to execute all these heavy barriers, and it is actually
> beneficial to avoid them altogether.
> 
> This patch makes the synchonization conditional, and ensures
> that we do not change the EL1 SRE settings if we do not need to.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> index 40c3b4c..5f8f80b 100644
> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> @@ -169,7 +169,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  	 * Make sure stores to the GIC via the memory mapped interface
>  	 * are now visible to the system register interface.
>  	 */
> -	dsb(st);
> +	if (!cpu_if->vgic_sre)
> +		dsb(st);
>  
>  	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
>  
> @@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  
>  	val = read_gicreg(ICC_SRE_EL2);
>  	write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
> -	isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
> -	write_gicreg(1, ICC_SRE_EL1);
> +
> +	if (!cpu_if->vgic_sre) {
> +		/* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
> +		isb();
> +		write_gicreg(1, ICC_SRE_EL1);

why the change in behavior to only write 1 to the register when
(!cpu_if->vgic_sre) ?

> +	}
>  }
>  
>  void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
> @@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  	 * been actually programmed with the value we want before
>  	 * starting to mess with the rest of the GIC.
>  	 */
> -	write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
> -	isb();
> +	if (!cpu_if->vgic_sre) {
> +		write_gicreg(0, ICC_SRE_EL1);
> +		isb();
> +	}
>  
>  	val = read_gicreg(ICH_VTR_EL2);
>  	max_lr_idx = vtr_to_max_lr_idx(val);
> @@ -305,8 +312,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  	 * (re)distributors. This ensure the guest will read the
>  	 * correct values from the memory-mapped interface.
>  	 */
> -	isb();
> -	dsb(sy);
> +	if (!cpu_if->vgic_sre) {
> +		isb();
> +		dsb(sy);
> +	}
>  	vcpu->arch.vgic_cpu.live_lrs = live_lrs;
>  
>  	/*
> -- 
> 2.1.4
> 

[PATCH 7/7] arm64: KVM: vgic-v3: Relax synchronization when SRE==1

[Christoffer Dall]

On Mon, May 23, 2016 at 01:37:03PM +0100, Marc Zyngier wrote:
> The GICv3 backend of the vgic is quite barrier heavy, in order
> to ensure synchronization of the system registers and the
> memory mapped view for a potential GICv2 guest.
> 
> But when the guest is using a GICv3 model, there is absolutely
> no need to execute all these heavy barriers, and it is actually
> beneficial to avoid them altogether.
> 
> This patch makes the synchonization conditional, and ensures
> that we do not change the EL1 SRE settings if we do not need to.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> index 40c3b4c..5f8f80b 100644
> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> @@ -169,7 +169,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  	 * Make sure stores to the GIC via the memory mapped interface
>  	 * are now visible to the system register interface.
>  	 */
> -	dsb(st);
> +	if (!cpu_if->vgic_sre)
> +		dsb(st);
>  
>  	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
>  
> @@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  
>  	val = read_gicreg(ICC_SRE_EL2);
>  	write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
> -	isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
> -	write_gicreg(1, ICC_SRE_EL1);
> +
> +	if (!cpu_if->vgic_sre) {
> +		/* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
> +		isb();
> +		write_gicreg(1, ICC_SRE_EL1);

why the change in behavior to only write 1 to the register when
(!cpu_if->vgic_sre) ?

> +	}
>  }
>  
>  void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
> @@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  	 * been actually programmed with the value we want before
>  	 * starting to mess with the rest of the GIC.
>  	 */
> -	write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
> -	isb();
> +	if (!cpu_if->vgic_sre) {
> +		write_gicreg(0, ICC_SRE_EL1);
> +		isb();
> +	}
>  
>  	val = read_gicreg(ICH_VTR_EL2);
>  	max_lr_idx = vtr_to_max_lr_idx(val);
> @@ -305,8 +312,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  	 * (re)distributors. This ensure the guest will read the
>  	 * correct values from the memory-mapped interface.
>  	 */
> -	isb();
> -	dsb(sy);
> +	if (!cpu_if->vgic_sre) {
> +		isb();
> +		dsb(sy);
> +	}
>  	vcpu->arch.vgic_cpu.live_lrs = live_lrs;
>  
>  	/*
> -- 
> 2.1.4
> 

Re: [PATCH 7/7] arm64: KVM: vgic-v3: Relax synchronization when SRE==1

[Marc Zyngier]

On 24/05/16 13:52, Christoffer Dall wrote:
> On Mon, May 23, 2016 at 01:37:03PM +0100, Marc Zyngier wrote:
>> The GICv3 backend of the vgic is quite barrier heavy, in order
>> to ensure synchronization of the system registers and the
>> memory mapped view for a potential GICv2 guest.
>>
>> But when the guest is using a GICv3 model, there is absolutely
>> no need to execute all these heavy barriers, and it is actually
>> beneficial to avoid them altogether.
>>
>> This patch makes the synchonization conditional, and ensures
>> that we do not change the EL1 SRE settings if we do not need to.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> index 40c3b4c..5f8f80b 100644
>> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> @@ -169,7 +169,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>>  	 * Make sure stores to the GIC via the memory mapped interface
>>  	 * are now visible to the system register interface.
>>  	 */
>> -	dsb(st);
>> +	if (!cpu_if->vgic_sre)
>> +		dsb(st);
>>  
>>  	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
>>  
>> @@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>>  
>>  	val = read_gicreg(ICC_SRE_EL2);
>>  	write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
>> -	isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
>> -	write_gicreg(1, ICC_SRE_EL1);
>> +
>> +	if (!cpu_if->vgic_sre) {
>> +		/* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
>> +		isb();
>> +		write_gicreg(1, ICC_SRE_EL1);
> 
> why the change in behavior to only write 1 to the register when
> (!cpu_if->vgic_sre) ?

If we're on a GICv3 host and emulating a GICv3 guest, then we never
changed the SRE mode (i.e. it is still set to 1). The only case we
change it is when we when have a GICv2 guest (i.e. when vgic_sre is zero).

So there is no need to restore the host configuration unless we actually
changed it...

> 
>> +	}
>>  }
>>  
>>  void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>> @@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>>  	 * been actually programmed with the value we want before
>>  	 * starting to mess with the rest of the GIC.
>>  	 */
>> -	write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
>> -	isb();
>> +	if (!cpu_if->vgic_sre) {
>> +		write_gicreg(0, ICC_SRE_EL1);
>> +		isb();
>> +	}

... here.

Does it make sense? I could also split this as part of another patch if
you think that's clearer.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 7/7] arm64: KVM: vgic-v3: Relax synchronization when SRE==1

[Marc Zyngier]

On 24/05/16 13:52, Christoffer Dall wrote:
> On Mon, May 23, 2016 at 01:37:03PM +0100, Marc Zyngier wrote:
>> The GICv3 backend of the vgic is quite barrier heavy, in order
>> to ensure synchronization of the system registers and the
>> memory mapped view for a potential GICv2 guest.
>>
>> But when the guest is using a GICv3 model, there is absolutely
>> no need to execute all these heavy barriers, and it is actually
>> beneficial to avoid them altogether.
>>
>> This patch makes the synchonization conditional, and ensures
>> that we do not change the EL1 SRE settings if we do not need to.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> index 40c3b4c..5f8f80b 100644
>> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> @@ -169,7 +169,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>>  	 * Make sure stores to the GIC via the memory mapped interface
>>  	 * are now visible to the system register interface.
>>  	 */
>> -	dsb(st);
>> +	if (!cpu_if->vgic_sre)
>> +		dsb(st);
>>  
>>  	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
>>  
>> @@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>>  
>>  	val = read_gicreg(ICC_SRE_EL2);
>>  	write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
>> -	isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
>> -	write_gicreg(1, ICC_SRE_EL1);
>> +
>> +	if (!cpu_if->vgic_sre) {
>> +		/* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
>> +		isb();
>> +		write_gicreg(1, ICC_SRE_EL1);
> 
> why the change in behavior to only write 1 to the register when
> (!cpu_if->vgic_sre) ?

If we're on a GICv3 host and emulating a GICv3 guest, then we never
changed the SRE mode (i.e. it is still set to 1). The only case we
change it is when we when have a GICv2 guest (i.e. when vgic_sre is zero).

So there is no need to restore the host configuration unless we actually
changed it...

> 
>> +	}
>>  }
>>  
>>  void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>> @@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>>  	 * been actually programmed with the value we want before
>>  	 * starting to mess with the rest of the GIC.
>>  	 */
>> -	write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
>> -	isb();
>> +	if (!cpu_if->vgic_sre) {
>> +		write_gicreg(0, ICC_SRE_EL1);
>> +		isb();
>> +	}

... here.

Does it make sense? I could also split this as part of another patch if
you think that's clearer.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 7/7] arm64: KVM: vgic-v3: Relax synchronization when SRE==1

[Christoffer Dall]

On Tue, May 24, 2016 at 02:02:19PM +0100, Marc Zyngier wrote:
> On 24/05/16 13:52, Christoffer Dall wrote:
> > On Mon, May 23, 2016 at 01:37:03PM +0100, Marc Zyngier wrote:
> >> The GICv3 backend of the vgic is quite barrier heavy, in order
> >> to ensure synchronization of the system registers and the
> >> memory mapped view for a potential GICv2 guest.
> >>
> >> But when the guest is using a GICv3 model, there is absolutely
> >> no need to execute all these heavy barriers, and it is actually
> >> beneficial to avoid them altogether.
> >>
> >> This patch makes the synchonization conditional, and ensures
> >> that we do not change the EL1 SRE settings if we do not need to.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
> >>  1 file changed, 16 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> index 40c3b4c..5f8f80b 100644
> >> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> @@ -169,7 +169,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
> >>  	 * Make sure stores to the GIC via the memory mapped interface
> >>  	 * are now visible to the system register interface.
> >>  	 */
> >> -	dsb(st);
> >> +	if (!cpu_if->vgic_sre)
> >> +		dsb(st);
> >>  
> >>  	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
> >>  
> >> @@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
> >>  
> >>  	val = read_gicreg(ICC_SRE_EL2);
> >>  	write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
> >> -	isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
> >> -	write_gicreg(1, ICC_SRE_EL1);
> >> +
> >> +	if (!cpu_if->vgic_sre) {
> >> +		/* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
> >> +		isb();
> >> +		write_gicreg(1, ICC_SRE_EL1);
> > 
> > why the change in behavior to only write 1 to the register when
> > (!cpu_if->vgic_sre) ?
> 
> If we're on a GICv3 host and emulating a GICv3 guest, then we never
> changed the SRE mode (i.e. it is still set to 1). The only case we
> change it is when we when have a GICv2 guest (i.e. when vgic_sre is zero).
> 
> So there is no need to restore the host configuration unless we actually
> changed it...
> 
> > 
> >> +	}
> >>  }
> >>  
> >>  void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
> >> @@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
> >>  	 * been actually programmed with the value we want before
> >>  	 * starting to mess with the rest of the GIC.
> >>  	 */
> >> -	write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
> >> -	isb();
> >> +	if (!cpu_if->vgic_sre) {
> >> +		write_gicreg(0, ICC_SRE_EL1);
> >> +		isb();
> >> +	}
> 
> ... here.
> 
> Does it make sense? I could also split this as part of another patch if
> you think that's clearer.

No makes totally sense, I just didn't see it because I'm GICed these
days.

Thanks for the explanation, and:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

[PATCH 7/7] arm64: KVM: vgic-v3: Relax synchronization when SRE==1

[Christoffer Dall]

On Tue, May 24, 2016 at 02:02:19PM +0100, Marc Zyngier wrote:
> On 24/05/16 13:52, Christoffer Dall wrote:
> > On Mon, May 23, 2016 at 01:37:03PM +0100, Marc Zyngier wrote:
> >> The GICv3 backend of the vgic is quite barrier heavy, in order
> >> to ensure synchronization of the system registers and the
> >> memory mapped view for a potential GICv2 guest.
> >>
> >> But when the guest is using a GICv3 model, there is absolutely
> >> no need to execute all these heavy barriers, and it is actually
> >> beneficial to avoid them altogether.
> >>
> >> This patch makes the synchonization conditional, and ensures
> >> that we do not change the EL1 SRE settings if we do not need to.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
> >>  1 file changed, 16 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> index 40c3b4c..5f8f80b 100644
> >> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> @@ -169,7 +169,8 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
> >>  	 * Make sure stores to the GIC via the memory mapped interface
> >>  	 * are now visible to the system register interface.
> >>  	 */
> >> -	dsb(st);
> >> +	if (!cpu_if->vgic_sre)
> >> +		dsb(st);
> >>  
> >>  	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
> >>  
> >> @@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
> >>  
> >>  	val = read_gicreg(ICC_SRE_EL2);
> >>  	write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
> >> -	isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
> >> -	write_gicreg(1, ICC_SRE_EL1);
> >> +
> >> +	if (!cpu_if->vgic_sre) {
> >> +		/* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
> >> +		isb();
> >> +		write_gicreg(1, ICC_SRE_EL1);
> > 
> > why the change in behavior to only write 1 to the register when
> > (!cpu_if->vgic_sre) ?
> 
> If we're on a GICv3 host and emulating a GICv3 guest, then we never
> changed the SRE mode (i.e. it is still set to 1). The only case we
> change it is when we when have a GICv2 guest (i.e. when vgic_sre is zero).
> 
> So there is no need to restore the host configuration unless we actually
> changed it...
> 
> > 
> >> +	}
> >>  }
> >>  
> >>  void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
> >> @@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
> >>  	 * been actually programmed with the value we want before
> >>  	 * starting to mess with the rest of the GIC.
> >>  	 */
> >> -	write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
> >> -	isb();
> >> +	if (!cpu_if->vgic_sre) {
> >> +		write_gicreg(0, ICC_SRE_EL1);
> >> +		isb();
> >> +	}
> 
> ... here.
> 
> Does it make sense? I could also split this as part of another patch if
> you think that's clearer.

No makes totally sense, I just didn't see it because I'm GICed these
days.

Thanks for the explanation, and:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>