| Message ID | 20230719175400.647154-1-rananta@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fix CPUHP logic for protected KVM | expand |
On Wed, Jul 19, 2023 at 05:54:00PM +0000, Raghavendra Rao Ananta wrote: > For protected kvm, the CPU hotplug 'down' logic currently brings > down the timer and vGIC, essentially disabling interrupts. However, > because of how the 'kvm_arm_hardware_enabled' flag is designed, it > never re-enables them back on the CPU hotplug 'up' path. Hence, > clean up the logic to maintain the CPU hotplug up/down symmetry. Correct me if I am wrong, but this issue exists outside of cpu hotplug, right? init_subsystems() calls _kvm_arch_hardware_enable() on all cores, which only sets up the hyp cpu context and not the percpu interrupts. Similar issue exists for the cpu that calls do_pkvm_init(). I'll also note kvm_arm_hardware_enabled is deceptively vague, as it only keeps track of whether or not the hyp cpu context has been initialized. May send a cleanup here in a bit. Perhaps this for the changelog: KVM: arm64: Fix hardware enable/disable flows for pKVM When running in protected mode, the hyp stub is disabled after pKVM is initialized, meaning the host cannot enable/disable the hyp at runtime. As such, kvm_arm_hardware_enabled is always 1 after initialization, and kvm_arch_hardware_enable() never enables the vgic maintenance irq or timer irqs. Unconditionally enable/disable the vgic + timer irqs in the respective calls, instead relying on the percpu bookkeeping in the generic code to keep track of which cpus have the interrupts unmasked. > Fixes: 466d27e48d7c ("KVM: arm64: Simplify the CPUHP logic") > Reported-by: Oliver Upton <oliver.upton@linux.dev> > Suggested-by: Oliver Upton <oliver.upton@linux.dev> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > --- > arch/arm64/kvm/arm.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index c2c14059f6a8..010ebfa69650 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -1867,14 +1867,10 @@ static void _kvm_arch_hardware_enable(void *discard) > > int kvm_arch_hardware_enable(void) > { > - int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled); > - > _kvm_arch_hardware_enable(NULL); > > - if (!was_enabled) { > - kvm_vgic_cpu_up(); > - kvm_timer_cpu_up(); > - } > + kvm_vgic_cpu_up(); > + kvm_timer_cpu_up(); > > return 0; > } > @@ -1889,10 +1885,8 @@ static void _kvm_arch_hardware_disable(void *discard) > > void kvm_arch_hardware_disable(void) > { > - if (__this_cpu_read(kvm_arm_hardware_enabled)) { > - kvm_timer_cpu_down(); > - kvm_vgic_cpu_down(); > - } > + kvm_timer_cpu_down(); > + kvm_vgic_cpu_down(); > > if (!is_protected_kvm_enabled()) > _kvm_arch_hardware_disable(NULL); > -- > 2.41.0.487.g6d72f3e995-goog >
On Wed, Jul 19, 2023 at 1:48 PM Oliver Upton <oliver.upton@linux.dev> wrote: > > On Wed, Jul 19, 2023 at 05:54:00PM +0000, Raghavendra Rao Ananta wrote: > > For protected kvm, the CPU hotplug 'down' logic currently brings > > down the timer and vGIC, essentially disabling interrupts. However, > > because of how the 'kvm_arm_hardware_enabled' flag is designed, it > > never re-enables them back on the CPU hotplug 'up' path. Hence, > > clean up the logic to maintain the CPU hotplug up/down symmetry. > > Correct me if I am wrong, but this issue exists outside of cpu hotplug, > right? init_subsystems() calls _kvm_arch_hardware_enable() on all cores, > which only sets up the hyp cpu context and not the percpu interrupts. > Similar issue exists for the cpu that calls do_pkvm_init(). > Ah, perhaps I looked at the from a different perspective, but this makes sense too. > I'll also note kvm_arm_hardware_enabled is deceptively vague, as it only > keeps track of whether or not the hyp cpu context has been initialized. > May send a cleanup here in a bit. > > Perhaps this for the changelog: > > KVM: arm64: Fix hardware enable/disable flows for pKVM > > When running in protected mode, the hyp stub is disabled after pKVM is > initialized, meaning the host cannot enable/disable the hyp at > runtime. As such, kvm_arm_hardware_enabled is always 1 after > initialization, and kvm_arch_hardware_enable() never enables the vgic > maintenance irq or timer irqs. > > Unconditionally enable/disable the vgic + timer irqs in the respective > calls, instead relying on the percpu bookkeeping in the generic code > to keep track of which cpus have the interrupts unmasked. > Sure, we can use this for v2. Thanks, Raghavendra > > Fixes: 466d27e48d7c ("KVM: arm64: Simplify the CPUHP logic") > > Reported-by: Oliver Upton <oliver.upton@linux.dev> > > Suggested-by: Oliver Upton <oliver.upton@linux.dev> > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > --- > > arch/arm64/kvm/arm.c | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index c2c14059f6a8..010ebfa69650 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -1867,14 +1867,10 @@ static void _kvm_arch_hardware_enable(void *discard) > > > > int kvm_arch_hardware_enable(void) > > { > > - int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled); > > - > > _kvm_arch_hardware_enable(NULL); > > > > - if (!was_enabled) { > > - kvm_vgic_cpu_up(); > > - kvm_timer_cpu_up(); > > - } > > + kvm_vgic_cpu_up(); > > + kvm_timer_cpu_up(); > > > > return 0; > > } > > @@ -1889,10 +1885,8 @@ static void _kvm_arch_hardware_disable(void *discard) > > > > void kvm_arch_hardware_disable(void) > > { > > - if (__this_cpu_read(kvm_arm_hardware_enabled)) { > > - kvm_timer_cpu_down(); > > - kvm_vgic_cpu_down(); > > - } > > + kvm_timer_cpu_down(); > > + kvm_vgic_cpu_down(); > > > > if (!is_protected_kvm_enabled()) > > _kvm_arch_hardware_disable(NULL); > > -- > > 2.41.0.487.g6d72f3e995-goog > > > > -- > Thanks, > Oliver
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index c2c14059f6a8..010ebfa69650 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1867,14 +1867,10 @@ static void _kvm_arch_hardware_enable(void *discard) int kvm_arch_hardware_enable(void) { - int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled); - _kvm_arch_hardware_enable(NULL); - if (!was_enabled) { - kvm_vgic_cpu_up(); - kvm_timer_cpu_up(); - } + kvm_vgic_cpu_up(); + kvm_timer_cpu_up(); return 0; } @@ -1889,10 +1885,8 @@ static void _kvm_arch_hardware_disable(void *discard) void kvm_arch_hardware_disable(void) { - if (__this_cpu_read(kvm_arm_hardware_enabled)) { - kvm_timer_cpu_down(); - kvm_vgic_cpu_down(); - } + kvm_timer_cpu_down(); + kvm_vgic_cpu_down(); if (!is_protected_kvm_enabled()) _kvm_arch_hardware_disable(NULL);
For protected kvm, the CPU hotplug 'down' logic currently brings down the timer and vGIC, essentially disabling interrupts. However, because of how the 'kvm_arm_hardware_enabled' flag is designed, it never re-enables them back on the CPU hotplug 'up' path. Hence, clean up the logic to maintain the CPU hotplug up/down symmetry. Fixes: 466d27e48d7c ("KVM: arm64: Simplify the CPUHP logic") Reported-by: Oliver Upton <oliver.upton@linux.dev> Suggested-by: Oliver Upton <oliver.upton@linux.dev> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> --- arch/arm64/kvm/arm.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)