| Message ID | 20211015125152.25198-13-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: drop hypercall function tables | expand |
On 15.10.2021 14:51, Juergen Gross wrote: > The HVM hypercall handler is missing incrementing the per hypercall > counters. Add that. > > The counters for PV are handled wrong, as they are not using > perf_incra() with the number of the hypercall as index, but are > incrementing the total number of hypercalls only. Fix that. Why do you say "total number"? Isn't it that all accounting goes into set_trap_table's slot, effectively making that slot a "total number" despite not being labeled that way? Also this fix renders largely redundant the calls_to_multicall counter. Could I talk you into deleting that at the same time? (As to the "not fully redundant": I consider it suspicious that this counter gets incremented at the bottom of the function, not at the top.) Finally I take it that with the Kconfig setting being under DEBUG, we don't consider security supported builds with PERF_COUNTERS enabled. Otherwise as a prereq I would think perfc_incra() would need teaching of array_index_nospec(). In any event, preferably with at least the description slightly adjusted, Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 21.10.21 17:19, Jan Beulich wrote: > On 15.10.2021 14:51, Juergen Gross wrote: >> The HVM hypercall handler is missing incrementing the per hypercall >> counters. Add that. >> >> The counters for PV are handled wrong, as they are not using >> perf_incra() with the number of the hypercall as index, but are >> incrementing the total number of hypercalls only. Fix that. > > Why do you say "total number"? Isn't it that all accounting goes into > set_trap_table's slot, effectively making that slot a "total number" > despite not being labeled that way? Oh, right. Will correct. > Also this fix renders largely redundant the calls_to_multicall counter. > Could I talk you into deleting that at the same time? (As to the "not > fully redundant": I consider it suspicious that this counter gets > incremented at the bottom of the function, not at the top.) I think I'll just another patch doing that. > Finally I take it that with the Kconfig setting being under DEBUG, we > don't consider security supported builds with PERF_COUNTERS enabled. > Otherwise as a prereq I would think perfc_incra() would need teaching > of array_index_nospec(). I agree. > In any event, preferably with at least the description slightly > adjusted, > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. Juergen
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index e766cf4c72..599921dc48 100644 --- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -214,6 +214,8 @@ int hvm_hypercall(struct cpu_user_regs *regs) ioreq_signal_mapcache_invalidate(); } + perfc_incra(hypercalls, eax); + return curr->hcall_preempted ? HVM_HCALL_preempted : HVM_HCALL_completed; } diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c index 9b575e5c0b..ec8d2c7f87 100644 --- a/xen/arch/x86/pv/hypercall.c +++ b/xen/arch/x86/pv/hypercall.c @@ -106,7 +106,7 @@ _pv_hypercall(struct cpu_user_regs *regs, bool compat) if ( curr->hcall_preempted ) regs->rip -= 2; - perfc_incr(hypercalls); + perfc_incra(hypercalls, eax); } enum mc_disposition pv_do_multicall_call(struct mc_state *state)
The HVM hypercall handler is missing incrementing the per hypercall counters. Add that. The counters for PV are handled wrong, as they are not using perf_incra() with the number of the hypercall as index, but are incrementing the total number of hypercalls only. Fix that. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/arch/x86/hvm/hypercall.c | 2 ++ xen/arch/x86/pv/hypercall.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)