Re: [Qemu-devel] [PATCH v1 22/22] petalogix-ml605: Make the LMB visible only to the CPU

[Edgar E. Iglesias]

On Mon, Dec 16, 2013 at 01:46:57PM +0100, Andreas Färber wrote:
> Hi Edgar,
> 
> Am 16.12.2013 09:06, schrieb address@hidden:
> > From: "Edgar E. Iglesias" <address@hidden>
> > 
> > Signed-off-by: Edgar E. Iglesias <address@hidden>
> > ---
> >  hw/microblaze/petalogix_ml605_mmu.c |   17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
> > b/hw/microblaze/petalogix_ml605_mmu.c
> > index 4009ff5..0a13b0e 100644
> > --- a/hw/microblaze/petalogix_ml605_mmu.c
> > +++ b/hw/microblaze/petalogix_ml605_mmu.c
> > @@ -88,10 +88,18 @@ petalogix_ml605_init(QEMUMachineInitArgs *args)
> >      hwaddr ddr_base = MEMORY_BASEADDR;
> >      MemoryRegion *phys_lmb_bram = g_new(MemoryRegion, 1);
> >      MemoryRegion *phys_ram = g_new(MemoryRegion, 1);
> > +    MemoryRegion *sysmem_alias = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mr_cpu_root = g_new(MemoryRegion, 1);
> > +    AddressSpace *as_cpu = g_malloc0(sizeof(*as_cpu));
> >      qemu_irq irq[32], *cpu_irq;
> >  
> > +    /* Setup the CPU specific address-space.  */
> > +    memory_region_init(mr_cpu_root, NULL, "as-cpu-root", INT64_MAX);
> > +    address_space_init(as_cpu, mr_cpu_root, "as/cpu");
> > +
> >      /* init CPUs */
> >      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
> > +    qdev_prop_set_address_space(DEVICE(cpu), "address-space", as_cpu);
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> >      if (err) {
> >          error_report("%s", error_get_pretty(err));
> > @@ -100,11 +108,18 @@ petalogix_ml605_init(QEMUMachineInitArgs *args)
> >  
> >      env = &cpu->env;
> >  
> > +    /* Populate the CPU AS with the LMB only visible to the CPU.  */
> > +    memory_region_init_alias(sysmem_alias, NULL, "sysmem_alias",
> > +                             address_space_mem, 0,
> > +                             memory_region_size(address_space_mem));
> > +    memory_region_add_subregion(mr_cpu_root, 0x00000000, sysmem_alias);
> > +
> 
> 
> Thanks for this series. I've been on vacation so couldn't review the
> previous RFC yet... I'm not entirely happy with the way this is pushing
> work to the machines here and wonder if we can simplify that some more:
> 
> For one, I don't like the allocation of AddressSpace and MemoryRegion at
> machine level. Would it be possible to enforce allocating a per-CPU
> AddressSpace and MemoryRegion at cpu.c level, ideally as embedded value
> rather than pointer field? Otherwise CPU hot-add is going to get rather
> complicated and error-prone.

It depends how you see it. The CPU is the user of an AS, but not necessarily
the owner/creator of the AS/port. I think the AS should be created and owned
by the container/board that creates the CPU. PMM elaborates more on this
in his reply.


> TCG loads/saves should always have a CPU[Arch]State associated. Would it
> work to always alias the system MemoryRegion again at cpu.c level with
> lowest priority for range [0,UINT64_MAX] and let derived CPUs do per-CPU
> MemoryRegions by adding MemoryRegions with higher priority?
> I guess QTest is going to be a culprit for this approach as you
> mentioned in the cover letter?
> That would limit machine changes to adding to the new CPU MemoryRegion
> instead of the global system one where appropriate.
> 
> Always allocating AddressSpace/MemoryRegion per CPU would at the same
> time avoid the need for these new qdev'y address space properties. If we
> do need the user to fiddle with this, then I would prefer making address
> spaces QOM objects and using a standard link<> property. An API to set
> it conveniently from code would of course still be fine then.


The reason why qdev props are nice for the CPU is that it makes the
interface to set the AS the same for all masters (DMA, CPUs etc).
For DMA capable devs, the device could even have multiple AS it operates
on which can be set by names. A CPU could in theory also need multiple AS,
but currently we have structural limitations that only allow support for one.

Cheers,
Edgar




> 
> Regards,
> Andreas
> 
> >      /* Attach emulated BRAM through the LMB.  */
> >      memory_region_init_ram(phys_lmb_bram, NULL, "petalogix_ml605.lmb_bram",
> >                             LMB_BRAM_SIZE);
> >      vmstate_register_ram_global(phys_lmb_bram);
> > -    memory_region_add_subregion(address_space_mem, 0x00000000, 
> > phys_lmb_bram);
> > +    memory_region_add_subregion_overlap(mr_cpu_root, 0x00000000,
> > +                                        phys_lmb_bram, 2);
> >  
> >      memory_region_init_ram(phys_ram, NULL, "petalogix_ml605.ram", 
> > ram_size);
> >      vmstate_register_ram_global(phys_ram);
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg