From owner-freebsd-embedded@FreeBSD.ORG Tue Feb 23 03:27:19 2010 Return-Path: Delivered-To: embedded@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E58541065670; Tue, 23 Feb 2010 03:27:19 +0000 (UTC) (envelope-from gnn@neville-neil.com) Received: from mail.meer.net (mail.meer.net [64.13.141.3]) by mx1.freebsd.org (Postfix) with ESMTP id CA4ED8FC18; Tue, 23 Feb 2010 03:27:18 +0000 (UTC) Received: from mail2.meer.net (mail2.meer.net [64.13.141.16]) by mail.meer.net (8.13.3/8.13.3/meer) with ESMTP id o1N3P3Xi048361; Mon, 22 Feb 2010 19:25:03 -0800 (PST) (envelope-from gnn@neville-neil.com) Received: from [192.168.0.12] (cpe-68-175-72-246.nyc.res.rr.com [68.175.72.246]) (authenticated bits=0) by mail2.meer.net (8.14.1/8.14.3) with ESMTP id o1N3P1rC028470 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Mon, 22 Feb 2010 19:25:02 -0800 (PST) (envelope-from gnn@neville-neil.com) Mime-Version: 1.0 (Apple Message framework v1077) Content-Type: text/plain; charset=us-ascii From: George Neville-Neil In-Reply-To: <867hqa9d0h.wl%koshy@unixconsulting.co.in> Date: Mon, 22 Feb 2010 22:25:00 -0500 Content-Transfer-Encoding: quoted-printable Message-Id: <3BF42672-9790-4D7F-9723-3D80601930B7@neville-neil.com> References: <42B59FCC-7A59-4383-BE4E-366B80B504BF@neville-neil.com> <867hqa9d0h.wl%koshy@unixconsulting.co.in> To: Joseph Koshy X-Mailer: Apple Mail (2.1077) Cc: embedded@freebsd.org, fabient@freebsd.org Subject: Re: First cut at hwpmc support on MIPS X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Feb 2010 03:27:20 -0000 On Feb 18, 2010, at 21:30 , Joseph Koshy wrote: >=20 >> Please review and let me know if you have comments. I'd like to = commit this >> in a week or so. >=20 >> http://people.freebsd.org/~gnn/mipshwpmc_1.diff >=20 > Nice work! >=20 Thanks. > Review comments, adding to those already commented on: >=20 > 1) There appears to be unnecessary gunk (UTF8?) in the manual page. >=20 > +.%B "MIPS32 24K Processor Core Family Software User<80><99>s = Manual" > ... > +to a D-cache miss. The LSU can signal a <80><9C>long = stall<80><9D> on a D-cache >=20 > etc. >=20 > You could use .Bq, .Sq. Plain ASCII quote marks would also work here. >=20 Fixed. > 2) The manual page is missing a short description of the capabilities > of these PMCs. >=20 Fixed. > 3) The code doesn't appear to support sampling; the manual page should > mention this. >=20 Fixed. > 4) The debug printf() in mips_intr should be removed: >=20 > +static int > +mips_intr(int cpu, struct trapframe *tf) > +{ > + printf("intr\n"); > + return 0; > +} >=20 Fixed. > 5) It would be help to split "hwpmc_mips.c" into two files: >=20 > - One for code specific to this PMC family. Say "hwpmc_mips24k.c". > - One for "generic" MIPS related code, e.g., > code to walk stacks. >=20 > You should expect that every manufacturer will have their own kind > of PMCs, with differing capabilities and programming models. >=20 > Catering for the variability upfront would be prudent. As > commented on by earlier reviewers, you may want to consider > changing symbol naming to suit. >=20 This is partly fixed, in that I moved to using symbols with 24K but I have not yet split the files. Probably easier now than later, but I'm not a huge fan of having the files proliferate for minor changes. I'm about to look at octeon and if I find that sufficiently different I'll do the split. > 6) These definitions will cause trouble on 64 bit MIPS systems: >=20 > +#define PMCLOG_READADDR PMCLOG_READ32 > +#define PMCLOG_EMITADDR PMCLOG_EMIT32 >=20 Good point. >=20 > 7) =46rom the definitions in the header file, these PMCs appear to > support the concept of sampling based on processor mode: >=20 > +#define MIPS_PMC_USER_ENABLE 0x08 /* Count in USER mode = */ > +#define MIPS_PMC_SUPER_ENABLE 0x04 /* Count in SUPERVISOR = mode */ > +#define MIPS_PMC_KERNEL_ENABLE 0x02 /* Count in KERNEL = mode */ >=20 > If that is the case, then you should support those modifiers in > libpmc's event parsing. The libpmc code in the patch appears to be > a stub: >=20 > +static int > +mips_allocate_pmc(enum pmc_event pe, char *ctrspec __unused, > + struct pmc_op_pmcallocate *pmc_config __unused) > +{ > + switch (pe) { > + default: > + break; > + } > + =20 > + return (0); > +} >=20 >=20 Is there any other processor that does this? Right now I make the chip sample in all modes by fiat. > 8) You can reduce the size of the following table in "hwpmc_mips.c", > by treating the pe_counter field as a set of flags. >=20 > +struct mips_event_code_map { > + enum pmc_event pe_ev; /* enum value */ > + uint8_t pe_counter; /* Which counter this can be = counted in. */ > + uint8_t pe_code; /* numeric code */ > +}; >=20 > +const struct mips_event_code_map mips_event_codes[] =3D { > + { PMC_EV_MIPS_CYCLE, 0, 0}, > + { PMC_EV_MIPS_CYCLE, 1, 0}, <<<--- repeated information=20 >=20 >=20 > 9) You'd want to support flags that control counting based on > processor modes. For this, you would want to pass down flags > from userland and change the `pm_mips_evsel' field to suit: >=20 > +static int > +mips_allocate_pmc(int cpu, int ri, struct pmc *pm, > + const struct pmc_op_pmcallocate *a) > +{ > ... > + pm->pm_md.pm_mips.pm_mips_evsel =3D config; >=20 Again, for both of these, is there an example I should work from? >=20 > 10) If the number and width of these PMCs are fixed, you should > document that in the manual page: >=20 > pmc_md_initialize() > { > + mips_npmcs =3D 2; > ... > + pcd->pcd_width =3D 32; /* XXX: Fix for 64 bit MIPS */ >=20 Fixed. Thanks for the feedback. Get back to me on these last couple of issues = and I'll finish the clean up, send out a new patch and then eventually = commit. Best, George