| Message ID | c554a16417ef22dbc29919a2747801be5ed7fede.1488068248.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
On 25 February 2017 at 18:31, BALATON Zoltan <balaton@eik.bme.hu> wrote: > Only the display controller part is created automatically on PCI > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > > 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 :-) > + 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. > + 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. > +} > + > +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
On Thu, 2 Mar 2017, Peter Maydell wrote: > On 25 February 2017 at 18:31, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> Only the display controller part is created automatically on PCI >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> >> 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 > >
On 2 March 2017 at 20:13, BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Thu, 2 Mar 2017, Peter Maydell wrote: >> >> On 25 February 2017 at 18:31, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>> >>> Only the display controller part is created automatically on PCI >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> >>> 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? The device realize function should fail: if (user settable property foo is wrong) { error_setg(errp, "foo must be X, Y or Z"); return; } >>> + 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? In this patch will be fine. >>> + 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? When I reviewed this patch I hadn't yet got to the later one which adds the vmsd and reset code (and this patch doesn't say in the commit that those will be added later). This is the PCI device bit, incidentally, but I do think that both PCI device and sysbus should have vmsd. > 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. Every device should have vmstate. In this case the bulk of it is going to be shared with the PCI device anyway. For machines which don't yet support migration between versions (which includes anything sh) we have greater flexibility to break migration compat if we need to. This is one of the many cases where old QEMU code doesn't meet modern standards. Our process for improving that is a gradual ratchet effect -- when we need to mess with a device anyway, we add the support for the new stuff. This gradually reduces the set of missing-migration-support devices and makes life easier for somebody who needs to add it to sh later. (A quick scan suggests that the other r2d devices which don't yet support vmstate are the fpga device and the pci host controller.) Nobody's ever going to actually do VM migration for most of these embedded boards, but snapshot save/load is a very useful debugging tool. (You can snapshot eg after a long kernel boot and just before triggering the guest bug you're investigating, and then your debug sessions can start with loading the snapshot. Particularly handy if you are going to turn on a lot of debug logging.) thanks -- PMM
On Thu, 2 Mar 2017, Peter Maydell wrote: > On 2 March 2017 at 20:13, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> On Thu, 2 Mar 2017, Peter Maydell wrote: >>> >>> On 25 February 2017 at 18:31, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>>> >>>> Only the display controller part is created automatically on PCI >>>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> >>>> 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? > > The device realize function should fail: > if (user settable property foo is wrong) { > error_setg(errp, "foo must be X, Y or Z"); > return; > } Actually I think this cannot fail because get_local_mem_size_index() will pick a valid value that's closest to the one specified. However the mem region was not created using this adjusted value. I've fixed this in the v3 I've just sent and also added a warning when adjusment was made. Thank you, BALATON Zoltan
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), + 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; + 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; +} + +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)
Only the display controller part is created automatically on PCI Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- v2: Split off removing dependency on base address to separate patch hw/display/sm501.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)