First cut at hwpmc support on MIPS

[George Neville-Neil]

On Feb 18, 2010, at 21:30 , Joseph Koshy wrote:

> 
>> Please review and let me know if you have comments.  I'd like to commit this
>> in a week or so.
> 
>> http://people.freebsd.org/~gnn/mipshwpmc_1.diff
> 
> Nice work!
> 

Thanks.

> Review comments, adding to those already commented on:
> 
> 1) There appears to be unnecessary gunk (UTF8?) in the manual page.
> 
> +.%B "MIPS32 24K Processor Core Family Software User<E2><80><99>s Manual"
> ...
> +to a D-cache miss. The LSU can signal a <E2><80><9C>long stall<E2><80><9D> on a D-cache
> 
> etc.
> 
> You could use .Bq, .Sq.  Plain ASCII quote marks would also work here.
> 

Fixed.

> 2) The manual page is missing a short description of the capabilities
>   of these PMCs.
> 

Fixed.


> 3) The code doesn't appear to support sampling; the manual page should
>   mention this.
> 

Fixed.

> 4) The debug printf() in mips_intr should be removed:
> 
> +static int
> +mips_intr(int cpu, struct trapframe *tf)
> +{
> +       printf("intr\n");
> +       return 0;
> +}
> 

Fixed.

> 5) It would be help to split "hwpmc_mips.c" into two files:
> 
>   - One for code specific to this PMC family.  Say "hwpmc_mips24k.c".
>   - One for "generic" MIPS related code, e.g.,
>     code to walk stacks.
> 
>   You should expect that every manufacturer will have their own kind
>   of PMCs, with differing capabilities and programming models.
> 
>   Catering for the variability upfront would be prudent.  As
>   commented on by earlier reviewers, you may want to consider
>   changing symbol naming to suit.
> 

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:
> 
> +#define        PMCLOG_READADDR         PMCLOG_READ32
> +#define        PMCLOG_EMITADDR         PMCLOG_EMIT32
> 

Good point.

> 
> 7) From the definitions in the header file, these PMCs appear to
>   support the concept of sampling based on processor mode:
> 
> +#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 */
> 
>   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:
> 
> +static int
> +mips_allocate_pmc(enum pmc_event pe, char *ctrspec __unused,
> +                 struct pmc_op_pmcallocate *pmc_config __unused)
> +{
> +       switch (pe) {
> +       default:
> +               break;
> +       }
> +       
> +       return (0);
> +}
> 
> 

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.
> 
> +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 */
> +};
> 
> +const struct mips_event_code_map mips_event_codes[] = {
> +       { PMC_EV_MIPS_CYCLE, 0, 0},
> +       { PMC_EV_MIPS_CYCLE, 1, 0},  <<<--- repeated information 
> 
> 
> 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:
> 
> +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 = config;
> 

Again, for both of these, is there an example I should work from?

> 
> 10) If the number and width of these PMCs are fixed, you should
>    document that in the manual page:
> 
> pmc_md_initialize()
> {
> +       mips_npmcs = 2;
> ...
> +       pcd->pcd_width = 32; /* XXX: Fix for 64 bit MIPS */
> 

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