| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+EHjTz3AAv2HyfHZBVjtB-0Y4c-_ifC4bPpx1sP0Wtm_Rqs2A@mail.gmail.com>
Date: Thu, 1 Jul 2021 10:45:18 +0100
From: Fuad Tabba <tabba@...gle.com>
To: Jean-Philippe Brucker <jean-philippe@...aro.org>
Cc: maz@...nel.org, salil.mehta@...wei.com, lorenzo.pieralisi@....com,
kvm@...r.kernel.org, corbet@....net, catalin.marinas@....com,
linux-kernel@...r.kernel.org, will@...nel.org,
jonathan.cameron@...wei.com, pbonzini@...hat.com,
kvmarm@...ts.cs.columbia.edu, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH 2/5] KVM: arm64: Move WFI execution to check_vcpu_requests()
Hi Jean-Philippe,
On Tue, Jun 8, 2021 at 4:54 PM Jean-Philippe Brucker
<jean-philippe@...aro.org> wrote:
>
> Prepare for WFI requests from userspace, by adding a suspend request and
> moving the WFI execution into check_vcpu_requests(), next to the
> power-off logic.
>
> vcpu->arch.mp_state, previously only RUNNABLE or STOPPED, supports an
> additional state HALTED and two new state transitions:
>
> RUNNABLE -> HALTED from WFI or PSCI CPU_SUSPEND (same vCPU)
> HALTED -> RUNNABLE vGIC IRQ, pending timer, signal
>
> There shouldn't be any functional change with this patch, even though
> the KVM_GET_MP_STATE ioctl could now in theory return
> KVM_MP_STATE_HALTED, which would break some users' mp_state support. In
> practice it should not happen because we do not return to userspace with
> HALTED state. Both WFI and PSCI CPU_SUSPEND stay in the vCPU run loop
> until the suspend request is consumed. It does feel fragile though,
> maybe we should explicitly return RUNNABLE in KVM_GET_MP_STATE in place
> of HALTED, to prevent future breakage.
It's not really a functional change, but it might introduce some
timing/scheduling changes I think.
Before your changes, the kvm_vcpu_block() would take place at the end
of the vCPU run loop, via handle_exit(). Now it takes place closer to
the beginning, after cond_resched() is called, and not if there is a
KVM_REQ_IRQ_PENDING.
If my observation is correct, would it be good to mention that?
Thanks,
/fuad
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@...aro.org>
> ---
> arch/arm64/include/asm/kvm_host.h | 2 ++
> arch/arm64/kvm/arm.c | 18 ++++++++++++++-
> arch/arm64/kvm/handle_exit.c | 3 +--
> arch/arm64/kvm/psci.c | 37 +++++++++++++------------------
> 4 files changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 55a04f4d5919..3ca732feb9a5 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -46,6 +46,7 @@
> #define KVM_REQ_VCPU_RESET KVM_ARCH_REQ(2)
> #define KVM_REQ_RECORD_STEAL KVM_ARCH_REQ(3)
> #define KVM_REQ_RELOAD_GICv4 KVM_ARCH_REQ(4)
> +#define KVM_REQ_SUSPEND KVM_ARCH_REQ(5)
>
> #define KVM_DIRTY_LOG_MANUAL_CAPS (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
> KVM_DIRTY_LOG_INITIALLY_SET)
> @@ -722,6 +723,7 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> struct kvm_device_attr *attr);
> void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
> bool kvm_arm_vcpu_is_off(struct kvm_vcpu *vcpu);
> +void kvm_arm_vcpu_suspend(struct kvm_vcpu *vcpu);
>
> /* Guest/host FPSIMD coordination helpers */
> int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index bcc24adb9c0a..d8cbaa0373c7 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -447,6 +447,12 @@ bool kvm_arm_vcpu_is_off(struct kvm_vcpu *vcpu)
> return vcpu->arch.mp_state == KVM_MP_STATE_STOPPED;
> }
>
> +void kvm_arm_vcpu_suspend(struct kvm_vcpu *vcpu)
> +{
> + vcpu->arch.mp_state = KVM_MP_STATE_HALTED;
> + kvm_make_request(KVM_REQ_SUSPEND, vcpu);
> +}
> +
> int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
> struct kvm_mp_state *mp_state)
> {
> @@ -667,6 +673,8 @@ static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>
> static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> {
> + bool irq_pending;
> +
> if (kvm_request_pending(vcpu)) {
> if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> vcpu_req_sleep(vcpu);
> @@ -678,7 +686,7 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> * Clear IRQ_PENDING requests that were made to guarantee
> * that a VCPU sees new virtual interrupts.
> */
> - kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
> + irq_pending = kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
>
> if (kvm_check_request(KVM_REQ_RECORD_STEAL, vcpu))
> kvm_update_stolen_time(vcpu);
> @@ -690,6 +698,14 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> vgic_v4_load(vcpu);
> preempt_enable();
> }
> +
> + if (kvm_check_request(KVM_REQ_SUSPEND, vcpu)) {
> + if (!irq_pending) {
> + kvm_vcpu_block(vcpu);
> + kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> + }
> + vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> + }
> }
> }
>
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 6f48336b1d86..9717df3104cf 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -95,8 +95,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
> } else {
> trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
> vcpu->stat.wfi_exit_stat++;
> - kvm_vcpu_block(vcpu);
> - kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> + kvm_arm_vcpu_suspend(vcpu);
> }
>
> kvm_incr_pc(vcpu);
> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index 24b4a2265dbd..42a307ceb95f 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -31,27 +31,6 @@ static unsigned long psci_affinity_mask(unsigned long affinity_level)
> return 0;
> }
>
> -static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
> -{
> - /*
> - * NOTE: For simplicity, we make VCPU suspend emulation to be
> - * same-as WFI (Wait-for-interrupt) emulation.
> - *
> - * This means for KVM the wakeup events are interrupts and
> - * this is consistent with intended use of StateID as described
> - * in section 5.4.1 of PSCI v0.2 specification (ARM DEN 0022A).
> - *
> - * Further, we also treat power-down request to be same as
> - * stand-by request as-per section 5.4.2 clause 3 of PSCI v0.2
> - * specification (ARM DEN 0022A). This means all suspend states
> - * for KVM will preserve the register state.
> - */
> - kvm_vcpu_block(vcpu);
> - kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> -
> - return PSCI_RET_SUCCESS;
> -}
> -
> static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> {
> struct vcpu_reset_state *reset_state;
> @@ -227,7 +206,21 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu)
> break;
> case PSCI_0_2_FN_CPU_SUSPEND:
> case PSCI_0_2_FN64_CPU_SUSPEND:
> - val = kvm_psci_vcpu_suspend(vcpu);
> + /*
> + * NOTE: For simplicity, we make VCPU suspend emulation to be
> + * same-as WFI (Wait-for-interrupt) emulation.
> + *
> + * This means for KVM the wakeup events are interrupts and this
> + * is consistent with intended use of StateID as described in
> + * section 5.4.1 of PSCI v0.2 specification (ARM DEN 0022A).
> + *
> + * Further, we also treat power-down request to be same as
> + * stand-by request as-per section 5.4.2 clause 3 of PSCI v0.2
> + * specification (ARM DEN 0022A). This means all suspend states
> + * for KVM will preserve the register state.
> + */
> + kvm_arm_vcpu_suspend(vcpu);
> + val = PSCI_RET_SUCCESS;
> break;
> case PSCI_0_2_FN_CPU_OFF:
> kvm_arm_vcpu_power_off(vcpu);
> --
> 2.31.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@...ts.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Powered by blists - more mailing lists