qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/6] qom: Allow to make integer qom p


From: Peter Crosthwaite
Subject: Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/6] qom: Allow to make integer qom properties writeable
Date: Wed, 2 Jul 2014 13:48:25 +1000

On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <address@hidden> wrote:
> We have helper functions to easily expose integers as QOM object properties.
> However, these are read only.

I think this is a good idea, and _ptr properties should have some write-ability.

> Let's introduce some easy helpers that would
> just write to the values.
>
> We also add a new parameter to the property registration that allows us to
> specify the setter function. If we need special magic to happen, we can make
> it happen in our own setters. Otherwise we can just use one of the provided
> helper setters that implement generic value writes for integers.
>
> With this we can easily create read-write qom variables, but maintain the
> flexibility to diverge slightly from the default case.
>

But I think it's inconsistent that the read path is automatic and
write path required an open-coded callback to be added. If you need
open-codedness then it can be achieved with just raw
object_property_set. The real issue is that the other side (e.g. the
read handler for a side-effected write) will need to be open coded too
leading to mass boiler plate (is this the problem you are trying to
solve?).

I think we can minimise the number of core QOM API use cases while
achieving your goal by:

1: Leaving the _ptr APIs as minimal having no open-coded capability.
2: Adding global trivial getter and setter fns that can be passed to
the lower level object_property_add.

When one side of a property (either read or write) has a side effect,
go open-coded (you can also call the trivial setter/getter from your
open coded fn for the actual ptr read/write to avoid device code
having to manage visitors). Then add the trivial setter/getter for the
other side. LOC should be roughly the same as this soln.

This also supports the rarer case of a property with read side effects
(can't think of a use case yet but i sure it's out there).

Regards,
Peter

> Signed-off-by: Alexander Graf <address@hidden>
> ---
>  hw/acpi/ich9.c       |  4 +--
>  hw/acpi/pcihp.c      |  2 +-
>  hw/acpi/piix4.c      | 12 ++++----
>  hw/i386/acpi-build.c |  2 +-
>  hw/isa/lpc_ich9.c    |  4 +--
>  include/qom/object.h | 84 
> +++++++++++++++++++++++++++++++++++++++++++++++++---
>  qom/object.c         | 13 +++++++-
>  ui/console.c         |  3 +-
>  8 files changed, 105 insertions(+), 19 deletions(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e7d6c77..36a3998 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -286,12 +286,12 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs 
> *pm, Error **errp)
>      pm->acpi_memory_hotplug.is_enabled = true;
>
>      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> -                                   &pm->pm_io_base, errp);
> +                                   &pm->pm_io_base, NULL, errp);
>      object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
>                          ich9_pm_get_gpe0_blk,
>                          NULL, NULL, pm, NULL);
>      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> -                                   &gpe0_len, errp);
> +                                   &gpe0_len, NULL, errp);
>      object_property_add_bool(obj, "memory-hotplug-support",
>                               ich9_pm_get_memory_hotplug_support,
>                               ich9_pm_set_memory_hotplug_support,
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index fae663a..978b785 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -312,7 +312,7 @@ void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
>
>          *bus_bsel = ACPI_PCIHP_BSEL_DEFAULT;
>          object_property_add_uint32_ptr(OBJECT(root_bus), 
> ACPI_PCIHP_PROP_BSEL,
> -                                       bus_bsel, NULL);
> +                                       bus_bsel, NULL, NULL);
>      }
>
>      memory_region_init_io(&s->io, NULL, &acpi_pcihp_io_ops, s,
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index b72b34e..76191fc 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -405,17 +405,17 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
>      static const uint16_t sci_int = 9;
>
>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> -                                  &acpi_enable_cmd, NULL);
> +                                  &acpi_enable_cmd, NULL, NULL);
>      object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> -                                  &acpi_disable_cmd, NULL);
> +                                  &acpi_disable_cmd, NULL, NULL);
>      object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
> -                                  &gpe0_blk, NULL);
> +                                  &gpe0_blk, NULL, NULL);
>      object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
> -                                  &gpe0_blk_len, NULL);
> +                                  &gpe0_blk_len, NULL, NULL);
>      object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
> -                                  &sci_int, NULL);
> +                                  &sci_int, NULL, NULL);
>      object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
> -                                  &s->io_base, NULL);
> +                                  &s->io_base, NULL, NULL);
>  }
>
>  static int piix4_pm_initfn(PCIDevice *dev)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ebc5f03..f2a58ab 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -750,7 +750,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>
>          *bus_bsel = (*bsel_alloc)++;
>          object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> -                                       bus_bsel, NULL);
> +                                       bus_bsel, NULL, NULL);
>      }
>
>      return bsel_alloc;
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index b846d81..8e598d7 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -556,9 +556,9 @@ static void ich9_lpc_add_properties(ICH9LPCState *lpc)
>                          ich9_lpc_get_sci_int,
>                          NULL, NULL, NULL, NULL);
>      object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_ENABLE_CMD,
> -                                  &acpi_enable_cmd, NULL);
> +                                  &acpi_enable_cmd, NULL, NULL);
>      object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
> -                                  &acpi_disable_cmd, NULL);
> +                                  &acpi_disable_cmd, NULL, NULL);
>
>      ich9_pm_add_properties(OBJECT(lpc), &lpc->pm, NULL);
>  }
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 8a05a81..deae138 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1207,52 +1207,128 @@ void object_property_add_bool(Object *obj, const 
> char *name,
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @v: pointer to value
> + * @set: setter function for this value, pass NULL for read-only
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Add an integer property in memory.  This function will add a
>   * property of type 'uint8'.
>   */
>  void object_property_add_uint8_ptr(Object *obj, const char *name,
> -                                   const uint8_t *v, Error **errp);
> +                                   const uint8_t *v,
> +                                   ObjectPropertyAccessor *set,
> +                                   Error **errp);
> +
> +/**
> + * property_set_uint8_ptr:
> + * @obj: the object to set the property in
> + * @v: visitor to the property
> + * @opaque: pointer to the integer value
> + * @name: name of the property
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Visitor function to set an integer value of type 'uint8' to a give value.
> + * Use this as the 'set' argument in object_property_add_uint8_ptr to get
> + * a read/write value.
> + */
> +void object_property_set_uint8_ptr(Object *obj, struct Visitor *v,
> +                                    void *opaque, const char *name,
> +                                    Error **errp);
>
>  /**
>   * object_property_add_uint16_ptr:
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @v: pointer to value
> + * @set: setter function for this value, pass NULL for read-only
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Add an integer property in memory.  This function will add a
>   * property of type 'uint16'.
>   */
>  void object_property_add_uint16_ptr(Object *obj, const char *name,
> -                                    const uint16_t *v, Error **errp);
> +                                    const uint16_t *v,
> +                                    ObjectPropertyAccessor *set,
> +                                    Error **errp);
> +
> +/**
> + * property_set_uint16_ptr:
> + * @obj: the object to set the property in
> + * @v: visitor to the property
> + * @opaque: pointer to the integer value
> + * @name: name of the property
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Visitor function to set an integer value of type 'uint16' to a give value.
> + * Use this as the 'set' argument in object_property_add_uint16_ptr to get
> + * a read/write value.
> + */
> +void object_property_set_uint16_ptr(Object *obj, struct Visitor *v,
> +                                    void *opaque, const char *name,
> +                                    Error **errp);
>
>  /**
>   * object_property_add_uint32_ptr:
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @v: pointer to value
> + * @set: setter function for this value, pass NULL for read-only
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Add an integer property in memory.  This function will add a
>   * property of type 'uint32'.
>   */
>  void object_property_add_uint32_ptr(Object *obj, const char *name,
> -                                    const uint32_t *v, Error **errp);
> +                                    const uint32_t *v,
> +                                    ObjectPropertyAccessor *set,
> +                                    Error **errp);
> +
> +/**
> + * property_set_uint32_ptr:
> + * @obj: the object to set the property in
> + * @v: visitor to the property
> + * @opaque: pointer to the integer value
> + * @name: name of the property
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Visitor function to set an integer value of type 'uint32' to a give value.
> + * Use this as the 'set' argument in object_property_add_uint32_ptr to get
> + * a read/write value.
> + */
> +void object_property_set_uint32_ptr(Object *obj, struct Visitor *v,
> +                                    void *opaque, const char *name,
> +                                    Error **errp);
>
>  /**
>   * object_property_add_uint64_ptr:
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @v: pointer to value
> + * @set: setter function for this value, pass NULL for read-only
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Add an integer property in memory.  This function will add a
>   * property of type 'uint64'.
>   */
>  void object_property_add_uint64_ptr(Object *obj, const char *name,
> -                                    const uint64_t *v, Error **Errp);
> +                                    const uint64_t *v,
> +                                    ObjectPropertyAccessor *set,
> +                                    Error **Errp);
> +
> +/**
> + * property_set_uint64_ptr:
> + * @obj: the object to set the property in
> + * @v: visitor to the property
> + * @opaque: pointer to the integer value
> + * @name: name of the property
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Visitor function to set an integer value of type 'uint64' to a give value.
> + * Use this as the 'set' argument in object_property_add_uint64_ptr to get
> + * a read/write value.
> + */
> +void object_property_set_uint64_ptr(Object *obj, struct Visitor *v,
> +                                    void *opaque, const char *name,
> +                                    Error **errp);
>
>  /**
>   * object_property_add_alias:
> diff --git a/qom/object.c b/qom/object.c
> index 73cd717..ff13b68 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1518,12 +1518,23 @@ static void 
> glue(glue(property_get_,size),_ptr)(Object *obj, Visitor *v,       \
>      glue(visit_type_,size)(v, &value, name, errp);                           
>   \
>  }                                                                            
>   \
>                                                                               
>   \
> +void glue(glue(object_property_set_,size),_ptr)(Object *obj, Visitor *v,     
>   \
> +                                                void *opaque,                
>   \
> +                                                const char *name,            
>   \
> +                                                Error **errp)                
>   \
> +{                                                                            
>   \
> +    glue(size,_t) value;                                                     
>   \
> +    glue(visit_type_,size)(v, &value, name, errp);                           
>   \
> +    *(glue(size,_t) *)opaque = value;                                        
>   \
> +}                                                                            
>   \
> +                                                                             
>   \
>  void glue(glue(object_property_add_,size),_ptr)(Object *obj, const char 
> *name, \
>                                                  const glue(size,_t) *v,      
>   \
> +                                                ObjectPropertyAccessor *set, 
>   \
>                                                  Error **errp)                
>   \
>  {                                                                            
>   \
>      ObjectPropertyAccessor *get = glue(glue(property_get_,size),_ptr);       
>   \
> -    object_property_add(obj, name, stringify(size), get, NULL, NULL, (void 
> *)v,\
> +    object_property_add(obj, name, stringify(size), get, set, NULL, (void 
> *)v, \
>                          errp);                                               
>   \
>  }                                                                            
>   \
>
> diff --git a/ui/console.c b/ui/console.c
> index ab84549..af61ac8 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1195,8 +1195,7 @@ static QemuConsole *new_console(DisplayState *ds, 
> console_type_t console_type,
>                               object_property_allow_set_link,
>                               OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                               &error_abort);
> -    object_property_add_uint32_ptr(obj, "head",
> -                                   &s->head, &error_abort);
> +    object_property_add_uint32_ptr(obj, "head", &s->head, NULL, 
> &error_abort);
>
>      if (!active_console || ((active_console->console_type != 
> GRAPHIC_CONSOLE) &&
>          (console_type == GRAPHIC_CONSOLE))) {
> --
> 1.8.1.4
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]