[patch][msrtool] Add K8 MSRs

[Marc Jones]

Add K8 support for some of the more important MSRs.

Signed-off-by: Marc Jones

[Carl-Daniel Hailfinger]

On 05.03.2009 21:57, Marc Jones wrote:
> Add K8 support for some of the more important MSRs.
>
> Signed-off-by: Marc Jones

If you fix the one bug mentioned below and can provide
explanations/answers for my other questions, this is
Acked-by: Carl-Daniel Hailfinger
> { 0xC001001A, MSRTYPE_RDWR, MSR2(0, 0), "TOP_MEM Register", "This register indicates the first byte of I/O above DRAM", {
> { 63, 24, RESERVED },
> { 39, 8, "TOM 16-9", "", PRESENT_HEX, {

I find the bit numbering in the description string here to be rather
unintuitive. IIRC these bits here are not shifted, so "TOM 29-32" would
be the correct name.
> 	{ BITVAL_EOT }
> }},
> { 31, 9, "TOM 8-0", "", PRESENT_HEX, {

Same here.
> 	{ BITVAL_EOT }
> }},
> { 22, 23, RESERVED },
> { BITS_EOT }
> }},

Can TOP_MEM really be larger than 4 GB?
> { 0xC001001D, MSRTYPE_RDWR, MSR2(0, 0), "TOP_MEM2 Register", "This register indicates the Top of Memory above 4GB", {
> { 63, 24, RESERVED },
> { 39, 8, "TOM2 16-9", "", PRESENT_HEX, {

Bit numbering.
> 	{ BITVAL_EOT }
> }},
> { 31, 9, "TOM2 8-0", "", PRESENT_HEX, {

Same here.
> 	{ BITVAL_EOT }
> }},
> { 22, 23, RESERVED },
> { BITS_EOT }
> }},

I thought newer AMD processors have more than 40 bit addressable memory.
> { 0xC0010019, MSRTYPE_RDWR, MSR2(0, 0), "IORRMask0", "This register holds the mask of the variable I/O range", {

Should be IORRMask1.
> { 63, 24, RESERVED },
> { 39, 8, "MASK 27-20", "", PRESENT_HEX, {
> 	{ BITVAL_EOT }
> }},
> { 31, 20, "MASK 20-0", "", PRESENT_HEX, {
> 	{ BITVAL_EOT }
> }},
> { 11, 1, "V:", "Enables variable I/O range registers", PRESENT_DEC, {
> 	{ MSR1(0), "V I/O range disabled" },
> 	{ MSR1(1), "V I/O range enabled" },
> 	{ BITVAL_EOT }
> }},
> { 10, 11, RESERVED },
> { BITS_EOT }
> }},
> Index: msrtool/msrutils.c
>
> --- msrtool.orig/msrutils.c	2009-03-05 11:58:19.000000000 -0700
> +++ msrtool/msrutils.c	2009-03-02 15:40:13.000000000 -0700
> @@ -137,7 +137,7 @@
>    return NULL;
>  }
>
> -const uint32_t msraddrbyname(const char *name) {
> +uint32_t msraddrbyname(const char *name) {

Any reason for the const removal? The changelog doesn't say anything
about it.
> uint8_t t;
>    const uint32_t addr = strtoul(name, NULL, 16);
>    const struct msrdef *m;

Regards,
Carl-Daniel

[Marc Jones]

On Thu, Mar 5, 2009 at 2:22 PM, Carl-Daniel Hailfinger
 wrote:
> On 05.03.2009 21:57, Marc Jones wrote:
> > Add K8 support for some of the more important MSRs.
> >
> > Signed-off-by: Marc Jones
>
> If you fix the one bug mentioned below and can provide
> explanations/answers for my other questions, this is
> Acked-by: Carl-Daniel Hailfinger
> > { 0xC001001A, MSRTYPE_RDWR, MSR2(0, 0), "TOP_MEM Register", "This register indicates the first byte of I/O above DRAM", {
> > { 63, 24, RESERVED },
> > { 39, 8, "TOM 16-9", "", PRESENT_HEX, {
>
> I find the bit numbering in the description string here to be rather
> unintuitive. IIRC these bits here are not shifted, so "TOM 29-32" would
> be the correct name.

Ok, changed.
> Can TOP_MEM really be larger than 4 GB?

I have never seen it but it seems so from the BKDG. You wouldn't be
able to hoist them memory in the MMIO hole if you did it this way.
> I thought newer AMD processors have more than 40 bit addressable memory.

Fam10 does.
> > { 0xC0010019, MSRTYPE_RDWR, MSR2(0, 0), "IORRMask0", "This register holds the mask of the variable I/O range", {
>
> Should be IORRMask1.

Fixed.
> > -const uint32_t msraddrbyname(const char *name) {
> > +uint32_t msraddrbyname(const char *name) {
>
> Any reason for the const removal? The changelog doesn't say anything
> about it.

That was to get it to compile with -Werror which is on by default. It
didn't look like that really needed to be a const.

Marc

[Stefan Reinauer]

Marc Jones wrote:
> > > -const uint32_t msraddrbyname(const char *name) {
> > > +uint32_t msraddrbyname(const char *name) {
> >
> > Any reason for the const removal? The changelog doesn't say anything
> > about it
>
> That was to get it to compile with -Werror which is on by default. It
> didn't look like that really needed to be a const.

Does the const make sense at all? It seems odd to define the variable
integer return value of a function const. What's that supposed to be
good for?

Stefan

[Peter Stuge]

Stefan Reinauer wrote:
> > > > -const uint32_t msraddrbyname(const char *name) {
> >
> > That was to get it to compile with -Werror which is on by
> > default. It didn't look like that really needed to be a const.
>
> Does the const make sense at all? It seems odd to define the
> variable integer return value of a function const. What's that
> supposed to be good for?

The value returned in the function is const. Is it OK to still have
the function return non-const?

msrtool.h:

struct msrdef {
        const uint32_t addr;

msrutils.c:

if (addr == m->addr)
        return m->addr;
if (!strcasecmp(name, m->symbol))
        return m->addr;

If I just made a too conservative assumption about types, I am all
for!

Acked-by: Peter Stuge

[Marc Jones]

On Fri, Mar 6, 2009 at 2:15 PM, Peter Stuge  wrote:
> Stefan Reinauer wrote:
> > > > > -const uint32_t msraddrbyname(const char *name) {
> > >
> > > That was to get it to compile with -Werror which is on by
> > > default. It didn't look like that really needed to be a const.
> >
> > Does the const make sense at all? It seems odd to define the
> > variable integer return value of a function const. What's that
> > supposed to be good for?
>
> The value returned in the function is const. Is it OK to still have
> the function return non-const?
>
> msrtool.h:
>
> struct msrdef {
>        const uint32_t addr;
>
> msrutils.c:
>
> if (addr == m->addr)
>        return m->addr;
> if (!strcasecmp(name, m->symbol))
>        return m->addr;
>
> If I just made a too conservative assumption about types, I am all
> for!
>
> Acked-by: Peter Stuge

Yes, That doesn't seem to bother the type checking but having the
return type not match the variable causes an error (warning).

r3985

Thanks,
Marc