Re: [perfmon2] [PATCH] libpfm: Add IBS support for AMD CPUs

[stephane e. <er...@go...>]

Robert,

I have applied your IBS patch. Please test it.

I do have a few remarks about things I like to see fixed:

  - SEL_HOST, SEL_GUEST are only available on family 10h, please
reject any attempts to use an older processors

  - IBS_OPTIONS_RANDEN is only available on IBSFETCHCTL, reject
attemps to set it on IBSOPSCTL

  - you expect the user to pass a 20-bit sampling value in maxcnt.
Internally you shift it by 4 because bits 3:0 are ignored.
     I would like to see a check to reject any maxcnt value with any
of the bottom 4 bits set. That can avoid problems later on.

  - you cannot assume that because you are on family 16, IBS registers
will always be available. You may have to share the
    PMU with another competing monitoring  session which may already
by using IBS. This is why you need to check against
    the bitmask of unavailable registers in inp->r_pmcs. I understand
that you may now call pfm_dispatch_events() with inp
    NULL, in that case you do what you're doing today (best effort).
But if inp is not NULL, I'd like to see a check against
    inp->r_pmcs for all IBS registers (config) you will be using.

  - you need to fill in the reg_addr fields for the IBS pmc you are
using. This is mostly for reference but could be used on systems
    not based on perfmon.

  - on all platforms the pfm_dispatch_events() routine needs to reject
inp == NULL when it relies on it.


Support for IBS in libpfm is a great addition, I am convinced it will
make this nice feature more easily available to users.

Thanks for you contributions.