From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37265) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmR4B-0003Fl-PW for qemu-devel@nongnu.org; Mon, 19 May 2014 13:09:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WmR44-0006ZN-7q for qemu-devel@nongnu.org; Mon, 19 May 2014 13:09:35 -0400 Received: from cantor2.suse.de ([195.135.220.15]:48166 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WmR43-0006ZE-T3 for qemu-devel@nongnu.org; Mon, 19 May 2014 13:09:28 -0400 Message-ID: <537A3AC5.1040603@suse.de> Date: Mon, 19 May 2014 19:09:25 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1392800720-2765-1-git-send-email-mark.cave-ayland@ilande.co.uk> <1392800720-2765-2-git-send-email-mark.cave-ayland@ilande.co.uk> <20140219133502.GA20305@dorilex> <5305247D.7060705@ilande.co.uk> <536B95E4.5020009@suse.de> <537A0133.8010609@ilande.co.uk> In-Reply-To: <537A0133.8010609@ilande.co.uk> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCHv3 1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark Cave-Ayland Cc: Peter Maydell , Leandro Dorileo , qemu-devel@nongnu.org, Blue Swirl , Bob Breuer , Anthony Liguori , Artyom Tarasenko Am 19.05.2014 15:03, schrieb Mark Cave-Ayland: > On 08/05/14 15:34, Andreas F=E4rber wrote: >> Am 19.02.2014 22:39, schrieb Mark Cave-Ayland: >>> On 19/02/14 13:35, Leandro Dorileo wrote: >>>>> +static void cg3_realizefn(DeviceState *dev, Error **errp) >>>>> +{ >>>>> + SysBusDevice *sbd =3D SYS_BUS_DEVICE(dev); >>>>> + CG3State *s =3D CG3(dev); >>>>> + int ret; >>>>> + char *fcode_filename; >>>>> + >>>>> + /* FCode ROM */ >>>>> + memory_region_init_ram(&s->rom, NULL, "cg3.prom", >>>>> FCODE_MAX_ROM_SIZE); >>>>> + vmstate_register_ram_global(&s->rom); >>>>> + memory_region_set_readonly(&s->rom, true); >>>>> + sysbus_init_mmio(sbd,&s->rom); >>>>> + >>>> >>>> >>>> I think this initialization code could be done in a SysBusDeviceClas= s >>>> init operation, >>>> don't you think? >>> >>> I think it's possible since these MemoryRegions don't depend upon >>> properties, but I leave that to Andres who seems reasonably happy wit= h >>> the patchset in its current form. >> >> memory_region_init_ram() and sysbus_init_mmio() could indeed be moved = to >> an .instance_init function, given that FCODE_MAX_ROM_SIZE is constant. >> The others no. It makes a difference when considering reentrancy of th= e >> property code via qom-set (just posted a patchset that makes playing >> with that easier), although there's probably more corner cases to >> consider. Could either of you follow up with a cleanup? >=20 > Is something like this correct? It also seems that the register I/O > region initialisation can get moved into the initfn since that doesn't > depend on any properties either. >=20 > If it looks good, I'll spin up a proper patchset that does the same to > TCX too. >=20 >=20 > --- a/hw/display/cg3.c > +++ b/hw/display/cg3.c > @@ -274,19 +274,30 @@ static const GraphicHwOps cg3_ops =3D { > .gfx_update =3D cg3_update_display, > }; >=20 > -static void cg3_realizefn(DeviceState *dev, Error **errp) > +static void cg3_initfn(Object *obj) > { > - SysBusDevice *sbd =3D SYS_BUS_DEVICE(dev); > - CG3State *s =3D CG3(dev); > - int ret; > - char *fcode_filename; > + SysBusDevice *sbd =3D SYS_BUS_DEVICE(obj); > + CG3State *s =3D CG3(obj); >=20 > /* FCode ROM */ > memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SI= ZE); > vmstate_register_ram_global(&s->rom); This has global effects, so must stay in realize. The rest looks good. Cheers, Andreas > memory_region_set_readonly(&s->rom, true); > sysbus_init_mmio(sbd, &s->rom); > + > + memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg", > + CG3_REG_SIZE); > + sysbus_init_mmio(sbd, &s->reg); > +} >=20 > +static void cg3_realizefn(DeviceState *dev, Error **errp) > +{ > + SysBusDevice *sbd =3D SYS_BUS_DEVICE(dev); > + CG3State *s =3D CG3(dev); > + int ret; > + char *fcode_filename; > + > + /* FCode ROM */ > fcode_filename =3D qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FIL= E); > if (fcode_filename) { > ret =3D load_image_targphys(fcode_filename, s->prom_addr, > @@ -296,10 +307,6 @@ static void cg3_realizefn(DeviceState *dev, Error > **errp) > } > } >=20 > - memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg", > - CG3_REG_SIZE); > - sysbus_init_mmio(sbd, &s->reg); > - > memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_siz= e); > vmstate_register_ram_global(&s->vram_mem); > sysbus_init_mmio(sbd, &s->vram_mem); > @@ -374,6 +381,7 @@ static const TypeInfo cg3_info =3D { > .name =3D TYPE_CG3, > .parent =3D TYPE_SYS_BUS_DEVICE, > .instance_size =3D sizeof(CG3State), > + .instance_init =3D cg3_initfn, > .class_init =3D cg3_class_init, > }; --=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