Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 18 Jul 2011 20:22:37 -0700
From:      Marcel Moolenaar <marcel@xcllnt.net>
To:        Attilio Rao <attilio@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org
Subject:   Re: 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...
Message-ID:  <451BAE6D-C142-4E24-AE43-3BEC325F43F6@xcllnt.net>
In-Reply-To: <CAJ-FndCoqed8qEegmqr_ytRtNpkurZmC8bNHEgHVm8ciAU4cXQ@mail.gmail.com>
References:  <201107181519.p6IFJfOK028280@svn.freebsd.org> <208D139E-CDC3-428D-8D5F-F772BFAF164C@xcllnt.net> <CAJ-FndDfQB-oJbc_R8b36LSjn=OzLRNZJ-P_%2BA1w=pkxhcUi_w@mail.gmail.com> <A51071FB-BB42-4F0F-81DB-C3EF13B6F39B@xcllnt.net> <CAJ-FndCoqed8qEegmqr_ytRtNpkurZmC8bNHEgHVm8ciAU4cXQ@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help

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

> 2011/7/19 Marcel Moolenaar <marcel@xcllnt.net>:
>>=20
>> On Jul 18, 2011, at 5:59 PM, Attilio Rao wrote:
>>=20
>>> 2011/7/19 Marcel Moolenaar <marcel@xcllnt.net>:
>>>>=20
>>>> On Jul 18, 2011, at 8:19 AM, Attilio Rao wrote:
>>>>=20
>>>>> Author: attilio
>>>>> Date: Mon Jul 18 15:19:40 2011
>>>>> New Revision: 224187
>>>>> URL: http://svn.freebsd.org/changeset/base/224187
>>>>>=20
>>>>> 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.
>>>>>=20
>>>>>  No MFC is previewed for this patch.
>>>>=20
>>>> You just broke ia64 and possibly other 64-bit architectures:
>>>>=20
>>>> ".word" declares a 16-bit integral on ia64 and the size symbols
>>>> are of type size_t (=3D64 bit). We'll be having misaligned loads
>>>> (=3D kernel panics) and/or reading garbage...
>>>=20
>>> 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.
>>=20
>> Well... all I can say is that assembly is the least transposable
>> language, besides of course machine code itself :-)
>>=20
>>> Anyway, what do you think about this patch? (I still need to test =
it):
>>> http://www.freebsd.org/~attilio/64bits-fixup.diff
>>=20
>> 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 :-)
>=20
> 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.
>=20
> 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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- sys/ia64/ia64/locore.S	(revision 224207)
+++ sys/ia64/ia64/locore.S	(working copy)
@@ -207,13 +207,13 @@
 	intr_n =3D intr_n + 1
 .endr
 EXPORT(sintrnames)
-	.word INTRCNT_COUNT * INTRNAME_LEN
+	data8 INTRCNT_COUNT * INTRNAME_LEN
=20
 	.align 8
 EXPORT(intrcnt)
 	.fill INTRCNT_COUNT, 8, 0
 EXPORT(sintrcnt)
-	.word INTRCNT_COUNT
+	data8 INTRCNT_COUNT
=20
 	.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 (=3D sizeof(size_t)) and you're good:

Index: /sys/ia64/ia64/locore.S
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- /sys/ia64/ia64/locore.S     (revision 224207)
+++ /sys/ia64/ia64/locore.S     (working copy)
@@ -207,13 +207,13 @@
        intr_n =3D intr_n + 1
 .endr
 EXPORT(sintrnames)
-       .word INTRCNT_COUNT * INTRNAME_LEN
+       data8 INTRCNT_COUNT * INTRNAME_LEN
=20
        .align 8
 EXPORT(intrcnt)
        .fill INTRCNT_COUNT, 8, 0
 EXPORT(sintrcnt)
-       .word INTRCNT_COUNT
+       data8 INTRCNT_COUNT * 8
=20
        .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,

--=20
Marcel Moolenaar
marcel@xcllnt.net





Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?451BAE6D-C142-4E24-AE43-3BEC325F43F6>