Hi, Am 19.02.2014 22:39, schrieb Mark Cave-Ayland: > On 19/02/14 13:35, Leandro Dorileo wrote: > > Hi Leandro, > >>> +static void cg3_realizefn(DeviceState *dev, Error **errp) >>> +{ >>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>> + CG3State *s = 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 SysBusDeviceClass >> 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 with > the patchset in its current form.
Just seeing this... 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 the 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? Regards, Andreas >>> + fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE); >>> + if (fcode_filename) { >>> + ret = load_image_targphys(fcode_filename, s->prom_addr, >>> + FCODE_MAX_ROM_SIZE); >>> + if (ret< 0 || ret> FCODE_MAX_ROM_SIZE) { >>> + error_report("cg3: could not load prom '%s'", >>> CG3_ROM_FILE); -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg