Re: [PATCH v2 9/9] spapr: implement nested-hv capability for the virtual hypervisor

["Cédric Le Goater" <clg@kaod.org>]

On 2/16/22 13:30, Nicholas Piggin wrote:
> Excerpts from Nicholas Piggin's message of February 16, 2022 9:38 pm:
>> Excerpts from Cédric Le Goater's message of February 16, 2022 8:52 pm:
>>> On 2/16/22 11:25, Nicholas Piggin wrote:
>>>> This implements the Nested KVM HV hcall API for spapr under TCG.
>>>>
>>>> The L2 is switched in when the H_ENTER_NESTED hcall is made, and the
>>>> L1 is switched back in returned from the hcall when a HV exception
>>>> is sent to the vhyp. Register state is copied in and out according to
>>>> the nested KVM HV hcall API specification.
>>>>
>>>> The hdecr timer is started when the L2 is switched in, and it provides
>>>> the HDEC / 0x980 return to L1.
>>>>
>>>> The MMU re-uses the bare metal radix 2-level page table walker by
>>>> using the get_pate method to point the MMU to the nested partition
>>>> table entry. MMU faults due to partition scope errors raise HV
>>>> exceptions and accordingly are routed back to the L1.
>>>>
>>>> The MMU does not tag translations for the L1 (direct) vs L2 (nested)
>>>> guests, so the TLB is flushed on any L1<->L2 transition (hcall entry
>>>> and exit).>
>>>> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>
>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>
>>> Some last comments below,
>>
>> [...]
>>
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index edbf3eeed0..852fe61b36 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -199,6 +199,9 @@ struct SpaprMachineState {
>>>>        bool has_graphics;
>>>>        uint32_t vsmt;       /* Virtual SMT mode (KVM's "core stride") */
>>>>    
>>>> +    /* Nested HV support (TCG only) */
>>>> +    uint64_t nested_ptcr;
>>>> +
>>>
>>> this is new state to migrate.
>>>
>>
>> [...]
>>
>>>> +/* Linux 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 */
>>>> +    uint64_t result;       /* Result of a system call */
>>>> +};
>>>
>>> I think we need to start moving all the spapr hcall definitions under
>>> spapr_hcall.h. It can come later.
>>
>> Sure.
>>
>> [...]
>>
>>>> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
>>>> index dab3dfc76c..b560514560 100644
>>>> --- a/include/hw/ppc/spapr_cpu_core.h
>>>> +++ b/include/hw/ppc/spapr_cpu_core.h
>>>> @@ -48,6 +48,11 @@ typedef struct SpaprCpuState {
>>>>        bool prod; /* not migrated, only used to improve dispatch latencies */
>>>>        struct ICPState *icp;
>>>>        struct XiveTCTX *tctx;
>>>> +
>>>> +    /* Fields for nested-HV support */
>>>> +    bool in_nested; /* true while the L2 is executing */
>>>> +    CPUPPCState *nested_host_state; /* holds the L1 state while L2 executes */
>>>> +    int64_t nested_tb_offset; /* L1->L2 TB offset */
>>>
>>> This needs a new vmstate.
>>
>> How about instead of the vmstate (we would need all the L1 state in
>> nested_host_state as well), we just add a migration blocker in the
>> L2 entry path. We could limit the max hdecr to say 1 second to
>> ensure it unblocks before long.
>>
>> I know migration blockers are not preferred but in this case it gives
>> us some iterations to debug and optimise first, which might change
>> the data to migrate.
> 
> This should be roughly the incremental patch to do this.

I think we can merge without it.

Adding support shouldn't be too complex and TCG migration of an L1
running L2 is not the most important feature today. It would be
better to have something clean (blocker if incomplete or a decent
support) before the 7.0 is released though

However, there is an issue with TCG migration and it has been there
for a while :

https://lore.kernel.org/qemu-devel/fb2e56cc-15d1-65ee-9d9c-64223483ed01@kaod.org/

Thanks,

C.