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

[Andreas Färber]

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.

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.

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