From owner-svn-src-all@FreeBSD.ORG Tue Jul 19 11:48:33 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E2974106564A; Tue, 19 Jul 2011 11:48:33 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-gw0-f54.google.com (mail-gw0-f54.google.com [74.125.83.54]) by mx1.freebsd.org (Postfix) with ESMTP id 7051B8FC08; Tue, 19 Jul 2011 11:48:33 +0000 (UTC) Received: by gwb15 with SMTP id 15so2095761gwb.13 for ; Tue, 19 Jul 2011 04:48:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=iSb6SwybnZGnZ0T3tnbBG7viCQQKyJ2FUPP/2lIo3/o=; b=GZRE8C/1d7+EH3iqkTC4rNIiaKRUnbIOH9vZvNqFMi8MHJTJj8Y2Gj/NdIL1q+cGCo cusjb49Og4lqO/PFR6Y+3hJ/hjPjAuUPMRnRhOb6qLknbLqi9GcHR7TrJvHwtjdT6BrA CIQqf6jwmQjyiCFidD0+O1ihs+gbPh9JQHWuc= MIME-Version: 1.0 Received: by 10.236.79.170 with SMTP id i30mr8750640yhe.4.1311076112827; Tue, 19 Jul 2011 04:48:32 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.236.105.169 with HTTP; Tue, 19 Jul 2011 04:48:32 -0700 (PDT) In-Reply-To: <451BAE6D-C142-4E24-AE43-3BEC325F43F6@xcllnt.net> References: <201107181519.p6IFJfOK028280@svn.freebsd.org> <208D139E-CDC3-428D-8D5F-F772BFAF164C@xcllnt.net> <451BAE6D-C142-4E24-AE43-3BEC325F43F6@xcllnt.net> Date: Tue, 19 Jul 2011 13:48:32 +0200 X-Google-Sender-Auth: xzf2TRke_44-qATIkKPz4Ke04T0 Message-ID: From: Attilio Rao To: Marcel Moolenaar Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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... X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jul 2011 11:48:34 -0000 2011/7/19 Marcel Moolenaar : > > On Jul 18, 2011, at 7:31 PM, Attilio Rao wrote: > >> 2011/7/19 Marcel Moolenaar : >>> >>> On Jul 18, 2011, at 5:59 PM, Attilio Rao wrote: >>> >>>> 2011/7/19 Marcel Moolenaar : >>>>> >>>>> 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