svn commit: r224187 - in head: sys/amd64/amd64 sys/arm/arm sys/arm/sa11x0 sys/i386/i386 sys/ia64/ia64 sys/kern sys/mips/mips sys/powerpc/aim sys/powerpc/booke sys/sparc64/sparc64 sys/sys usr.bin/vm...

[Marcel Moolenaar]

On Jul 18, 2011, at 7:31 PM, Attilio Rao wrote:

> 2011/7/19 Marcel Moolenaar <marcel at xcllnt.net>:
>> 
>> On Jul 18, 2011, at 5:59 PM, Attilio Rao wrote:
>> 
>>> 2011/7/19 Marcel Moolenaar <marcel at xcllnt.net>:
>>>> 
>>>> On Jul 18, 2011, at 8:19 AM, Attilio Rao wrote:
>>>> 
>>>>> Author: attilio
>>>>> Date: Mon Jul 18 15:19:40 2011
>>>>> New Revision: 224187
>>>>> URL: http://svn.freebsd.org/changeset/base/224187
>>>>> 
>>>>> Log:
>>>>>  - Remove the eintrcnt/eintrnames usage and introduce the concept of
>>>>>    sintrcnt/sintrnames which are symbols containing the size of the 2
>>>>>    tables.
>>>>>  - For amd64/i386 remove the storage of intr* stuff from assembly files.
>>>>>    This area can be widely improved by applying the same to other
>>>>>    architectures and likely finding an unified approach among them and
>>>>>    move the whole code to be MI. More work in this area is expected to
>>>>>    happen fairly soon.
>>>>> 
>>>>>  No MFC is previewed for this patch.
>>>> 
>>>> You just broke ia64 and possibly other 64-bit architectures:
>>>> 
>>>> ".word" declares a 16-bit integral on ia64 and the size symbols
>>>> are of type size_t (=64 bit). We'll be having misaligned loads
>>>> (= kernel panics) and/or reading garbage...
>>> 
>>> I'm a bit surprised of this though.
>>> .hword was supposed to be the 16-bit integral, while .word was
>>> supposed to be the 32-bits one, if I read my "info as" on amd64.
>> 
>> Well... all I can say is that assembly is the least transposable
>> language, besides of course machine code itself :-)
>> 
>>> Anyway, what do you think about this patch? (I still need to test it):
>>> http://www.freebsd.org/~attilio/64bits-fixup.diff
>> 
>> Looks good to me, though I don't know enough about mips to comment
>> on that. I'm not going to be anal about the use of ".quad" instead
>> of "data8" for ia64 -- let's get it fixed first (I think we have
>> ".byte" in locore.S anyway :-)
> 
> We do.
> Anyway, I've updated the patch in order to use data8 in ia64 case (you
> are the maintainer, so you have the last word) even if I'm not sure
> there is a real need to discourage .quad.
> 
> Thanks for pointing at this breakage, please review and approve in case.

The change for ia64 is not quite right. This is what you have
in the latest patch:

Index: sys/ia64/ia64/locore.S
===================================================================
--- sys/ia64/ia64/locore.S	(revision 224207)
+++ sys/ia64/ia64/locore.S	(working copy)
@@ -207,13 +207,13 @@
 	intr_n = intr_n + 1
 .endr
 EXPORT(sintrnames)
-	.word INTRCNT_COUNT * INTRNAME_LEN
+	data8 INTRCNT_COUNT * INTRNAME_LEN
 
 	.align 8
 EXPORT(intrcnt)
 	.fill INTRCNT_COUNT, 8, 0
 EXPORT(sintrcnt)
-	.word INTRCNT_COUNT
+	data8 INTRCNT_COUNT
 
 	.text
 	// in0:	image base

It defines sintrcnt as a 64-bit integral with value INTRCNT_COUNT,
which is merely the number of counters, not the storage size of
intrcnt itself. Multiply by 8 (= sizeof(size_t)) and you're good:

Index: /sys/ia64/ia64/locore.S
===================================================================
--- /sys/ia64/ia64/locore.S     (revision 224207)
+++ /sys/ia64/ia64/locore.S     (working copy)
@@ -207,13 +207,13 @@
        intr_n = intr_n + 1
 .endr
 EXPORT(sintrnames)
-       .word INTRCNT_COUNT * INTRNAME_LEN
+       data8 INTRCNT_COUNT * INTRNAME_LEN
 
        .align 8
 EXPORT(intrcnt)
        .fill INTRCNT_COUNT, 8, 0
 EXPORT(sintrcnt)
-       .word INTRCNT_COUNT
+       data8 INTRCNT_COUNT * 8
 
        .text
        // in0: image base


Sorry for pointing this out in the rebound. I just tested the patch
for ia64 and noticed interrupt counters we're getting fetched right.

You did all the other architectures in the patch right, so with this
last tweak it's reviewed and approved from my end.

FYI,

-- 
Marcel Moolenaar
marcel at xcllnt.net