* [PATCH 0/2] GICv3 world-switch fixes for -next
@ 2017-04-10 9:19 Marc Zyngier
2017-04-10 9:19 ` [PATCH 1/2] KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating a GICv2 Marc Zyngier
2017-04-10 9:19 ` [PATCH 2/2] KVM: arm/arm64: vgic-v3: Fix off-by-one LR access Marc Zyngier
0 siblings, 2 replies; 6+ messages in thread
From: Marc Zyngier @ 2017-04-10 9:19 UTC (permalink / raw)
To: linux-arm-kernel
Here's a couple of fixes for issues I've spotted when running our
current -next branch on a GICv3 system.
The first patch fixes our v2-on-v3 handling of VMCR_EL2, which cannot
be safely save/restored when ICC_SRE_EL1.SRE is set (in that case,
VMCR_EL2.VFIQen is RES1, and Group0 interrupts get delivered as
FIQ). This is effectively de-optimizing VMCR_EL2 save/restore for
v2-on-v3, which is a pretty rare use case.
The second patch fixes a typo that leads to a HYP panic when the CPU
accesses an unimplemented list register, or the corruption of some
memory if all 16 architectural list registers are used.
Both patches tested on -rc6 + kvmarm/next on a rk3399 platform.
Marc Zyngier (2):
KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating
a GICv2
KVM: arm/arm64: vgic-v3: Fix off-by-one LR access
virt/kvm/arm/hyp/vgic-v3-sr.c | 10 +++++++---
virt/kvm/arm/vgic/vgic.c | 26 ++++++++++++++++++++------
2 files changed, 27 insertions(+), 9 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating a GICv2
2017-04-10 9:19 [PATCH 0/2] GICv3 world-switch fixes for -next Marc Zyngier
@ 2017-04-10 9:19 ` Marc Zyngier
2017-04-19 10:08 ` Christoffer Dall
2017-04-10 9:19 ` [PATCH 2/2] KVM: arm/arm64: vgic-v3: Fix off-by-one LR access Marc Zyngier
1 sibling, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2017-04-10 9:19 UTC (permalink / raw)
To: linux-arm-kernel
When emulating a GICv2-on-GICv3, special care must be taken to only
save/restore VMCR_EL2 when ICC_SRE_EL1.SRE is cleared. Otherwise,
all Group-0 interrupts end-up being delivered as FIQ, which is
probably not what the guest expects, as demonstrated here with
an unhappy EFI:
FIQ Exception at 0x000000013BD21CC4
This means that we cannot perform the load/put trick when dealing
with VMCR_EL2 (because the host has SRE set), and we have to deal
with it in the world-switch.
Fortunately, this is not the most common case (modern guests should
be able to deal with GICv3 directly), and the performance is not worse
than what it was before the VMCR optimization.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
virt/kvm/arm/hyp/vgic-v3-sr.c | 8 ++++++--
virt/kvm/arm/vgic/vgic.c | 26 ++++++++++++++++++++------
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
index 3d0b1ddb6929..91922c1eddc8 100644
--- a/virt/kvm/arm/hyp/vgic-v3-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
@@ -128,8 +128,10 @@ 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.
*/
- if (!cpu_if->vgic_sre)
+ if (!cpu_if->vgic_sre) {
dsb(st);
+ cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2);
+ }
if (used_lrs) {
int i;
@@ -205,11 +207,13 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
* delivered as a FIQ to the guest, with potentially fatal
* consequences. So we must make sure that ICC_SRE_EL1 has
* been actually programmed with the value we want before
- * starting to mess with the rest of the GIC.
+ * starting to mess with the rest of the GIC, and VMCR_EL2 in
+ * particular.
*/
if (!cpu_if->vgic_sre) {
write_gicreg(0, ICC_SRE_EL1);
isb();
+ write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2);
}
val = read_gicreg(ICH_VTR_EL2);
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 3d0979c30721..32205f2ba6ff 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -667,10 +667,20 @@ void kvm_vgic_load(struct kvm_vcpu *vcpu)
if (unlikely(!vgic_initialized(vcpu->kvm)))
return;
- if (kvm_vgic_global_state.type == VGIC_V2)
+ if (kvm_vgic_global_state.type == VGIC_V2) {
vgic_v2_load(vcpu);
- else
- vgic_v3_load(vcpu);
+ } else {
+ struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
+
+ /*
+ * If dealing with a GICv2 emulation on GICv3,
+ * VMCR_EL2.VFIQen is dependent on ICC_SRE_EL1.SRE,
+ * and we have to perform the VMCR_EL2 save/restore in
+ * the world switch.
+ */
+ if (likely(cpu_if->vgic_sre))
+ vgic_v3_load(vcpu);
+ }
}
void kvm_vgic_put(struct kvm_vcpu *vcpu)
@@ -678,10 +688,14 @@ void kvm_vgic_put(struct kvm_vcpu *vcpu)
if (unlikely(!vgic_initialized(vcpu->kvm)))
return;
- if (kvm_vgic_global_state.type == VGIC_V2)
+ if (kvm_vgic_global_state.type == VGIC_V2) {
vgic_v2_put(vcpu);
- else
- vgic_v3_put(vcpu);
+ } else {
+ struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
+
+ if (likely(cpu_if->vgic_sre))
+ vgic_v3_put(vcpu);
+ }
}
int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: arm/arm64: vgic-v3: Fix off-by-one LR access
2017-04-10 9:19 [PATCH 0/2] GICv3 world-switch fixes for -next Marc Zyngier
2017-04-10 9:19 ` [PATCH 1/2] KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating a GICv2 Marc Zyngier
@ 2017-04-10 9:19 ` Marc Zyngier
2017-04-19 10:09 ` Christoffer Dall
1 sibling, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2017-04-10 9:19 UTC (permalink / raw)
To: linux-arm-kernel
When iterating over the used LRs, be careful not to try to access
an unused LR, or even an unimplemented one if you're unlucky...
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
virt/kvm/arm/hyp/vgic-v3-sr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
index 91922c1eddc8..bce6037cf01d 100644
--- a/virt/kvm/arm/hyp/vgic-v3-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
@@ -143,7 +143,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
val = read_gicreg(ICH_VTR_EL2);
nr_pri_bits = vtr_to_nr_pri_bits(val);
- for (i = 0; i <= used_lrs; i++) {
+ for (i = 0; i < used_lrs; i++) {
if (cpu_if->vgic_elrsr & (1 << i))
cpu_if->vgic_lr[i] &= ~ICH_LR_STATE;
else
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating a GICv2
2017-04-10 9:19 ` [PATCH 1/2] KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating a GICv2 Marc Zyngier
@ 2017-04-19 10:08 ` Christoffer Dall
2017-04-19 10:31 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Christoffer Dall @ 2017-04-19 10:08 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 10, 2017 at 10:19:43AM +0100, Marc Zyngier wrote:
> When emulating a GICv2-on-GICv3, special care must be taken to only
> save/restore VMCR_EL2 when ICC_SRE_EL1.SRE is cleared. Otherwise,
> all Group-0 interrupts end-up being delivered as FIQ, which is
> probably not what the guest expects, as demonstrated here with
> an unhappy EFI:
>
> FIQ Exception at 0x000000013BD21CC4
>
> This means that we cannot perform the load/put trick when dealing
> with VMCR_EL2 (because the host has SRE set), and we have to deal
> with it in the world-switch.
>
> Fortunately, this is not the most common case (modern guests should
> be able to deal with GICv3 directly), and the performance is not worse
> than what it was before the VMCR optimization.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> virt/kvm/arm/hyp/vgic-v3-sr.c | 8 ++++++--
> virt/kvm/arm/vgic/vgic.c | 26 ++++++++++++++++++++------
> 2 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> index 3d0b1ddb6929..91922c1eddc8 100644
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> @@ -128,8 +128,10 @@ 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.
> */
> - if (!cpu_if->vgic_sre)
> + if (!cpu_if->vgic_sre) {
> dsb(st);
> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2);
> + }
>
> if (used_lrs) {
> int i;
> @@ -205,11 +207,13 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
> * delivered as a FIQ to the guest, with potentially fatal
> * consequences. So we must make sure that ICC_SRE_EL1 has
> * been actually programmed with the value we want before
> - * starting to mess with the rest of the GIC.
> + * starting to mess with the rest of the GIC, and VMCR_EL2 in
> + * particular.
> */
> if (!cpu_if->vgic_sre) {
> write_gicreg(0, ICC_SRE_EL1);
> isb();
> + write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2);
> }
>
> val = read_gicreg(ICH_VTR_EL2);
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 3d0979c30721..32205f2ba6ff 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -667,10 +667,20 @@ void kvm_vgic_load(struct kvm_vcpu *vcpu)
> if (unlikely(!vgic_initialized(vcpu->kvm)))
> return;
>
> - if (kvm_vgic_global_state.type == VGIC_V2)
> + if (kvm_vgic_global_state.type == VGIC_V2) {
> vgic_v2_load(vcpu);
> - else
> - vgic_v3_load(vcpu);
> + } else {
> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + /*
> + * If dealing with a GICv2 emulation on GICv3,
> + * VMCR_EL2.VFIQen is dependent on ICC_SRE_EL1.SRE,
> + * and we have to perform the VMCR_EL2 save/restore in
> + * the world switch.
> + */
> + if (likely(cpu_if->vgic_sre))
> + vgic_v3_load(vcpu);
I wonder if this conditional should really be inside vgic_v3_load
instead?
> + }
> }
>
> void kvm_vgic_put(struct kvm_vcpu *vcpu)
> @@ -678,10 +688,14 @@ void kvm_vgic_put(struct kvm_vcpu *vcpu)
> if (unlikely(!vgic_initialized(vcpu->kvm)))
> return;
>
> - if (kvm_vgic_global_state.type == VGIC_V2)
> + if (kvm_vgic_global_state.type == VGIC_V2) {
> vgic_v2_put(vcpu);
> - else
> - vgic_v3_put(vcpu);
> + } else {
> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (likely(cpu_if->vgic_sre))
> + vgic_v3_put(vcpu);
And same here?
> + }
> }
>
> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
> --
> 2.11.0
>
Otherwise looks good.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: arm/arm64: vgic-v3: Fix off-by-one LR access
2017-04-10 9:19 ` [PATCH 2/2] KVM: arm/arm64: vgic-v3: Fix off-by-one LR access Marc Zyngier
@ 2017-04-19 10:09 ` Christoffer Dall
0 siblings, 0 replies; 6+ messages in thread
From: Christoffer Dall @ 2017-04-19 10:09 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Apr 10, 2017 at 10:19:44AM +0100, Marc Zyngier wrote:
> When iterating over the used LRs, be careful not to try to access
> an unused LR, or even an unimplemented one if you're unlucky...
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> virt/kvm/arm/hyp/vgic-v3-sr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> index 91922c1eddc8..bce6037cf01d 100644
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> @@ -143,7 +143,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
> val = read_gicreg(ICH_VTR_EL2);
> nr_pri_bits = vtr_to_nr_pri_bits(val);
>
> - for (i = 0; i <= used_lrs; i++) {
> + for (i = 0; i < used_lrs; i++) {
> if (cpu_if->vgic_elrsr & (1 << i))
> cpu_if->vgic_lr[i] &= ~ICH_LR_STATE;
> else
> --
> 2.11.0
>
Whoops, thanks for fixing my stupid mistake:
Reviewed-by: Christoffer Dall <cdall@linaro.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating a GICv2
2017-04-19 10:08 ` Christoffer Dall
@ 2017-04-19 10:31 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2017-04-19 10:31 UTC (permalink / raw)
To: linux-arm-kernel
On 19/04/17 11:08, Christoffer Dall wrote:
> On Mon, Apr 10, 2017 at 10:19:43AM +0100, Marc Zyngier wrote:
>> When emulating a GICv2-on-GICv3, special care must be taken to only
>> save/restore VMCR_EL2 when ICC_SRE_EL1.SRE is cleared. Otherwise,
>> all Group-0 interrupts end-up being delivered as FIQ, which is
>> probably not what the guest expects, as demonstrated here with
>> an unhappy EFI:
>>
>> FIQ Exception at 0x000000013BD21CC4
>>
>> This means that we cannot perform the load/put trick when dealing
>> with VMCR_EL2 (because the host has SRE set), and we have to deal
>> with it in the world-switch.
>>
>> Fortunately, this is not the most common case (modern guests should
>> be able to deal with GICv3 directly), and the performance is not worse
>> than what it was before the VMCR optimization.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> virt/kvm/arm/hyp/vgic-v3-sr.c | 8 ++++++--
>> virt/kvm/arm/vgic/vgic.c | 26 ++++++++++++++++++++------
>> 2 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
>> index 3d0b1ddb6929..91922c1eddc8 100644
>> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
>> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
>> @@ -128,8 +128,10 @@ 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.
>> */
>> - if (!cpu_if->vgic_sre)
>> + if (!cpu_if->vgic_sre) {
>> dsb(st);
>> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2);
>> + }
>>
>> if (used_lrs) {
>> int i;
>> @@ -205,11 +207,13 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>> * delivered as a FIQ to the guest, with potentially fatal
>> * consequences. So we must make sure that ICC_SRE_EL1 has
>> * been actually programmed with the value we want before
>> - * starting to mess with the rest of the GIC.
>> + * starting to mess with the rest of the GIC, and VMCR_EL2 in
>> + * particular.
>> */
>> if (!cpu_if->vgic_sre) {
>> write_gicreg(0, ICC_SRE_EL1);
>> isb();
>> + write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2);
>> }
>>
>> val = read_gicreg(ICH_VTR_EL2);
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 3d0979c30721..32205f2ba6ff 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -667,10 +667,20 @@ void kvm_vgic_load(struct kvm_vcpu *vcpu)
>> if (unlikely(!vgic_initialized(vcpu->kvm)))
>> return;
>>
>> - if (kvm_vgic_global_state.type == VGIC_V2)
>> + if (kvm_vgic_global_state.type == VGIC_V2) {
>> vgic_v2_load(vcpu);
>> - else
>> - vgic_v3_load(vcpu);
>> + } else {
>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>> +
>> + /*
>> + * If dealing with a GICv2 emulation on GICv3,
>> + * VMCR_EL2.VFIQen is dependent on ICC_SRE_EL1.SRE,
>> + * and we have to perform the VMCR_EL2 save/restore in
>> + * the world switch.
>> + */
>> + if (likely(cpu_if->vgic_sre))
>> + vgic_v3_load(vcpu);
>
> I wonder if this conditional should really be inside vgic_v3_load
> instead?
That's a good point. It will keep the wart out of the main vgic code and
make it an "implementation detail".
>
>> + }
>> }
>>
>> void kvm_vgic_put(struct kvm_vcpu *vcpu)
>> @@ -678,10 +688,14 @@ void kvm_vgic_put(struct kvm_vcpu *vcpu)
>> if (unlikely(!vgic_initialized(vcpu->kvm)))
>> return;
>>
>> - if (kvm_vgic_global_state.type == VGIC_V2)
>> + if (kvm_vgic_global_state.type == VGIC_V2) {
>> vgic_v2_put(vcpu);
>> - else
>> - vgic_v3_put(vcpu);
>> + } else {
>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>> +
>> + if (likely(cpu_if->vgic_sre))
>> + vgic_v3_put(vcpu);
>
> And same here?
>
>> + }
>> }
>>
>> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
>> --
>> 2.11.0
>>
>
> Otherwise looks good.
Thanks. I'll respin this patch shortly.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-19 10:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-10 9:19 [PATCH 0/2] GICv3 world-switch fixes for -next Marc Zyngier
2017-04-10 9:19 ` [PATCH 1/2] KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating a GICv2 Marc Zyngier
2017-04-19 10:08 ` Christoffer Dall
2017-04-19 10:31 ` Marc Zyngier
2017-04-10 9:19 ` [PATCH 2/2] KVM: arm/arm64: vgic-v3: Fix off-by-one LR access Marc Zyngier
2017-04-19 10:09 ` Christoffer Dall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).