On Thu, May 04, 2017 at 02:51:39PM +0200, Paolo Bonzini wrote:
>
>
> On 04/05/2017 14:06, Andrew Jones wrote:
> >>> +VCPU threads may need to consider requests before and/or after calling
> >>> +functions that may put them to sleep, e.g. kvm_vcpu_block(). Whether they
> >>> +do or not, and, if they do, which requests need consideration, is
> >>> +architecture dependent. kvm_vcpu_block() calls kvm_arch_vcpu_runnable()
> >>> +to check if it should awaken. One reason to do so is to provide
> >>> +architectures a function where requests may be checked if necessary.
> >> What did you have in mind here?
> > I was trying to point out vcpu request concerns with respect to sleeping
> > vcpus, but to stay as general as possible. I can't really think of
> > anything else to say here, other than to give some hypothetical example.
> > For a while I was thinking I might check requests (kvm_request_pending())
> > from the kvm_arch_vcpu_runnable() call for ARM, but then changed my mind
> > on that - leaving it only checking the pause and power_off booleans.
> > Anyway, I don't think the above paragraph is "wrong", but if it's
> > confusing then I can change / remove it as people like. Just let me know
> > how you'd like it changed :-)
>
> I think the x86 scheme, where you only process requests once you have
> decided you'll get IN_GUEST_MODE, is a good one.
>
> That is, they _may_ check some requests in kvm_arch_vcpu_runnable but
> not process them.
This was my thought too, but checking that there are pending requests
seems like a valid reason to unblock - although only for certain requests.
>
> For ARM this would be:
>
> if (vcpu->arch.power_off || vcpu->arch.pause) {
> vcpu_sleep(vcpu);
> ret = 0;
> } else {
> ret = vcpu_enter_guest(vcpu);
> }
>
> where vcpu_enter_guest is basically the "while (ret > 0)" loop in
> kvm_arch_vcpu_ioctl_run:
I'm not sure this refactoring is necessary, but I can experiment
with it.
>
>
> /*
> * Check conditions before entering the guest
> */
> cond_resched();
>
> update_vttbr(vcpu->kvm);
> preempt_disable();
> ...
> if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> vcpu->arch.power_off || vcpu->arch.pause) {
This needs to check kvm_requests_pending(), like patch 3/10 adds.
> local_irq_enable();
> kvm_pmu_sync_hwstate(vcpu);
> kvm_timer_sync_hwstate(vcpu);
> kvm_vgic_sync_hwstate(vcpu);
> preempt_enable();
> return ret;
> }
> ...
> preempt_enable();
> return handle_exit(vcpu, run, ret);
>
> In your case, you don't need to check any request in
> kvm_arch_vcpu_runnable, I think.
Right. That was might final determination as well, so I don't check
any requests there with this series. I still tried writing this paragraph
to capture the general idea though, as I still think it's a valid idea to
want to check for certain pending requests in kvm_arch_vcpu_runnable(),
in order to know if a wakeup is necessary.
> This split would also solve my review
> doubt from "Re: [PATCH v3 05/10] KVM: arm/arm64: don't clear exit
I haven't received your doubt yet. Problem with mail delivery? Or did
you forget to send it :-)
> request from caller".
>
> Paolo
Thanks,
drew
