| Message ID | 20241219034035.1826173-3-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ppc: misc ppc patches | expand |
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);
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); > >
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); >> >>
On Fri Dec 20, 2024 at 7:14 PM AEST, 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)); I think I wanted to know which way it was failing, but I guess assert_cmpint() will tell us? I'll change. > > > + spapr_cpu->dispatched = dispatch; > > + > > + return; > > Returning here unconditionally makes below code unreachable. Good catch. That was a testing hack. Will respin and retest. Thanks, Nick
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h index 68f70834832..feba3446194 100644 --- a/include/hw/ppc/spapr_cpu_core.h +++ b/include/hw/ppc/spapr_cpu_core.h @@ -46,6 +46,7 @@ typedef struct SpaprCpuState { uint64_t vpa_addr; uint64_t slb_shadow_addr, slb_shadow_size; uint64_t dtl_addr, dtl_size; + bool dispatched; /* for vpa dispatch counter tracking */ bool prod; /* not migrated, only used to improve dispatch latencies */ struct ICPState *icp; struct XiveTCTX *tctx; @@ -60,4 +61,6 @@ static inline SpaprCpuState *spapr_cpu_state(PowerPCCPU *cpu) return (SpaprCpuState *)cpu->machine_data; } +void vpa_dispatch(CPUState *cs, SpaprCpuState *spapr_cpu, bool dispatch); + #endif diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 3b022e8da9e..fa05e0c5156 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -4519,47 +4519,15 @@ static void spapr_cpu_exec_enter(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu) { SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu); - /* These are only called by TCG, KVM maintains dispatch state */ - spapr_cpu->prod = false; - if (spapr_cpu->vpa_addr) { + if (!spapr_cpu->dispatched) { CPUState *cs = CPU(cpu); - uint32_t dispatch; - - dispatch = ldl_be_phys(cs->as, - spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER); - dispatch++; - if ((dispatch & 1) != 0) { - qemu_log_mask(LOG_GUEST_ERROR, - "VPA: incorrect dispatch counter value for " - "dispatched partition %u, correcting.\n", dispatch); - dispatch++; - } - stl_be_phys(cs->as, - spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, dispatch); + vpa_dispatch(cs, spapr_cpu, true); } } static void spapr_cpu_exec_exit(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu) { - SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu); - - if (spapr_cpu->vpa_addr) { - CPUState *cs = CPU(cpu); - uint32_t dispatch; - - dispatch = ldl_be_phys(cs->as, - spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER); - dispatch++; - if ((dispatch & 1) != 1) { - qemu_log_mask(LOG_GUEST_ERROR, - "VPA: incorrect dispatch counter value for " - "preempted partition %u, correcting.\n", dispatch); - dispatch++; - } - stl_be_phys(cs->as, - spapr_cpu->vpa_addr + VPA_DISPATCH_COUNTER, dispatch); - } } static void spapr_machine_class_init(ObjectClass *oc, void *data) 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); + } + spapr_cpu->dispatched = dispatch; + + return; + + if (!spapr_cpu->vpa_addr) { + return; + } + + /* 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);
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(-)