Re: [Qemu-devel] [PATCH v4 4/8] bcm2835_peripherals: add rollup device for bcm2835 peripherals

[Andrew Baumann <Andrew.Baumann@microsoft.com>]

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Tuesday, 26 January 2016 00:03
> 
> On Mon, Jan 25, 2016 at 10:23 PM, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> >> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> >> Sent: Monday, 25 January 2016 22:14
> >>
> >> On Fri, Jan 15, 2016 at 3:58 PM, Andrew Baumann
> >> <Andrew.Baumann@microsoft.com> wrote:
> > [...]
> >> > +static void bcm2835_peripherals_init(Object *obj)
> >> > +{
> >> > +    BCM2835PeripheralState *s = BCM2835_PERIPHERALS(obj);
> >> > +
> >> > +    /* Memory region for peripheral devices, which we export to our
> >> parent */
> >> > +    memory_region_init_io(&s->peri_mr, obj, NULL, s, "bcm2835-
> >> peripherals",
> >> > +                          0x1000000);
> >>
> >> Should this just be normal memory_region_init?
> >
> > I think so -- it's just a container region, and I probably copy and pasted the
> API here. The two MR init APIs seem almost but not-quite identical when
> NULL callbacks are used. Can you briefly explain the difference?
> >
> 
> hmm I guess the defaulting to &unassigned_mem_ops. memory_region_init
> looks to assume you will populate the usable subregions. Are you
> relying on unassigned ops?

Actually, memory_region_initfn also sets the unassigned_ops, so the only difference appears to be mr->terminates, which is set to true for init_io. This flag affects the behaviour of render_memory_region(), which terminates early if !mr->terminates. So the difference is that the gaps between the subregions (i.e., the actual devices) wouldn't be rendered. But what difference this makes in practice is unclear to me. It can only matter if the CPU accesses an undefined / unimplemented device, but even then it looks to me like it would end up at the same unassigned ops code anyway, so I suspect it's irrelevant.

If memory_region_init works, I'll go with that.

Andrew