From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37415) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTCWD-0001EK-6r for qemu-devel@nongnu.org; Tue, 09 Feb 2016 12:56:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTCWC-000411-0U for qemu-devel@nongnu.org; Tue, 09 Feb 2016 12:56:05 -0500 Received: from mail-vk0-x22b.google.com ([2607:f8b0:400c:c05::22b]:34249) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTCWB-00040h-Pa for qemu-devel@nongnu.org; Tue, 09 Feb 2016 12:56:03 -0500 Received: by mail-vk0-x22b.google.com with SMTP id e185so122254093vkb.1 for ; Tue, 09 Feb 2016 09:56:03 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <56BA266D.1080808@codeaurora.org> References: <9a4369604c653bd21eca8509aa8b160d8cda951f.1454720020.git.alistair.francis@xilinx.com> <56BA266D.1080808@codeaurora.org> From: Peter Maydell Date: Tue, 9 Feb 2016 17:55:43 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christopher Covington Cc: Aaron Lindsay , Peter Crosthwaite , Nathan Rossi , QEMU Developers , Alistair Francis On 9 February 2016 at 17:48, Christopher Covington wrote: > On 02/09/2016 12:19 PM, Peter Maydell wrote: >> On 6 February 2016 at 00:55, Alistair Francis >> wrote: >>> Signed-off-by: Aaron Lindsay >>> Signed-off-by: Alistair Francis >>> Tested-by: Nathan Rossi >>> --- >>> >>> target-arm/cpu-qom.h | 2 ++ >>> target-arm/cpu.c | 2 ++ >>> target-arm/cpu64.c | 2 ++ >>> target-arm/helper.c | 8 ++++++++ >>> 4 files changed, 14 insertions(+) >>> >>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h >>> index 07c0a71..1cc4502 100644 >>> --- a/target-arm/cpu-qom.h >>> +++ b/target-arm/cpu-qom.h >>> @@ -148,6 +148,8 @@ typedef struct ARMCPU { >>> uint32_t id_pfr0; >>> uint32_t id_pfr1; >>> uint32_t id_dfr0; >>> + uint32_t pmceid0; >>> + uint32_t pmceid1; >>> uint32_t id_afr0; >>> uint32_t id_mmfr0; >>> uint32_t id_mmfr1; >>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >>> index 7ddbf3d..937f845 100644 >>> --- a/target-arm/cpu.c >>> +++ b/target-arm/cpu.c >>> @@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj) >>> cpu->id_pfr0 = 0x00001131; >>> cpu->id_pfr1 = 0x00011011; >>> cpu->id_dfr0 = 0x02010555; >>> + cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */ >> >> These are: >> SW_INCR # insn architecturally executed, cc pass, software increment >> INST_RETIRED # insn architecturally executed >> CPU_CYCLES # cycle >> >> However we don't actually implement any of these, so should >> we be advertising them? > > Perhaps I'm missing something, but I was under the impression that CPU > cycle accounting was implemented as pmccntr_read/write in > target-arm/helper.c. Yes, but that's not the same as implementing it as an event visible and controllable via the PM[X]EVCNTR and PM[X]EVTYPER register interface. (The cycle count has its own set of dedicated registers.) > The instruction count event may need a wrapper around cpu_get_icount(). > > SWINC is pretty trivial, but I don't think we actually use it, other > than for some testing (but unfortunately not yet part of the > kvm-unit-tests patchset). Yep, so we could implement an event or two. The question is (a) is it useful to do so? (quite possibly, though trying to get any kind of legitimate perf data out of a model is at best dicey) (b) is it useful to advertise their support in this patchset but not actually implement the events? (much less clear) If we want to add real events I think that should probably be its own patchset, and for this patchset we should stick to bringing the 64-bit PMU implementation into line with our existing 32-bit implementation (which is a "we support no event counters, only the separate cycle counter" implementation, which I think is architecturally permitted: PMCR_EL0.N == 0) Similarly in a later patch I don't think we should implement the non-trivial PM[X]EVCNTR/PM[X]EVTYPER registers and the selectors until we actually implement some real events to count, ie we should postpone that to that future patchset, not put it in this one. thanks -- PMM