Re: [coreboot] [patch][msrtool] Add K8 MSRs

[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