[01/14] sm501: replace PROP_PTR with PROP_LINK

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/sm501.c | 5 +++--
 hw/sh4/r2d.c       | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/sm501.c | 5 +++--
>  hw/sh4/r2d.c       | 3 ++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 1f33c87e65..a87d18efab 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1930,7 +1930,7 @@ typedef struct {
>      SM501State state;
>      uint32_t vram_size;
>      uint32_t base;
> -    void *chr_state;
> +    Chardev *chr_state;
>  } SM501SysBusState;
>
>  static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> @@ -1968,7 +1968,8 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>  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),
> +    DEFINE_PROP_LINK("chr-state", SM501SysBusState, chr_state,
> +                     TYPE_CHARDEV, Chardev *),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index ee0840f380..5780ee85d9 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -272,7 +272,8 @@ static void r2d_init(MachineState *machine)
>      busdev = SYS_BUS_DEVICE(dev);
>      qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
>      qdev_prop_set_uint32(dev, "base", 0x10000000);
> -    qdev_prop_set_ptr(dev, "chr-state", serial_hd(2));
> +    object_property_set_link(OBJECT(dev), OBJECT(serial_hd(2)),
> +                             "chr-state", &error_abort);
>      qdev_init_nofail(dev);
>      sysbus_mmio_map(busdev, 0, 0x10000000);
>      sysbus_mmio_map(busdev, 1, 0x13e00000);

We have a typed way of passing a Chardev to devices:
use qdev_prop_set_chr(). Unfortunately it's not trivially
easy to drop in here, because it sets a property defined
with DEFINE_PROP_CHR to set a field of type CharBackend
(note, not Chardev, and not a pointer) in the device struct.
But serial_mm_init() wants a Chardev*, because it is a
non-QOM interface to the serial device and is manually
doing the qemu_chr_fe_init() that connects the Chardev
to its own CharBackend. The QOM CHR property setter wants
to do that qemu_chr_fe_init() itself.

So I think the right fix here is to properly QOMify the
code which is not QOMified, ie hw/char/serial.c, in a
way that means that the various "memory mapped 16650-ish
devices" we have can use it and can define a
TYPE_CHARDEV property.

In general I think our uses of PROP_PTR are code smells
that indicate places where we have not properly converted
code over to the general approach that the QOM/qdev
design desires; but we should be getting rid of PROP_PTR
by actually doing all those (difficult) conversions.
Merely removing PROP_PTR itself by rephrasing the dubious
inter-device connections in a way that makes them harder
to grep for doesn't seem to me to be necessarily worth
doing. Is the existence of PROP_PTR getting in your way
for a change you want to make ?

thanks
-- PMM

Hi

On Fri, Oct 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/display/sm501.c | 5 +++--
> >  hw/sh4/r2d.c       | 3 ++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index 1f33c87e65..a87d18efab 100644
> > --- a/hw/display/sm501.c
> > +++ b/hw/display/sm501.c
> > @@ -1930,7 +1930,7 @@ typedef struct {
> >      SM501State state;
> >      uint32_t vram_size;
> >      uint32_t base;
> > -    void *chr_state;
> > +    Chardev *chr_state;
> >  } SM501SysBusState;
> >
> >  static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> > @@ -1968,7 +1968,8 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> >  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),
> > +    DEFINE_PROP_LINK("chr-state", SM501SysBusState, chr_state,
> > +                     TYPE_CHARDEV, Chardev *),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> > index ee0840f380..5780ee85d9 100644
> > --- a/hw/sh4/r2d.c
> > +++ b/hw/sh4/r2d.c
> > @@ -272,7 +272,8 @@ static void r2d_init(MachineState *machine)
> >      busdev = SYS_BUS_DEVICE(dev);
> >      qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
> >      qdev_prop_set_uint32(dev, "base", 0x10000000);
> > -    qdev_prop_set_ptr(dev, "chr-state", serial_hd(2));
> > +    object_property_set_link(OBJECT(dev), OBJECT(serial_hd(2)),
> > +                             "chr-state", &error_abort);
> >      qdev_init_nofail(dev);
> >      sysbus_mmio_map(busdev, 0, 0x10000000);
> >      sysbus_mmio_map(busdev, 1, 0x13e00000);
>
> We have a typed way of passing a Chardev to devices:
> use qdev_prop_set_chr(). Unfortunately it's not trivially
> easy to drop in here, because it sets a property defined
> with DEFINE_PROP_CHR to set a field of type CharBackend
> (note, not Chardev, and not a pointer) in the device struct.
> But serial_mm_init() wants a Chardev*, because it is a
> non-QOM interface to the serial device and is manually
> doing the qemu_chr_fe_init() that connects the Chardev
> to its own CharBackend. The QOM CHR property setter wants
> to do that qemu_chr_fe_init() itself.
>
> So I think the right fix here is to properly QOMify the
> code which is not QOMified, ie hw/char/serial.c, in a
> way that means that the various "memory mapped 16650-ish
> devices" we have can use it and can define a
> TYPE_CHARDEV property.

I see, I can look at that.

> In general I think our uses of PROP_PTR are code smells
> that indicate places where we have not properly converted
> code over to the general approach that the QOM/qdev
> design desires; but we should be getting rid of PROP_PTR
> by actually doing all those (difficult) conversions.

I think all user_creatable = false are smelly in that regard.

> Merely removing PROP_PTR itself by rephrasing the dubious
> inter-device connections in a way that makes them harder
> to grep for doesn't seem to me to be necessarily worth

grep for user_creatable = false

> doing. Is the existence of PROP_PTR getting in your way
> for a change you want to make ?

Yes, I am looking at improving the qdev vs object and class vs
instance properties. I have a larger series of wip refactoring. This
initial series is preliminary cleanup that would help.

On Fri, 18 Oct 2019 at 17:36, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> On Fri, Oct 18, 2019 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > So I think the right fix here is to properly QOMify the
> > code which is not QOMified, ie hw/char/serial.c, in a
> > way that means that the various "memory mapped 16650-ish
> > devices" we have can use it and can define a
> > TYPE_CHARDEV property.
>
> I see, I can look at that.

It would certainly be a nice cleanup/refactoring to have;
it's likely quite painful though as so many things use
serial_mm_init, many of them not properly QOMified themselves;
and because SerialState is used by x86 it's likely to be
tricky to handle for migration-compat reasons.

> > In general I think our uses of PROP_PTR are code smells
> > that indicate places where we have not properly converted
> > code over to the general approach that the QOM/qdev
> > design desires; but we should be getting rid of PROP_PTR
> > by actually doing all those (difficult) conversions.
>
> I think all user_creatable = false are smelly in that regard.

I dunno, I tend to think that "not user creatable" is the
default state of things. Almost all devices aren't user
creatable, because they need to be wired up and put at
suitable addresses and IRQ lines. Pluggable devices like
PCI, ISA, etc are the weird exceptions that are usefully
user-creatable.

> > Merely removing PROP_PTR itself by rephrasing the dubious
> > inter-device connections in a way that makes them harder
> > to grep for doesn't seem to me to be necessarily worth
>
> grep for user_creatable = false
>
> > doing. Is the existence of PROP_PTR getting in your way
> > for a change you want to make ?
>
> Yes, I am looking at improving the qdev vs object and class vs
> instance properties. I have a larger series of wip refactoring. This
> initial series is preliminary cleanup that would help.

OK, that makes sense. I'll have a look through the series
again; if we don't want to do the nice cleanups now then
probably these changes are ok, possibly with some TODO
comments noting what we ideally ought to do instead.

thanks
-- PMM