Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 19 Jul 2011 13:48:32 +0200
From:      Attilio Rao <attilio@freebsd.org>
To:        Marcel Moolenaar <marcel@xcllnt.net>
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:  <CAJ-FndCenbmtmQWw9JFQ_zNH1Am6vg6-CQGzY45YrYeCc9M9ZQ@mail.gmail.com>
In-Reply-To: <451BAE6D-C142-4E24-AE43-3BEC325F43F6@xcllnt.net>
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> <451BAE6D-C142-4E24-AE43-3BEC325F43F6@xcllnt.net>

next in thread | previous in thread | raw e-mail | index | archive | help
2011/7/19 Marcel Moolenaar <marcel@xcllnt.net>:
>
> On Jul 18, 2011, at 7:31 PM, Attilio Rao wrote:
>
>> 2011/7/19 Marcel Moolenaar <marcel@xcllnt.net>:
>>>
>>> On Jul 18, 2011, at 5:59 PM, Attilio Rao wrote:
>>>
>>>> 2011/7/19 Marcel Moolenaar <marcel@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:
>>>>>> =C2=A0- Remove the eintrcnt/eintrnames usage and introduce the conce=
pt of
>>>>>> =C2=A0 =C2=A0sintrcnt/sintrnames which are symbols containing the si=
ze of the 2
>>>>>> =C2=A0 =C2=A0tables.
>>>>>> =C2=A0- For amd64/i386 remove the storage of intr* stuff from assemb=
ly files.
>>>>>> =C2=A0 =C2=A0This area can be widely improved by applying the same t=
o other
>>>>>> =C2=A0 =C2=A0architectures and likely finding an unified approach am=
ong them and
>>>>>> =C2=A0 =C2=A0move the whole code to be MI. More work in this area is=
 expected to
>>>>>> =C2=A0 =C2=A0happen fairly soon.
>>>>>>
>>>>>> =C2=A0No 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 (=3D64 bit). We'll be having misaligned loads
>>>>> (=3D 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
> =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 =C2=A0 =C2=A0 =C2=A0(revision 224207)
> +++ sys/ia64/ia64/locore.S =C2=A0 =C2=A0 =C2=A0(working copy)
> @@ -207,13 +207,13 @@
> =C2=A0 =C2=A0 =C2=A0 =C2=A0intr_n =3D intr_n + 1
> =C2=A0.endr
> =C2=A0EXPORT(sintrnames)
> - =C2=A0 =C2=A0 =C2=A0 .word INTRCNT_COUNT * INTRNAME_LEN
> + =C2=A0 =C2=A0 =C2=A0 data8 INTRCNT_COUNT * INTRNAME_LEN
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0.align 8
> =C2=A0EXPORT(intrcnt)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0.fill INTRCNT_COUNT, 8, 0
> =C2=A0EXPORT(sintrcnt)
> - =C2=A0 =C2=A0 =C2=A0 .word INTRCNT_COUNT
> + =C2=A0 =C2=A0 =C2=A0 data8 INTRCNT_COUNT
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0.text
> =C2=A0 =C2=A0 =C2=A0 =C2=A0// 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 =C2=A0 =C2=A0 (revision 224207)
> +++ /sys/ia64/ia64/locore.S =C2=A0 =C2=A0 (working copy)
> @@ -207,13 +207,13 @@
> =C2=A0 =C2=A0 =C2=A0 =C2=A0intr_n =3D intr_n + 1
> =C2=A0.endr
> =C2=A0EXPORT(sintrnames)
> - =C2=A0 =C2=A0 =C2=A0 .word INTRCNT_COUNT * INTRNAME_LEN
> + =C2=A0 =C2=A0 =C2=A0 data8 INTRCNT_COUNT * INTRNAME_LEN
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0.align 8
> =C2=A0EXPORT(intrcnt)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0.fill INTRCNT_COUNT, 8, 0
> =C2=A0EXPORT(sintrcnt)
> - =C2=A0 =C2=A0 =C2=A0 .word INTRCNT_COUNT
> + =C2=A0 =C2=A0 =C2=A0 data8 INTRCNT_COUNT * 8
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0.text
> =C2=A0 =C2=A0 =C2=A0 =C2=A0// 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.

Thanks Marcel for the time you spent on this.

Attilio


--=20
Peace can only be achieved by understanding - A. Einstein



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndCenbmtmQWw9JFQ_zNH1Am6vg6-CQGzY45YrYeCc9M9ZQ>