| Message ID | 20191023173154.30051-22-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On 10/23/19 7:31 PM, Marc-André Lureau wrote: > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/dma/sparc32_dma.c | 2 +- > hw/net/lance.c | 5 ++--- > hw/net/pcnet-pci.c | 2 +- > hw/net/pcnet.h | 2 +- > 4 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c > index 0e5bbcdc7f..3e4da0c47f 100644 > --- a/hw/dma/sparc32_dma.c > +++ b/hw/dma/sparc32_dma.c > @@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp) > d = qdev_create(NULL, TYPE_LANCE); > object_property_add_child(OBJECT(dev), "lance", OBJECT(d), errp); > qdev_set_nic_properties(d, nd); > - qdev_prop_set_ptr(d, "dma", dev); > + object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp); > qdev_init_nofail(d); > } > > diff --git a/hw/net/lance.c b/hw/net/lance.c > index 6631e2a4e0..4d96299041 100644 > --- a/hw/net/lance.c > +++ b/hw/net/lance.c > @@ -138,7 +138,8 @@ static void lance_instance_init(Object *obj) > } > > static Property lance_properties[] = { > - DEFINE_PROP_PTR("dma", SysBusPCNetState, state.dma_opaque), > + DEFINE_PROP_LINK("dma", SysBusPCNetState, state.dma_opaque, > + TYPE_DEVICE, DeviceState *), > DEFINE_NIC_PROPERTIES(SysBusPCNetState, state.conf), > DEFINE_PROP_END_OF_LIST(), > }; > @@ -153,8 +154,6 @@ static void lance_class_init(ObjectClass *klass, void *data) > dc->reset = lance_reset; > dc->vmsd = &vmstate_lance; > dc->props = lance_properties; > - /* Reason: pointer property "dma" */ > - dc->user_creatable = false; But we still can not start it with the -device option and set the dma, can we? > } > > static const TypeInfo lance_info = { > diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c > index 4723c30c79..d067d21e2c 100644 > --- a/hw/net/pcnet-pci.c > +++ b/hw/net/pcnet-pci.c > @@ -231,7 +231,7 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp) > s->irq = pci_allocate_irq(pci_dev); > s->phys_mem_read = pci_physical_memory_read; > s->phys_mem_write = pci_physical_memory_write; > - s->dma_opaque = pci_dev; > + s->dma_opaque = DEVICE(pci_dev); > > pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); > } > diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h > index 28d19a5c6f..f49b213c57 100644 > --- a/hw/net/pcnet.h > +++ b/hw/net/pcnet.h > @@ -50,7 +50,7 @@ struct PCNetState_st { > uint8_t *buf, int len, int do_bswap); > void (*phys_mem_write)(void *dma_opaque, hwaddr addr, > uint8_t *buf, int len, int do_bswap); > - void *dma_opaque; > + DeviceState *dma_opaque; > int tx_busy; > int looptest; > }; >
Hi On Thu, Oct 24, 2019 at 1:01 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 10/23/19 7:31 PM, Marc-André Lureau wrote: > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > hw/dma/sparc32_dma.c | 2 +- > > hw/net/lance.c | 5 ++--- > > hw/net/pcnet-pci.c | 2 +- > > hw/net/pcnet.h | 2 +- > > 4 files changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c > > index 0e5bbcdc7f..3e4da0c47f 100644 > > --- a/hw/dma/sparc32_dma.c > > +++ b/hw/dma/sparc32_dma.c > > @@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp) > > d = qdev_create(NULL, TYPE_LANCE); > > object_property_add_child(OBJECT(dev), "lance", OBJECT(d), errp); > > qdev_set_nic_properties(d, nd); > > - qdev_prop_set_ptr(d, "dma", dev); > > + object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp); > > qdev_init_nofail(d); > > } > > > > diff --git a/hw/net/lance.c b/hw/net/lance.c > > index 6631e2a4e0..4d96299041 100644 > > --- a/hw/net/lance.c > > +++ b/hw/net/lance.c > > @@ -138,7 +138,8 @@ static void lance_instance_init(Object *obj) > > } > > > > static Property lance_properties[] = { > > - DEFINE_PROP_PTR("dma", SysBusPCNetState, state.dma_opaque), > > + DEFINE_PROP_LINK("dma", SysBusPCNetState, state.dma_opaque, > > + TYPE_DEVICE, DeviceState *), > > DEFINE_NIC_PROPERTIES(SysBusPCNetState, state.conf), > > DEFINE_PROP_END_OF_LIST(), > > }; > > @@ -153,8 +154,6 @@ static void lance_class_init(ObjectClass *klass, void *data) > > dc->reset = lance_reset; > > dc->vmsd = &vmstate_lance; > > dc->props = lance_properties; > > - /* Reason: pointer property "dma" */ > > - dc->user_creatable = false; > > But we still can not start it with the -device option and set the dma, > can we? This is a sysbus device, so you can't. I'll add a commit comment. In theory, link property allows you to pass a QOM path to reference a QOM instance from -device. > > > } > > > > static const TypeInfo lance_info = { > > diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c > > index 4723c30c79..d067d21e2c 100644 > > --- a/hw/net/pcnet-pci.c > > +++ b/hw/net/pcnet-pci.c > > @@ -231,7 +231,7 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp) > > s->irq = pci_allocate_irq(pci_dev); > > s->phys_mem_read = pci_physical_memory_read; > > s->phys_mem_write = pci_physical_memory_write; > > - s->dma_opaque = pci_dev; > > + s->dma_opaque = DEVICE(pci_dev); > > > > pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); > > } > > diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h > > index 28d19a5c6f..f49b213c57 100644 > > --- a/hw/net/pcnet.h > > +++ b/hw/net/pcnet.h > > @@ -50,7 +50,7 @@ struct PCNetState_st { > > uint8_t *buf, int len, int do_bswap); > > void (*phys_mem_write)(void *dma_opaque, hwaddr addr, > > uint8_t *buf, int len, int do_bswap); > > - void *dma_opaque; > > + DeviceState *dma_opaque; > > int tx_busy; > > int looptest; > > }; > >
On 10/24/19 1:09 PM, Marc-André Lureau wrote: > Hi > > > On Thu, Oct 24, 2019 at 1:01 AM Philippe Mathieu-Daudé > <philmd@redhat.com> wrote: >> >> On 10/23/19 7:31 PM, Marc-André Lureau wrote: >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> hw/dma/sparc32_dma.c | 2 +- >>> hw/net/lance.c | 5 ++--- >>> hw/net/pcnet-pci.c | 2 +- >>> hw/net/pcnet.h | 2 +- >>> 4 files changed, 5 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c >>> index 0e5bbcdc7f..3e4da0c47f 100644 >>> --- a/hw/dma/sparc32_dma.c >>> +++ b/hw/dma/sparc32_dma.c >>> @@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp) >>> d = qdev_create(NULL, TYPE_LANCE); >>> object_property_add_child(OBJECT(dev), "lance", OBJECT(d), errp); >>> qdev_set_nic_properties(d, nd); >>> - qdev_prop_set_ptr(d, "dma", dev); >>> + object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp); >>> qdev_init_nofail(d); >>> } >>> >>> diff --git a/hw/net/lance.c b/hw/net/lance.c >>> index 6631e2a4e0..4d96299041 100644 >>> --- a/hw/net/lance.c >>> +++ b/hw/net/lance.c >>> @@ -138,7 +138,8 @@ static void lance_instance_init(Object *obj) >>> } >>> >>> static Property lance_properties[] = { >>> - DEFINE_PROP_PTR("dma", SysBusPCNetState, state.dma_opaque), >>> + DEFINE_PROP_LINK("dma", SysBusPCNetState, state.dma_opaque, >>> + TYPE_DEVICE, DeviceState *), >>> DEFINE_NIC_PROPERTIES(SysBusPCNetState, state.conf), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> @@ -153,8 +154,6 @@ static void lance_class_init(ObjectClass *klass, void *data) >>> dc->reset = lance_reset; >>> dc->vmsd = &vmstate_lance; >>> dc->props = lance_properties; >>> - /* Reason: pointer property "dma" */ >>> - dc->user_creatable = false; >> >> But we still can not start it with the -device option and set the dma, >> can we? > > This is a sysbus device, so you can't. I'll add a commit comment. Ah OK, understood now. With comment: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > In theory, link property allows you to pass a QOM path to reference a > QOM instance from -device. Just wondering, if we had a "bus_address" property to the abstract SysBus class (and eventually "bus_name" for later) we could create/map sysbus devices from command line? >> >>> } >>> >>> static const TypeInfo lance_info = { >>> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c >>> index 4723c30c79..d067d21e2c 100644 >>> --- a/hw/net/pcnet-pci.c >>> +++ b/hw/net/pcnet-pci.c >>> @@ -231,7 +231,7 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp) >>> s->irq = pci_allocate_irq(pci_dev); >>> s->phys_mem_read = pci_physical_memory_read; >>> s->phys_mem_write = pci_physical_memory_write; >>> - s->dma_opaque = pci_dev; >>> + s->dma_opaque = DEVICE(pci_dev); >>> >>> pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); >>> } >>> diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h >>> index 28d19a5c6f..f49b213c57 100644 >>> --- a/hw/net/pcnet.h >>> +++ b/hw/net/pcnet.h >>> @@ -50,7 +50,7 @@ struct PCNetState_st { >>> uint8_t *buf, int len, int do_bswap); >>> void (*phys_mem_write)(void *dma_opaque, hwaddr addr, >>> uint8_t *buf, int len, int do_bswap); >>> - void *dma_opaque; >>> + DeviceState *dma_opaque; >>> int tx_busy; >>> int looptest; >>> }; >>>
On Thu, 24 Oct 2019 at 12:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Just wondering, if we had a "bus_address" property to the abstract > SysBus class (and eventually "bus_name" for later) we could create/map > sysbus devices from command line? I don't think this is a good plan -- users shouldn't have to know about the memory map of their boards. Plus it doesn't deal with the complications of multiple address spaces, DMA, wiring up irq lines to an interrupt controller, SoC reset handling, clocks, power-managment... Command line -device was designed for pluggable devices, where in the world of real hardware the device can be physically plugged and unplugged and there's a clear interface that can be modelled. You can't add an extra UART to an embedded board in real hardware either. The only plausible argument I've seen for command-line plugging of embedded devices is as a sort of side-effect of having a configuration language syntax for them for the purpose of being able to write board models as data-driven config files rather than in C code. But that would be a lot of design and engineering work, and if we want that I think we should approach it forwards, not arrive at it backwards by adding gradual tweaks like 'address' properties to devices. thanks -- PMM
On Thu, Oct 24, 2019 at 2:17 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 10/24/19 1:09 PM, Marc-André Lureau wrote: > > Hi > > > > > > On Thu, Oct 24, 2019 at 1:01 AM Philippe Mathieu-Daudé > > <philmd@redhat.com> wrote: > >> > >> On 10/23/19 7:31 PM, Marc-André Lureau wrote: > >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > >>> --- > >>> hw/dma/sparc32_dma.c | 2 +- > >>> hw/net/lance.c | 5 ++--- > >>> hw/net/pcnet-pci.c | 2 +- > >>> hw/net/pcnet.h | 2 +- > >>> 4 files changed, 5 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c > >>> index 0e5bbcdc7f..3e4da0c47f 100644 > >>> --- a/hw/dma/sparc32_dma.c > >>> +++ b/hw/dma/sparc32_dma.c > >>> @@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp) > >>> d = qdev_create(NULL, TYPE_LANCE); > >>> object_property_add_child(OBJECT(dev), "lance", OBJECT(d), errp); > >>> qdev_set_nic_properties(d, nd); > >>> - qdev_prop_set_ptr(d, "dma", dev); > >>> + object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp); > >>> qdev_init_nofail(d); > >>> } > >>> > >>> diff --git a/hw/net/lance.c b/hw/net/lance.c > >>> index 6631e2a4e0..4d96299041 100644 > >>> --- a/hw/net/lance.c > >>> +++ b/hw/net/lance.c > >>> @@ -138,7 +138,8 @@ static void lance_instance_init(Object *obj) > >>> } > >>> > >>> static Property lance_properties[] = { > >>> - DEFINE_PROP_PTR("dma", SysBusPCNetState, state.dma_opaque), > >>> + DEFINE_PROP_LINK("dma", SysBusPCNetState, state.dma_opaque, > >>> + TYPE_DEVICE, DeviceState *), > >>> DEFINE_NIC_PROPERTIES(SysBusPCNetState, state.conf), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> @@ -153,8 +154,6 @@ static void lance_class_init(ObjectClass *klass, void *data) > >>> dc->reset = lance_reset; > >>> dc->vmsd = &vmstate_lance; > >>> dc->props = lance_properties; > >>> - /* Reason: pointer property "dma" */ > >>> - dc->user_creatable = false; > >> > >> But we still can not start it with the -device option and set the dma, > >> can we? > > > > This is a sysbus device, so you can't. I'll add a commit comment. > > Ah OK, understood now. > > With comment: > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > > In theory, link property allows you to pass a QOM path to reference a > > QOM instance from -device. > > Just wondering, if we had a "bus_address" property to the abstract > SysBus class (and eventually "bus_name" for later) we could create/map > sysbus devices from command line? I can't tell much, as I am not very familiar with the various sysbus devices. I think you'll have troubles to specify the various io/mmio & irq to map to. In theory though, we could probably go in that direction, even perhaps make "machine" from command line or description file... I am not sure it's worth though. > > >> > >>> } > >>> > >>> static const TypeInfo lance_info = { > >>> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c > >>> index 4723c30c79..d067d21e2c 100644 > >>> --- a/hw/net/pcnet-pci.c > >>> +++ b/hw/net/pcnet-pci.c > >>> @@ -231,7 +231,7 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp) > >>> s->irq = pci_allocate_irq(pci_dev); > >>> s->phys_mem_read = pci_physical_memory_read; > >>> s->phys_mem_write = pci_physical_memory_write; > >>> - s->dma_opaque = pci_dev; > >>> + s->dma_opaque = DEVICE(pci_dev); > >>> > >>> pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); > >>> } > >>> diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h > >>> index 28d19a5c6f..f49b213c57 100644 > >>> --- a/hw/net/pcnet.h > >>> +++ b/hw/net/pcnet.h > >>> @@ -50,7 +50,7 @@ struct PCNetState_st { > >>> uint8_t *buf, int len, int do_bswap); > >>> void (*phys_mem_write)(void *dma_opaque, hwaddr addr, > >>> uint8_t *buf, int len, int do_bswap); > >>> - void *dma_opaque; > >>> + DeviceState *dma_opaque; > >>> int tx_busy; > >>> int looptest; > >>> }; > >>> >
On Thu, Oct 24, 2019 at 12:52:28PM +0100, Peter Maydell wrote: > On Thu, 24 Oct 2019 at 12:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Just wondering, if we had a "bus_address" property to the abstract > > SysBus class (and eventually "bus_name" for later) we could create/map > > sysbus devices from command line? > > I don't think this is a good plan -- users shouldn't have to know > about the memory map of their boards. Plus it doesn't deal with > the complications of multiple address spaces, DMA, wiring up > irq lines to an interrupt controller, SoC reset handling, > clocks, power-managment... Command line -device was designed > for pluggable devices, where in the world of real hardware > the device can be physically plugged and unplugged and there's > a clear interface that can be modelled. You can't add an > extra UART to an embedded board in real hardware either. > > The only plausible argument I've seen for command-line > plugging of embedded devices is as a sort of side-effect > of having a configuration language syntax for them for > the purpose of being able to write board models as > data-driven config files rather than in C code. But > that would be a lot of design and engineering work, and > if we want that I think we should approach it forwards, > not arrive at it backwards by adding gradual tweaks like > 'address' properties to devices. The QEMU community spent years designing QOM and QMP with that goal. Which other pieces to you consider to be missing, to make you reject making gradual changes towards it? I agree we shouldn't be introducing new external interfaces without careful thought. But I welcome gradual internal API changes that are helpful for our long term goals.
On Thu, 24 Oct 2019 at 19:07, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Oct 24, 2019 at 12:52:28PM +0100, Peter Maydell wrote: > > I don't think this is a good plan -- users shouldn't have to know > > about the memory map of their boards. Plus it doesn't deal with > > the complications of multiple address spaces, DMA, wiring up > > irq lines to an interrupt controller, SoC reset handling, > > clocks, power-managment... Command line -device was designed > > for pluggable devices, where in the world of real hardware > > the device can be physically plugged and unplugged and there's > > a clear interface that can be modelled. You can't add an > > extra UART to an embedded board in real hardware either. > > > > The only plausible argument I've seen for command-line > > plugging of embedded devices is as a sort of side-effect > > of having a configuration language syntax for them for > > the purpose of being able to write board models as > > data-driven config files rather than in C code. But > > that would be a lot of design and engineering work, and > > if we want that I think we should approach it forwards, > > not arrive at it backwards by adding gradual tweaks like > > 'address' properties to devices. > > The QEMU community spent years designing QOM and QMP with that > goal. Which other pieces to you consider to be missing, to > make you reject making gradual changes towards it? QOM is an *internal* object model. It's fine for building machine models *in C*. We have no mechanism for doing this on the command line or via QMP, because there are lots of parts of machine models (listed in the first paragraph above) which aren't possible to do with purely generic links and properties. > I agree we shouldn't be introducing new external interfaces > without careful thought. But I welcome gradual internal API > changes that are helpful for our long term goals. Yeah, I have no objection to useful internal changes that move generally in directions we'd like to go. But if "machines via config files" really is a goal I'd like to see at least a sketch of a design, rationale, summary of what would need to change and proposals for what would be done. And I don't think we should be exposing "MMIO addresses" to users without at least some idea of why that would fit in with other things we would be doing to move towards where we're going. (TBH: I also don't really see 'machines via config files' as a serious project goal -- we haven't really moved towards doing that in a decade, as far as I can see.) thanks -- PMM
diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c index 0e5bbcdc7f..3e4da0c47f 100644 --- a/hw/dma/sparc32_dma.c +++ b/hw/dma/sparc32_dma.c @@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp) d = qdev_create(NULL, TYPE_LANCE); object_property_add_child(OBJECT(dev), "lance", OBJECT(d), errp); qdev_set_nic_properties(d, nd); - qdev_prop_set_ptr(d, "dma", dev); + object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp); qdev_init_nofail(d); } diff --git a/hw/net/lance.c b/hw/net/lance.c index 6631e2a4e0..4d96299041 100644 --- a/hw/net/lance.c +++ b/hw/net/lance.c @@ -138,7 +138,8 @@ static void lance_instance_init(Object *obj) } static Property lance_properties[] = { - DEFINE_PROP_PTR("dma", SysBusPCNetState, state.dma_opaque), + DEFINE_PROP_LINK("dma", SysBusPCNetState, state.dma_opaque, + TYPE_DEVICE, DeviceState *), DEFINE_NIC_PROPERTIES(SysBusPCNetState, state.conf), DEFINE_PROP_END_OF_LIST(), }; @@ -153,8 +154,6 @@ static void lance_class_init(ObjectClass *klass, void *data) dc->reset = lance_reset; dc->vmsd = &vmstate_lance; dc->props = lance_properties; - /* Reason: pointer property "dma" */ - dc->user_creatable = false; } static const TypeInfo lance_info = { diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c index 4723c30c79..d067d21e2c 100644 --- a/hw/net/pcnet-pci.c +++ b/hw/net/pcnet-pci.c @@ -231,7 +231,7 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp) s->irq = pci_allocate_irq(pci_dev); s->phys_mem_read = pci_physical_memory_read; s->phys_mem_write = pci_physical_memory_write; - s->dma_opaque = pci_dev; + s->dma_opaque = DEVICE(pci_dev); pcnet_common_init(DEVICE(pci_dev), s, &net_pci_pcnet_info); } diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h index 28d19a5c6f..f49b213c57 100644 --- a/hw/net/pcnet.h +++ b/hw/net/pcnet.h @@ -50,7 +50,7 @@ struct PCNetState_st { uint8_t *buf, int len, int do_bswap); void (*phys_mem_write)(void *dma_opaque, hwaddr addr, uint8_t *buf, int len, int do_bswap); - void *dma_opaque; + DeviceState *dma_opaque; int tx_busy; int looptest; };