Re: [Qemu-trivial] [Qemu-devel] [PATCH 03/10] sm501: QOMify

[BALATON Zoltan]

On Sat, 25 Feb 2017, Peter Maydell wrote:
> On 24 February 2017 at 20:23, BALATON Zoltan <address@hidden> wrote:
> > The SM501/SM502 is a multimedia chip that besides a display controller also
> > contains some other functions (see
> > http://cateee.net/lkddb/web-lkddb/MFD_SM501.html) and this is what is
> > emulated here as these are part of the chip itself.
>
> Hmm; that's pretty ugly but I guess it's sort of like a container
> device that needs to contain various things inside it.
>
> > > > -    /* create qemu graphic console */
> > > > -    s->con = graphic_console_init(DEVICE(dev), 0, &sm501_ops, s);
> > > > +static Property sm501_sysbus_properties[] = {
> > > > +    DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> > > > +    DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0),
> > > > +    DEFINE_PROP_PTR("chr-state", SM501SysBusState, chr_state),
>
> Pointer properties and properties which tell the device what
> address it's at are both signs that something's not quite
> modelled right. There may be no better way to do things right
> now, or then again there may be.

Maybe but I think that would not be part of this series but some other 
clean up separately. This series makes it a bit cleaner but does not aim 
to fix everything.

> > > > +    DEFINE_PROP_END_OF_LIST(),
> > > > +};
> > >
> > >
> > > > +static void sm501_sysbus_class_init(ObjectClass *klass, void *data)
> > > > +{
> > > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > > +
> > > > +    dc->realize = sm501_realize_sysbus;
> > > > +    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> > > > +    dc->desc = "SM501 Multimedia Companion";
> > > > +    dc->props = sm501_sysbus_properties;
> > > > +/* Note: pointer property "chr-state" may remain null, thus
> > > > + * no need for dc->cannot_instantiate_with_device_add_yet = true;
> > > > + */
> > > > }
> > >
> > >
> > > You also need a VMState struct registered in dc->vmsd and a reset
> > > function registered in dc->reset.
> >
> >
> > Even if no migration is supported? Is there a simple example I could look at
> > on what should go into these?
>
> The idea is to support migration. There are examples of doing
> VMState structures all over the tree. You just need a structure
> that lists all the bits of your state data structure that
> contain guest-modifiable state.
>
> Reset is straightforward: it's just a function that resets
> the state of the device as if the system had been hard
> powercycled.

I've tried to add these but vmstate is only added to the PCI version 
because the OHCI device I've looked at also does the same, which is likely 
beacuse the sysbus version is only used on SH4 which does not support 
migration anyway. The PCI verison can be instantiated on machines that 
have migration support so it makes sense to do it there. I hope this is 
acceptable.