From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54134) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsXZk-0001xe-3X for qemu-devel@nongnu.org; Mon, 16 Dec 2013 07:47:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VsXZd-0001KC-UQ for qemu-devel@nongnu.org; Mon, 16 Dec 2013 07:47:08 -0500 Received: from cantor2.suse.de ([195.135.220.15]:38260 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsXZd-0001JW-JV for qemu-devel@nongnu.org; Mon, 16 Dec 2013 07:47:01 -0500 Message-ID: <52AEF641.7050401@suse.de> Date: Mon, 16 Dec 2013 13:46:57 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1387181170-23267-1-git-send-email-edgar.iglesias@gmail.com> <1387181170-23267-23-git-send-email-edgar.iglesias@gmail.com> In-Reply-To: <1387181170-23267-23-git-send-email-edgar.iglesias@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v1 22/22] petalogix-ml605: Make the LMB visible only to the CPU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: edgar.iglesias@gmail.com, qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, blauwirbel@gmail.com, aliguori@amazon.com, pcrost@xilinx.com, pbonzini@redhat.com, aurelien@aurel32.net, rth@twiddle.net Hi Edgar, Am 16.12.2013 09:06, schrieb edgar.iglesias@gmail.com: > From: "Edgar E. Iglesias" >=20 > Signed-off-by: Edgar E. Iglesias > --- > hw/microblaze/petalogix_ml605_mmu.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) >=20 > diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalo= gix_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 =3D MEMORY_BASEADDR; > MemoryRegion *phys_lmb_bram =3D g_new(MemoryRegion, 1); > MemoryRegion *phys_ram =3D g_new(MemoryRegion, 1); > + MemoryRegion *sysmem_alias =3D g_new(MemoryRegion, 1); > + MemoryRegion *mr_cpu_root =3D g_new(MemoryRegion, 1); > + AddressSpace *as_cpu =3D g_malloc0(sizeof(*as_cpu)); > qemu_irq irq[32], *cpu_irq; > =20 > + /* 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 =3D 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) > =20 > env =3D &cpu->env; > =20 > + /* 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_b= ram", > LMB_BRAM_SIZE); > vmstate_register_ram_global(phys_lmb_bram); > - memory_region_add_subregion(address_space_mem, 0x00000000, phys_lm= b_bram); > + memory_region_add_subregion_overlap(mr_cpu_root, 0x00000000, > + phys_lmb_bram, 2); > =20 > memory_region_init_ram(phys_ram, NULL, "petalogix_ml605.ram", ram_= size); > vmstate_register_ram_global(phys_ram); --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg