Re: [Qemu-devel] [PATCH 08/20] memory: store MemoryRegionSection pointers in phys_map

[Avi Kivity <avi@redhat.com>]

On 03/07/2012 09:32 PM, Peter Maydell wrote:
> On 7 March 2012 17:49, Peter Maydell <peter.maydell@linaro.org> 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?

>  <optimism>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