Re: [PATCH 2/5] spapr: Fix vpa dispatch count for record-replay

[Harsh Prateek Bora]

Hi Nick,

On 12/19/24 09:10, Nicholas Piggin wrote:
> The dispatch count is a field in guest memory that the hypervisor
> increments when preempting and dispatching the guest. This was not
> being done deterministically with respect to icount, because tcg
> exec exit is not deterministic (e.g., an async event could cause it).
>
> Change vpa dispatch count increment to keep track of whether the
> vCPU is considered dispatched or not, and only consider it preempted
> when calling cede / confer / join / stop-self / etc.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   include/hw/ppc/spapr_cpu_core.h |  3 +++
>   hw/ppc/spapr.c                  | 36 ++-------------------------------
>   hw/ppc/spapr_hcall.c            | 33 ++++++++++++++++++++++++++++++
>   hw/ppc/spapr_rtas.c             |  1 +
>   4 files changed, 39 insertions(+), 34 deletions(-)

<snipped>

> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 5e1d020e3df..907e09c2c36 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -487,6 +487,36 @@ static target_ulong h_register_vpa(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>       return ret;
>   }
>   
> +void vpa_dispatch(CPUState *cs, SpaprCpuState *spapr_cpu, bool dispatch)
> +{
> +    uint32_t counter;
> +
> +    if (!dispatch) {
> +        assert(spapr_cpu->dispatched);
> +    } else {
> +        assert(!spapr_cpu->dispatched);
> +    }

Would it look better as:
       assert(!(dispatch == spapr_cpu->dispatched));

> +    spapr_cpu->dispatched = dispatch;
> +
> +    return;

Returning here unconditionally makes below code unreachable.

> +
> +    if (!spapr_cpu->vpa_addr) {
> +        return;
> +    }

This could be moved to beginning or just after assert.

regards,
Harsh

> +
> +    /* These are only called by TCG, KVM maintains dispatch state */
> +    counter = ldl_be_phys(cs->as, spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER);
> +    counter++;
> +    if ((counter & 1) != dispatch) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "VPA: incorrect dispatch counter value for "
> +                      "%s partition %u, correcting.\n",
> +                      dispatch ? "preempted" : "running", counter);
> +        counter++;
> +    }
> +    stl_be_phys(cs->as, spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, counter);
> +}
> +
>   static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>                              target_ulong opcode, target_ulong *args)
>   {
> @@ -505,6 +535,7 @@ static target_ulong h_cede(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>   
>       if (!cpu_has_work(cs)) {
>           cs->halted = 1;
> +        vpa_dispatch(cs, spapr_cpu, false);
>           cs->exception_index = EXCP_HLT;
>           cs->exit_request = 1;
>           ppc_maybe_interrupt(env);
> @@ -531,6 +562,8 @@ static target_ulong h_confer_self(PowerPCCPU *cpu)
>       cs->exit_request = 1;
>       ppc_maybe_interrupt(&cpu->env);
>   
> +    vpa_dispatch(cs, spapr_cpu, false);
> +
>       return H_SUCCESS;
>   }
>   
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index f329693c554..8ce42302234 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -216,6 +216,7 @@ static void rtas_stop_self(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>        */
>       env->spr[SPR_PSSCR] |= PSSCR_EC;
>       cs->halted = 1;
> +    vpa_dispatch(cs, spapr_cpu_state(cpu), false);
>       ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
>       kvmppc_set_reg_ppc_online(cpu, 0);
>       qemu_cpu_kick(cs);