Re: [Qemu-devel] [PATCH v2 05/14] sm501: Add emulation of chip connected via PCI

[BALATON Zoltan]

On Thu, 2 Mar 2017, Peter Maydell wrote:
> On 25 February 2017 at 18:31, BALATON Zoltan <address@hidden> wrote:
> > Only the display controller part is created automatically on PCI
> >
> > Signed-off-by: BALATON Zoltan <address@hidden>
> > ---
> >
> > v2: Split off removing dependency on base address to separate patch
> >
> >  hw/display/sm501.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index 1cda127..d9219bd 100644
> > --- a/hw/display/sm501.c
> > +++ b/hw/display/sm501.c
> > @@ -31,6 +31,7 @@
> >  #include "ui/console.h"
> >  #include "hw/devices.h"
> >  #include "hw/sysbus.h"
> > +#include "hw/pci/pci.h"
> >  #include "qemu/range.h"
> >  #include "ui/pixel_ops.h"
> >  #include "exec/address-spaces.h"
> > @@ -1507,9 +1508,60 @@ static const TypeInfo sm501_sysbus_info = {
> >      .class_init    = sm501_sysbus_class_init,
> >  };
> >
> > +#define TYPE_PCI_SM501 "sm501"
> > +#define PCI_SM501(obj) OBJECT_CHECK(SM501PCIState, (obj), TYPE_PCI_SM501)
> > +
> > +typedef struct {
> > +    /*< private >*/
> > +    PCIDevice parent_obj;
> > +    /*< public >*/
> > +    SM501State state;
> > +    uint32_t vram_size;
> > +} SM501PCIState;
> > +
> > +static void sm501_realize_pci(PCIDevice *dev, Error **errp)
> > +{
> > +    SM501PCIState *s = PCI_SM501(dev);
> > +
> > +    sm501_init(&s->state, DEVICE(dev), s->vram_size);
> > +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
> > +                     &s->state.local_mem_region);
> > +    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
> > +                     &s->state.mmio_region);
> > +}
> > +
> > +static Property sm501_pci_properties[] = {
> > +    DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size,
> > +                       64 * 1024 * 1024),
>
> Something needs to sanity check this value, because it's
> user settable in the PCI device. (In the sysbus version you
> can only create it from board code, and we can assume board
> code doesn't pass us silly values.)
>
> Or you could just hardcode the PCI device to always be 64MB,
> I guess :-)

Maybe a check could be added to get_local_mem_size_index(). What should it 
do for invalid values?

> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void sm501_pci_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > +
> > +    k->realize = sm501_realize_pci;
> > +    k->vendor_id = 0x126f;
> > +    k->device_id = 0x0501;
>
> We could define some constants in include/hw/pci/pci_ids.h
> for these, I suppose.

Should I add that as well? Should it be in this or separate patch?

> > +    k->class_id = PCI_CLASS_DISPLAY_OTHER;
> > +    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> > +    dc->desc = "SM501 Display Controller";
> > +    dc->props = sm501_pci_properties;
> > +    dc->hotpluggable = false;
>
> This needs a reset and vmsd as well.

Why? OK reset makes sense but if this is only used in sh and I think that 
might have other devices that don't support migration then adding a vmsd 
here only achieves that if we ever change anything in the state the vmsd 
needs to be changed, compatibility ensured, etc. And that's for nothing if 
the sh machine cannot be migrated anyway. There seems to be a lot of 
boilerplate already, it may exceed the actual code at some point.

But I can add vmstate for sysbus version if you think it's needed, I just 
don't see the point.

> > +}
> > +
> > +static const TypeInfo sm501_pci_info = {
> > +    .name          = TYPE_PCI_SM501,
> > +    .parent        = TYPE_PCI_DEVICE,
> > +    .instance_size = sizeof(SM501PCIState),
> > +    .class_init    = sm501_pci_class_init,
> > +};
> > +
> >  static void sm501_register_types(void)
> >  {
> >      type_register_static(&sm501_sysbus_info);
> > +    type_register_static(&sm501_pci_info);
> >  }
> >
> >  type_init(sm501_register_types)
> > --
> > 2.7.4
>
> thanks
> -- PMM