KVM: arm64: Discard PC update state on vcpu reset
Subsystems (3)
Marc Zyngier
maintainerPATCH 1/1 KVM: arm64: Discard PC update state on vcpu reset
Our vcpu reset suffers from a particularly interesting flaw, as it does not correctly deal with state that will have an effect on the execution flow out of reset.
Take the following completely random example, never seen in the wild and that never resulted in a couple of sleepless nights: /s
- vcpu-A issues a PSCI_CPU_OFF using the SMC conduit
- SMC being a trapped instruction (as opposed to HVC which is always normally executed), we annotate the vcpu as needing to skip the next instruction, which is the SMC itself
- vcpu-A is now safely off
- vcpu-B issues a PSCI_CPU_ON for vcpu-A, providing a starting PC
- vcpu-A gets reset, get the new PC, and is sent on its merry way
- right at the point of entering the guest, we notice that a PC increment is pending (remember the earlier SMC?)
- vcpu-A skips its first instruction...
What could possibly go wrong?
Well, I'm glad you asked. For pKVM as a NV guest, that first instruction is extremely significant, as it indicates whether the CPU is booting or resuming. Having skipped that instruction, nothing makes any sense anymore, and CPU hotplugging fails.
This is all caused by the decoupling of PC update from the handling of an exception that triggers such update, making it non-obvious what affects what when.
Fix this train wreck by discarding all the PC-affecting state on vcpu reset.
--- arch/arm64/kvm/reset.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
arch/arm64/kvm/reset.c +14 -0
@@ -247,6 +247,20 @@ void kvm_reset_vcpu(struct kvm_vcpu *vcpu)kvm_vcpu_set_be (vcpu); *vcpu_pc (vcpu) = target_pc; + +/* +* We may come from a state where either a PC update was +* pending (SMC call resulting in PC being increpented to +* skip the SMC) or a pending exception. Make sure we get +* rid of all that, as this cannot be valid out of reset. +* +* Note that clearing the exception mask also clears PC +* updates, but that's an implementation detail, and we +* really want to make it explicit. +*/ +vcpu_clear_flag (vcpu,PENDING_EXCEPTION ); +vcpu_clear_flag (vcpu,EXCEPT_MASK ); +vcpu_clear_flag (vcpu,INCREMENT_PC );vcpu_set_reg (vcpu,0 , reset_state.r0); }
Signature · 1 line
2.47.3
Quoted 22 lines · Marc Zyngier
Our vcpu reset suffers from a particularly interesting flaw, as it does not correctly deal with state that will have an effect on the execution flow out of reset.
Take the following completely random example, never seen in the wild and that never resulted in a couple of sleepless nights: /s
- vcpu-A issues a
PSCI_CPU_OFFusing the SMC conduit- SMC being a trapped instruction (as opposed to HVC which is always normally executed), we annotate the vcpu as needing to skip the next instruction, which is the SMC itself
- vcpu-A is now safely off
- vcpu-B issues a
PSCI_CPU_ONfor vcpu-A, providing a starting PC- vcpu-A gets reset, get the new PC, and is sent on its merry way
- right at the point of entering the guest, we notice that a PC increment is pending (remember the earlier SMC?)
Quoted 8 lines
- vcpu-A skips its first instruction...
What could possibly go wrong?
Well, I'm glad you asked. For pKVM as a NV guest, that first instruction is extremely significant, as it indicates whether the CPU is booting or resuming. Having skipped that instruction, nothing makes any sense anymore, and CPU hotplugging fails.
Would the normal method of offlining/onlining via sysfs also be affected?
Quoted 7 lines
This is all caused by the decoupling of PC update from the handling of an exception that triggers such update, making it non-obvious what affects what when.
Fix this train wreck by discarding all the PC-affecting state on vcpu reset.
Good job on tracking it down.. makes you wonder why the DSB "fixed" things!
Quoted 13 lines
Fixes f5e30680616ab ("KVM: arm64: Move __adjust_pc out of line")Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/reset.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)arch/arm64/kvm/reset.c +0
-0
arch/arm64/kvm/reset.c void kvm_reset_vcpu(struct kvm_vcpu *vcpu) · line 253
Quoted 7 lines
arch/arm64/kvm/reset.c +4
-0@@ -247,6 +247,20 @@ void kvm_reset_vcpu(struct kvm_vcpu *vcpu)kvm_vcpu_set_be (vcpu); *vcpu_pc (vcpu) = target_pc; + +/* +* We may come from a state where either a PC update was +* pending (SMC call resulting in PC being increpented to
incremented
arch/arm64/kvm/reset.c void kvm_reset_vcpu(struct kvm_vcpu *vcpu) · line 266
Quoted 13 lines
arch/arm64/kvm/reset.c +10
-0+* skip the SMC) or a pending exception. Make sure we get +* rid of all that, as this cannot be valid out of reset. +* +* Note that clearing the exception mask also clears PC +* updates, but that's an implementation detail, and we +* really want to make it explicit. +*/ +vcpu_clear_flag (vcpu,PENDING_EXCEPTION ); +vcpu_clear_flag (vcpu,EXCEPT_MASK ); +vcpu_clear_flag (vcpu,INCREMENT_PC );vcpu_set_reg (vcpu,0 , reset_state.r0); }
Thanks,
Joey
Marc Zyngier
maintainerQuoted 26 lines · Joey Gouly
Quoted 24 lines · Marc Zyngier
Our vcpu reset suffers from a particularly interesting flaw, as it does not correctly deal with state that will have an effect on the execution flow out of reset.
Take the following completely random example, never seen in the wild and that never resulted in a couple of sleepless nights: /s
- vcpu-A issues a
PSCI_CPU_OFFusing the SMC conduit- SMC being a trapped instruction (as opposed to HVC which is always normally executed), we annotate the vcpu as needing to skip the next instruction, which is the SMC itself
- vcpu-A is now safely off
- vcpu-B issues a
PSCI_CPU_ONfor vcpu-A, providing a starting PC- vcpu-A gets reset, get the new PC, and is sent on its merry way
- right at the point of entering the guest, we notice that a PC increment is pending (remember the earlier SMC?)
- vcpu-A skips its first instruction...
Quoted 8 lines
Quoted 6 lines
What could possibly go wrong?
Well, I'm glad you asked. For pKVM as a NV guest, that first instruction is extremely significant, as it indicates whether the CPU is booting or resuming. Having skipped that instruction, nothing makes any sense anymore, and CPU hotplugging fails.
Would the normal method of offlining/onlining via sysfs also be affected?
I'm not sure there's any other. That's how I tested it, anyway, since you can't do a late onlining with pKVM.
Quoted 9 lines
Quoted 7 lines
This is all caused by the decoupling of PC update from the handling of an exception that triggers such update, making it non-obvious what affects what when.
Fix this train wreck by discarding all the PC-affecting state on vcpu reset.
Good job on tracking it down.. makes you wonder why the DSB "fixed" things!
Because it was exactly at the correct spot to hide the crap. But NOP, or even UDF would have had the same effect.
Quoted 22 lines
Quoted 14 lines
Fixes f5e30680616ab ("KVM: arm64: Move __adjust_pc out of line")Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/reset.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)arch/arm64/kvm/reset.c +0
-0arch/arm64/kvm/reset.c void kvm_reset_vcpu(struct kvm_vcpu *vcpu) · line 253
Quoted 7 lines
arch/arm64/kvm/reset.c +4
-0@@ -247,6 +247,20 @@ void kvm_reset_vcpu(struct kvm_vcpu *vcpu)kvm_vcpu_set_be (vcpu); *vcpu_pc (vcpu) = target_pc; + +/* +* We may come from a state where either a PC update was +* pending (SMC call resulting in PC being increpented to incremented
Quoted 15 lines
arch/arm64/kvm/reset.c void kvm_reset_vcpu(struct kvm_vcpu *vcpu) · line 266
Quoted 13 lines
arch/arm64/kvm/reset.c +10
-0+* skip the SMC) or a pending exception. Make sure we get +* rid of all that, as this cannot be valid out of reset. +* +* Note that clearing the exception mask also clears PC +* updates, but that's an implementation detail, and we +* really want to make it explicit. +*/ +vcpu_clear_flag (vcpu,PENDING_EXCEPTION ); +vcpu_clear_flag (vcpu,EXCEPT_MASK ); +vcpu_clear_flag (vcpu,INCREMENT_PC );vcpu_set_reg (vcpu,0 , reset_state.r0); }Reviewed-by Joey Gouly <[email protected]>
Thanks!
M.
Signature · 1 line
Jazz isn't dead. It just smells funny.
Quoted 59 lines · Marc Zyngier
Our vcpu reset suffers from a particularly interesting flaw, as it does not correctly deal with state that will have an effect on the execution flow out of reset.
Take the following completely random example, never seen in the wild and that never resulted in a couple of sleepless nights: /s
- vcpu-A issues a
PSCI_CPU_OFFusing the SMC conduit- SMC being a trapped instruction (as opposed to HVC which is always normally executed), we annotate the vcpu as needing to skip the next instruction, which is the SMC itself
- vcpu-A is now safely off
- vcpu-B issues a
PSCI_CPU_ONfor vcpu-A, providing a starting PC- vcpu-A gets reset, get the new PC, and is sent on its merry way
- right at the point of entering the guest, we notice that a PC increment is pending (remember the earlier SMC?)
- vcpu-A skips its first instruction...
What could possibly go wrong?
Well, I'm glad you asked. For pKVM as a NV guest, that first instruction is extremely significant, as it indicates whether the CPU is booting or resuming. Having skipped that instruction, nothing makes any sense anymore, and CPU hotplugging fails.
This is all caused by the decoupling of PC update from the handling of an exception that triggers such update, making it non-obvious what affects what when.
Fix this train wreck by discarding all the PC-affecting state on vcpu reset.
Fixes f5e30680616ab ("KVM: arm64: Move __adjust_pc out of line")Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/reset.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)arch/arm64/kvm/reset.c +6
-0@@ -247,6 +247,20 @@ void kvm_reset_vcpu(struct kvm_vcpu *vcpu)kvm_vcpu_set_be (vcpu); *vcpu_pc (vcpu) = target_pc; + +/* +* We may come from a state where either a PC update was +* pending (SMC call resulting in PC being increpented to +* skip the SMC) or a pending exception. Make sure we get +* rid of all that, as this cannot be valid out of reset.
arch/arm64/kvm/reset.c void kvm_reset_vcpu(struct kvm_vcpu *vcpu) · line 265
Quoted 10 lines
arch/arm64/kvm/reset.c +8
-0+* +* Note that clearing the exception mask also clears PC +* updates, but that's an implementation detail, and we +* really want to make it explicit. +*/ +vcpu_clear_flag (vcpu,PENDING_EXCEPTION ); +vcpu_clear_flag (vcpu,EXCEPT_MASK ); +vcpu_clear_flag (vcpu,INCREMENT_PC );vcpu_set_reg (vcpu,0 , reset_state.r0); }
Wow! Thats it finally !! Glad you found the root cause.
Quoted 8 lines · Marc Zyngier
Our vcpu reset suffers from a particularly interesting flaw, as it does not correctly deal with state that will have an effect on the execution flow out of reset.
Take the following completely random example, never seen in the wild and that never resulted in a couple of sleepless nights: /s
[...]
Applied to fixes, thanks!
[1/1] KVM: arm64: Discard PC update state on vcpu reset
commit: 1744a6ef48b9a48f017e3e1a0d05de0a6978396e
Cheers,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.