From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46729) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S5a0E-00044o-J2 for qemu-devel@nongnu.org; Thu, 08 Mar 2012 04:51:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S5Zzq-0002LG-0r for qemu-devel@nongnu.org; Thu, 08 Mar 2012 04:51:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36782) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S5Zzp-0002L0-PF for qemu-devel@nongnu.org; Thu, 08 Mar 2012 04:50:53 -0500 Message-ID: <4F5880F9.2080905@redhat.com> Date: Thu, 08 Mar 2012 11:50:49 +0200 From: Avi Kivity MIME-Version: 1.0 References: <1329211670-11548-1-git-send-email-avi@redhat.com> <1329211670-11548-9-git-send-email-avi@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 08/20] memory: store MemoryRegionSection pointers in phys_map List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org On 03/07/2012 09:32 PM, Peter Maydell wrote: > On 7 March 2012 17:49, Peter Maydell wrote: > > git bisect blames this commit (5312bd8b3) for causing a Linux kernel > > on spitz to produce a bunch of pxa2xx_i2c warnings that weren't > > being emitted before: > > What seems to happen here is that we register a memory region > (this is for the second i2c device in hw/pxa2xx.c): > > memory_region_init_io(&s->iomem, &pxa2xx_i2c_ops, s, > "pxa2xx-i2x", s->region_size); > > where region_size is 0x100. We then map it at 0x40f00100 > (via sysbus_mmio_map). This used to result in our read and write > functions being called with offsets from the start of the page, > so in this case for the register at 0x90 into the device the > passed in addr would be 0x190. There is some hackery in pxa2xx_i2c_init > to work out what the offset is from the start of the region > when we map the device, we pass it in as a qdev 'offset' > property, and then read/write can fix things up to get the > actual register offset. > > With this commit read and write functions are now passed the actual > offset from the start of the device region, ie 0x90. So the hackery > ends up doing fixing up it doesn't need to do, and generates negative > offsets which cause the diagnostic messages. > > So it seems like the new behaviour is more like the right thing, > but was it an intentional change? I don't recall whether it was intentional or not (i.e., whether I was aware I was changing behaviour or not), but it's certainly the desired behaviour. > Should we just drop the offset > hackery as a workaround for a now-fixed bug? Yes. I'll live the patch to you. > > Are we running into the "mapping devices at non-page-offsets isn't > supported" issue here? It wasn't supported? > Is that now supported after this > patch series? I'm sure that there are some rough edges but I made quite an effort to cover corner cases. For example, a region that starts and ends and non-aligned offsets, but spans more than a page, ought to work. > (I think the other devices I know of which include workarounds > for being passed relative-to-page-base addresses handle it by > masking out the high bits of the address, eg arm11mpcore.c, > so they weren't broken by this commit.) I assumed this was due to the days where absolute addresses were passed, but yes. -- error compiling committee.c: too many arguments to function