From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45085) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dVtxu-0005os-S2 for qemu-devel@nongnu.org; Fri, 14 Jul 2017 02:20:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dVtxt-0004Cl-P9 for qemu-devel@nongnu.org; Fri, 14 Jul 2017 02:20:38 -0400 Message-ID: <1500013222.7221.1.camel@aj.id.au> From: Andrew Jeffery Date: Fri, 14 Jul 2017 15:50:22 +0930 In-Reply-To: References: <20170630030058.28943-1-andrew@aj.id.au> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-2UB3BkSyBZB/ABeTxBvo" Mime-Version: 1.0 Subject: Re: [Qemu-devel] [RFC PATCH 1/1] memory: Support unaligned accesses on aligned-only models List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: qemu-arm@nongnu.org, joel@jms.id.au, clg@kaod.org, f4bug@amsat.org --=-2UB3BkSyBZB/ABeTxBvo Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Paolo, Thanks for taking a look! On Thu, 2017-07-13 at 14:05 +0200, Paolo Bonzini wrote: > On 30/06/2017 05:00, Andrew Jeffery wrote: > > This RFC patch stems from a discussion on a patch for an ADC model[1] w= here it > > was pointed out that I should be able to use the .impl member of > > MemoryRegionOps to constrain how my read() and write() callbacks where = invoked. > >=20 > > I tried Phil's suggested approach and found I got reads of size 4, but = with an > > address that was not 4-byte aligned. > >=20 > > Looking at the source for access_with_adjusted_size() lead to the comme= nt > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* FIXME: support unaligned access? */ > >=20 > > which at least suggests that the implementation isn't complete. > >=20 > > So, this patch is a quick and incomplete attempt at resolving the issue= to see > > whether I'm on the right track or way off in the weeds. > >=20 > > I've lightly tested it with the ADC model mentioned above, and it appea= rs to do > > the right thing (I changed the values generated by the ADC to distingui= sh > > between the lower and upper 16 bits). >=20 > I think the idea is okay. >=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0access_addr[0] =3D align_down(addr, access_siz= e); > > +=C2=A0=C2=A0=C2=A0=C2=A0access_addr[1] =3D align_up(addr + size, acces= s_size); > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for (cur =3D access_ad= dr[0]; cur < access_addr[1]; cur +=3D access_size) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0uint64_t mask_bounds[2]; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0mask_bounds[0] =3D MAX(addr, cur) - cur; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0mask_bounds[1] =3D > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0MIN(addr + size, align_up(cur + 1, access_size))= - cur; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0access_mask =3D (-1ULL << mask_bounds[0] * 8) & > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0(-1ULL >> (64 - mask_bounds[1] * 8)); >=20 > Please use MAKE_64BIT_MASK. Okay. >=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0r |=3D access(mr, cur, &access_value, access_size, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(MAX(addr, cur) - addr), access_mask= , attrs); > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0/* XXX: Can't do this hack for writes */ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0access_value >>=3D mask_bounds[0] * 8; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} >=20 > Can you subtract access_addr[0] from mask_bounds[0] and mask_bounds[1] > (instead of cur) to remove the need for this right shift? I haven't looked at the patch since I sent it. Given you think the idea of the patch is okay I'll get back to working on it and think about this. Cheers, Andrew >=20 > Thanks, >=20 > Paolo --=-2UB3BkSyBZB/ABeTxBvo Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQIcBAABCgAGBQJZaGKmAAoJEJ0dnzgO5LT5k6IP/0tbvIBFL0okRlpM9nhRRC+N qYUPBmgY7NVBQ2ihkPuHU5iW0OHdB/jul1Ztzo2FT8IDSzavclvIerU2k/LqJtnP Et8CFqoW4nFfk2cyhOM0JtdFqr6auFL75tbZxnG6bZq1kqluyHkZAtG4CvmfYQRE yBw8lEpLrKOUdVTobEEkJUUziUibHOyrSFoVBihU7t42lZO3SV8BV2061qtiJxeM SKZXjaDf1O4PvLUtk+nbL7jMZ4+u5c78R6c+jdSlTDSB79L1rIRsA3pLu6HMGm3K dOeRmtH2Oq2WeDlm+uBxhCNcMirEu0m1Bgee1rI+MieNgkFr0UgLW944u2oYYEe+ G8Touj2TRuldY8anD9mEnxm4ZzLaHxM37KETI24CGvAsCiwJXtxxXYpFyLJF7VyO /SoOAkegLGKTIA2S1fNaJmFcGCzwUugT3WchsQ9XQRSO7F6i22Aw3At+WzrdVzf8 Jh2N9bVxnF8YsnmBzA+ot1XZCvetcfw6RMfgCNcPcn0gg1qGc5UGt6UE6ec+cGh3 Tc7/mx/qt9cWfRVYsGTNo3fyP5279Pngxxm+ggYWLKDC7Yb9YebZLLz/zbbjwoHo W210K8Zh6TihlXGPbvfkxRvqGDBAEX0oWQy4FNmqzcvzkObFx6TmycN6+ji/XDWn jJkfNb1fd5INENcOwhZK =TKBj -----END PGP SIGNATURE----- --=-2UB3BkSyBZB/ABeTxBvo--