Excerpts from Nicholas Piggin's message of February 27, 2021 10:21 am:
> Excerpts from Fabiano Rosas's message of February 27, 2021 8:37 am:
>> Nicholas Piggin <npiggin@xxxxxxxxx> writes:
>>
>> Hi, thanks for this. It helped me clarify a bunch of details that I
>> haven't understood while reading the asm code.
>
> Yes, me too :)
>
>>> +/*
>>> + * void kvmppc_p9_enter_guest(struct vcpu *vcpu);
>>> + *
>>> + * Enter the guest on a ISAv3.0 or later system where we have exactly
>>> + * one vcpu per vcore, and both the host and guest are radix, and threads
>>> + * are set to "indepdent mode".
>>> + */
>>> +.balign IFETCH_ALIGN_BYTES
>>> +_GLOBAL(kvmppc_p9_enter_guest)
>>> +EXPORT_SYMBOL_GPL(kvmppc_p9_enter_guest)
>>> + mflr r0
>>> + std r0,PPC_LR_STKOFF(r1)
>>> + stdu r1,-SFS(r1)
>>> +
>>> + std r1,HSTATE_HOST_R1(r13)
>>> + std r3,STACK_SLOT_VCPU(r1)
>>
>> The vcpu was stored on the paca previously. I don't get the change,
>> could you explain?
>
> The stack is a nicer place to keep things. We only need one place to
> save the stack, then most things can come from there. There's actually
> more paca stuff we could move into local variables or onto the stack
> too.
>
> It was probably done like this because PR KVM which probably can't
> access the stack in real mode when running in an LPAR, and came across
> to the HV code that way. New path doesn't require it.
>
>>> +kvmppc_p9_exit_interrupt:
>>> + std r1,HSTATE_SCRATCH1(r13)
>>> + std r3,HSTATE_SCRATCH2(r13)
>>> + ld r1,HSTATE_HOST_R1(r13)
>>> + ld r3,STACK_SLOT_VCPU(r1)
>>> +
>>> + std r9,VCPU_CR(r3)
>>> +
>>> +1:
>>> + std r11,VCPU_PC(r3)
>>> + std r12,VCPU_MSR(r3)
>>> +
>>> + reg = 14
>>> + .rept 18
>>> + std reg,__VCPU_GPR(reg)(r3)
>>> + reg = reg + 1
>>> + .endr
>>> +
>>> + /* r1, r3, r9-r13 are saved to vcpu by C code */
>>
>> If we just saved r1 and r3, why don't we put them in the vcpu right now?
>> That would avoid having the C code knowing about scratch areas.
>
> Putting it in C avoids having the asm code know about scratch areas.
>
>>> @@ -4429,11 +4432,19 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
>>> else
>>> r = kvmppc_run_vcpu(vcpu);
>>>
>>> - if (run->exit_reason == KVM_EXIT_PAPR_HCALL &&
>>> - !(vcpu->arch.shregs.msr & MSR_PR)) {
>>> - trace_kvm_hcall_enter(vcpu);
>>> - r = kvmppc_pseries_do_hcall(vcpu);
>>> - trace_kvm_hcall_exit(vcpu, r);
>>> + if (run->exit_reason == KVM_EXIT_PAPR_HCALL) {
>>> + if (unlikely(vcpu->arch.shregs.msr & MSR_PR)) {
>>> + /*
>>> + * Guest userspace executed sc 1, reflect it
>>> + * back as a privileged program check interrupt.
>>> + */
>>> + kvmppc_core_queue_program(vcpu, SRR1_PROGPRIV);
>>> + r = RESUME_GUEST;
>>
>> This is in conflict with this snippet in kvmppc_handle_exit_hv:
>>
>> case BOOK3S_INTERRUPT_SYSCALL:
>> {
>> /* hcall - punt to userspace */
>> int i;
>>
>> /* hypercall with MSR_PR has already been handled in rmode,
>> * and never reaches here.
>> */
>>
>> That function already queues some 0x700s so maybe we could move this one
>> in there as well.
>
> I don't think it conflicts, but I think perhaps it should go in the
> patch which removed the real mode handlers.
Oh I'm wrong, it's actually the other way around by the looks.
Okay I'll fix this up.
Thanks,
Nick
