On Sat, Oct 14, 2017 at 09:13:25PM +0200, Christoffer Dall wrote: > On Fri, Sep 29, 2017 at 01:30:40PM +0200, Andrew Jones wrote: > > Conceptually, kvm_arch_vcpu_runnable() should be "not waiting, > > or waiting for interrupts and interrupts are pending", > > > > !waiting-uninterruptable && > > (!waiting-for-interrupts || interrupts-pending) > > > > but the implementation was only > > > > !waiting-uninterruptable && interrupts-pending > > > > Thanks to the context of the two callers there wasn't an issue, > > however, to future-proof the function, this patch implements the > > conceptual condition by applying mp_state to track waiting- > > uninterruptable vs. waiting-for-interrupts. > > > > Signed-off-by: Andrew Jones <drjo...@redhat.com> > > --- > > Documentation/virtual/kvm/api.txt | 10 ++++++---- > > virt/kvm/arm/arm.c | 13 +++++++++---- > > 2 files changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/virtual/kvm/api.txt > > b/Documentation/virtual/kvm/api.txt > > index e63a35fafef0..7c0bb1ae10a2 100644 > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -1071,7 +1071,7 @@ Possible values are: > > - KVM_MP_STATE_INIT_RECEIVED: the vcpu has received an INIT signal, and > > is > > now ready for a SIPI [x86] > > - KVM_MP_STATE_HALTED: the vcpu has executed a HLT instruction > > and > > - is waiting for an interrupt [x86] > > + is waiting for an interrupt > > [x86,arm/arm64] > > - KVM_MP_STATE_SIPI_RECEIVED: the vcpu has just received a SIPI (vector > > accessible via KVM_GET_VCPU_EVENTS) [x86] > > - KVM_MP_STATE_STOPPED: the vcpu is stopped [s390,arm/arm64] > > @@ -1087,8 +1087,9 @@ these architectures. > > > > For arm/arm64: > > > > -The only states that are valid are KVM_MP_STATE_STOPPED and > > -KVM_MP_STATE_RUNNABLE which reflect if the vcpu is paused or not. > > +The only states that are valid are KVM_MP_STATE_STOPPED, > > KVM_MP_STATE_HALTED, > > +and KVM_MP_STATE_RUNNABLE which reflect if the vcpu is powered-off, waiting > > +for interrupts, or powered-on and not waiting for interrupts. > > > > Is it valid to introduce another value after we've publicly declared > that it's not valid? What if userspace has an assert(state == > KVM_MP_STATE_STOPPED || state == KVM_MP_STATE_RUNNABLE) ? > > At least we should make that is not the case.
The thought had crossed my mind that we might need a new VCPU feature, but it didn't seem necessary after checking QEMU code, and I didn't like the idea of adding one. But... if we consider some theoretical KVM userspace, then maybe we need it? > > > 4.39 KVM_SET_MP_STATE > > > > @@ -1108,7 +1109,8 @@ these architectures. > > For arm/arm64: > > > > The only states that are valid are KVM_MP_STATE_STOPPED and > > -KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be paused or not. > > +KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be > > +powered-off or not. > > > > It looks pretty dodgy to me that we can return an MP state which we > cannot set again. Feels like this could break migration. While respinning for Marc's changes, I had some second thoughts about this too. I was going to do some testing with QEMU and maybe change it and QEMU so that it would save/restore whatever the state is, including KVM_MP_STATE_HALTED. I'll do that checking and experimenting. > > > 4.40 KVM_SET_IDENTITY_MAP_ADDR > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > > index 220a3dbda76c..5bc9b0d2fd0f 100644 > > --- a/virt/kvm/arm/arm.c > > +++ b/virt/kvm/arm/arm.c > > @@ -414,13 +414,16 @@ static bool vcpu_should_sleep(struct kvm_vcpu *vcpu) > > * kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled > > * @vcpu: The VCPU pointer > > * > > - * If the guest CPU is not waiting for interrupts or an interrupt line is > > - * asserted, the CPU is by definition runnable. > > + * If the VCPU is not waiting at all (including sleeping, which is waiting > > + * uninterruptably), or it's waiting for interrupts but interrupts are > > + * pending, then it is by definition runnable. > > */ > > int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu) > > { > > - return (!!vcpu->arch.irq_lines || kvm_vgic_vcpu_pending_irq(vcpu)) > > - && !vcpu_should_sleep(vcpu); > > + return !vcpu_should_sleep(vcpu) && > > + (vcpu->arch.mp_state != KVM_MP_STATE_HALTED || > > + (!!vcpu->arch.irq_lines || > > + kvm_vgic_vcpu_pending_irq(vcpu))); > > This is hard to read. How about: > > bool irq_pending = !!vcpu->arch.irq_lines || > kvm_vgic_vcpu_pending_irq(vcpu); > > if (vcpu_should_sleep(vcpu) > return false; > else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED && !irq_pending) > return false; > return true; Using local variables does improve this, but I think I prefer bool halted = vcpu->arch.mp_state == KVM_MP_STATE_HALTED; bool irq_pending = !!vcpu->arch.irq_lines || kvm_vgic_vcpu_pending_irq(vcpu); return !vcpu_should_sleep(vcpu) && (!halted || irq_pending); > > > } > > > > bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu) > > @@ -582,7 +585,9 @@ void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu) > > void kvm_arm_emulate_wfi(struct kvm_vcpu *vcpu) > > { > > vcpu->stat.wfi_exit_stat++; > > + vcpu->arch.mp_state = KVM_MP_STATE_HALTED; > > kvm_vcpu_block(vcpu); > > + vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE; > > kvm_clear_request(KVM_REQ_UNHALT, vcpu); > > } > > > > -- > > 2.13.5 > > > > Thanks, > -Christoffer Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm