Re: [RFC PATCH 3/3] spapr: implement nested-hv support for the TCG virtual hypervisor

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Cédric Le Goater's message of February 15, 2022 4:31 am:
> On 2/10/22 07:53, Nicholas Piggin wrote:
>> +void cpu_ppc_hdecr_init (CPUPPCState *env)
>> +{
>> +    PowerPCCPU *cpu = env_archcpu(env);
>> +
>> +    assert(env->tb_env->hdecr_timer == NULL);
>> +
>> +    env->tb_env->hdecr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_hdecr_cb,
>> +                                                 cpu);
>> +}
>> +
>> +void cpu_ppc_hdecr_exit (CPUPPCState *env)
>> +{
>> +    PowerPCCPU *cpu = env_archcpu(env);
>> +
>> +    timer_free(env->tb_env->hdecr_timer);
>> +    env->tb_env->hdecr_timer = NULL;
>> +
>> +    cpu_ppc_hdecr_lower(cpu);
>> +}
> 
> 
> So these are called every time a L2 enters or exits ?

Yes.

>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 3d6ec309dd..f0c3f726f2 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1273,6 +1273,8 @@ static void emulate_spapr_hypercall(PPCVirtualHypervisor *vhyp,
>>       if (msr_pr) {
>>           hcall_dprintf("Hypercall made with MSR[PR]=1\n");
>>           env->gpr[3] = H_PRIVILEGE;
>> +    } else if (env->gpr[3] == KVMPPC_H_ENTER_NESTED) {
>> +        spapr_enter_nested(cpu);
> 
> Can not this be in the hcall table ?

See reply to Fabiano, I think it can be.

>>       } else {
>>           env->gpr[3] = spapr_hypercall(cpu, env->gpr[3], &env->gpr[4]);
>>       }
>> @@ -4465,6 +4467,17 @@ PowerPCCPU *spapr_find_cpu(int vcpu_id)
>>       return NULL;
>>   }
>>   
>> +static bool spapr_cpu_in_nested(PowerPCCPU *cpu)
>> +{
>> +    return cpu->in_spapr_nested;
>> +}
> 
> This handler is not very much used.

Yeah, I have improved that in the current code, hopefully doesn't 
open-code cpu->in_spapr_nested anywhere outside spapr specifics.

>> +void spapr_enter_nested(PowerPCCPU *cpu)
>> +{
>> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> +    CPUState *cs = CPU(cpu);
>> +    CPUPPCState *env = &cpu->env;
>> +    target_ulong hv_ptr = env->gpr[4];
>> +    target_ulong regs_ptr = env->gpr[5];
>> +    target_ulong hdec, now = cpu_ppc_load_tbl(env);
>> +    struct kvmppc_hv_guest_state *hvstate;
>> +    struct kvmppc_hv_guest_state hv_state;
>> +    struct kvmppc_pt_regs *regs;
>> +    hwaddr len;
>> +    uint32_t cr;
>> +    int i;
>> +
>> +    if (cpu->in_spapr_nested) {
>> +        env->gpr[3] = H_FUNCTION;
>> +        return;
>> +    }
>> +    if (spapr->nested_ptcr == 0) {
>> +        env->gpr[3] = H_NOT_AVAILABLE;
>> +        return;
>> +    }
>> +
>> +    len = sizeof(*hvstate);
>> +    hvstate = cpu_physical_memory_map(hv_ptr, &len, 
> 
> Are you writing to the state ? address_space_map() is a better pratice.

Yes, in exit_nested it gets written. I'll take a look at 
address_space_map().

>> @@ -916,7 +924,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>>           env->nip += 4;
>>   
>>           /* "PAPR mode" built-in hypercall emulation */
>> -        if ((lev == 1) && cpu->vhyp) {
>> +        if ((lev == 1) && cpu->vhyp && !cpu->in_spapr_nested) {
> 
> an helper for (cpu->vhyp && !cpu->in_spapr_nested) would help.

Yeah, I put some helpers in the mmu code since this patch which is 
nicer. Will try to do the same for this.

Thanks,
Nick