Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region

[Andreas Färber]

Am 08.10.2012 20:58, schrieb Alexander Graf:
> 
> On 08.10.2012, at 19:21, Andreas Färber wrote:
> 
>> Am 08.10.2012 18:46, schrieb Bharat Bhushan:
>>> +    s = sysbus_from_qdev(dev);
>>
>> s = SYS_BUS_DEVICE(dev);
> 
> Mind to explain why? The rest of the code uses the above helper :).

SYS_BUS_DEVICE() is the QOM cast macro, relying on binary struct layout.

sysbus_from_qdev() is an old qdev helper, relying on container_of() and
field naming.

I'm lobbying for using the new constructs in new code. You can argue of
course that SysBus will go away some day so it doesn't matter much here.
But we had issues with PCIHostState not adhering to QOM layout
requirements (e.g., pSeries), so using the cast consistently helps to
avoid/detect such issues.

>>> +    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET, 
>>> s->mmio[0].memory);
>>
>> ... I wonder if fiddling with SysBus MMIO is a good idea.
>> s->mmio[0].addr is not getting assigned this way, which is checked as
>> condition for deleting the subregion. But sysbus_mmio_map() only adds to
>> / deletes from get_system_memory().
>> The alternative would be using a custom field rather than the
>> SysBus-internal one. Avi/Alex?
> 
> I honestly don't mind either way. We could
> 
>   a) add a new sysbus helper for maps in memory region containers
>   b) not use sysbus at all
>   c) do it this way
> 
> Anything else wouldn't improve the situation. I really don't care which way 
> we go. To me, c) is fine.

b) is not that easily possible for the PHB, since we still haven't seen
a new round of i440fx patches (PCIHostState is based on SysBusDevice, so
all PHBs would need to get converted at once).

I'd also be in favor of c) with Avi's suggestion of using the accessor
function to obtain the MemoryRegion* (data caging).

Andreas

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