Re: [Qemu-devel] [PATCH] hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region

["Andreas Färber" <afaerber@suse.de>]

Am 14.03.2012 13:45, schrieb Peter Maydell:
> On 9 March 2012 08:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I'm happy to tweak the commit message if you have suggestions
>> for wording to add.
> 
> How about this redrafting?
> 
> ===begin===
> hw/pxa2xx.c: Fix handling of pxa2xx_i2c variable offset within region
> 
> The pxa2xx I2C controller can have its registers at an arbitrary offset
> within the MemoryRegion it creates. We use this to create two controllers,
> one which covers a region of size 0x10000 with registers starting at an
> offset 0x1600 into that region, and a second one which covers a region
> of size just 0x100 with the registers starting at the base of the region.
> 
> The implementation of this offsetting uses two qdev properties, "offset"
> (which sets the offset which must be subtracted from the address to
> get the offset into the actual register bank) and "size", which is the
> size of the MemoryRegion. We were actually using "offset" for two
> purposes: firstly the required one of handling the registers not being
> at the base of the MemoryRegion, and secondly as a workaround for a
> deficiency of QEMU. Until commit 5312bd8b3, if a MemoryRegion was mapped
> at a non-page boundary, the address passed into the read and write
> functions would be the offset from the start of the page, not the
> offset from the start of the MemoryRegion. So when calculating the value
> to set the "offset" qdev property we included a rounding to a page
> boundary.
> 
> Following commit 5312bd8b3 MemoryRegion read/write functions are now
> correctly passed the offset from the base of the region, and our
> workaround now means we're subtracting too much from addresses, resulting
> in warnings like "pxa2xx_i2c_read: Bad register 0xffffff90".
> The fix for this is simply to remove the rounding to a page boundary;
> this allows us to slightly simplify the expression since
>   base - (base & (~region_size)) == base & region_size
> 
> The qdev property "offset" itself must remain because it is still
> performing its primary job of handling register banks not being at
> the base of the MemoryRegion.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ===endit===

That's more text than I expected but a perfect explanation. In short it
translates to us dropping the page alignment as a bugfix and simplifying
the resulting expression in one go.

Reviewed-by: Andreas Färber <afaerber@suse.de>

Sorry for loosing track of this message, my inbox works more like a
stack than a queue these days...

I'd still like to change the "region_size" being off by one (0xff rather
than the above 0x100) in a follow-up patch. That should not stop us
applying this immediate fix.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg