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

[BALATON Zoltan]

On Fri, 20 Dec 2024, Harsh Prateek Bora wrote:
> 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));

That's more cryptic than the one above. Maybe with ternary operator to 
avoid the if? Like
assert(dispatch ? spapr_cpu->dispatched : !spapr_cpu->dispatched)

Regards,
BALATON Zoltan

> > +    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);