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

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of February 15, 2022 12:32 am:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> This implements the nested-hv hcall API for spapr under TCG.
>> It's still a bit rough around the edges, concept seems to work.
>>
>> Some HV exceptions can be raised now in the TCG spapr machine when
>> running a nested guest. The main ones are the lev==1 syscall, the
>> hdecr, hdsi and hisi, and h_virt external interrupts. These are
>> dealt with in the interrupt delivery code by noticing MSR[HV] raised
>> and instead of switching the machine to HV mode, it exits the
>> H_ENTER_NESTED hcall with the interrupt vector as return value as
>> required by the hcall API.
>>
>> Address translation is provided by the 2-level page table walker
>> that is implemented for the pnv machine. The partition scope page
>> table is pointed to the L1's partition scope, and a few tests have
>> to take into account that nested-hv translations are 2-level. This
>> could perhaps be tidied up a bit e.g., with a 'bool two_level = ...'
>> but it's surprisingly little code.
>>
>> There is no TLB tagging between L1 and L2 translations at the moment
>> so the TLB is flushed on any L1<->L2 transition (hcall entry and exit).
>>
>> XXX: stop doing atomic RC on page table walks (not for nested but in general)
>>
>> not-yet-Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> 
> Hi Nick,
> 
> Sorry it took me so long to get to this. It think overall it looks
> good. I have some comments about code structure that you probably
> already know about.

Great thanks for the comments.

> I don't have answers to your XXX questions but I think it's ok to handle
> them later individually as they come up, after this is merged.

I've been able to remove most of them I think. I'll repost soon.

>> ---
>>  hw/ppc/ppc.c               |  20 +++
>>  hw/ppc/spapr.c             |  16 ++
>>  hw/ppc/spapr_caps.c        |   5 +-
>>  hw/ppc/spapr_hcall.c       | 316 +++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/ppc.h       |   3 +
>>  include/hw/ppc/spapr.h     |  75 ++++++++-
>>  target/ppc/cpu.h           |   6 +
>>  target/ppc/excp_helper.c   |  60 ++++---
>>  target/ppc/helper_regs.c   |   1 +
>>  target/ppc/mmu-book3s-v3.c |  20 ++-
>>  target/ppc/mmu-radix64.c   |  15 +-
>>  11 files changed, 499 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
>> index a7c262db93..135900a6f4 100644
>> --- a/hw/ppc/ppc.c
>> +++ b/hw/ppc/ppc.c
>> @@ -1083,6 +1083,26 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, 
>> uint32_t freq)
>>      return &cpu_ppc_set_tb_clk;
>>  }
>>  
>> +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);
>> +}
>> +
>>  /* Specific helpers for POWER & PowerPC 601 RTC */
>>  void cpu_ppc601_store_rtcu (CPUPPCState *env, uint32_t value)
>>  {
>> 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);
> 
> This looks like it could be plumbed along with spapr_hypercall below.

Hmm. when "QEMU" returns from this hypercall, TCG should be running the
L2 guest with L2 registers but the L1 itself has not returned from the
hcall yet.

So when I wrote it first the idea was that we don't want to overwrite
the L2's r3 register with env->gpr[3] return value... But now you make
me think about it the hcall implementation could return the L2's r3
value here.

Interesting point. I'll try it.

>> +static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
>> +                               SpaprMachineState *spapr,
>> +                               target_ulong opcode,
>> +                               target_ulong *args)
>> +{
>> +    /*
>> +     * This HCALL is not required, L1 KVM will take a slow path and walk the
>> +     * page tables manually to do the data copy.
>> +     */
> 
> Kind of unrelated to the patch itself but I thought the point of this
> hcall was less about performance and more about Ln not having access to
> the KVM memslots for the Ln+2.
> 
> KVM accesses a guest address space by using the GFN to find the memory
> slot and from there the userspace address that is part of the
> memslot. Accesses from Ln -> Ln+1 can use that mechanism because any Ln
> will have slots registered for its guests.
> 
> For L0 -> L2, we can go via radix quadrants because the L0 is the real
> HV (that's how L0 handles this hcall after all).
> 
> For L1 -> L3, we need the hcall since L1 cannot access quadrants 1 and 2
> and it does not have the memslot so it cannot use the QEMU address.
> 
> Does that make sense?

AFAIKS it's not used for Ln->Ln+2, only for Ln->Ln+1. In kvmppc_ld for
example the load_from_eaddr fast path uses this (or quadrants in case of
L0), and the fallback does kvmppc_xlate.

This patch did boot a L3 up to busybox, so it seems not to be required.

>> +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, true);
>> +    if (!hvstate || len != sizeof(*hvstate)) {
>> +        env->gpr[3] = H_PARAMETER;
>> +        return;
>> +    }
>> +
>> +    memcpy(&hv_state, hvstate, len);
>> +
>> +    cpu_physical_memory_unmap(hvstate, len, 0 /* read */, len /* access len 
>> */);
>> +
>> +    if (hv_state.version != HV_GUEST_STATE_VERSION) {
> 
> KVM uses > here. Should we do the same? Daniel and I have been
> discussing if we need some sort of version negotiation for nested KVM
> since recently someone tried to use an L0 version 1 with an L1 version
> 2:
> 
> https://gitlab.com/qemu-project/qemu/-/issues/860

Good question, I'm not quite sure how that would all be handled.
Actually we don't use the version 2 registers AFAIKS so we could
support both.

> 
>> +        env->gpr[3] = H_PARAMETER;
>> +        return;
>> +    }
>> +
> 
> <snip>
> 
>> +/* 64-bit powerpc pt_regs struct used by nested HV */
>> +struct kvmppc_pt_regs {
>> +    uint64_t gpr[32];
>> +    uint64_t nip;
>> +    uint64_t msr;
>> +    uint64_t orig_gpr3;    /* Used for restarting system calls */
>> +    uint64_t ctr;
>> +    uint64_t link;
>> +    uint64_t xer;
>> +    uint64_t ccr;
>> +    uint64_t softe;        /* Soft enabled/disabled */
>> +    uint64_t trap;         /* Reason for being here */
>> +    uint64_t dar;          /* Fault registers */
>> +    uint64_t dsisr;        /* on 4xx/Book-E used for ESR */
> 
> There's no 4xx/booke support for spapr nested.

Yeah that's just copy/paste from Linux.

>> @@ -375,9 +375,17 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu,
>>       * will prevent setting of the HV bit which some exceptions might need
>>       * to do.
>>       */
>> -    env->msr = msr & env->msr_mask;
>> -    hreg_compute_hflags(env);
>> -    env->nip = vector;
>> +    if (cpu->vhyp && cpu->in_spapr_nested && (msr & MSR_HVB)) {
>> +        PPCVirtualHypervisorClass *vhc =
>> +            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>> +        // Deliver interrupt to L1 by returning from the H_ENTER_NESTED call
>> +        vhc->exit_nested(cpu, excp);
>> +    } else {
>> +        env->nip = vector;
>> +        env->msr = msr & env->msr_mask;
>> +        hreg_compute_hflags(env);
>> +    }
>> +
> 
> All of this should fit better at the end of powerpc_excp_books. Even if
> we need to duplicate it for fwnmi_machine_check.

Okay I'll try that.

>>      /* Reset exception state */
>>      cs->exception_index = POWERPC_EXCP_NONE;
>>      env->error_code = 0;
>> @@ -548,7 +556,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>>      /* Save MSR */
>>      env->spr[srr1] = msr;
>>  
>> -    powerpc_set_excp_state(cpu, vector, new_msr);
>> +    powerpc_set_excp_state(cpu, excp, vector, new_msr);
>>  }
>>  
>>  static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>> @@ -742,7 +750,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>>      /* Save MSR */
>>      env->spr[SPR_SRR1] = msr;
>>  
>> -    powerpc_set_excp_state(cpu, vector, new_msr);
>> +    powerpc_set_excp_state(cpu, excp, vector, new_msr);
>>  }
>>  
>>  #ifdef TARGET_PPC64
>> @@ -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) {
>>              PPCVirtualHypervisorClass *vhc =
>>                  PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>>              vhc->hypercall(cpu->vhyp, cpu);
>> @@ -1004,18 +1012,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int 
>> excp)
>>          break;
>>      }
>>  
>> -    /* Sanity check */
>> -    if (!(env->msr_mask & MSR_HVB)) {
>> -        if (new_msr & MSR_HVB) {
>> -            cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
>> -                      "no HV support\n", excp);
>> -        }
>> -        if (srr0 == SPR_HSRR0) {
>> -            cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
>> -                      "no HV support\n", excp);
>> -        }
>> -    }
>> -
>>      /*
>>       * Sort out endianness of interrupt, this differs depending on the
>>       * CPU, the HV mode, etc...
>> @@ -1037,7 +1033,19 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int 
>> excp)
>>      /* This can update new_msr and vector if AIL applies */
>>      ppc_excp_apply_ail(cpu, excp_model, excp, msr, &new_msr, &vector);
>>  
>> -    powerpc_set_excp_state(cpu, vector, new_msr);
>> +    powerpc_set_excp_state(cpu, excp, vector, new_msr);
>> +
>> +    /* Sanity check */
>> +    if (!(env->msr_mask & MSR_HVB)) {
>> +        if (env->msr & MSR_HVB) {
>> +            cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
>> +                      "no HV support\n", excp);
>> +        }
> 
> FYI, this doesn't exist anymore.
> 
>> +        if (0 && srr0 == SPR_HSRR0) {
>> +            cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
>> +                      "no HV support\n", excp);
>> +        }
> 
> msr_mask should have MSR_HVB independently of MSR_HV being actually set,
> so this shouldn't be causing any problems. Is it?

Hmm, I'll have to re-check that. It did crash but that might have been a
slightly different version of the test.

>> @@ -1613,7 +1621,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>>          /* HEIC blocks delivery to the hypervisor */
>>          if ((async_deliver && !(heic && msr_hv && !msr_pr)) ||
>>              (env->has_hv_mode && msr_hv == 0 && !lpes0)) {
>> -            powerpc_excp(cpu, POWERPC_EXCP_EXTERNAL);
>> +            if (cpu->in_spapr_nested) {
>> +                powerpc_excp(cpu, POWERPC_EXCP_HVIRT);
>> +            } else {
>> +                powerpc_excp(cpu, POWERPC_EXCP_EXTERNAL);
>> +            }
> 
> This function affects other CPUs, so it's better to leave this as it
> were and change the exception inside powerpc_excp_books, like we do for
> HV_EMUL.

Good point.

>>              return;
>>          }
>>      }
>> @@ -1723,7 +1735,7 @@ void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, 
>> target_ulong vector)
>>          msr |= (1ULL << MSR_LE);
>>      }
>>  
>> -    powerpc_set_excp_state(cpu, vector, msr);
>> +    powerpc_set_excp_state(cpu, POWERPC_EXCP_MCHECK, vector, msr);
>>  }
>>  
>>  bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
>> index 5b12cb03c9..7f92606522 100644
>> --- a/target/ppc/helper_regs.c
>> +++ b/target/ppc/helper_regs.c
>> @@ -163,6 +163,7 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState 
>> *env)
>>          immu_idx |= msr & (1 << MSR_IS) ? 2 : 0;
>>          dmmu_idx |= msr & (1 << MSR_DS) ? 2 : 0;
>>      } else {
>> +        /* Could have nested IDX instead of HV to avoid tlb flush on nested 
>> enter/exit? */
> 
> Sounds like a good idea. However I don't know enough about softmmu stuff
> to point a direction here.

Yep, the trivial attempt (just add in_spapr_nested as a new bit in the 
mmu idx) failed :P Performance is not horrible as is though so it's not
necessary for the first version.

Thanks,
Nick