From owner-freebsd-toolchain@freebsd.org Fri Sep 21 20:15:36 2018 Return-Path: Delivered-To: freebsd-toolchain@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 4FBFB10A2F8B for ; Fri, 21 Sep 2018 20:15:36 +0000 (UTC) (envelope-from marklmi@yahoo.com) Received: from sonic312-22.consmr.mail.gq1.yahoo.com (sonic312-22.consmr.mail.gq1.yahoo.com [98.137.69.203]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id CF9F78E760 for ; Fri, 21 Sep 2018 20:15:35 +0000 (UTC) (envelope-from marklmi@yahoo.com) X-YMail-OSG: md.fQ2UVM1nGSDuPm1OGy7ZGQB9VUxbT8QANwFGTHZmxSJ8BVPjXKVN8_f_bZEO w5HqR6X6ItHZrj1MRcsxDdIYR2cJGSPtG8xJbvsVzoxwb3RXaLbhpSnGAoLwDPLXOP88R2kliqFl IJwLvp1i1R9PG3o1G6CnlR5YrrAJjcXuEnmpMVh15cNg_OuvSZOi2YchZcYKIDsZBqr2u_67osu7 G.e3bqPl924.Jg8TE.dl.E3iFoSoZgCPDS9Mn836NpkaBLVVEmYmO_m8P2R2fpBRcoX9YtxBSiGU 6.9gnBACp8L1hg2vK.KHqyREO3tTa5JVVmVwGDIzYpxkEmU9yjdvJF1HrpFnUCjS1f1VSRGucJRZ WfGktc3.dUztzHVfF3EyCy7PEQAnsc53scwb31hncNrEtNanLPUh4AduYLUyP9TAnlDUC3UEiIg. 1Q3xaLuEnzIEn8F5lzN8fbdVF.OxOobyQHUpbxOG8LHXEA6pZ5v4.hWV4PJvnS8lgvKu1pkRPJrd wz1ejgM_FOU20lI5nEz_Meo5yqPipXXgEsvEU3C6ihUYselpo.Pg_ocO2_jUGD1Q9hu6KhGLY7qF oPDyOWmrFyx.QTgifG_Qz68MTuaMs0HBR6fcO4SONxsWJJr9OlVI6KqbMJAe9.yPqZp_kVaTE9tF 45ThAX6oBiWKXlhlSyHyOwTQuch3TDr5cCvAuwXlkVtCzjkorfH4HH_aT3ub3zp5_mRogvRpjUTX GfPJsq_X1sSM11XWUp5J0XuLxnr7IcascMKZWIMpbpfxPeQ7nuYYGYCjE0D5nqO3KuP1wb7WK2HA c5cG7yDqTx6zyyWY71Zi420KVqrdxjH5fnoQ8P7NoNlq0mdm.KWObvCrrT1YalyoFd2mL6jUfwV9 0x75kmBMlGiVqtoX6RpxKY.RTpaXw5_uw9BO3A297D4r6.Fp_TAMA15N2FD3s3Zm_pZJxtTzv6zG IsNWHpjcgBpCdhCtXu1aMptDzxx9eqddJOjX2bvzTFwVe2PvnqVaRXCFQUGcCCKZstsJ7tDgztJJ 5Nv8iaL6T2w-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic312.consmr.mail.gq1.yahoo.com with HTTP; Fri, 21 Sep 2018 20:15:28 +0000 Received: from ip70-189-131-151.lv.lv.cox.net (EHLO [192.168.0.105]) ([70.189.131.151]) by smtp414.mail.gq1.yahoo.com (Oath Hermes SMTP Server) with ESMTPA ID ddb9285f827cbf93eed35fe4292197d7; Fri, 21 Sep 2018 19:55:12 +0000 (UTC) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: Re: Does sys/dev/fxp/if_fxp.c using cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16) make any sense? From: Mark Millard In-Reply-To: Date: Fri, 21 Sep 2018 12:55:11 -0700 Cc: "freebsd-hackers@freebsd.org" , FreeBSD Toolchain Content-Transfer-Encoding: quoted-printable Message-Id: References: <3EBF1660-6CD5-4C80-A36D-4DE945073006@yahoo.com> <3E0252F3-3E65-4A2B-B17A-3BBFBAFFD5F6@yahoo.com> To: Warner Losh X-Mailer: Apple Mail (2.3445.9.1) X-BeenThere: freebsd-toolchain@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: Maintenance of FreeBSD's integrated toolchain List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Sep 2018 20:15:36 -0000 On 2018-Sep-21, at 12:21 PM, Warner Losh wrote: >> On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers = wrote: >> [I decided that freebsd-hackers might be better for this, >> under a different wording.] >>=20 >> sys/dev/fxp/if_fxp.c uses the statement: >>=20 >> cbp->tbd[-1].tb_size =3D htole32(m->m_pkthdr.tso_segsz << 16); >>=20 >> But sys/dev/fxp/if_fxpreg.h has the types involved as: >>=20 >> struct fxp_cb_tx { >> uint16_t cb_status; >> uint16_t cb_command; >> uint32_t link_addr; >> uint32_t tbd_array_addr; >> uint16_t byte_count; >> uint8_t tx_threshold; >> uint8_t tbd_number; >>=20 >> /* >> * The following structure isn't actually part of the TxCB, >> * unless the extended TxCB feature is being used. In this >> * case, the first two elements of the structure below are >> * fetched along with the TxCB. >> */ >> union { >> struct fxp_ipcb ipcb; >> struct fxp_tbd tbd[FXP_NTXSEG + 1]; >> } tx_cb_u; >> }; >>=20 >> So cbp->tbd is not pointing into the middle of an array. >> Thus the cbp->tbd[-1].tb_size =3D . . . assignment trashes memory >> from what I can tell. >>=20 >> /usr/src/sys/dev/fxp/if_fxp.c has the [-1] assignment in: >>=20 >> /* Configure TSO. */ >> if (m->m_pkthdr.csum_flags & CSUM_TSO) { >> cbp->tbd[-1].tb_size =3D htole32(m->m_pkthdr.tso_segsz = << 16); >> cbp->tbd[1].tb_size |=3D htole32(tcp_payload << 16); >> cbp->ipcb_ip_schedule |=3D FXP_IPCB_LARGESEND_ENABLE | >> FXP_IPCB_IP_CHECKSUM_ENABLE | >> FXP_IPCB_TCP_PACKET | >> FXP_IPCB_TCPUDP_CHECKSUM_ENABLE; >> } >>=20 >> This cbp->tbd[-1].tb_size use goes back to -r185330 in 2008-Nov-26. >>=20 >> This is also when the "+ 1" was added to the: >>=20 >> struct fxp_tbd tbd[FXP_NTXSEG + 1] >>=20 >> above. >>=20 >> clang 7 via xtoolchain-llvm70 reported: >>=20 >> --- if_fxp.o --- >> /usr/src/sys/dev/fxp/if_fxp.c:1630:3: error: array index -1 is before = the beginning of the array [-Werror,-Warray-bounds] >> cbp->tbd[-1].tb_size =3D htole32(m->m_pkthdr.tso_segsz = << 16); >> ^ ~~ >> /usr/src/sys/dev/fxp/if_fxpreg.h:297:3: note: array 'tbd' declared = here >> struct fxp_tbd tbd[FXP_NTXSEG + 1]; >> ^ >> 1 error generated. >> *** [if_fxp.o] Error code 1 >>=20 >> It does look odd to me. >>=20 > So I did some digging into this a few months ago. >=20 > It turns out the code is right, kinda. >=20 > So, what's it's doing with the [-1] is as follows: >=20 > the sizeof tbd is 8 bytes long. So, we're dereferencing 8 before the = array, which points to tbd_array_addr. However tb_size is the second = element of that, so that points us at count + tx_threshold + tbd_number. = So, this code is setting count =3D 0 and tx_threshold to the low order = bits of the segment size and tbd_number to the high order bits. We set = the count =3D 0 later anyway, so that part of it isn't so bad. >=20 > Since this is in hardware, it may be special meanings for these bits, = and this 'trick' is used to just do one write rather than two. I've not = looked for the hardware manual to see if this is kosher, but that's what = we're doing. It's not as crazy stupid as it sounds at first blush. Thanks for the explanation. Too bad the code does not document the hack. Looks like xtoolchain-llvm70 use will require avoiding reporting this as an error that stops buildkernel, a change for the build environment. Of course, that may well wait for head to be 13 instead of 12. =3D=3D=3D Mark Millard marklmi at yahoo.com ( dsl-only.net went away in early 2018-Mar)