* [Qemu-devel] [PATCH 1/6] qom: macroify integer property helpers
2014-07-01 21:49 [Qemu-devel] [PATCH 0/6] Dynamic sysbus device allocation support Alexander Graf
@ 2014-07-01 21:49 ` Alexander Graf
2014-07-02 3:29 ` Peter Crosthwaite
2014-07-01 21:49 ` [Qemu-devel] [PATCH 2/6] qom: Allow to make integer qom properties writeable Alexander Graf
` (4 subsequent siblings)
5 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-01 21:49 UTC (permalink / raw)
To: qemu-ppc
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
sean.stalley, pbonzini, afaerber
We have a bunch of nice helpers that allow us to easily register an integer
field as QOM property. However, we have those duplicated for every integer
size available.
This is very cumbersome (and prone to bugs) to work with and extend, so let's
strip out the only difference there is (the size) and generate the actual
functions via a macro.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
qom/object.c | 83 ++++++++++++++++++------------------------------------------
1 file changed, 24 insertions(+), 59 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index 0e8267b..73cd717 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1507,65 +1507,30 @@ static char *qdev_get_type(Object *obj, Error **errp)
return g_strdup(object_get_typename(obj));
}
-static void property_get_uint8_ptr(Object *obj, Visitor *v,
- void *opaque, const char *name,
- Error **errp)
-{
- uint8_t value = *(uint8_t *)opaque;
- visit_type_uint8(v, &value, name, errp);
-}
-
-static void property_get_uint16_ptr(Object *obj, Visitor *v,
- void *opaque, const char *name,
- Error **errp)
-{
- uint16_t value = *(uint16_t *)opaque;
- visit_type_uint16(v, &value, name, errp);
-}
-
-static void property_get_uint32_ptr(Object *obj, Visitor *v,
- void *opaque, const char *name,
- Error **errp)
-{
- uint32_t value = *(uint32_t *)opaque;
- visit_type_uint32(v, &value, name, errp);
-}
-
-static void property_get_uint64_ptr(Object *obj, Visitor *v,
- void *opaque, const char *name,
- Error **errp)
-{
- uint64_t value = *(uint64_t *)opaque;
- visit_type_uint64(v, &value, name, errp);
-}
-
-void object_property_add_uint8_ptr(Object *obj, const char *name,
- const uint8_t *v, Error **errp)
-{
- object_property_add(obj, name, "uint8", property_get_uint8_ptr,
- NULL, NULL, (void *)v, errp);
-}
-
-void object_property_add_uint16_ptr(Object *obj, const char *name,
- const uint16_t *v, Error **errp)
-{
- object_property_add(obj, name, "uint16", property_get_uint16_ptr,
- NULL, NULL, (void *)v, errp);
-}
-
-void object_property_add_uint32_ptr(Object *obj, const char *name,
- const uint32_t *v, Error **errp)
-{
- object_property_add(obj, name, "uint32", property_get_uint32_ptr,
- NULL, NULL, (void *)v, errp);
-}
-
-void object_property_add_uint64_ptr(Object *obj, const char *name,
- const uint64_t *v, Error **errp)
-{
- object_property_add(obj, name, "uint64", property_get_uint64_ptr,
- NULL, NULL, (void *)v, errp);
-}
+#define DECLARE_INTEGER_VISITOR(size) \
+ \
+static void glue(glue(property_get_,size),_ptr)(Object *obj, Visitor *v, \
+ void *opaque, \
+ const char *name, \
+ Error **errp) \
+{ \
+ glue(size,_t) value = *(glue(size,_t) *)opaque; \
+ glue(visit_type_,size)(v, &value, name, errp); \
+} \
+ \
+void glue(glue(object_property_add_,size),_ptr)(Object *obj, const char *name, \
+ const glue(size,_t) *v, \
+ Error **errp) \
+{ \
+ ObjectPropertyAccessor *get = glue(glue(property_get_,size),_ptr); \
+ object_property_add(obj, name, stringify(size), get, NULL, NULL, (void *)v,\
+ errp); \
+} \
+
+DECLARE_INTEGER_VISITOR(uint8)
+DECLARE_INTEGER_VISITOR(uint16)
+DECLARE_INTEGER_VISITOR(uint32)
+DECLARE_INTEGER_VISITOR(uint64)
typedef struct {
Object *target_obj;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 1/6] qom: macroify integer property helpers
2014-07-01 21:49 ` [Qemu-devel] [PATCH 1/6] qom: macroify integer property helpers Alexander Graf
@ 2014-07-02 3:29 ` Peter Crosthwaite
2014-07-02 7:39 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Peter Crosthwaite @ 2014-07-02 3:29 UTC (permalink / raw)
To: Alexander Graf
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, sean.stalley, Paolo Bonzini,
Andreas Färber
On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> wrote:
> We have a bunch of nice helpers that allow us to easily register an integer
> field as QOM property. However, we have those duplicated for every integer
> size available.
>
> This is very cumbersome (and prone to bugs) to work with and extend, so let's
> strip out the only difference there is (the size) and generate the actual
> functions via a macro.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> qom/object.c | 83 ++++++++++++++++++------------------------------------------
> 1 file changed, 24 insertions(+), 59 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 0e8267b..73cd717 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1507,65 +1507,30 @@ static char *qdev_get_type(Object *obj, Error **errp)
> return g_strdup(object_get_typename(obj));
> }
>
> -static void property_get_uint8_ptr(Object *obj, Visitor *v,
> - void *opaque, const char *name,
> - Error **errp)
> -{
> - uint8_t value = *(uint8_t *)opaque;
> - visit_type_uint8(v, &value, name, errp);
> -}
> -
> -static void property_get_uint16_ptr(Object *obj, Visitor *v,
> - void *opaque, const char *name,
> - Error **errp)
> -{
> - uint16_t value = *(uint16_t *)opaque;
> - visit_type_uint16(v, &value, name, errp);
> -}
> -
> -static void property_get_uint32_ptr(Object *obj, Visitor *v,
> - void *opaque, const char *name,
> - Error **errp)
> -{
> - uint32_t value = *(uint32_t *)opaque;
> - visit_type_uint32(v, &value, name, errp);
> -}
> -
> -static void property_get_uint64_ptr(Object *obj, Visitor *v,
> - void *opaque, const char *name,
> - Error **errp)
> -{
> - uint64_t value = *(uint64_t *)opaque;
> - visit_type_uint64(v, &value, name, errp);
> -}
> -
> -void object_property_add_uint8_ptr(Object *obj, const char *name,
> - const uint8_t *v, Error **errp)
> -{
> - object_property_add(obj, name, "uint8", property_get_uint8_ptr,
> - NULL, NULL, (void *)v, errp);
> -}
> -
> -void object_property_add_uint16_ptr(Object *obj, const char *name,
> - const uint16_t *v, Error **errp)
> -{
> - object_property_add(obj, name, "uint16", property_get_uint16_ptr,
> - NULL, NULL, (void *)v, errp);
> -}
> -
> -void object_property_add_uint32_ptr(Object *obj, const char *name,
> - const uint32_t *v, Error **errp)
> -{
> - object_property_add(obj, name, "uint32", property_get_uint32_ptr,
> - NULL, NULL, (void *)v, errp);
> -}
> -
> -void object_property_add_uint64_ptr(Object *obj, const char *name,
> - const uint64_t *v, Error **errp)
> -{
> - object_property_add(obj, name, "uint64", property_get_uint64_ptr,
> - NULL, NULL, (void *)v, errp);
> -}
> +#define DECLARE_INTEGER_VISITOR(size) \
macro needs a better name. DECLARE_PROP_SET_GET - something to make it
clear its about properties.
> + \
> +static void glue(glue(property_get_,size),_ptr)(Object *obj, Visitor *v, \
> + void *opaque, \
> + const char *name, \
> + Error **errp) \
> +{ \
> + glue(size,_t) value = *(glue(size,_t) *)opaque; \
> + glue(visit_type_,size)(v, &value, name, errp); \
> +} \
> + \
> +void glue(glue(object_property_add_,size),_ptr)(Object *obj, const char *name, \
> + const glue(size,_t) *v, \
> + Error **errp) \
> +{ \
> + ObjectPropertyAccessor *get = glue(glue(property_get_,size),_ptr); \
> + object_property_add(obj, name, stringify(size), get, NULL, NULL, (void *)v,\
> + errp); \
> +} \
> +
> +DECLARE_INTEGER_VISITOR(uint8)
> +DECLARE_INTEGER_VISITOR(uint16)
> +DECLARE_INTEGER_VISITOR(uint32)
> +DECLARE_INTEGER_VISITOR(uint64)
>
Worth getting bool working this way too? Theres been a few times now
where I have wanted to object_property_add_bool_ptr. Perhaps:
#define DECLARE_PROP_SET_GET(name, type)
DECLARE_PROP_SET_GET(uint8, uint8_t)
DECLARE_PROP_SET_GET(uint16, uint16_t)
DECLARE_PROP_SET_GET(uint32, uint32_t)
DECLARE_PROP_SET_GET(uint64, uint64_t)
DECLARE_PROP_SET_GET(bool, bool)
Can actually add the bool one later in follow-up patch but just
thinking ahead to get the macro right for wider usage.
Regards,
Peter
> typedef struct {
> Object *target_obj;
> --
> 1.8.1.4
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 1/6] qom: macroify integer property helpers
2014-07-02 3:29 ` Peter Crosthwaite
@ 2014-07-02 7:39 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 7:39 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, sean.stalley, Paolo Bonzini,
Andreas Färber
On 02.07.14 05:29, Peter Crosthwaite wrote:
> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> wrote:
>> We have a bunch of nice helpers that allow us to easily register an integer
>> field as QOM property. However, we have those duplicated for every integer
>> size available.
>>
>> This is very cumbersome (and prone to bugs) to work with and extend, so let's
>> strip out the only difference there is (the size) and generate the actual
>> functions via a macro.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> qom/object.c | 83 ++++++++++++++++++------------------------------------------
>> 1 file changed, 24 insertions(+), 59 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 0e8267b..73cd717 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -1507,65 +1507,30 @@ static char *qdev_get_type(Object *obj, Error **errp)
>> return g_strdup(object_get_typename(obj));
>> }
>>
>> -static void property_get_uint8_ptr(Object *obj, Visitor *v,
>> - void *opaque, const char *name,
>> - Error **errp)
>> -{
>> - uint8_t value = *(uint8_t *)opaque;
>> - visit_type_uint8(v, &value, name, errp);
>> -}
>> -
>> -static void property_get_uint16_ptr(Object *obj, Visitor *v,
>> - void *opaque, const char *name,
>> - Error **errp)
>> -{
>> - uint16_t value = *(uint16_t *)opaque;
>> - visit_type_uint16(v, &value, name, errp);
>> -}
>> -
>> -static void property_get_uint32_ptr(Object *obj, Visitor *v,
>> - void *opaque, const char *name,
>> - Error **errp)
>> -{
>> - uint32_t value = *(uint32_t *)opaque;
>> - visit_type_uint32(v, &value, name, errp);
>> -}
>> -
>> -static void property_get_uint64_ptr(Object *obj, Visitor *v,
>> - void *opaque, const char *name,
>> - Error **errp)
>> -{
>> - uint64_t value = *(uint64_t *)opaque;
>> - visit_type_uint64(v, &value, name, errp);
>> -}
>> -
>> -void object_property_add_uint8_ptr(Object *obj, const char *name,
>> - const uint8_t *v, Error **errp)
>> -{
>> - object_property_add(obj, name, "uint8", property_get_uint8_ptr,
>> - NULL, NULL, (void *)v, errp);
>> -}
>> -
>> -void object_property_add_uint16_ptr(Object *obj, const char *name,
>> - const uint16_t *v, Error **errp)
>> -{
>> - object_property_add(obj, name, "uint16", property_get_uint16_ptr,
>> - NULL, NULL, (void *)v, errp);
>> -}
>> -
>> -void object_property_add_uint32_ptr(Object *obj, const char *name,
>> - const uint32_t *v, Error **errp)
>> -{
>> - object_property_add(obj, name, "uint32", property_get_uint32_ptr,
>> - NULL, NULL, (void *)v, errp);
>> -}
>> -
>> -void object_property_add_uint64_ptr(Object *obj, const char *name,
>> - const uint64_t *v, Error **errp)
>> -{
>> - object_property_add(obj, name, "uint64", property_get_uint64_ptr,
>> - NULL, NULL, (void *)v, errp);
>> -}
>> +#define DECLARE_INTEGER_VISITOR(size) \
> macro needs a better name. DECLARE_PROP_SET_GET - something to make it
> clear its about properties.
>
>> + \
>> +static void glue(glue(property_get_,size),_ptr)(Object *obj, Visitor *v, \
>> + void *opaque, \
>> + const char *name, \
>> + Error **errp) \
>> +{ \
>> + glue(size,_t) value = *(glue(size,_t) *)opaque; \
>> + glue(visit_type_,size)(v, &value, name, errp); \
>> +} \
>> + \
>> +void glue(glue(object_property_add_,size),_ptr)(Object *obj, const char *name, \
>> + const glue(size,_t) *v, \
>> + Error **errp) \
>> +{ \
>> + ObjectPropertyAccessor *get = glue(glue(property_get_,size),_ptr); \
>> + object_property_add(obj, name, stringify(size), get, NULL, NULL, (void *)v,\
>> + errp); \
>> +} \
>> +
>> +DECLARE_INTEGER_VISITOR(uint8)
>> +DECLARE_INTEGER_VISITOR(uint16)
>> +DECLARE_INTEGER_VISITOR(uint32)
>> +DECLARE_INTEGER_VISITOR(uint64)
>>
> Worth getting bool working this way too? Theres been a few times now
> where I have wanted to object_property_add_bool_ptr. Perhaps:
>
> #define DECLARE_PROP_SET_GET(name, type)
>
> DECLARE_PROP_SET_GET(uint8, uint8_t)
> DECLARE_PROP_SET_GET(uint16, uint16_t)
> DECLARE_PROP_SET_GET(uint32, uint32_t)
> DECLARE_PROP_SET_GET(uint64, uint64_t)
> DECLARE_PROP_SET_GET(bool, bool)
>
> Can actually add the bool one later in follow-up patch but just
> thinking ahead to get the macro right for wider usage.
I think that makes sense. Let me change it :).
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 2/6] qom: Allow to make integer qom properties writeable
2014-07-01 21:49 [Qemu-devel] [PATCH 0/6] Dynamic sysbus device allocation support Alexander Graf
2014-07-01 21:49 ` [Qemu-devel] [PATCH 1/6] qom: macroify integer property helpers Alexander Graf
@ 2014-07-01 21:49 ` Alexander Graf
2014-07-02 3:48 ` Peter Crosthwaite
2014-07-01 21:49 ` [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints Alexander Graf
` (3 subsequent siblings)
5 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-01 21:49 UTC (permalink / raw)
To: qemu-ppc
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
sean.stalley, pbonzini, afaerber
We have helper functions to easily expose integers as QOM object properties.
However, these are read only. 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.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
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
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] qom: Allow to make integer qom properties writeable
2014-07-01 21:49 ` [Qemu-devel] [PATCH 2/6] qom: Allow to make integer qom properties writeable Alexander Graf
@ 2014-07-02 3:48 ` Peter Crosthwaite
2014-07-02 7:46 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Peter Crosthwaite @ 2014-07-02 3:48 UTC (permalink / raw)
To: Alexander Graf
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Paolo Bonzini,
Andreas Färber
On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> 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 <agraf@suse.de>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] qom: Allow to make integer qom properties writeable
2014-07-02 3:48 ` Peter Crosthwaite
@ 2014-07-02 7:46 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 7:46 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Paolo Bonzini,
Andreas Färber
On 02.07.14 05:48, Peter Crosthwaite wrote:
> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> 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 was trying to think of a nice middle ground between flexibility that's
needed and flexibility that's required. I think it's very common to have
* read-only properties
* read-write properties
* read-write properties with special handling for the write path
(checks mostly, boundary or permission)
> 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.
That'd be an alternative approach - I would just open codedly call
object_property_add with the global helpers then. I'm not sure that's
incredibly nicer though.
In the code flow you describe above, a read-only accessor would be
declared the same way it does today. Read-write ones would already have
to go via object_property_add.
So we either add clumsy code in the actual user files (which we try to
reduce) or we end up adding another set of _rw functions (which is what
I did at first - it's ugly) or we add an enum as parameter (which I did
then, but it's also ugly, see next email).
I'm not passionate about the approach I ended up taking, but I couldn't
think of a better middle ground. If you are passionate about it though I
can easily declare the getters public as well and just use
object_property_add() explicitly.
> 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).
Yeah, I can't really think of too many properties that really should
have read side effects ;).
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-01 21:49 [Qemu-devel] [PATCH 0/6] Dynamic sysbus device allocation support Alexander Graf
2014-07-01 21:49 ` [Qemu-devel] [PATCH 1/6] qom: macroify integer property helpers Alexander Graf
2014-07-01 21:49 ` [Qemu-devel] [PATCH 2/6] qom: Allow to make integer qom properties writeable Alexander Graf
@ 2014-07-01 21:49 ` Alexander Graf
2014-07-02 4:12 ` Peter Crosthwaite
2014-07-01 21:49 ` [Qemu-devel] [PATCH 4/6] sysbus: Make devices spawnable via -device Alexander Graf
` (2 subsequent siblings)
5 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-01 21:49 UTC (permalink / raw)
To: qemu-ppc
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
sean.stalley, pbonzini, afaerber
We want to give the user the ability to tell our machine file where he wants
to have devices mapped to. This patch adds code to create these hints
dynamically and expose them as object properties that can only be modified
before device realization.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
hw/core/sysbus.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++--
include/hw/sysbus.h | 6 +++++
2 files changed, 77 insertions(+), 2 deletions(-)
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index f4e760d..84cd0cf 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -86,6 +86,54 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
sysbus_mmio_map_common(dev, n, addr, true, priority);
}
+static void sysbus_property_set_uint32_ptr(Object *obj, Visitor *v,
+ void *opaque, const char *name,
+ Error **errp)
+{
+ DeviceState *dev = DEVICE(obj);
+
+ if (dev->realized) {
+ qdev_prop_set_after_realize(dev, name, errp);
+ return;
+ }
+
+ object_property_set_uint32_ptr(obj, v, opaque, name, errp);
+}
+
+static void sysbus_property_set_uint64_ptr(Object *obj, Visitor *v,
+ void *opaque, const char *name,
+ Error **errp)
+{
+ DeviceState *dev = DEVICE(obj);
+
+ if (dev->realized) {
+ qdev_prop_set_after_realize(dev, name, errp);
+ return;
+ }
+
+ object_property_set_uint64_ptr(obj, v, opaque, name, errp);
+}
+
+static void sysbus_init_prop(SysBusDevice *dev, const char *propstr, int n,
+ void *ptr, int size)
+{
+ char *name = g_strdup_printf(propstr, n);
+ Object *obj = OBJECT(dev);
+
+ switch (size) {
+ case sizeof(uint32_t):
+ object_property_add_uint32_ptr(obj, name, ptr,
+ sysbus_property_set_uint32_ptr, NULL);
+ break;
+ case sizeof(uint64_t):
+ object_property_add_uint64_ptr(obj, name, ptr,
+ sysbus_property_set_uint64_ptr, NULL);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+}
+
/* Request an IRQ source. The actual IRQ object may be populated later. */
void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
{
@@ -93,7 +141,13 @@ void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
assert(dev->num_irq < QDEV_MAX_IRQ);
n = dev->num_irq++;
+ dev->user_irqs = g_realloc(dev->user_irqs,
+ sizeof(*dev->user_irqs) * (n + 1));
+ dev->user_irqs[n] = (uint32_t)SYSBUS_DYNAMIC;
dev->irqp[n] = p;
+
+ sysbus_init_prop(dev, "irq[%d]", n, &dev->user_irqs[n],
+ sizeof(dev->user_irqs[n]));
}
/* Pass IRQs from a target device. */
@@ -103,7 +157,7 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
assert(dev->num_irq == 0);
dev->num_irq = target->num_irq;
for (i = 0; i < dev->num_irq; i++) {
- dev->irqp[i] = target->irqp[i];
+ sysbus_init_irq(dev, target->irqp[i]);
}
}
@@ -113,8 +167,14 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
assert(dev->num_mmio < QDEV_MAX_MMIO);
n = dev->num_mmio++;
+ dev->user_mmios = g_realloc(dev->user_mmios,
+ sizeof(*dev->user_mmios) * (n + 1));
+ dev->user_mmios[n] = SYSBUS_DYNAMIC;
dev->mmio[n].addr = -1;
dev->mmio[n].memory = memory;
+
+ sysbus_init_prop(dev, "mmio[%d]", n, &dev->user_mmios[n],
+ sizeof(dev->user_mmios[n]));
}
MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n)
@@ -127,8 +187,17 @@ void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size)
pio_addr_t i;
for (i = 0; i < size; i++) {
+ int n;
+
assert(dev->num_pio < QDEV_MAX_PIO);
- dev->pio[dev->num_pio++] = ioport++;
+ n = dev->num_pio++;
+ dev->user_pios = g_realloc(dev->user_pios,
+ sizeof(*dev->user_pios) * (n + 1));
+ dev->user_pios[n] = (uint32_t)SYSBUS_DYNAMIC;
+ dev->pio[n] = ioport++;
+
+ sysbus_init_prop(dev, "pio[%d]", n, &dev->user_pios[n],
+ sizeof(dev->user_pios[n]));
}
}
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index f5aaa05..870e7cc 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -9,6 +9,7 @@
#define QDEV_MAX_MMIO 32
#define QDEV_MAX_PIO 32
#define QDEV_MAX_IRQ 512
+#define SYSBUS_DYNAMIC -1ULL
#define TYPE_SYSTEM_BUS "System"
#define SYSTEM_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
@@ -56,6 +57,11 @@ struct SysBusDevice {
} mmio[QDEV_MAX_MMIO];
int num_pio;
pio_addr_t pio[QDEV_MAX_PIO];
+
+ /* These may be set by the user as hints where to map the device */
+ uint64_t *user_mmios;
+ uint32_t *user_irqs;
+ uint32_t *user_pios;
};
void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-01 21:49 ` [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints Alexander Graf
@ 2014-07-02 4:12 ` Peter Crosthwaite
2014-07-02 8:24 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Peter Crosthwaite @ 2014-07-02 4:12 UTC (permalink / raw)
To: Alexander Graf
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Paolo Bonzini,
Andreas Färber
On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> wrote:
> We want to give the user the ability to tell our machine file where he wants
> to have devices mapped to. This patch adds code to create these hints
> dynamically and expose them as object properties that can only be modified
> before device realization.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> hw/core/sysbus.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> include/hw/sysbus.h | 6 +++++
> 2 files changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index f4e760d..84cd0cf 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -86,6 +86,54 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
> sysbus_mmio_map_common(dev, n, addr, true, priority);
> }
>
> +static void sysbus_property_set_uint32_ptr(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + DeviceState *dev = DEVICE(obj);
> +
> + if (dev->realized) {
> + qdev_prop_set_after_realize(dev, name, errp);
> + return;
> + }
> +
So this suggests your reasoning for side effected _ptr write is just
for validity checking. So another approach could be to add a "check"
function to the _ptr variants (rather than an open coded) setter. This
has the advantage of being consistent with what we already do for
object_property_add_link.
> + object_property_set_uint32_ptr(obj, v, opaque, name, errp);
> +}
> +
> +static void sysbus_property_set_uint64_ptr(Object *obj, Visitor *v,
> + void *opaque, const char *name,
> + Error **errp)
> +{
> + DeviceState *dev = DEVICE(obj);
> +
> + if (dev->realized) {
> + qdev_prop_set_after_realize(dev, name, errp);
> + return;
> + }
> +
> + object_property_set_uint64_ptr(obj, v, opaque, name, errp);
> +}
> +
> +static void sysbus_init_prop(SysBusDevice *dev, const char *propstr, int n,
sysbus_init_int_prop.
> + void *ptr, int size)
> +{
> + char *name = g_strdup_printf(propstr, n);
> + Object *obj = OBJECT(dev);
> +
> + switch (size) {
> + case sizeof(uint32_t):
Is it easier to just go lowest common denom of 64-bit int props for
everything to avoid the sizeof stuffs?
> + object_property_add_uint32_ptr(obj, name, ptr,
> + sysbus_property_set_uint32_ptr, NULL);
> + break;
> + case sizeof(uint64_t):
> + object_property_add_uint64_ptr(obj, name, ptr,
> + sysbus_property_set_uint64_ptr, NULL);
> + break;
> + default:
> + g_assert_not_reached();
> + }
> +}
> +
> /* Request an IRQ source. The actual IRQ object may be populated later. */
> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
> {
> @@ -93,7 +141,13 @@ void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>
> assert(dev->num_irq < QDEV_MAX_IRQ);
> n = dev->num_irq++;
> + dev->user_irqs = g_realloc(dev->user_irqs,
> + sizeof(*dev->user_irqs) * (n + 1));
Will the QOM framework take references to this allocated area before
the final realloc? I had this problem with IRQs leading to this patch
to remove uses of realloc for QOM:
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00051.html
> + dev->user_irqs[n] = (uint32_t)SYSBUS_DYNAMIC;
> dev->irqp[n] = p;
> +
> + sysbus_init_prop(dev, "irq[%d]", n, &dev->user_irqs[n],
> + sizeof(dev->user_irqs[n]));
You pass a ref to reallocable area here.
Perhaps a cleaner solution is to just malloc a single uint64_t here
locally and pass it off to QOM. Then free the malloc in the property
finalizer ...
> }
>
> /* Pass IRQs from a target device. */
> @@ -103,7 +157,7 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
> assert(dev->num_irq == 0);
> dev->num_irq = target->num_irq;
sysbus_init_irq does num_irq incrementing itself so does this need to go?
> for (i = 0; i < dev->num_irq; i++) {
> - dev->irqp[i] = target->irqp[i];
> + sysbus_init_irq(dev, target->irqp[i]);
> }
> }
>
> @@ -113,8 +167,14 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>
> assert(dev->num_mmio < QDEV_MAX_MMIO);
> n = dev->num_mmio++;
> + dev->user_mmios = g_realloc(dev->user_mmios,
> + sizeof(*dev->user_mmios) * (n + 1));
> + dev->user_mmios[n] = SYSBUS_DYNAMIC;
> dev->mmio[n].addr = -1;
> dev->mmio[n].memory = memory;
> +
> + sysbus_init_prop(dev, "mmio[%d]", n, &dev->user_mmios[n],
> + sizeof(dev->user_mmios[n]));
You might be able to drop the %d once Paolo wildcard array property
adder stuff gets through.
> }
>
> MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n)
> @@ -127,8 +187,17 @@ void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size)
> pio_addr_t i;
>
> for (i = 0; i < size; i++) {
> + int n;
> +
> assert(dev->num_pio < QDEV_MAX_PIO);
> - dev->pio[dev->num_pio++] = ioport++;
> + n = dev->num_pio++;
> + dev->user_pios = g_realloc(dev->user_pios,
> + sizeof(*dev->user_pios) * (n + 1));
> + dev->user_pios[n] = (uint32_t)SYSBUS_DYNAMIC;
> + dev->pio[n] = ioport++;
> +
> + sysbus_init_prop(dev, "pio[%d]", n, &dev->user_pios[n],
> + sizeof(dev->user_pios[n]));
> }
> }
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index f5aaa05..870e7cc 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -9,6 +9,7 @@
> #define QDEV_MAX_MMIO 32
> #define QDEV_MAX_PIO 32
> #define QDEV_MAX_IRQ 512
> +#define SYSBUS_DYNAMIC -1ULL
>
> #define TYPE_SYSTEM_BUS "System"
> #define SYSTEM_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
> @@ -56,6 +57,11 @@ struct SysBusDevice {
> } mmio[QDEV_MAX_MMIO];
> int num_pio;
> pio_addr_t pio[QDEV_MAX_PIO];
> +
> + /* These may be set by the user as hints where to map the device */
> + uint64_t *user_mmios;
> + uint32_t *user_irqs;
> + uint32_t *user_pios;
With the single malloc/free-on-finalise approach to the properties
there's no longer a need for this new state at all.
Regards,
Peter
> };
>
> void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
> --
> 1.8.1.4
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-02 4:12 ` Peter Crosthwaite
@ 2014-07-02 8:24 ` Alexander Graf
2014-07-02 8:26 ` Paolo Bonzini
2014-07-02 9:03 ` Peter Crosthwaite
0 siblings, 2 replies; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 8:24 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Paolo Bonzini,
Andreas Färber
On 02.07.14 06:12, Peter Crosthwaite wrote:
> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> wrote:
>> We want to give the user the ability to tell our machine file where he wants
>> to have devices mapped to. This patch adds code to create these hints
>> dynamically and expose them as object properties that can only be modified
>> before device realization.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/core/sysbus.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/sysbus.h | 6 +++++
>> 2 files changed, 77 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index f4e760d..84cd0cf 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -86,6 +86,54 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
>> sysbus_mmio_map_common(dev, n, addr, true, priority);
>> }
>>
>> +static void sysbus_property_set_uint32_ptr(Object *obj, Visitor *v,
>> + void *opaque, const char *name,
>> + Error **errp)
>> +{
>> + DeviceState *dev = DEVICE(obj);
>> +
>> + if (dev->realized) {
>> + qdev_prop_set_after_realize(dev, name, errp);
>> + return;
>> + }
>> +
> So this suggests your reasoning for side effected _ptr write is just
> for validity checking. So another approach could be to add a "check"
> function to the _ptr variants (rather than an open coded) setter. This
> has the advantage of being consistent with what we already do for
> object_property_add_link.
Heh, yes. Unfortunately "realized" is a field in DeviceStruct which we
don't have access to from object.c.
In fact, this is exactly what I wanted to do before this approach. I
introduced an enum that was either
* read-only
* read-write
* read-write-before-realize
and wanted to do all the checking in object.c.
But then I realized that object.c really shouldn't be aware of
DeviceState and threw away the idea.
>
>> + object_property_set_uint32_ptr(obj, v, opaque, name, errp);
>> +}
>> +
>> +static void sysbus_property_set_uint64_ptr(Object *obj, Visitor *v,
>> + void *opaque, const char *name,
>> + Error **errp)
>> +{
>> + DeviceState *dev = DEVICE(obj);
>> +
>> + if (dev->realized) {
>> + qdev_prop_set_after_realize(dev, name, errp);
>> + return;
>> + }
>> +
>> + object_property_set_uint64_ptr(obj, v, opaque, name, errp);
>> +}
>> +
>> +static void sysbus_init_prop(SysBusDevice *dev, const char *propstr, int n,
> sysbus_init_int_prop.
Ok.
>
>> + void *ptr, int size)
>> +{
>> + char *name = g_strdup_printf(propstr, n);
>> + Object *obj = OBJECT(dev);
>> +
>> + switch (size) {
>> + case sizeof(uint32_t):
> Is it easier to just go lowest common denom of 64-bit int props for
> everything to avoid the sizeof stuffs?
Hrm, interesting idea. Let me give it a shot :).
>
>> + object_property_add_uint32_ptr(obj, name, ptr,
>> + sysbus_property_set_uint32_ptr, NULL);
>> + break;
>> + case sizeof(uint64_t):
>> + object_property_add_uint64_ptr(obj, name, ptr,
>> + sysbus_property_set_uint64_ptr, NULL);
>> + break;
>> + default:
>> + g_assert_not_reached();
>> + }
>> +}
>> +
>> /* Request an IRQ source. The actual IRQ object may be populated later. */
>> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>> {
>> @@ -93,7 +141,13 @@ void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>>
>> assert(dev->num_irq < QDEV_MAX_IRQ);
>> n = dev->num_irq++;
>> + dev->user_irqs = g_realloc(dev->user_irqs,
>> + sizeof(*dev->user_irqs) * (n + 1));
> Will the QOM framework take references to this allocated area before
> the final realloc? I had this problem with IRQs leading to this patch
> to remove uses of realloc for QOM:
>
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00051.html
>
>> + dev->user_irqs[n] = (uint32_t)SYSBUS_DYNAMIC;
>> dev->irqp[n] = p;
>> +
>> + sysbus_init_prop(dev, "irq[%d]", n, &dev->user_irqs[n],
>> + sizeof(dev->user_irqs[n]));
> You pass a ref to reallocable area here.
>
> Perhaps a cleaner solution is to just malloc a single uint64_t here
> locally and pass it off to QOM. Then free the malloc in the property
> finalizer ...
Heh, you can always add another level of abstraction ;).
>
>> }
>>
>> /* Pass IRQs from a target device. */
>> @@ -103,7 +157,7 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
>> assert(dev->num_irq == 0);
>> dev->num_irq = target->num_irq;
> sysbus_init_irq does num_irq incrementing itself so does this need to go?
Yikes - must have sneaked back in on patch reshuffling. Yes, of course.
>
>> for (i = 0; i < dev->num_irq; i++) {
>> - dev->irqp[i] = target->irqp[i];
>> + sysbus_init_irq(dev, target->irqp[i]);
>> }
>> }
>>
>> @@ -113,8 +167,14 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>>
>> assert(dev->num_mmio < QDEV_MAX_MMIO);
>> n = dev->num_mmio++;
>> + dev->user_mmios = g_realloc(dev->user_mmios,
>> + sizeof(*dev->user_mmios) * (n + 1));
>> + dev->user_mmios[n] = SYSBUS_DYNAMIC;
>> dev->mmio[n].addr = -1;
>> dev->mmio[n].memory = memory;
>> +
>> + sysbus_init_prop(dev, "mmio[%d]", n, &dev->user_mmios[n],
>> + sizeof(dev->user_mmios[n]));
> You might be able to drop the %d once Paolo wildcard array property
> adder stuff gets through.
>
>> }
>>
>> MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n)
>> @@ -127,8 +187,17 @@ void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size)
>> pio_addr_t i;
>>
>> for (i = 0; i < size; i++) {
>> + int n;
>> +
>> assert(dev->num_pio < QDEV_MAX_PIO);
>> - dev->pio[dev->num_pio++] = ioport++;
>> + n = dev->num_pio++;
>> + dev->user_pios = g_realloc(dev->user_pios,
>> + sizeof(*dev->user_pios) * (n + 1));
>> + dev->user_pios[n] = (uint32_t)SYSBUS_DYNAMIC;
>> + dev->pio[n] = ioport++;
>> +
>> + sysbus_init_prop(dev, "pio[%d]", n, &dev->user_pios[n],
>> + sizeof(dev->user_pios[n]));
>> }
>> }
>>
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index f5aaa05..870e7cc 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -9,6 +9,7 @@
>> #define QDEV_MAX_MMIO 32
>> #define QDEV_MAX_PIO 32
>> #define QDEV_MAX_IRQ 512
>> +#define SYSBUS_DYNAMIC -1ULL
>>
>> #define TYPE_SYSTEM_BUS "System"
>> #define SYSTEM_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
>> @@ -56,6 +57,11 @@ struct SysBusDevice {
>> } mmio[QDEV_MAX_MMIO];
>> int num_pio;
>> pio_addr_t pio[QDEV_MAX_PIO];
>> +
>> + /* These may be set by the user as hints where to map the device */
>> + uint64_t *user_mmios;
>> + uint32_t *user_irqs;
>> + uint32_t *user_pios;
> With the single malloc/free-on-finalise approach to the properties
> there's no longer a need for this new state at all.
I'll need to keep a reference to the pointers in here so that I can
still write to them one last time after realize from the machine file to
make sure I convert "dynamic" properties to their respective numbers.
Or do you think it'd be better to make the setter check whether the
property is SYSBUS_DYNAMIC and only allow writes if it is? Then I can
just always use the QOM setter functions.
That still leaves the question on how the finalize function would know
which variables to free when I don't have any reference at all in my
state :).
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-02 8:24 ` Alexander Graf
@ 2014-07-02 8:26 ` Paolo Bonzini
2014-07-02 9:03 ` Peter Crosthwaite
1 sibling, 0 replies; 33+ messages in thread
From: Paolo Bonzini @ 2014-07-02 8:26 UTC (permalink / raw)
To: Alexander Graf, Peter Crosthwaite
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Andreas Färber
Il 02/07/2014 10:24, Alexander Graf ha scritto:
>>>
>> So this suggests your reasoning for side effected _ptr write is just
>> for validity checking. So another approach could be to add a "check"
>> function to the _ptr variants (rather than an open coded) setter. This
>> has the advantage of being consistent with what we already do for
>> object_property_add_link.
>
> Heh, yes. Unfortunately "realized" is a field in DeviceStruct which we
> don't have access to from object.c.
>
> In fact, this is exactly what I wanted to do before this approach. I
> introduced an enum that was either
>
> * read-only
> * read-write
> * read-write-before-realize
>
> and wanted to do all the checking in object.c.
>
> But then I realized that object.c really shouldn't be aware of
> DeviceState and threw away the idea.
I think your solution is pretty. However, it's probably time to split
qom/object.c and/or include/qom/object.h.
Paolo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-02 8:24 ` Alexander Graf
2014-07-02 8:26 ` Paolo Bonzini
@ 2014-07-02 9:03 ` Peter Crosthwaite
2014-07-02 9:07 ` Alexander Graf
1 sibling, 1 reply; 33+ messages in thread
From: Peter Crosthwaite @ 2014-07-02 9:03 UTC (permalink / raw)
To: Alexander Graf
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Paolo Bonzini,
Andreas Färber
On Wed, Jul 2, 2014 at 6:24 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 02.07.14 06:12, Peter Crosthwaite wrote:
>>
>> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> We want to give the user the ability to tell our machine file where he
>>> wants
>>> to have devices mapped to. This patch adds code to create these hints
>>> dynamically and expose them as object properties that can only be
>>> modified
>>> before device realization.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> hw/core/sysbus.c | 73
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>> include/hw/sysbus.h | 6 +++++
>>> 2 files changed, 77 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>>> index f4e760d..84cd0cf 100644
>>> --- a/hw/core/sysbus.c
>>> +++ b/hw/core/sysbus.c
>>> @@ -86,6 +86,54 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n,
>>> hwaddr addr,
>>> sysbus_mmio_map_common(dev, n, addr, true, priority);
>>> }
>>>
>>> +static void sysbus_property_set_uint32_ptr(Object *obj, Visitor *v,
>>> + void *opaque, const char
>>> *name,
>>> + Error **errp)
>>> +{
>>> + DeviceState *dev = DEVICE(obj);
>>> +
>>> + if (dev->realized) {
>>> + qdev_prop_set_after_realize(dev, name, errp);
>>> + return;
>>> + }
>>> +
>>
>> So this suggests your reasoning for side effected _ptr write is just
>> for validity checking. So another approach could be to add a "check"
>> function to the _ptr variants (rather than an open coded) setter. This
>> has the advantage of being consistent with what we already do for
>> object_property_add_link.
>
>
> Heh, yes. Unfortunately "realized" is a field in DeviceStruct which we don't
> have access to from object.c.
>
> In fact, this is exactly what I wanted to do before this approach. I
> introduced an enum that was either
>
> * read-only
> * read-write
> * read-write-before-realize
>
> and wanted to do all the checking in object.c.
>
> But then I realized that object.c really shouldn't be aware of DeviceState
> and threw away the idea.
>
So the way this is handled for links is its an open coded check
function added by the property adder. Check
qdev_prop_allow_set_link_before_realize() for a precedent.
>
>>
>>> + object_property_set_uint32_ptr(obj, v, opaque, name, errp);
>>> +}
>>> +
>>> +static void sysbus_property_set_uint64_ptr(Object *obj, Visitor *v,
>>> + void *opaque, const char
>>> *name,
>>> + Error **errp)
>>> +{
>>> + DeviceState *dev = DEVICE(obj);
>>> +
>>> + if (dev->realized) {
>>> + qdev_prop_set_after_realize(dev, name, errp);
>>> + return;
>>> + }
>>> +
>>> + object_property_set_uint64_ptr(obj, v, opaque, name, errp);
>>> +}
>>> +
>>> +static void sysbus_init_prop(SysBusDevice *dev, const char *propstr, int
>>> n,
>>
>> sysbus_init_int_prop.
>
>
> Ok.
>
>
>>
>>> + void *ptr, int size)
>>> +{
>>> + char *name = g_strdup_printf(propstr, n);
>>> + Object *obj = OBJECT(dev);
>>> +
>>> + switch (size) {
>>> + case sizeof(uint32_t):
>>
>> Is it easier to just go lowest common denom of 64-bit int props for
>> everything to avoid the sizeof stuffs?
>
>
> Hrm, interesting idea. Let me give it a shot :).
>
>
>>
>>> + object_property_add_uint32_ptr(obj, name, ptr,
>>> + sysbus_property_set_uint32_ptr,
>>> NULL);
>>> + break;
>>> + case sizeof(uint64_t):
>>> + object_property_add_uint64_ptr(obj, name, ptr,
>>> + sysbus_property_set_uint64_ptr,
>>> NULL);
>>> + break;
>>> + default:
>>> + g_assert_not_reached();
>>> + }
>>> +}
>>> +
>>> /* Request an IRQ source. The actual IRQ object may be populated
>>> later. */
>>> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>>> {
>>> @@ -93,7 +141,13 @@ void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>>>
>>> assert(dev->num_irq < QDEV_MAX_IRQ);
>>> n = dev->num_irq++;
>>> + dev->user_irqs = g_realloc(dev->user_irqs,
>>> + sizeof(*dev->user_irqs) * (n + 1));
>>
>> Will the QOM framework take references to this allocated area before
>> the final realloc? I had this problem with IRQs leading to this patch
>> to remove uses of realloc for QOM:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00051.html
>>
>>> + dev->user_irqs[n] = (uint32_t)SYSBUS_DYNAMIC;
>>> dev->irqp[n] = p;
>>> +
>>> + sysbus_init_prop(dev, "irq[%d]", n, &dev->user_irqs[n],
>>> + sizeof(dev->user_irqs[n]));
>>
>> You pass a ref to reallocable area here.
>>
>> Perhaps a cleaner solution is to just malloc a single uint64_t here
>> locally and pass it off to QOM. Then free the malloc in the property
>> finalizer ...
>
>
> Heh, you can always add another level of abstraction ;).
>
I'm not following? Do you mean another level of indirection?
>
>>
>>> }
>>>
>>> /* Pass IRQs from a target device. */
>>> @@ -103,7 +157,7 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice
>>> *target)
>>> assert(dev->num_irq == 0);
>>> dev->num_irq = target->num_irq;
>>
>> sysbus_init_irq does num_irq incrementing itself so does this need to go?
>
>
> Yikes - must have sneaked back in on patch reshuffling. Yes, of course.
>
>
>>
>>> for (i = 0; i < dev->num_irq; i++) {
>>> - dev->irqp[i] = target->irqp[i];
>>> + sysbus_init_irq(dev, target->irqp[i]);
>>> }
>>> }
>>>
>>> @@ -113,8 +167,14 @@ void sysbus_init_mmio(SysBusDevice *dev,
>>> MemoryRegion *memory)
>>>
>>> assert(dev->num_mmio < QDEV_MAX_MMIO);
>>> n = dev->num_mmio++;
>>> + dev->user_mmios = g_realloc(dev->user_mmios,
>>> + sizeof(*dev->user_mmios) * (n + 1));
>>> + dev->user_mmios[n] = SYSBUS_DYNAMIC;
>>> dev->mmio[n].addr = -1;
>>> dev->mmio[n].memory = memory;
>>> +
>>> + sysbus_init_prop(dev, "mmio[%d]", n, &dev->user_mmios[n],
>>> + sizeof(dev->user_mmios[n]));
>>
>> You might be able to drop the %d once Paolo wildcard array property
>> adder stuff gets through.
>>
>>> }
>>>
>>> MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n)
>>> @@ -127,8 +187,17 @@ void sysbus_init_ioports(SysBusDevice *dev,
>>> pio_addr_t ioport, pio_addr_t size)
>>> pio_addr_t i;
>>>
>>> for (i = 0; i < size; i++) {
>>> + int n;
>>> +
>>> assert(dev->num_pio < QDEV_MAX_PIO);
>>> - dev->pio[dev->num_pio++] = ioport++;
>>> + n = dev->num_pio++;
>>> + dev->user_pios = g_realloc(dev->user_pios,
>>> + sizeof(*dev->user_pios) * (n + 1));
>>> + dev->user_pios[n] = (uint32_t)SYSBUS_DYNAMIC;
>>> + dev->pio[n] = ioport++;
>>> +
>>> + sysbus_init_prop(dev, "pio[%d]", n, &dev->user_pios[n],
>>> + sizeof(dev->user_pios[n]));
>>> }
>>> }
>>>
>>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>>> index f5aaa05..870e7cc 100644
>>> --- a/include/hw/sysbus.h
>>> +++ b/include/hw/sysbus.h
>>> @@ -9,6 +9,7 @@
>>> #define QDEV_MAX_MMIO 32
>>> #define QDEV_MAX_PIO 32
>>> #define QDEV_MAX_IRQ 512
>>> +#define SYSBUS_DYNAMIC -1ULL
>>>
>>> #define TYPE_SYSTEM_BUS "System"
>>> #define SYSTEM_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
>>> @@ -56,6 +57,11 @@ struct SysBusDevice {
>>> } mmio[QDEV_MAX_MMIO];
>>> int num_pio;
>>> pio_addr_t pio[QDEV_MAX_PIO];
>>> +
>>> + /* These may be set by the user as hints where to map the device */
>>> + uint64_t *user_mmios;
>>> + uint32_t *user_irqs;
>>> + uint32_t *user_pios;
>>
>> With the single malloc/free-on-finalise approach to the properties
>> there's no longer a need for this new state at all.
>
>
> I'll need to keep a reference to the pointers in here so that I can still
> write to them one last time after realize from the machine file to make sure
> I convert "dynamic" properties to their respective numbers.
>
> Or do you think it'd be better to make the setter
Or a sysbus-specific check fn
> check whether the property
> is SYSBUS_DYNAMIC and only allow writes if it is? Then I can just always use
> the QOM setter functions.
>
Yep. You can use the setters on your own object in absence of local ptr.
> That still leaves the question on how the finalize function
It's not the isntance finalise that would do the free-ing, it's the
individual property finalizers.To implement you could add a
"free-the-ptr" option to object_property_add_*_ptr that installs the
relevant finalizer or you can fall back to full open coded properties
and set the property finalize callback.
would know which
> variables to free when I don't have any reference at all in my state :).
>
Regards,
Peter
>
> Alex
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-02 9:03 ` Peter Crosthwaite
@ 2014-07-02 9:07 ` Alexander Graf
2014-07-02 9:17 ` Paolo Bonzini
0 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 9:07 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Paolo Bonzini,
Andreas Färber
On 02.07.14 11:03, Peter Crosthwaite wrote:
> On Wed, Jul 2, 2014 at 6:24 PM, Alexander Graf <agraf@suse.de> wrote:
>> On 02.07.14 06:12, Peter Crosthwaite wrote:
>>> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> wrote:
>>>> We want to give the user the ability to tell our machine file where he
>>>> wants
>>>> to have devices mapped to. This patch adds code to create these hints
>>>> dynamically and expose them as object properties that can only be
>>>> modified
>>>> before device realization.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> hw/core/sysbus.c | 73
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>> include/hw/sysbus.h | 6 +++++
>>>> 2 files changed, 77 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>>>> index f4e760d..84cd0cf 100644
>>>> --- a/hw/core/sysbus.c
>>>> +++ b/hw/core/sysbus.c
>>>> @@ -86,6 +86,54 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n,
>>>> hwaddr addr,
>>>> sysbus_mmio_map_common(dev, n, addr, true, priority);
>>>> }
>>>>
>>>> +static void sysbus_property_set_uint32_ptr(Object *obj, Visitor *v,
>>>> + void *opaque, const char
>>>> *name,
>>>> + Error **errp)
>>>> +{
>>>> + DeviceState *dev = DEVICE(obj);
>>>> +
>>>> + if (dev->realized) {
>>>> + qdev_prop_set_after_realize(dev, name, errp);
>>>> + return;
>>>> + }
>>>> +
>>> So this suggests your reasoning for side effected _ptr write is just
>>> for validity checking. So another approach could be to add a "check"
>>> function to the _ptr variants (rather than an open coded) setter. This
>>> has the advantage of being consistent with what we already do for
>>> object_property_add_link.
>>
>> Heh, yes. Unfortunately "realized" is a field in DeviceStruct which we don't
>> have access to from object.c.
>>
>> In fact, this is exactly what I wanted to do before this approach. I
>> introduced an enum that was either
>>
>> * read-only
>> * read-write
>> * read-write-before-realize
>>
>> and wanted to do all the checking in object.c.
>>
>> But then I realized that object.c really shouldn't be aware of DeviceState
>> and threw away the idea.
>>
> So the way this is handled for links is its an open coded check
> function added by the property adder. Check
> qdev_prop_allow_set_link_before_realize() for a precedent.
Yeah, I realized that this is what you meant only after I sent the mail
:). Considering that this a deeply philosophical question and Paolo
likes the wrapper approach better, I don't think I'll really touch this
though.
Instead, I'll export all the simple integer get/set helpers to the world
and use object_property_add directly. That way I can also hook in my
release function that I need with this approach.
>
>>>> + object_property_set_uint32_ptr(obj, v, opaque, name, errp);
>>>> +}
>>>> +
>>>> +static void sysbus_property_set_uint64_ptr(Object *obj, Visitor *v,
>>>> + void *opaque, const char
>>>> *name,
>>>> + Error **errp)
>>>> +{
>>>> + DeviceState *dev = DEVICE(obj);
>>>> +
>>>> + if (dev->realized) {
>>>> + qdev_prop_set_after_realize(dev, name, errp);
>>>> + return;
>>>> + }
>>>> +
>>>> + object_property_set_uint64_ptr(obj, v, opaque, name, errp);
>>>> +}
>>>> +
>>>> +static void sysbus_init_prop(SysBusDevice *dev, const char *propstr, int
>>>> n,
>>> sysbus_init_int_prop.
>>
>> Ok.
>>
>>
>>>> + void *ptr, int size)
>>>> +{
>>>> + char *name = g_strdup_printf(propstr, n);
>>>> + Object *obj = OBJECT(dev);
>>>> +
>>>> + switch (size) {
>>>> + case sizeof(uint32_t):
>>> Is it easier to just go lowest common denom of 64-bit int props for
>>> everything to avoid the sizeof stuffs?
>>
>> Hrm, interesting idea. Let me give it a shot :).
>>
>>
>>>> + object_property_add_uint32_ptr(obj, name, ptr,
>>>> + sysbus_property_set_uint32_ptr,
>>>> NULL);
>>>> + break;
>>>> + case sizeof(uint64_t):
>>>> + object_property_add_uint64_ptr(obj, name, ptr,
>>>> + sysbus_property_set_uint64_ptr,
>>>> NULL);
>>>> + break;
>>>> + default:
>>>> + g_assert_not_reached();
>>>> + }
>>>> +}
>>>> +
>>>> /* Request an IRQ source. The actual IRQ object may be populated
>>>> later. */
>>>> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>>>> {
>>>> @@ -93,7 +141,13 @@ void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>>>>
>>>> assert(dev->num_irq < QDEV_MAX_IRQ);
>>>> n = dev->num_irq++;
>>>> + dev->user_irqs = g_realloc(dev->user_irqs,
>>>> + sizeof(*dev->user_irqs) * (n + 1));
>>> Will the QOM framework take references to this allocated area before
>>> the final realloc? I had this problem with IRQs leading to this patch
>>> to remove uses of realloc for QOM:
>>>
>>> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00051.html
>>>
>>>> + dev->user_irqs[n] = (uint32_t)SYSBUS_DYNAMIC;
>>>> dev->irqp[n] = p;
>>>> +
>>>> + sysbus_init_prop(dev, "irq[%d]", n, &dev->user_irqs[n],
>>>> + sizeof(dev->user_irqs[n]));
>>> You pass a ref to reallocable area here.
>>>
>>> Perhaps a cleaner solution is to just malloc a single uint64_t here
>>> locally and pass it off to QOM. Then free the malloc in the property
>>> finalizer ...
>>
>> Heh, you can always add another level of abstraction ;).
>>
> I'm not following? Do you mean another level of indirection?
Same same :).
>
>>>> }
>>>>
>>>> /* Pass IRQs from a target device. */
>>>> @@ -103,7 +157,7 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice
>>>> *target)
>>>> assert(dev->num_irq == 0);
>>>> dev->num_irq = target->num_irq;
>>> sysbus_init_irq does num_irq incrementing itself so does this need to go?
>>
>> Yikes - must have sneaked back in on patch reshuffling. Yes, of course.
>>
>>
>>>> for (i = 0; i < dev->num_irq; i++) {
>>>> - dev->irqp[i] = target->irqp[i];
>>>> + sysbus_init_irq(dev, target->irqp[i]);
>>>> }
>>>> }
>>>>
>>>> @@ -113,8 +167,14 @@ void sysbus_init_mmio(SysBusDevice *dev,
>>>> MemoryRegion *memory)
>>>>
>>>> assert(dev->num_mmio < QDEV_MAX_MMIO);
>>>> n = dev->num_mmio++;
>>>> + dev->user_mmios = g_realloc(dev->user_mmios,
>>>> + sizeof(*dev->user_mmios) * (n + 1));
>>>> + dev->user_mmios[n] = SYSBUS_DYNAMIC;
>>>> dev->mmio[n].addr = -1;
>>>> dev->mmio[n].memory = memory;
>>>> +
>>>> + sysbus_init_prop(dev, "mmio[%d]", n, &dev->user_mmios[n],
>>>> + sizeof(dev->user_mmios[n]));
>>> You might be able to drop the %d once Paolo wildcard array property
>>> adder stuff gets through.
>>>
>>>> }
>>>>
>>>> MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n)
>>>> @@ -127,8 +187,17 @@ void sysbus_init_ioports(SysBusDevice *dev,
>>>> pio_addr_t ioport, pio_addr_t size)
>>>> pio_addr_t i;
>>>>
>>>> for (i = 0; i < size; i++) {
>>>> + int n;
>>>> +
>>>> assert(dev->num_pio < QDEV_MAX_PIO);
>>>> - dev->pio[dev->num_pio++] = ioport++;
>>>> + n = dev->num_pio++;
>>>> + dev->user_pios = g_realloc(dev->user_pios,
>>>> + sizeof(*dev->user_pios) * (n + 1));
>>>> + dev->user_pios[n] = (uint32_t)SYSBUS_DYNAMIC;
>>>> + dev->pio[n] = ioport++;
>>>> +
>>>> + sysbus_init_prop(dev, "pio[%d]", n, &dev->user_pios[n],
>>>> + sizeof(dev->user_pios[n]));
>>>> }
>>>> }
>>>>
>>>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>>>> index f5aaa05..870e7cc 100644
>>>> --- a/include/hw/sysbus.h
>>>> +++ b/include/hw/sysbus.h
>>>> @@ -9,6 +9,7 @@
>>>> #define QDEV_MAX_MMIO 32
>>>> #define QDEV_MAX_PIO 32
>>>> #define QDEV_MAX_IRQ 512
>>>> +#define SYSBUS_DYNAMIC -1ULL
>>>>
>>>> #define TYPE_SYSTEM_BUS "System"
>>>> #define SYSTEM_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
>>>> @@ -56,6 +57,11 @@ struct SysBusDevice {
>>>> } mmio[QDEV_MAX_MMIO];
>>>> int num_pio;
>>>> pio_addr_t pio[QDEV_MAX_PIO];
>>>> +
>>>> + /* These may be set by the user as hints where to map the device */
>>>> + uint64_t *user_mmios;
>>>> + uint32_t *user_irqs;
>>>> + uint32_t *user_pios;
>>> With the single malloc/free-on-finalise approach to the properties
>>> there's no longer a need for this new state at all.
>>
>> I'll need to keep a reference to the pointers in here so that I can still
>> write to them one last time after realize from the machine file to make sure
>> I convert "dynamic" properties to their respective numbers.
>>
>> Or do you think it'd be better to make the setter
> Or a sysbus-specific check fn
>
>> check whether the property
>> is SYSBUS_DYNAMIC and only allow writes if it is? Then I can just always use
>> the QOM setter functions.
>>
> Yep. You can use the setters on your own object in absence of local ptr.
>
>> That still leaves the question on how the finalize function
> It's not the isntance finalise that would do the free-ing, it's the
> individual property finalizers.To implement you could add a
> "free-the-ptr" option to object_property_add_*_ptr that installs the
> relevant finalizer or you can fall back to full open coded properties
> and set the property finalize callback.
-ETOOMANYOPTIONS. I'll stick with object_property_add and export a
generic g_free release helper instead :).
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-02 9:07 ` Alexander Graf
@ 2014-07-02 9:17 ` Paolo Bonzini
2014-07-02 9:19 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Paolo Bonzini @ 2014-07-02 9:17 UTC (permalink / raw)
To: Alexander Graf, Peter Crosthwaite
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Andreas Färber
Il 02/07/2014 11:07, Alexander Graf ha scritto:
>> So the way this is handled for links is its an open coded check
>> function added by the property adder. Check
>> qdev_prop_allow_set_link_before_realize() for a precedent.
However, unlike Alex's case the link setter is complicated and a simple
tail call won't do. It first computes the "val" argument that is passed
to the check function, then calls the check function, then does the
actual set.
Memory hotplug is using "val", so we cannot simply change the check
function's signature in such a way that we would use a tail call.
> I'll export all the simple integer get/set helpers to the world
> and use object_property_add directly. That way I can also hook in my
> release function that I need with this approach.
Good idea. But please make a new header file.
Paolo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-02 9:17 ` Paolo Bonzini
@ 2014-07-02 9:19 ` Alexander Graf
2014-07-02 9:26 ` Paolo Bonzini
0 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 9:19 UTC (permalink / raw)
To: Paolo Bonzini, Peter Crosthwaite
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Andreas Färber
On 02.07.14 11:17, Paolo Bonzini wrote:
> Il 02/07/2014 11:07, Alexander Graf ha scritto:
>>> So the way this is handled for links is its an open coded check
>>> function added by the property adder. Check
>>> qdev_prop_allow_set_link_before_realize() for a precedent.
>
> However, unlike Alex's case the link setter is complicated and a
> simple tail call won't do. It first computes the "val" argument that
> is passed to the check function, then calls the check function, then
> does the actual set.
>
> Memory hotplug is using "val", so we cannot simply change the check
> function's signature in such a way that we would use a tail call.
>
>> I'll export all the simple integer get/set helpers to the world
>> and use object_property_add directly. That way I can also hook in my
>> release function that I need with this approach.
>
> Good idea. But please make a new header file.
New header file, but same .c file? Why?
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints
2014-07-02 9:19 ` Alexander Graf
@ 2014-07-02 9:26 ` Paolo Bonzini
0 siblings, 0 replies; 33+ messages in thread
From: Paolo Bonzini @ 2014-07-02 9:26 UTC (permalink / raw)
To: Alexander Graf, Peter Crosthwaite
Cc: Peter Maydell, Eric Auger, qemu-devel@nongnu.org Developers,
qemu-ppc Mailing List, Stalley, Sean, Andreas Färber
Il 02/07/2014 11:19, Alexander Graf ha scritto:
>
> On 02.07.14 11:17, Paolo Bonzini wrote:
>> Il 02/07/2014 11:07, Alexander Graf ha scritto:
>>>> So the way this is handled for links is its an open coded check
>>>> function added by the property adder. Check
>>>> qdev_prop_allow_set_link_before_realize() for a precedent.
>>
>> However, unlike Alex's case the link setter is complicated and a
>> simple tail call won't do. It first computes the "val" argument that
>> is passed to the check function, then calls the check function, then
>> does the actual set.
>>
>> Memory hotplug is using "val", so we cannot simply change the check
>> function's signature in such a way that we would use a tail call.
>>
>>> I'll export all the simple integer get/set helpers to the world
>>> and use object_property_add directly. That way I can also hook in my
>>> release function that I need with this approach.
>>
>> Good idea. But please make a new header file.
>
> New header file, but same .c file? Why?
I would all but complain about different .c file. :)
Paolo
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 4/6] sysbus: Make devices spawnable via -device
2014-07-01 21:49 [Qemu-devel] [PATCH 0/6] Dynamic sysbus device allocation support Alexander Graf
` (2 preceding siblings ...)
2014-07-01 21:49 ` [Qemu-devel] [PATCH 3/6] sysbus: Add user map hints Alexander Graf
@ 2014-07-01 21:49 ` Alexander Graf
2014-07-02 6:32 ` Paolo Bonzini
2014-07-01 21:49 ` [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices Alexander Graf
2014-07-01 21:49 ` [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree Alexander Graf
5 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-01 21:49 UTC (permalink / raw)
To: qemu-ppc
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
sean.stalley, pbonzini, afaerber
Now that we can properly map sysbus devices that haven't been connected to
something forcefully by C code, we can allow the -device command line option
to spawn them.
For machines that don't implement dynamic sysbus assignment in their board
files we add a new property "has-dynamic-sysbus" to the machine object.
Without that property available, we bail out when we see dynamically spawned
sysbus devices, like we did before.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
hw/core/machine.c | 41 +++++++++++++++++++++++++++++++++++++++++
hw/core/sysbus.c | 7 -------
include/hw/boards.h | 2 ++
3 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index cbba679..10f0bb1 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -12,6 +12,9 @@
#include "hw/boards.h"
#include "qapi/visitor.h"
+#include "hw/sysbus.h"
+#include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
static char *machine_get_accel(Object *obj, Error **errp)
{
@@ -235,8 +238,42 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
ms->firmware = g_strdup(value);
}
+static int search_for_sysbus_device(Object *obj, void *opaque)
+{
+ if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
+ /* Container or different device, traverse it for children */
+ return object_child_foreach(obj, search_for_sysbus_device, opaque);
+ }
+
+ error_report("Device '%s' can not be handled by this machine",
+ qdev_fw_name(DEVICE(obj)));
+ exit(1);
+}
+
+static void machine_init_notify(Notifier *notifier, void *data)
+{
+ Object *machine = qdev_get_machine();
+ Object *container;
+
+ if (object_property_find(machine, "has-dynamic-sysbus", NULL)) {
+ /* Our machine can handle dynamic sysbus devices, we're all good */
+ return;
+ }
+
+ /*
+ * Loop through all dynamically created devices and check whether there
+ * are sysbus devices among them. If there are, error out.
+ */
+ container = container_get(machine, "/peripheral");
+ search_for_sysbus_device(container, NULL);
+ container = container_get(machine, "/peripheral-anon");
+ search_for_sysbus_device(container, NULL);
+}
+
static void machine_initfn(Object *obj)
{
+ MachineState *ms = MACHINE(obj);
+
object_property_add_str(obj, "accel",
machine_get_accel, machine_set_accel, NULL);
object_property_add_bool(obj, "kernel_irqchip",
@@ -274,6 +311,10 @@ static void machine_initfn(Object *obj)
object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
object_property_add_str(obj, "firmware",
machine_get_firmware, machine_set_firmware, NULL);
+
+ /* Register notifier when init is done for sysbus sanity checks */
+ ms->sysbus_notifier.notify = machine_init_notify;
+ qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
}
static void machine_finalize(Object *obj)
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 84cd0cf..5d33bb8 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -326,13 +326,6 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data)
DeviceClass *k = DEVICE_CLASS(klass);
k->init = sysbus_device_init;
k->bus_type = TYPE_SYSTEM_BUS;
- /*
- * device_add plugs devices into suitable bus. For "real" buses,
- * that actually connects the device. For sysbus, the connections
- * need to be made separately, and device_add can't do that. The
- * device would be left unconnected, and could not possibly work.
- */
- k->cannot_instantiate_with_device_add_yet = true;
}
static const TypeInfo sysbus_device_type_info = {
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 605a970..0d93af3 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -110,6 +110,8 @@ struct MachineClass {
struct MachineState {
/*< private >*/
Object parent_obj;
+ Notifier sysbus_notifier;
+
/*< public >*/
char *accel;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] sysbus: Make devices spawnable via -device
2014-07-01 21:49 ` [Qemu-devel] [PATCH 4/6] sysbus: Make devices spawnable via -device Alexander Graf
@ 2014-07-02 6:32 ` Paolo Bonzini
2014-07-02 15:36 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Paolo Bonzini @ 2014-07-02 6:32 UTC (permalink / raw)
To: Alexander Graf, qemu-ppc
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
sean.stalley, afaerber
Il 01/07/2014 23:49, Alexander Graf ha scritto:
> +
> +static void machine_init_notify(Notifier *notifier, void *data)
> +{
> + Object *machine = qdev_get_machine();
> + Object *container;
> +
> + if (object_property_find(machine, "has-dynamic-sysbus", NULL)) {
> + /* Our machine can handle dynamic sysbus devices, we're all good */
> + return;
> + }
Does it need to be a property, or can it simply be a bool in MachineClass?
Paolo
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] sysbus: Make devices spawnable via -device
2014-07-02 6:32 ` Paolo Bonzini
@ 2014-07-02 15:36 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 15:36 UTC (permalink / raw)
To: Paolo Bonzini, qemu-ppc
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
sean.stalley, afaerber
On 02.07.14 08:32, Paolo Bonzini wrote:
> Il 01/07/2014 23:49, Alexander Graf ha scritto:
>> +
>> +static void machine_init_notify(Notifier *notifier, void *data)
>> +{
>> + Object *machine = qdev_get_machine();
>> + Object *container;
>> +
>> + if (object_property_find(machine, "has-dynamic-sysbus", NULL)) {
>> + /* Our machine can handle dynamic sysbus devices, we're all
>> good */
>> + return;
>> + }
>
> Does it need to be a property, or can it simply be a bool in
> MachineClass?
Sure - I'll change it to be a bool in MachineClass and QEMUMachine (I
really don't want to have the qom-machinification also as part of this
patch set)
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-01 21:49 [Qemu-devel] [PATCH 0/6] Dynamic sysbus device allocation support Alexander Graf
` (3 preceding siblings ...)
2014-07-01 21:49 ` [Qemu-devel] [PATCH 4/6] sysbus: Make devices spawnable via -device Alexander Graf
@ 2014-07-01 21:49 ` Alexander Graf
2014-07-01 22:50 ` Scott Wood
2014-07-01 21:49 ` [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree Alexander Graf
5 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-01 21:49 UTC (permalink / raw)
To: qemu-ppc
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
sean.stalley, pbonzini, afaerber
For e500 our approach to supporting dynamically spawned sysbus devices is to
create a simple bus from the guest's point of view within which we map those
devices dynamically.
We allocate memory regions always within the "platform" hole in address
space and map IRQs to predetermined IRQ lines that are reserved for platform
device usage.
This maps really nicely into device tree logic, so we can just tell the
guest about our virtual simple bus in device tree as well.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
hw/ppc/e500.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/e500.h | 1 +
hw/ppc/e500plat.c | 1 +
3 files changed, 253 insertions(+)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index bb2e75f..bf704b0 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -36,6 +36,7 @@
#include "exec/address-spaces.h"
#include "qemu/host-utils.h"
#include "hw/pci-host/ppce500.h"
+#include "qemu/error-report.h"
#define EPAPR_MAGIC (0x45504150)
#define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"
@@ -47,6 +48,14 @@
#define RAM_SIZES_ALIGN (64UL << 20)
+#define E500_PLATFORM_BASE 0xF0000000ULL
+#define E500_PLATFORM_HOLE (128ULL * 1024 * 1024) /* 128 MB */
+#define E500_PLATFORM_PAGE_SHIFT 12
+#define E500_PLATFORM_HOLE_PAGES (E500_PLATFORM_HOLE >> \
+ E500_PLATFORM_PAGE_SHIFT)
+#define E500_PLATFORM_FIRST_IRQ 5
+#define E500_PLATFORM_NUM_IRQS 10
+
/* TODO: parameterize */
#define MPC8544_CCSRBAR_BASE 0xE0000000ULL
#define MPC8544_CCSRBAR_SIZE 0x00100000ULL
@@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
}
}
+typedef struct PlatformDevtreeData {
+ void *fdt;
+ const char *mpic;
+ int irq_start;
+ const char *node;
+ int id;
+} PlatformDevtreeData;
+
+static int sysbus_device_create_devtree(Object *obj, void *opaque)
+{
+ PlatformDevtreeData *data = opaque;
+ Object *dev;
+ SysBusDevice *sbdev;
+ bool matched = false;
+
+ dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
+ sbdev = (SysBusDevice *)dev;
+
+ if (!sbdev) {
+ /* Container, traverse it for children */
+ return object_child_foreach(obj, sysbus_device_create_devtree, data);
+ }
+
+ if (matched) {
+ data->id++;
+ } else {
+ error_report("Device %s is not supported by this machine yet.",
+ qdev_fw_name(DEVICE(dev)));
+ exit(1);
+ }
+
+ return 0;
+}
+
+static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
+ const char *mpic, int irq_start,
+ int nr_irqs)
+{
+ const char platcomp[] = "qemu,platform\0simple-bus";
+ PlatformDevtreeData data;
+ Object *container;
+
+ /* Create a /platform node that we can put all devices into */
+
+ qemu_fdt_add_subnode(fdt, node);
+ qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
+ qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
+
+ /* Our platform hole is less than 32bit big, so 1 cell is enough for address
+ and size */
+ qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
+ qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
+ qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
+ E500_PLATFORM_HOLE);
+
+ qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
+
+ /* Loop through all devices and create nodes for known ones */
+
+ data.fdt = fdt;
+ data.mpic = mpic;
+ data.irq_start = irq_start;
+ data.node = node;
+ data.id = 0;
+
+ container = container_get(qdev_get_machine(), "/peripheral");
+ sysbus_device_create_devtree(container, &data);
+ container = container_get(qdev_get_machine(), "/peripheral-anon");
+ sysbus_device_create_devtree(container, &data);
+}
+
static int ppce500_load_device_tree(MachineState *machine,
PPCE500Params *params,
hwaddr addr,
@@ -379,6 +459,14 @@ static int ppce500_load_device_tree(MachineState *machine,
qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3);
qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci);
+ if (params->has_platform) {
+ gchar *node = g_strdup_printf("/platform@%llx", E500_PLATFORM_BASE);
+ platform_create_devtree(fdt, node, E500_PLATFORM_BASE, mpic,
+ E500_PLATFORM_FIRST_IRQ,
+ E500_PLATFORM_NUM_IRQS);
+ g_free(node);
+ }
+
params->fixup_devtree(params, fdt);
if (toplevel_compat) {
@@ -618,6 +706,155 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
return mpic;
}
+typedef struct PlatformNotifier {
+ Notifier notifier;
+ MemoryRegion *address_space_mem;
+ qemu_irq *mpic;
+} PlatformNotifier;
+
+typedef struct PlatformInitData {
+ unsigned long *used_irqs;
+ unsigned long *used_mem;
+ MemoryRegion *mem;
+ qemu_irq *irqs;
+ int device_count;
+} PlatformInitData;
+
+static int platform_map_irq(SysBusDevice *sbdev, int n, unsigned long *used_irqs,
+ qemu_irq *platform_irqs)
+{
+ int max_irqs = E500_PLATFORM_NUM_IRQS;
+ int irqn = sbdev->user_irqs[n];
+
+ if (irqn == (uint32_t)SYSBUS_DYNAMIC) {
+ /* Find the first available IRQ */
+ irqn = find_first_zero_bit(used_irqs, max_irqs);
+ }
+
+ if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) {
+ hw_error("e500: IRQ %d is already allocated or no free IRQ left", irqn);
+ }
+
+ sysbus_connect_irq(sbdev, n, platform_irqs[irqn]);
+ sbdev->user_irqs[n] = irqn;
+
+ return 0;
+}
+
+static int platform_map_mmio(SysBusDevice *sbdev, int n, unsigned long *used_mem,
+ MemoryRegion *pmem)
+{
+ MemoryRegion *device_mem = sbdev->mmio[n].memory;
+ uint64_t size = memory_region_size(device_mem);
+ uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT);
+ uint64_t page_mask = page_size - 1;
+ uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_PAGE_SHIFT;
+ hwaddr addr = sbdev->user_mmios[n];
+ int page;
+ int i;
+
+ page = addr >> E500_PLATFORM_PAGE_SHIFT;
+ if (addr == (uint64_t)SYSBUS_DYNAMIC) {
+ uint64_t size_pages_align;
+
+ /* Align the region to at least its own size granularity */
+ if (is_power_of_2(size_pages)) {
+ size_pages_align = size_pages;
+ } else {
+ size_pages_align = pow2floor(size_pages) << 1;
+ }
+
+ /* Find the first available region that fits */
+ page = bitmap_find_next_zero_area(used_mem, E500_PLATFORM_HOLE_PAGES, 0,
+ size_pages, size_pages_align);
+
+ addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT;
+ }
+
+ if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) ||
+ (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) < size_pages)) {
+ hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or "
+ "no slot left", addr, size);
+ }
+
+ for (i = page; i < (page + size_pages); i++) {
+ set_bit(i, used_mem);
+ }
+
+ memory_region_add_subregion(pmem, addr, device_mem);
+ sbdev->mmio[n].addr = addr;
+ sbdev->user_mmios[n] = addr;
+
+ return 0;
+}
+
+static int sysbus_device_check(Object *obj, void *opaque)
+{
+ PlatformInitData *init = opaque;
+ Object *dev;
+ SysBusDevice *sbdev;
+ int i;
+
+ dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
+ sbdev = (SysBusDevice *)dev;
+
+ if (!sbdev) {
+ /* Container, traverse it for children */
+ return object_child_foreach(obj, sysbus_device_check, opaque);
+ }
+
+ /* Connect sysbus device to virtual platform bus */
+ for (i = 0; i < sbdev->num_irq; i++) {
+ if (!sbdev->irqp[i]) {
+ /* This IRQ is an incoming IRQ, we can't wire those here */
+ continue;
+ }
+ platform_map_irq(sbdev, i, init->used_irqs, init->irqs);
+ }
+
+ for (i = 0; i < sbdev->num_mmio; i++) {
+ platform_map_mmio(sbdev, i, init->used_mem, init->mem);
+ }
+
+ return 0;
+}
+
+static void sysbus_devices_init(MemoryRegion *address_space_mem,
+ qemu_irq *mpic)
+{
+ DECLARE_BITMAP(used_irqs, E500_PLATFORM_NUM_IRQS);
+ DECLARE_BITMAP(used_mem, E500_PLATFORM_HOLE_PAGES);
+ MemoryRegion *platform_region = g_new(MemoryRegion, 1);
+ Object *container;
+ PlatformInitData init = {
+ .used_irqs = used_irqs,
+ .used_mem = used_mem,
+ .mem = platform_region,
+ .irqs = &mpic[E500_PLATFORM_FIRST_IRQ],
+ };
+
+ memory_region_init(platform_region, NULL, "platform devices",
+ E500_PLATFORM_HOLE);
+
+ bitmap_clear(used_irqs, 0, E500_PLATFORM_NUM_IRQS);
+ bitmap_clear(used_mem, 0, E500_PLATFORM_HOLE_PAGES);
+
+ /* Loop through all sysbus devices that were spawened outside the machine */
+ container = container_get(qdev_get_machine(), "/peripheral");
+ sysbus_device_check(container, &init);
+ container = container_get(qdev_get_machine(), "/peripheral-anon");
+ sysbus_device_check(container, &init);
+
+ memory_region_add_subregion(address_space_mem, E500_PLATFORM_BASE,
+ platform_region);
+}
+
+static void sysbus_devices_init_notify(Notifier *notifier, void *data)
+{
+ PlatformNotifier *pn = (PlatformNotifier *)notifier;
+ sysbus_devices_init(pn->address_space_mem, pn->mpic);
+}
+
void ppce500_init(MachineState *machine, PPCE500Params *params)
{
MemoryRegion *address_space_mem = get_system_memory();
@@ -770,6 +1007,20 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
cur_base = (32 * 1024 * 1024);
}
+ /* Platform Devices */
+ if (params->has_platform) {
+ PlatformNotifier *notifier = g_new(PlatformNotifier, 1);
+ Object *obj = OBJECT(machine);
+
+ notifier->notifier.notify = sysbus_devices_init_notify;
+ notifier->address_space_mem = address_space_mem;
+ notifier->mpic = mpic;
+ qemu_add_machine_init_done_notifier(¬ifier->notifier);
+
+ /* Promote the machine as dynamic sysbus capable */
+ object_property_add_bool(obj, "has-dynamic-sysbus", NULL, NULL, NULL);
+ }
+
/* Load kernel. */
if (machine->kernel_filename) {
kernel_base = cur_base;
diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 08b25fa..3a588ed 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -11,6 +11,7 @@ typedef struct PPCE500Params {
void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
int mpic_version;
+ bool has_platform;
} PPCE500Params;
void ppce500_init(MachineState *machine, PPCE500Params *params);
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 27df31d..bb84283 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -35,6 +35,7 @@ static void e500plat_init(MachineState *machine)
.pci_nr_slots = PCI_SLOT_MAX - 1,
.fixup_devtree = e500plat_fixup_devtree,
.mpic_version = OPENPIC_MODEL_FSL_MPIC_42,
+ .has_platform = true,
};
/* Older KVM versions don't support EPR which breaks guests when we announce
--
1.8.1.4
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-01 21:49 ` [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices Alexander Graf
@ 2014-07-01 22:50 ` Scott Wood
2014-07-02 17:12 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Scott Wood @ 2014-07-01 22:50 UTC (permalink / raw)
To: Alexander Graf
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote:
> For e500 our approach to supporting dynamically spawned sysbus devices is to
> create a simple bus from the guest's point of view within which we map those
> devices dynamically.
>
> We allocate memory regions always within the "platform" hole in address
> space and map IRQs to predetermined IRQ lines that are reserved for platform
> device usage.
>
> This maps really nicely into device tree logic, so we can just tell the
> guest about our virtual simple bus in device tree as well.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> hw/ppc/e500.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/e500.h | 1 +
> hw/ppc/e500plat.c | 1 +
> 3 files changed, 253 insertions(+)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index bb2e75f..bf704b0 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -36,6 +36,7 @@
> #include "exec/address-spaces.h"
> #include "qemu/host-utils.h"
> #include "hw/pci-host/ppce500.h"
> +#include "qemu/error-report.h"
>
> #define EPAPR_MAGIC (0x45504150)
> #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"
> @@ -47,6 +48,14 @@
>
> #define RAM_SIZES_ALIGN (64UL << 20)
>
> +#define E500_PLATFORM_BASE 0xF0000000ULL
Should the IRQ and address range be parameterized? Even if the platform
bus is going to be restricted to only e500plat, it seems like it would
be good to keep all assumptions that are e500plat-specific inside the
e500plat file.
Plus, let's please not hardcode any more addresses that are going to be
a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
blocking that, but that has a TODO to parameterize). How about
0xf00000000ULL? Unless of course we're emulating an e500v1, which
doesn't support 36-bit physical addressing. Parameterization would help
there as well.
> +#define E500_PLATFORM_HOLE (128ULL * 1024 * 1024) /* 128 MB */
> +#define E500_PLATFORM_PAGE_SHIFT 12
> +#define E500_PLATFORM_HOLE_PAGES (E500_PLATFORM_HOLE >> \
> + E500_PLATFORM_PAGE_SHIFT)
> +#define E500_PLATFORM_FIRST_IRQ 5
> +#define E500_PLATFORM_NUM_IRQS 10
What is the "hole"? If that's meant to be the size to go along with
E500_PLATFORM_BASE, that seems like odd terminology.
> +
> /* TODO: parameterize */
> #define MPC8544_CCSRBAR_BASE 0xE0000000ULL
> #define MPC8544_CCSRBAR_SIZE 0x00100000ULL
> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
> }
> }
>
> +typedef struct PlatformDevtreeData {
> + void *fdt;
> + const char *mpic;
> + int irq_start;
> + const char *node;
> + int id;
> +} PlatformDevtreeData;
What is id? How does irq_start work?
> +static int sysbus_device_create_devtree(Object *obj, void *opaque)
> +{
> + PlatformDevtreeData *data = opaque;
> + Object *dev;
> + SysBusDevice *sbdev;
> + bool matched = false;
> +
> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
> + sbdev = (SysBusDevice *)dev;
> +
> + if (!sbdev) {
> + /* Container, traverse it for children */
> + return object_child_foreach(obj, sysbus_device_create_devtree, data);
> + }
> +
> + if (matched) {
> + data->id++;
> + } else {
> + error_report("Device %s is not supported by this machine yet.",
> + qdev_fw_name(DEVICE(dev)));
> + exit(1);
> + }
> +
> + return 0;
> +}
It's not clear to me how this function is creating a device tree node.
> +
> +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
> + const char *mpic, int irq_start,
> + int nr_irqs)
> +{
> + const char platcomp[] = "qemu,platform\0simple-bus";
> + PlatformDevtreeData data;
> + Object *container;
> +
> + /* Create a /platform node that we can put all devices into */
> +
> + qemu_fdt_add_subnode(fdt, node);
> + qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
> + qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
Where did this device_type come from?
device_type is deprecated and new uses should not be introduced.
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 08b25fa..3a588ed 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -11,6 +11,7 @@ typedef struct PPCE500Params {
> void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
>
> int mpic_version;
> + bool has_platform;
> } PPCE500Params;
It would be clearer to refer to this as "platform bus", "platform
devices", or similar rather than just "platform" -- the latter is
confusing relative to existing use of the word "platform", such as
e500plat.c.
-Scott
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-01 22:50 ` Scott Wood
@ 2014-07-02 17:12 ` Alexander Graf
2014-07-02 17:26 ` Scott Wood
0 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 17:12 UTC (permalink / raw)
To: Scott Wood
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On 02.07.14 00:50, Scott Wood wrote:
> On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote:
>> For e500 our approach to supporting dynamically spawned sysbus devices is to
>> create a simple bus from the guest's point of view within which we map those
>> devices dynamically.
>>
>> We allocate memory regions always within the "platform" hole in address
>> space and map IRQs to predetermined IRQ lines that are reserved for platform
>> device usage.
>>
>> This maps really nicely into device tree logic, so we can just tell the
>> guest about our virtual simple bus in device tree as well.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ppc/e500.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/ppc/e500.h | 1 +
>> hw/ppc/e500plat.c | 1 +
>> 3 files changed, 253 insertions(+)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index bb2e75f..bf704b0 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -36,6 +36,7 @@
>> #include "exec/address-spaces.h"
>> #include "qemu/host-utils.h"
>> #include "hw/pci-host/ppce500.h"
>> +#include "qemu/error-report.h"
>>
>> #define EPAPR_MAGIC (0x45504150)
>> #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"
>> @@ -47,6 +48,14 @@
>>
>> #define RAM_SIZES_ALIGN (64UL << 20)
>>
>> +#define E500_PLATFORM_BASE 0xF0000000ULL
> Should the IRQ and address range be parameterized? Even if the platform
> bus is going to be restricted to only e500plat, it seems like it would
> be good to keep all assumptions that are e500plat-specific inside the
> e500plat file.
Good idea. The only thing I'll leave here is the page_shift. The fact
that we only allocate in 4k chunks is inherently implementation specific.
> Plus, let's please not hardcode any more addresses that are going to be
> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
> blocking that, but that has a TODO to parameterize). How about
> 0xf00000000ULL? Unless of course we're emulating an e500v1, which
> doesn't support 36-bit physical addressing. Parameterization would help
> there as well.
I don't think we have to worry about e500v1. I'll change it :).
>
>> +#define E500_PLATFORM_HOLE (128ULL * 1024 * 1024) /* 128 MB */
>> +#define E500_PLATFORM_PAGE_SHIFT 12
>> +#define E500_PLATFORM_HOLE_PAGES (E500_PLATFORM_HOLE >> \
>> + E500_PLATFORM_PAGE_SHIFT)
>> +#define E500_PLATFORM_FIRST_IRQ 5
>> +#define E500_PLATFORM_NUM_IRQS 10
> What is the "hole"? If that's meant to be the size to go along with
> E500_PLATFORM_BASE, that seems like odd terminology.
True - renamed to "size".
>
>> +
>> /* TODO: parameterize */
>> #define MPC8544_CCSRBAR_BASE 0xE0000000ULL
>> #define MPC8544_CCSRBAR_SIZE 0x00100000ULL
>> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
>> }
>> }
>>
>> +typedef struct PlatformDevtreeData {
>> + void *fdt;
>> + const char *mpic;
>> + int irq_start;
>> + const char *node;
>> + int id;
>> +} PlatformDevtreeData;
> What is id? How does irq_start work?
"id" is just a linear counter over all devices in the platform bus so
that if you need to have a unique identifier, you can have one.
"irq_start" is the offset of the first mpic irq that's connected to the
platform bus.
>
>> +static int sysbus_device_create_devtree(Object *obj, void *opaque)
>> +{
>> + PlatformDevtreeData *data = opaque;
>> + Object *dev;
>> + SysBusDevice *sbdev;
>> + bool matched = false;
>> +
>> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
>> + sbdev = (SysBusDevice *)dev;
>> +
>> + if (!sbdev) {
>> + /* Container, traverse it for children */
>> + return object_child_foreach(obj, sysbus_device_create_devtree, data);
>> + }
>> +
>> + if (matched) {
>> + data->id++;
>> + } else {
>> + error_report("Device %s is not supported by this machine yet.",
>> + qdev_fw_name(DEVICE(dev)));
>> + exit(1);
>> + }
>> +
>> + return 0;
>> +}
> It's not clear to me how this function is creating a device tree node.
It's not yet - it's only the stub that allows to plug in specific device
code that then generates device tree nodes :).
>
>> +
>> +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr,
>> + const char *mpic, int irq_start,
>> + int nr_irqs)
>> +{
>> + const char platcomp[] = "qemu,platform\0simple-bus";
>> + PlatformDevtreeData data;
>> + Object *container;
>> +
>> + /* Create a /platform node that we can put all devices into */
>> +
>> + qemu_fdt_add_subnode(fdt, node);
>> + qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
>> + qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
> Where did this device_type come from?
>
> device_type is deprecated and new uses should not be introduced.
Fair enough, will remove it :)
>
>> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
>> index 08b25fa..3a588ed 100644
>> --- a/hw/ppc/e500.h
>> +++ b/hw/ppc/e500.h
>> @@ -11,6 +11,7 @@ typedef struct PPCE500Params {
>> void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
>>
>> int mpic_version;
>> + bool has_platform;
>> } PPCE500Params;
> It would be clearer to refer to this as "platform bus", "platform
> devices", or similar rather than just "platform" -- the latter is
> confusing relative to existing use of the word "platform", such as
> e500plat.c.
Renamed to "platform_bus" :). In fact, I'll go through the file with a
small sweeper and try to make sure we're reasonably consistent in our
naming.
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-02 17:12 ` Alexander Graf
@ 2014-07-02 17:26 ` Scott Wood
2014-07-02 17:30 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Scott Wood @ 2014-07-02 17:26 UTC (permalink / raw)
To: Alexander Graf
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote:
> On 02.07.14 00:50, Scott Wood wrote:
> > Plus, let's please not hardcode any more addresses that are going to be
> > a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
> > blocking that, but that has a TODO to parameterize). How about
> > 0xf00000000ULL? Unless of course we're emulating an e500v1, which
> > doesn't support 36-bit physical addressing. Parameterization would help
> > there as well.
>
> I don't think we have to worry about e500v1. I'll change it :).
We theoretically support it elsewhere... Once parameterized, it
shouldn't be hard to base the address for this, CCSRBAR, and similar
things on whether MAS7 is supported.
> >> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
> >> }
> >> }
> >>
> >> +typedef struct PlatformDevtreeData {
> >> + void *fdt;
> >> + const char *mpic;
> >> + int irq_start;
> >> + const char *node;
> >> + int id;
> >> +} PlatformDevtreeData;
> > What is id? How does irq_start work?
>
> "id" is just a linear counter over all devices in the platform bus so
> that if you need to have a unique identifier, you can have one.
>
> "irq_start" is the offset of the first mpic irq that's connected to the
> platform bus.
OK, but why is that here but no irq_end, and no address range? How do
allocations from the irq range happen?
> >> +static int sysbus_device_create_devtree(Object *obj, void *opaque)
> >> +{
> >> + PlatformDevtreeData *data = opaque;
> >> + Object *dev;
> >> + SysBusDevice *sbdev;
> >> + bool matched = false;
> >> +
> >> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
> >> + sbdev = (SysBusDevice *)dev;
> >> +
> >> + if (!sbdev) {
> >> + /* Container, traverse it for children */
> >> + return object_child_foreach(obj, sysbus_device_create_devtree, data);
> >> + }
> >> +
> >> + if (matched) {
> >> + data->id++;
> >> + } else {
> >> + error_report("Device %s is not supported by this machine yet.",
> >> + qdev_fw_name(DEVICE(dev)));
> >> + exit(1);
> >> + }
> >> +
> >> + return 0;
> >> +}
> > It's not clear to me how this function is creating a device tree node.
>
> It's not yet - it's only the stub that allows to plug in specific device
> code that then generates device tree nodes :).
How does the plugging in work?
It looks like all this does is increment id.
-Scott
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-02 17:26 ` Scott Wood
@ 2014-07-02 17:30 ` Alexander Graf
2014-07-02 17:52 ` Scott Wood
0 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 17:30 UTC (permalink / raw)
To: Scott Wood
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On 02.07.14 19:26, Scott Wood wrote:
> On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote:
>> On 02.07.14 00:50, Scott Wood wrote:
>>> Plus, let's please not hardcode any more addresses that are going to be
>>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
>>> blocking that, but that has a TODO to parameterize). How about
>>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which
>>> doesn't support 36-bit physical addressing. Parameterization would help
>>> there as well.
>> I don't think we have to worry about e500v1. I'll change it :).
> We theoretically support it elsewhere... Once parameterized, it
> shouldn't be hard to base the address for this, CCSRBAR, and similar
> things on whether MAS7 is supported.
It gets parametrized in the machine file, CPU selection comes after
machine selection. So parameterizing it doesn't really solve it.
However, again, I don't think we have to worry about it.
>
>>>> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
>>>> }
>>>> }
>>>>
>>>> +typedef struct PlatformDevtreeData {
>>>> + void *fdt;
>>>> + const char *mpic;
>>>> + int irq_start;
>>>> + const char *node;
>>>> + int id;
>>>> +} PlatformDevtreeData;
>>> What is id? How does irq_start work?
>> "id" is just a linear counter over all devices in the platform bus so
>> that if you need to have a unique identifier, you can have one.
>>
>> "irq_start" is the offset of the first mpic irq that's connected to the
>> platform bus.
> OK, but why is that here but no irq_end, and no address range? How do
> allocations from the irq range happen?
There are 2 phases:
1) Device association with the machine
2) Device tree generation
The allocation of IRQ ranges happens during the association phase. That
phase also updates all the hints in the devices to reflect their current
IRQ (and MMIO) mappings. The device tree generation phase only needs to
read those bits then - and add the IRQ offset to get from the "platform
bus IRQ range" to "MPIC IRQ range".
>
>>>> +static int sysbus_device_create_devtree(Object *obj, void *opaque)
>>>> +{
>>>> + PlatformDevtreeData *data = opaque;
>>>> + Object *dev;
>>>> + SysBusDevice *sbdev;
>>>> + bool matched = false;
>>>> +
>>>> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
>>>> + sbdev = (SysBusDevice *)dev;
>>>> +
>>>> + if (!sbdev) {
>>>> + /* Container, traverse it for children */
>>>> + return object_child_foreach(obj, sysbus_device_create_devtree, data);
>>>> + }
>>>> +
>>>> + if (matched) {
>>>> + data->id++;
>>>> + } else {
>>>> + error_report("Device %s is not supported by this machine yet.",
>>>> + qdev_fw_name(DEVICE(dev)));
>>>> + exit(1);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>> It's not clear to me how this function is creating a device tree node.
>> It's not yet - it's only the stub that allows to plug in specific device
>> code that then generates device tree nodes :).
> How does the plugging in work?
>
> It looks like all this does is increment id.
I'm not sure I understand. The plugging in is different code :). This
really only does increment an id. Maybe I'll just remove it if it
confuses you?
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-02 17:30 ` Alexander Graf
@ 2014-07-02 17:52 ` Scott Wood
2014-07-02 17:59 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Scott Wood @ 2014-07-02 17:52 UTC (permalink / raw)
To: Alexander Graf
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote:
> On 02.07.14 19:26, Scott Wood wrote:
> > On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote:
> >> On 02.07.14 00:50, Scott Wood wrote:
> >>> Plus, let's please not hardcode any more addresses that are going to be
> >>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
> >>> blocking that, but that has a TODO to parameterize). How about
> >>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which
> >>> doesn't support 36-bit physical addressing. Parameterization would help
> >>> there as well.
> >> I don't think we have to worry about e500v1. I'll change it :).
> > We theoretically support it elsewhere... Once parameterized, it
> > shouldn't be hard to base the address for this, CCSRBAR, and similar
> > things on whether MAS7 is supported.
>
> It gets parametrized in the machine file, CPU selection comes after
> machine selection. So parameterizing it doesn't really solve it.
Why can't e500plat_init() look at args->cpu_model? Or the
parameterization could take two sets of addresses, one for a 32-bit
layout and one for a 36-bit layout. It might make sense to make this a
user-settable parameter; some OSes might not be able to handle a 36-bit
layout (or might not be as efficient at handling it) even on e500v2.
Many of the e500v2 boards can be built for either a 32 or 36 bit address
layout in U-Boot.
> However, again, I don't think we have to worry about it.
It's not a huge worry, but it'd be nice to not break it gratuitously.
If we do break it we should explicitly disallow e500v1 with e500plat.
> >>>> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
> >>>> }
> >>>> }
> >>>>
> >>>> +typedef struct PlatformDevtreeData {
> >>>> + void *fdt;
> >>>> + const char *mpic;
> >>>> + int irq_start;
> >>>> + const char *node;
> >>>> + int id;
> >>>> +} PlatformDevtreeData;
> >>> What is id? How does irq_start work?
> >> "id" is just a linear counter over all devices in the platform bus so
> >> that if you need to have a unique identifier, you can have one.
> >>
> >> "irq_start" is the offset of the first mpic irq that's connected to the
> >> platform bus.
> > OK, but why is that here but no irq_end, and no address range? How do
> > allocations from the irq range happen?
>
> There are 2 phases:
>
> 1) Device association with the machine
> 2) Device tree generation
>
> The allocation of IRQ ranges happens during the association phase. That
> phase also updates all the hints in the devices to reflect their current
> IRQ (and MMIO) mappings. The device tree generation phase only needs to
> read those bits then - and add the IRQ offset to get from the "platform
> bus IRQ range" to "MPIC IRQ range".
I think the answer to my original question is that irqs are allocated
based on zero because they go in an array, while memory regions are
allocated with their actual addresses because they don't.
> >>>> +static int sysbus_device_create_devtree(Object *obj, void *opaque)
> >>>> +{
> >>>> + PlatformDevtreeData *data = opaque;
> >>>> + Object *dev;
> >>>> + SysBusDevice *sbdev;
> >>>> + bool matched = false;
> >>>> +
> >>>> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
> >>>> + sbdev = (SysBusDevice *)dev;
> >>>> +
> >>>> + if (!sbdev) {
> >>>> + /* Container, traverse it for children */
> >>>> + return object_child_foreach(obj, sysbus_device_create_devtree, data);
> >>>> + }
> >>>> +
> >>>> + if (matched) {
> >>>> + data->id++;
> >>>> + } else {
> >>>> + error_report("Device %s is not supported by this machine yet.",
> >>>> + qdev_fw_name(DEVICE(dev)));
> >>>> + exit(1);
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>> It's not clear to me how this function is creating a device tree node.
> >> It's not yet - it's only the stub that allows to plug in specific device
> >> code that then generates device tree nodes :).
> > How does the plugging in work?
> >
> > It looks like all this does is increment id.
>
> I'm not sure I understand. The plugging in is different code :). This
> really only does increment an id. Maybe I'll just remove it if it
> confuses you?
My confusion is that it is called sysbus_device_create_devtree(), not
sysbus_device_alloc_id(). Am I missing some sort of virtual function
mechanism here that would allow this function to be replaced?
/me looks at patch 6/6 again
Oh, you just add to this function in future patches. I was expecting
something fancier given the QOM stuff and my misunderstanding about what
file patch 6/6 was touching. :-)
-Scott
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-02 17:52 ` Scott Wood
@ 2014-07-02 17:59 ` Alexander Graf
2014-07-02 19:34 ` Scott Wood
0 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 17:59 UTC (permalink / raw)
To: Scott Wood
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On 02.07.14 19:52, Scott Wood wrote:
> On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote:
>> On 02.07.14 19:26, Scott Wood wrote:
>>> On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote:
>>>> On 02.07.14 00:50, Scott Wood wrote:
>>>>> Plus, let's please not hardcode any more addresses that are going to be
>>>>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
>>>>> blocking that, but that has a TODO to parameterize). How about
>>>>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which
>>>>> doesn't support 36-bit physical addressing. Parameterization would help
>>>>> there as well.
>>>> I don't think we have to worry about e500v1. I'll change it :).
>>> We theoretically support it elsewhere... Once parameterized, it
>>> shouldn't be hard to base the address for this, CCSRBAR, and similar
>>> things on whether MAS7 is supported.
>> It gets parametrized in the machine file, CPU selection comes after
>> machine selection. So parameterizing it doesn't really solve it.
> Why can't e500plat_init() look at args->cpu_model? Or the
> parameterization could take two sets of addresses, one for a 32-bit
> layout and one for a 36-bit layout. It might make sense to make this a
> user-settable parameter; some OSes might not be able to handle a 36-bit
> layout (or might not be as efficient at handling it) even on e500v2.
> Many of the e500v2 boards can be built for either a 32 or 36 bit address
> layout in U-Boot.
>
>> However, again, I don't think we have to worry about it.
> It's not a huge worry, but it'd be nice to not break it gratuitously.
> If we do break it we should explicitly disallow e500v1 with e500plat.
I'd prefer if we don't overparameterize - it'll just become a headache
further down. Today we don't explicitly disallow anything anywhere - you
could theoretically stick a G3 into e500plat. I don't see why we should
start with heavy sanity checks now :).
Plus, the machine works just fine today if you don't pass in -device
eTSEC. It's not like we're moving all devices to the new "platform bus".
>
>>>>>> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +typedef struct PlatformDevtreeData {
>>>>>> + void *fdt;
>>>>>> + const char *mpic;
>>>>>> + int irq_start;
>>>>>> + const char *node;
>>>>>> + int id;
>>>>>> +} PlatformDevtreeData;
>>>>> What is id? How does irq_start work?
>>>> "id" is just a linear counter over all devices in the platform bus so
>>>> that if you need to have a unique identifier, you can have one.
>>>>
>>>> "irq_start" is the offset of the first mpic irq that's connected to the
>>>> platform bus.
>>> OK, but why is that here but no irq_end, and no address range? How do
>>> allocations from the irq range happen?
>> There are 2 phases:
>>
>> 1) Device association with the machine
>> 2) Device tree generation
>>
>> The allocation of IRQ ranges happens during the association phase. That
>> phase also updates all the hints in the devices to reflect their current
>> IRQ (and MMIO) mappings. The device tree generation phase only needs to
>> read those bits then - and add the IRQ offset to get from the "platform
>> bus IRQ range" to "MPIC IRQ range".
> I think the answer to my original question is that irqs are allocated
> based on zero because they go in an array, while memory regions are
> allocated with their actual addresses because they don't.
Memory regions are allocated based on zero as well, they get mapped into
a subregion. From a device's point of view, the regions for MMIO and
IRQs that it sees all start at 0 relative to the platform bus.
>
>>>>>> +static int sysbus_device_create_devtree(Object *obj, void *opaque)
>>>>>> +{
>>>>>> + PlatformDevtreeData *data = opaque;
>>>>>> + Object *dev;
>>>>>> + SysBusDevice *sbdev;
>>>>>> + bool matched = false;
>>>>>> +
>>>>>> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
>>>>>> + sbdev = (SysBusDevice *)dev;
>>>>>> +
>>>>>> + if (!sbdev) {
>>>>>> + /* Container, traverse it for children */
>>>>>> + return object_child_foreach(obj, sysbus_device_create_devtree, data);
>>>>>> + }
>>>>>> +
>>>>>> + if (matched) {
>>>>>> + data->id++;
>>>>>> + } else {
>>>>>> + error_report("Device %s is not supported by this machine yet.",
>>>>>> + qdev_fw_name(DEVICE(dev)));
>>>>>> + exit(1);
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>> It's not clear to me how this function is creating a device tree node.
>>>> It's not yet - it's only the stub that allows to plug in specific device
>>>> code that then generates device tree nodes :).
>>> How does the plugging in work?
>>>
>>> It looks like all this does is increment id.
>> I'm not sure I understand. The plugging in is different code :). This
>> really only does increment an id. Maybe I'll just remove it if it
>> confuses you?
> My confusion is that it is called sysbus_device_create_devtree(), not
> sysbus_device_alloc_id(). Am I missing some sort of virtual function
> mechanism here that would allow this function to be replaced?
I've removed the id bit - hope that makes it more obvious :).
>
> /me looks at patch 6/6 again
>
> Oh, you just add to this function in future patches. I was expecting
> something fancier given the QOM stuff and my misunderstanding about what
> file patch 6/6 was touching. :-)
Heh, yeah, nothing impressively fancy :).
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-02 17:59 ` Alexander Graf
@ 2014-07-02 19:34 ` Scott Wood
2014-07-02 20:59 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Scott Wood @ 2014-07-02 19:34 UTC (permalink / raw)
To: Alexander Graf
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On Wed, 2014-07-02 at 19:59 +0200, Alexander Graf wrote:
> On 02.07.14 19:52, Scott Wood wrote:
> > On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote:
> >> On 02.07.14 19:26, Scott Wood wrote:
> >>> On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote:
> >>>> On 02.07.14 00:50, Scott Wood wrote:
> >>>>> Plus, let's please not hardcode any more addresses that are going to be
> >>>>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
> >>>>> blocking that, but that has a TODO to parameterize). How about
> >>>>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which
> >>>>> doesn't support 36-bit physical addressing. Parameterization would help
> >>>>> there as well.
> >>>> I don't think we have to worry about e500v1. I'll change it :).
> >>> We theoretically support it elsewhere... Once parameterized, it
> >>> shouldn't be hard to base the address for this, CCSRBAR, and similar
> >>> things on whether MAS7 is supported.
> >> It gets parametrized in the machine file, CPU selection comes after
> >> machine selection. So parameterizing it doesn't really solve it.
> > Why can't e500plat_init() look at args->cpu_model? Or the
> > parameterization could take two sets of addresses, one for a 32-bit
> > layout and one for a 36-bit layout. It might make sense to make this a
> > user-settable parameter; some OSes might not be able to handle a 36-bit
> > layout (or might not be as efficient at handling it) even on e500v2.
> > Many of the e500v2 boards can be built for either a 32 or 36 bit address
> > layout in U-Boot.
> >
> >> However, again, I don't think we have to worry about it.
> > It's not a huge worry, but it'd be nice to not break it gratuitously.
> > If we do break it we should explicitly disallow e500v1 with e500plat.
>
> I'd prefer if we don't overparameterize - it'll just become a headache
> further down.
"We shouldn't overparameterize" is a tautology. The question is what
constitutes "over". I don't think this is excessive. Again, it's
parameterization that U-Boot already does, even disregarding e500v1, and
QEMU plays the role of U-Boot to a certain degree (even in the new mode
of actually running U-Boot, the address map is fixed).
Perhaps it could be simplified by just saying that, in 36-bit mode, all
physical addresses other than RAM have 0xf prepended. This is similar
to what U-Boot does.
> Today we don't explicitly disallow anything anywhere - you
> could theoretically stick a G3 into e500plat. I don't see why we should
> start with heavy sanity checks now :).
Ugh.
It should at least be documented, since unlike a G3, e500v1 isn't an
unrealistic expectation on a platform called e500plat.
> Plus, the machine works just fine today if you don't pass in -device
> eTSEC. It's not like we're moving all devices to the new "platform bus".
We have a TODO to move CCSR as well.
-Scott
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices
2014-07-02 19:34 ` Scott Wood
@ 2014-07-02 20:59 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 20:59 UTC (permalink / raw)
To: Scott Wood
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On 02.07.14 21:34, Scott Wood wrote:
> On Wed, 2014-07-02 at 19:59 +0200, Alexander Graf wrote:
>> On 02.07.14 19:52, Scott Wood wrote:
>>> On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote:
>>>> On 02.07.14 19:26, Scott Wood wrote:
>>>>> On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote:
>>>>>> On 02.07.14 00:50, Scott Wood wrote:
>>>>>>> Plus, let's please not hardcode any more addresses that are going to be
>>>>>>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
>>>>>>> blocking that, but that has a TODO to parameterize). How about
>>>>>>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which
>>>>>>> doesn't support 36-bit physical addressing. Parameterization would help
>>>>>>> there as well.
>>>>>> I don't think we have to worry about e500v1. I'll change it :).
>>>>> We theoretically support it elsewhere... Once parameterized, it
>>>>> shouldn't be hard to base the address for this, CCSRBAR, and similar
>>>>> things on whether MAS7 is supported.
>>>> It gets parametrized in the machine file, CPU selection comes after
>>>> machine selection. So parameterizing it doesn't really solve it.
>>> Why can't e500plat_init() look at args->cpu_model? Or the
>>> parameterization could take two sets of addresses, one for a 32-bit
>>> layout and one for a 36-bit layout. It might make sense to make this a
>>> user-settable parameter; some OSes might not be able to handle a 36-bit
>>> layout (or might not be as efficient at handling it) even on e500v2.
>>> Many of the e500v2 boards can be built for either a 32 or 36 bit address
>>> layout in U-Boot.
>>>
>>>> However, again, I don't think we have to worry about it.
>>> It's not a huge worry, but it'd be nice to not break it gratuitously.
>>> If we do break it we should explicitly disallow e500v1 with e500plat.
>> I'd prefer if we don't overparameterize - it'll just become a headache
>> further down.
> "We shouldn't overparameterize" is a tautology. The question is what
> constitutes "over". I don't think this is excessive. Again, it's
> parameterization that U-Boot already does, even disregarding e500v1, and
> QEMU plays the role of U-Boot to a certain degree (even in the new mode
> of actually running U-Boot, the address map is fixed).
>
> Perhaps it could be simplified by just saying that, in 36-bit mode, all
> physical addresses other than RAM have 0xf prepended. This is similar
> to what U-Boot does.
I think we should just have another machine type for that case - one
that is 32bit and one that is 36bit compatible.
>
>> Today we don't explicitly disallow anything anywhere - you
>> could theoretically stick a G3 into e500plat. I don't see why we should
>> start with heavy sanity checks now :).
> Ugh.
>
> It should at least be documented, since unlike a G3, e500v1 isn't an
> unrealistic expectation on a platform called e500plat.
>
>> Plus, the machine works just fine today if you don't pass in -device
>> eTSEC. It's not like we're moving all devices to the new "platform bus".
> We have a TODO to move CCSR as well.
Yes, that's certainly a good goal to have :).
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree
2014-07-01 21:49 [Qemu-devel] [PATCH 0/6] Dynamic sysbus device allocation support Alexander Graf
` (4 preceding siblings ...)
2014-07-01 21:49 ` [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices Alexander Graf
@ 2014-07-01 21:49 ` Alexander Graf
2014-07-01 22:56 ` Scott Wood
5 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-01 21:49 UTC (permalink / raw)
To: qemu-ppc
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
sean.stalley, pbonzini, afaerber
This patch adds support to expose eTSEC devices in the dynamically created
guest facing device tree. This allows us to expose eTSEC devices into guests
without changes in the machine file.
Because we can now tell the guest about eTSEC devices this patch allows the
user to specify eTSEC devices via -device at all.
Signed-off-by: Alexander Graf <agraf@suse.de>
---
hw/ppc/e500.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index bf704b0..bebff6f 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -37,6 +37,7 @@
#include "qemu/host-utils.h"
#include "hw/pci-host/ppce500.h"
#include "qemu/error-report.h"
+#include "hw/net/fsl_etsec/etsec.h"
#define EPAPR_MAGIC (0x45504150)
#define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"
@@ -139,6 +140,34 @@ typedef struct PlatformDevtreeData {
int id;
} PlatformDevtreeData;
+static int create_devtree_etsec(eTSEC *etsec, PlatformDevtreeData *data)
+{
+ SysBusDevice *sbdev = &etsec->busdev;
+ gchar *node = g_strdup_printf("/platform/ethernet@%d", data->id);
+ gchar *group = g_strdup_printf("%s/queue-group", node);
+ void *fdt = data->fdt;
+
+ qemu_fdt_add_subnode(fdt, node);
+ qemu_fdt_setprop_string(fdt, node, "device_type", "network");
+ qemu_fdt_setprop_string(fdt, node, "compatible", "fsl,etsec2");
+ qemu_fdt_setprop_string(fdt, node, "model", "eTSEC");
+ qemu_fdt_setprop(fdt, node, "local-mac-address", etsec->conf.macaddr.a, 6);
+ qemu_fdt_setprop_cells(fdt, node, "fixed-link", 0, 1, 1000, 0, 0);
+
+ qemu_fdt_add_subnode(fdt, group);
+ qemu_fdt_setprop_cells(fdt, group, "reg", sbdev->user_mmios[0], 0x1000);
+ qemu_fdt_setprop_phandle(fdt, group, "interrupt-parent", data->mpic);
+ qemu_fdt_setprop_cells(fdt, group, "interrupts",
+ data->irq_start + sbdev->user_irqs[0], 0x0,
+ data->irq_start + sbdev->user_irqs[1], 0x0,
+ data->irq_start + sbdev->user_irqs[2], 0x0);
+
+ g_free(node);
+ g_free(group);
+
+ return 0;
+}
+
static int sysbus_device_create_devtree(Object *obj, void *opaque)
{
PlatformDevtreeData *data = opaque;
@@ -154,6 +183,11 @@ static int sysbus_device_create_devtree(Object *obj, void *opaque)
return object_child_foreach(obj, sysbus_device_create_devtree, data);
}
+ if (object_dynamic_cast(obj, TYPE_ETSEC_COMMON)) {
+ create_devtree_etsec(ETSEC_COMMON(dev), data);
+ matched = true;
+ }
+
if (matched) {
data->id++;
} else {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree
2014-07-01 21:49 ` [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree Alexander Graf
@ 2014-07-01 22:56 ` Scott Wood
2014-07-02 17:24 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Scott Wood @ 2014-07-01 22:56 UTC (permalink / raw)
To: Alexander Graf
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote:
> This patch adds support to expose eTSEC devices in the dynamically created
> guest facing device tree. This allows us to expose eTSEC devices into guests
> without changes in the machine file.
>
> Because we can now tell the guest about eTSEC devices this patch allows the
> user to specify eTSEC devices via -device at all.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> hw/ppc/e500.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index bf704b0..bebff6f 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -37,6 +37,7 @@
> #include "qemu/host-utils.h"
> #include "hw/pci-host/ppce500.h"
> #include "qemu/error-report.h"
> +#include "hw/net/fsl_etsec/etsec.h"
>
> #define EPAPR_MAGIC (0x45504150)
> #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"
> @@ -139,6 +140,34 @@ typedef struct PlatformDevtreeData {
> int id;
> } PlatformDevtreeData;
>
> +static int create_devtree_etsec(eTSEC *etsec, PlatformDevtreeData *data)
> +{
> + SysBusDevice *sbdev = &etsec->busdev;
> + gchar *node = g_strdup_printf("/platform/ethernet@%d", data->id);
The unit address is supposed to match reg. It's not an arbitrary
disambiguator.
> + gchar *group = g_strdup_printf("%s/queue-group", node);
> + void *fdt = data->fdt;
> +
> + qemu_fdt_add_subnode(fdt, node);
> + qemu_fdt_setprop_string(fdt, node, "device_type", "network");
> + qemu_fdt_setprop_string(fdt, node, "compatible", "fsl,etsec2");
> + qemu_fdt_setprop_string(fdt, node, "model", "eTSEC");
> + qemu_fdt_setprop(fdt, node, "local-mac-address", etsec->conf.macaddr.a, 6);
> + qemu_fdt_setprop_cells(fdt, node, "fixed-link", 0, 1, 1000, 0, 0);
> +
> + qemu_fdt_add_subnode(fdt, group);
> + qemu_fdt_setprop_cells(fdt, group, "reg", sbdev->user_mmios[0], 0x1000);
> + qemu_fdt_setprop_phandle(fdt, group, "interrupt-parent", data->mpic);
Why not do interrupt-parent in the parent node, or top of tree?
> + qemu_fdt_setprop_cells(fdt, group, "interrupts",
> + data->irq_start + sbdev->user_irqs[0], 0x0,
> + data->irq_start + sbdev->user_irqs[1], 0x0,
> + data->irq_start + sbdev->user_irqs[2], 0x0);
Are we still using two-cell interrupt specifiers? If so, we should
switch before the assumption gets encoded into random device files.
Also, why are these interrupts edge triggered?
-Scott
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree
2014-07-01 22:56 ` Scott Wood
@ 2014-07-02 17:24 ` Alexander Graf
2014-07-02 17:32 ` Scott Wood
0 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 17:24 UTC (permalink / raw)
To: Scott Wood
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On 02.07.14 00:56, Scott Wood wrote:
> On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote:
>> This patch adds support to expose eTSEC devices in the dynamically created
>> guest facing device tree. This allows us to expose eTSEC devices into guests
>> without changes in the machine file.
>>
>> Because we can now tell the guest about eTSEC devices this patch allows the
>> user to specify eTSEC devices via -device at all.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ppc/e500.c | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index bf704b0..bebff6f 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -37,6 +37,7 @@
>> #include "qemu/host-utils.h"
>> #include "hw/pci-host/ppce500.h"
>> #include "qemu/error-report.h"
>> +#include "hw/net/fsl_etsec/etsec.h"
>>
>> #define EPAPR_MAGIC (0x45504150)
>> #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"
>> @@ -139,6 +140,34 @@ typedef struct PlatformDevtreeData {
>> int id;
>> } PlatformDevtreeData;
>>
>> +static int create_devtree_etsec(eTSEC *etsec, PlatformDevtreeData *data)
>> +{
>> + SysBusDevice *sbdev = &etsec->busdev;
>> + gchar *node = g_strdup_printf("/platform/ethernet@%d", data->id);
> The unit address is supposed to match reg. It's not an arbitrary
> disambiguator.
So what do we do in case we don't have any reg, but only an IRQ line? Oh
well - I guess we can cross that line when we get to it.
>
>> + gchar *group = g_strdup_printf("%s/queue-group", node);
>> + void *fdt = data->fdt;
>> +
>> + qemu_fdt_add_subnode(fdt, node);
>> + qemu_fdt_setprop_string(fdt, node, "device_type", "network");
>> + qemu_fdt_setprop_string(fdt, node, "compatible", "fsl,etsec2");
>> + qemu_fdt_setprop_string(fdt, node, "model", "eTSEC");
>> + qemu_fdt_setprop(fdt, node, "local-mac-address", etsec->conf.macaddr.a, 6);
>> + qemu_fdt_setprop_cells(fdt, node, "fixed-link", 0, 1, 1000, 0, 0);
>> +
>> + qemu_fdt_add_subnode(fdt, group);
>> + qemu_fdt_setprop_cells(fdt, group, "reg", sbdev->user_mmios[0], 0x1000);
>> + qemu_fdt_setprop_phandle(fdt, group, "interrupt-parent", data->mpic);
> Why not do interrupt-parent in the parent node, or top of tree?
Parent sounds appealing :). In fact, it's already there - this copy is
simply useless.
>
>> + qemu_fdt_setprop_cells(fdt, group, "interrupts",
>> + data->irq_start + sbdev->user_irqs[0], 0x0,
>> + data->irq_start + sbdev->user_irqs[1], 0x0,
>> + data->irq_start + sbdev->user_irqs[2], 0x0);
> Are we still using two-cell interrupt specifiers? If so, we should
> switch before the assumption gets encoded into random device files.
Random device files should never get any device tree bits encoded.
Device tree generation is responsibility of the machine file.
So we can easily convert the whole thing to the 4-cell when we start to
support different interrupt types :)
> Also, why are these interrupts edge triggered?
Good catch - they're 0x2 on real hardware.
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree
2014-07-02 17:24 ` Alexander Graf
@ 2014-07-02 17:32 ` Scott Wood
2014-07-02 17:34 ` Alexander Graf
0 siblings, 1 reply; 33+ messages in thread
From: Scott Wood @ 2014-07-02 17:32 UTC (permalink / raw)
To: Alexander Graf
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On Wed, 2014-07-02 at 19:24 +0200, Alexander Graf wrote:
> On 02.07.14 00:56, Scott Wood wrote:
> > On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote:
> >> This patch adds support to expose eTSEC devices in the dynamically created
> >> guest facing device tree. This allows us to expose eTSEC devices into guests
> >> without changes in the machine file.
> >>
> >> Because we can now tell the guest about eTSEC devices this patch allows the
> >> user to specify eTSEC devices via -device at all.
> >>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> hw/ppc/e500.c | 34 ++++++++++++++++++++++++++++++++++
> >> 1 file changed, 34 insertions(+)
> >>
> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> >> index bf704b0..bebff6f 100644
> >> --- a/hw/ppc/e500.c
> >> +++ b/hw/ppc/e500.c
> >> @@ -37,6 +37,7 @@
> >> #include "qemu/host-utils.h"
> >> #include "hw/pci-host/ppce500.h"
> >> #include "qemu/error-report.h"
> >> +#include "hw/net/fsl_etsec/etsec.h"
> >>
> >> #define EPAPR_MAGIC (0x45504150)
> >> #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"
> >> @@ -139,6 +140,34 @@ typedef struct PlatformDevtreeData {
> >> int id;
> >> } PlatformDevtreeData;
> >>
> >> +static int create_devtree_etsec(eTSEC *etsec, PlatformDevtreeData *data)
> >> +{
> >> + SysBusDevice *sbdev = &etsec->busdev;
> >> + gchar *node = g_strdup_printf("/platform/ethernet@%d", data->id);
> > The unit address is supposed to match reg. It's not an arbitrary
> > disambiguator.
>
> So what do we do in case we don't have any reg, but only an IRQ line? Oh
> well - I guess we can cross that line when we get to it.
To be theoretically correct (i.e. something that wouldn't break if used
in a real Open Firmware) you'd either leave out the unit address and put
the disambiguation directly in the name, or have a zero-length reg that
corresponds to ranges or a child node's reg.
If you just want to match what we currently do in the real hardware fdt,
use the reg of the first group node.
> >> + qemu_fdt_setprop_cells(fdt, group, "interrupts",
> >> + data->irq_start + sbdev->user_irqs[0], 0x0,
> >> + data->irq_start + sbdev->user_irqs[1], 0x0,
> >> + data->irq_start + sbdev->user_irqs[2], 0x0);
> > Are we still using two-cell interrupt specifiers? If so, we should
> > switch before the assumption gets encoded into random device files.
>
> Random device files should never get any device tree bits encoded.
> Device tree generation is responsibility of the machine file.
Sigh. I missed that this is in e500.c rather than the eTSEC file. This
approach will not scale if we ever have multiple platforms wanting to
create device trees with overlap in the devices they want to describe.
-Scott
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree
2014-07-02 17:32 ` Scott Wood
@ 2014-07-02 17:34 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-07-02 17:34 UTC (permalink / raw)
To: Scott Wood
Cc: peter.maydell, peter.crosthwaite, eric.auger, qemu-devel,
qemu-ppc, sean.stalley, pbonzini, afaerber
On 02.07.14 19:32, Scott Wood wrote:
> On Wed, 2014-07-02 at 19:24 +0200, Alexander Graf wrote:
>> On 02.07.14 00:56, Scott Wood wrote:
>>> On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote:
>>>> This patch adds support to expose eTSEC devices in the dynamically created
>>>> guest facing device tree. This allows us to expose eTSEC devices into guests
>>>> without changes in the machine file.
>>>>
>>>> Because we can now tell the guest about eTSEC devices this patch allows the
>>>> user to specify eTSEC devices via -device at all.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> hw/ppc/e500.c | 34 ++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>>> index bf704b0..bebff6f 100644
>>>> --- a/hw/ppc/e500.c
>>>> +++ b/hw/ppc/e500.c
>>>> @@ -37,6 +37,7 @@
>>>> #include "qemu/host-utils.h"
>>>> #include "hw/pci-host/ppce500.h"
>>>> #include "qemu/error-report.h"
>>>> +#include "hw/net/fsl_etsec/etsec.h"
>>>>
>>>> #define EPAPR_MAGIC (0x45504150)
>>>> #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"
>>>> @@ -139,6 +140,34 @@ typedef struct PlatformDevtreeData {
>>>> int id;
>>>> } PlatformDevtreeData;
>>>>
>>>> +static int create_devtree_etsec(eTSEC *etsec, PlatformDevtreeData *data)
>>>> +{
>>>> + SysBusDevice *sbdev = &etsec->busdev;
>>>> + gchar *node = g_strdup_printf("/platform/ethernet@%d", data->id);
>>> The unit address is supposed to match reg. It's not an arbitrary
>>> disambiguator.
>> So what do we do in case we don't have any reg, but only an IRQ line? Oh
>> well - I guess we can cross that line when we get to it.
> To be theoretically correct (i.e. something that wouldn't break if used
> in a real Open Firmware) you'd either leave out the unit address and put
> the disambiguation directly in the name, or have a zero-length reg that
> corresponds to ranges or a child node's reg.
>
> If you just want to match what we currently do in the real hardware fdt,
> use the reg of the first group node.
>
>>>> + qemu_fdt_setprop_cells(fdt, group, "interrupts",
>>>> + data->irq_start + sbdev->user_irqs[0], 0x0,
>>>> + data->irq_start + sbdev->user_irqs[1], 0x0,
>>>> + data->irq_start + sbdev->user_irqs[2], 0x0);
>>> Are we still using two-cell interrupt specifiers? If so, we should
>>> switch before the assumption gets encoded into random device files.
>> Random device files should never get any device tree bits encoded.
>> Device tree generation is responsibility of the machine file.
> Sigh. I missed that this is in e500.c rather than the eTSEC file. This
> approach will not scale if we ever have multiple platforms wanting to
> create device trees with overlap in the devices they want to describe.
It has to - device trees differ too much between architectures (and
potentially even machines) to have them reasonably live in device files.
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread