On Wed, Feb 24, 2021 at 12:58:02PM -0300, Fabiano Rosas wrote:
> > @@ -1590,6 +1662,24 @@ static int kvmppc_handle_nested_exit(struct kvm_vcpu *vcpu)
> > if (!xics_on_xive())
> > kvmppc_xics_rm_complete(vcpu, 0);
> > break;
> > + case BOOK3S_INTERRUPT_SYSCALL:
> > + {
> > + unsigned long req = kvmppc_get_gpr(vcpu, 3);
> > +
> > + /*
> > + * The H_RPT_INVALIDATE hcalls issued by nested
> > + * guests for process scoped invalidations when
> > + * GTSE=0, are handled here in L0.
> > + */
> > + if (req == H_RPT_INVALIDATE) {
> > + kvmppc_nested_rpt_invalidate(vcpu);
> > + r = RESUME_GUEST;
> > + break;
> > + }
>
> I'm inclined to say this is a bit too early. We're handling the hcall
> before kvmhv_run_single_vcpu has fully finished and we'll skip some
> code that has been running in all guest exits:
>
> if (trap) {
> if (!nested)
> r = kvmppc_handle_exit_hv(vcpu, current);
> else
> r = kvmppc_handle_nested_exit(vcpu); <--- we're here
> }
> vcpu->arch.ret = r;
>
> (...)
>
> vcpu->arch.ceded = 0;
>
> vc->vcore_state = VCORE_INACTIVE;
> trace_kvmppc_run_core(vc, 1);
>
> done:
> kvmppc_remove_runnable(vc, vcpu);
> trace_kvmppc_run_vcpu_exit(vcpu);
>
> return vcpu->arch.ret;
>
> Especially the kvmppc_remove_runnable function because it sets the
> vcpu state:
>
> vcpu->arch.state = KVMPPC_VCPU_BUSY_IN_HOST;
>
> which should be the case if we're handling a hypercall.
>
> I suggest we do similarly to the L1 exit code and defer the hcall
> handling until after kvmppc_run_single_vcpu has exited, still inside the
> is_kvmppc_resume_guest(r) loop.
>
> So we'd set:
> case BOOK3S_INTERRUPT_SYSCALL:
> vcpu->run->exit_reason = KVM_EXIT_PAPR_HCALL;
> r = RESUME_HOST;
> break;
>
> and perhaps introduce a new kvmppc_pseries_do_nested_hcall that's called
> after kvmppc_run_single_vcpu.
Yes, looks like we should, but I wasn't sure if an exit similar to L1
exit for hcall handling is needed here too, hence took this approach.
Paul, could you please clarify?
Regards,
Bharata.
