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

[Harsh Prateek Bora]

On 12/20/24 14:59, BALATON Zoltan wrote:
> 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)

Yeh, initially I thought of ternary op, but thought of it as more 
cryptic. I guess either is fine. Minor correction though:

  assert(dispatch ? !spapr_cpu->dispatched : spapr_cpu->dispatched)

regards,
Harsh

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