[Qemu-arm] [PATCH 0/3] Allow int props with no default, use them in ARM

[Qemu-arm] [PATCH 0/3] Allow int props with no default, use them in ARM

[Peter Maydell]

This patchset follows up on a suggestion by Markus for
allowing devices to implement integer properties which
don't have a default value specified in their Property
struct.

My use case for this is ARM, where we have a property
pmsav7-dregion whose correct default value depends on
which exact ARM CPU this is. That means we can't have
a single constant default value specified in the
DEFINE_PROP_UINT32(), but on the other hand if we just
prevent the qdev prop system from setting a default itself
we can naturally get the right default value by having
the structure field be correctly set to the default for
the CPU in the first place.

Patch 1 cleans up a corner case where we had just one
Property struct which was using the default value
(because it has a PropertyInfo with a non-NULL set_default_value
field) but not explicitly setting .defval.[ui].

Patch 2 adds .set_default to Property, uses it as the gate
on calling .set_default_value rather than just whether
.set_default_value is non-NULL, and adds some macros
so that devices can use the new no-default-value int props.

Patch 3 uses this so M3 and M4 have the right default
for the number of PMSA regions rather than getting the
different R7 default.

thanks
-- PMM

Peter Maydell (3):
  qdev-properties.h: Explicitly set the default value for arraylen
    properties
  qdev: support properties which don't set a default value
  target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

 include/hw/qdev-core.h       |  1 +
 include/hw/qdev-properties.h | 21 +++++++++++++++++++++
 hw/core/qdev.c               |  2 +-
 target/arm/cpu.c             | 12 +++++++++++-
 4 files changed, 34 insertions(+), 2 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 0/3] Allow int props with no default, use them in ARM

[Peter Maydell]

This patchset follows up on a suggestion by Markus for
allowing devices to implement integer properties which
don't have a default value specified in their Property
struct.

My use case for this is ARM, where we have a property
pmsav7-dregion whose correct default value depends on
which exact ARM CPU this is. That means we can't have
a single constant default value specified in the
DEFINE_PROP_UINT32(), but on the other hand if we just
prevent the qdev prop system from setting a default itself
we can naturally get the right default value by having
the structure field be correctly set to the default for
the CPU in the first place.

Patch 1 cleans up a corner case where we had just one
Property struct which was using the default value
(because it has a PropertyInfo with a non-NULL set_default_value
field) but not explicitly setting .defval.[ui].

Patch 2 adds .set_default to Property, uses it as the gate
on calling .set_default_value rather than just whether
.set_default_value is non-NULL, and adds some macros
so that devices can use the new no-default-value int props.

Patch 3 uses this so M3 and M4 have the right default
for the number of PMSA regions rather than getting the
different R7 default.

thanks
-- PMM

Peter Maydell (3):
  qdev-properties.h: Explicitly set the default value for arraylen
    properties
  qdev: support properties which don't set a default value
  target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

 include/hw/qdev-core.h       |  1 +
 include/hw/qdev-properties.h | 21 +++++++++++++++++++++
 hw/core/qdev.c               |  2 +-
 target/arm/cpu.c             | 12 +++++++++++-
 4 files changed, 34 insertions(+), 2 deletions(-)

-- 
2.7.4

[Qemu-arm] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Peter Maydell]

In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
which has a .set_default_value member we will set the field to a default
value. That default value will be zero, by the C rule that struct
initialization sets unmentioned members to zero if at least one member
is initialized. However it's clearer to state it explicitly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
If my eyeball of the sources is correct, this is the only case we
have of a Property struct that uses a PropertyInfo with a non-NULL
.set_default_value but which doesn't explicitly set .defval.u.
---
 include/hw/qdev-properties.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0604c33..36d040c 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
                           _arrayfield, _arrayprop, _arraytype) {        \
         .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
         .info = &(qdev_prop_arraylen),                                  \
+        .defval.u = 0,                                                  \
         .offset = offsetof(_state, _field)                              \
             + type_check(uint32_t, typeof_field(_state, _field)),       \
         .arrayinfo = &(_arrayprop),                                     \
-- 
2.7.4

[Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Peter Maydell]

In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
which has a .set_default_value member we will set the field to a default
value. That default value will be zero, by the C rule that struct
initialization sets unmentioned members to zero if at least one member
is initialized. However it's clearer to state it explicitly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
If my eyeball of the sources is correct, this is the only case we
have of a Property struct that uses a PropertyInfo with a non-NULL
.set_default_value but which doesn't explicitly set .defval.u.
---
 include/hw/qdev-properties.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0604c33..36d040c 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
                           _arrayfield, _arrayprop, _arraytype) {        \
         .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
         .info = &(qdev_prop_arraylen),                                  \
+        .defval.u = 0,                                                  \
         .offset = offsetof(_state, _field)                              \
             + type_check(uint32_t, typeof_field(_state, _field)),       \
         .arrayinfo = &(_arrayprop),                                     \
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Marc-André Lureau]

----- Original Message -----
> In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
> which has a .set_default_value member we will set the field to a default
> value. That default value will be zero, by the C rule that struct
> initialization sets unmentioned members to zero if at least one member
> is initialized. However it's clearer to state it explicitly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
> If my eyeball of the sources is correct, this is the only case we
> have of a Property struct that uses a PropertyInfo with a non-NULL
> .set_default_value but which doesn't explicitly set .defval.u.
> ---
>  include/hw/qdev-properties.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0604c33..36d040c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
>          .arrayinfo = &(_arrayprop),                                     \
> --
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Marc-André Lureau]

----- Original Message -----
> In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
> which has a .set_default_value member we will set the field to a default
> value. That default value will be zero, by the C rule that struct
> initialization sets unmentioned members to zero if at least one member
> is initialized. However it's clearer to state it explicitly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
> If my eyeball of the sources is correct, this is the only case we
> have of a Property struct that uses a PropertyInfo with a non-NULL
> .set_default_value but which doesn't explicitly set .defval.u.
> ---
>  include/hw/qdev-properties.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0604c33..36d040c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
>          .arrayinfo = &(_arrayprop),                                     \
> --
> 2.7.4
> 
> 

Re: [Qemu-arm] [Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
> which has a .set_default_value member we will set the field to a default
> value. That default value will be zero, by the C rule that struct
> initialization sets unmentioned members to zero if at least one member
> is initialized. However it's clearer to state it explicitly.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> If my eyeball of the sources is correct, this is the only case we
> have of a Property struct that uses a PropertyInfo with a non-NULL
> .set_default_value but which doesn't explicitly set .defval.u.
> ---
>  include/hw/qdev-properties.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0604c33..36d040c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
>          .arrayinfo = &(_arrayprop),                                     \

Hmm.

.info and .defval apply to the array *length*.
.info.set_default_value() is set_default_uint(), which uses .defval.u.
Setting it here is correct.

The array *elements* are described by .arrayinfo.  There is no
.arraydefval, but that's okay, because .arrayinfo.set_default_value()
doesn't get called.  Correct?

Re: [Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
> which has a .set_default_value member we will set the field to a default
> value. That default value will be zero, by the C rule that struct
> initialization sets unmentioned members to zero if at least one member
> is initialized. However it's clearer to state it explicitly.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> If my eyeball of the sources is correct, this is the only case we
> have of a Property struct that uses a PropertyInfo with a non-NULL
> .set_default_value but which doesn't explicitly set .defval.u.
> ---
>  include/hw/qdev-properties.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0604c33..36d040c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
>          .arrayinfo = &(_arrayprop),                                     \

Hmm.

.info and .defval apply to the array *length*.
.info.set_default_value() is set_default_uint(), which uses .defval.u.
Setting it here is correct.

The array *elements* are described by .arrayinfo.  There is no
.arraydefval, but that's okay, because .arrayinfo.set_default_value()
doesn't get called.  Correct?

Re: [Qemu-arm] [Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Peter Maydell]

On 13 July 2017 at 15:11, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
>> which has a .set_default_value member we will set the field to a default
>> value. That default value will be zero, by the C rule that struct
>> initialization sets unmentioned members to zero if at least one member
>> is initialized. However it's clearer to state it explicitly.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> If my eyeball of the sources is correct, this is the only case we
>> have of a Property struct that uses a PropertyInfo with a non-NULL
>> .set_default_value but which doesn't explicitly set .defval.u.
>> ---
>>  include/hw/qdev-properties.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 0604c33..36d040c 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
>>                            _arrayfield, _arrayprop, _arraytype) {        \
>>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>>          .info = &(qdev_prop_arraylen),                                  \
>> +        .defval.u = 0,                                                  \
>>          .offset = offsetof(_state, _field)                              \
>>              + type_check(uint32_t, typeof_field(_state, _field)),       \
>>          .arrayinfo = &(_arrayprop),                                     \
>
> Hmm.
>
> .info and .defval apply to the array *length*.
> .info.set_default_value() is set_default_uint(), which uses .defval.u.
> Setting it here is correct.
>
> The array *elements* are described by .arrayinfo.  There is no
> .arraydefval, but that's okay, because .arrayinfo.set_default_value()
> doesn't get called.  Correct?

Correct. (There's no way to specify a default value for array properties.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Peter Maydell]

On 13 July 2017 at 15:11, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
>> which has a .set_default_value member we will set the field to a default
>> value. That default value will be zero, by the C rule that struct
>> initialization sets unmentioned members to zero if at least one member
>> is initialized. However it's clearer to state it explicitly.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> If my eyeball of the sources is correct, this is the only case we
>> have of a Property struct that uses a PropertyInfo with a non-NULL
>> .set_default_value but which doesn't explicitly set .defval.u.
>> ---
>>  include/hw/qdev-properties.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 0604c33..36d040c 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
>>                            _arrayfield, _arrayprop, _arraytype) {        \
>>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>>          .info = &(qdev_prop_arraylen),                                  \
>> +        .defval.u = 0,                                                  \
>>          .offset = offsetof(_state, _field)                              \
>>              + type_check(uint32_t, typeof_field(_state, _field)),       \
>>          .arrayinfo = &(_arrayprop),                                     \
>
> Hmm.
>
> .info and .defval apply to the array *length*.
> .info.set_default_value() is set_default_uint(), which uses .defval.u.
> Setting it here is correct.
>
> The array *elements* are described by .arrayinfo.  There is no
> .arraydefval, but that's okay, because .arrayinfo.set_default_value()
> doesn't get called.  Correct?

Correct. (There's no way to specify a default value for array properties.)

thanks
-- PMM

Re: [Qemu-arm] [Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 July 2017 at 15:11, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
>>> which has a .set_default_value member we will set the field to a default
>>> value. That default value will be zero, by the C rule that struct
>>> initialization sets unmentioned members to zero if at least one member
>>> is initialized. However it's clearer to state it explicitly.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> If my eyeball of the sources is correct, this is the only case we
>>> have of a Property struct that uses a PropertyInfo with a non-NULL
>>> .set_default_value but which doesn't explicitly set .defval.u.
>>> ---
>>>  include/hw/qdev-properties.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>>> index 0604c33..36d040c 100644
>>> --- a/include/hw/qdev-properties.h
>>> +++ b/include/hw/qdev-properties.h
>>> @@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
>>>                            _arrayfield, _arrayprop, _arraytype) {        \
>>>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>>>          .info = &(qdev_prop_arraylen),                                  \
>>> +        .defval.u = 0,                                                  \
>>>          .offset = offsetof(_state, _field)                              \
>>>              + type_check(uint32_t, typeof_field(_state, _field)),       \
>>>          .arrayinfo = &(_arrayprop),                                     \
>>
>> Hmm.
>>
>> .info and .defval apply to the array *length*.
>> .info.set_default_value() is set_default_uint(), which uses .defval.u.
>> Setting it here is correct.
>>
>> The array *elements* are described by .arrayinfo.  There is no
>> .arraydefval, but that's okay, because .arrayinfo.set_default_value()
>> doesn't get called.  Correct?
>
> Correct. (There's no way to specify a default value for array properties.)

Thanks!

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] qdev-properties.h: Explicitly set the default value for arraylen properties

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 July 2017 at 15:11, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> In DEFINE_PROP_ARRAY, because we use a PropertyInfo (qdev_prop_arraylen)
>>> which has a .set_default_value member we will set the field to a default
>>> value. That default value will be zero, by the C rule that struct
>>> initialization sets unmentioned members to zero if at least one member
>>> is initialized. However it's clearer to state it explicitly.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> If my eyeball of the sources is correct, this is the only case we
>>> have of a Property struct that uses a PropertyInfo with a non-NULL
>>> .set_default_value but which doesn't explicitly set .defval.u.
>>> ---
>>>  include/hw/qdev-properties.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>>> index 0604c33..36d040c 100644
>>> --- a/include/hw/qdev-properties.h
>>> +++ b/include/hw/qdev-properties.h
>>> @@ -110,6 +110,7 @@ extern PropertyInfo qdev_prop_arraylen;
>>>                            _arrayfield, _arrayprop, _arraytype) {        \
>>>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>>>          .info = &(qdev_prop_arraylen),                                  \
>>> +        .defval.u = 0,                                                  \
>>>          .offset = offsetof(_state, _field)                              \
>>>              + type_check(uint32_t, typeof_field(_state, _field)),       \
>>>          .arrayinfo = &(_arrayprop),                                     \
>>
>> Hmm.
>>
>> .info and .defval apply to the array *length*.
>> .info.set_default_value() is set_default_uint(), which uses .defval.u.
>> Setting it here is correct.
>>
>> The array *elements* are described by .arrayinfo.  There is no
>> .arraydefval, but that's okay, because .arrayinfo.set_default_value()
>> doesn't get called.  Correct?
>
> Correct. (There's no way to specify a default value for array properties.)

Thanks!

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-arm] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

In some situations it's useful to have a qdev property which doesn't
automatically set its default value when qdev_property_add_static is
called (for instance when the default value is not constant).

Support this by adding a flag to the Property struct indicating
whether to set the default value.  This replaces the existing test
for whether the PorpertyInfo set_default_value function pointer is
NULL, and we set the .set_default field to true for all those cases
of struct Property which use a PropertyInfo with a non-NULL
set_default_value, so behaviour remains the same as before.

We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
of wanting to set an integer property with no default value.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Since the previous patch fixed the only case that was
using a default value but not explicitly setting .defval.[ui],
this patch is a bit easier to review: .set_default = true
is added in exactly the places that initialize .defval.[ui].
---
 include/hw/qdev-core.h       |  1 +
 include/hw/qdev-properties.h | 20 ++++++++++++++++++++
 hw/core/qdev.c               |  2 +-
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 9d7c1c0..d110163 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -226,6 +226,7 @@ struct Property {
     PropertyInfo *info;
     ptrdiff_t    offset;
     uint8_t      bitnr;
+    bool         set_default;
     union {
         int64_t i;
         uint64_t u;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 36d040c..66816a5 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
         .info      = &(_prop),                                          \
         .offset    = offsetof(_state, _field)                           \
             + type_check(_type,typeof_field(_state, _field)),           \
+        .set_default = true,                                            \
         .defval.i  = (_type)_defval,                                    \
         }
 
+#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \
+        .name      = (_name),                                           \
+        .info      = &(_prop),                                          \
+        .offset    = offsetof(_state, _field)                           \
+            + type_check(_type, typeof_field(_state, _field)),          \
+        }
+
 #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
         .name      = (_name),                                    \
         .info      = &(qdev_prop_bit),                           \
         .bitnr    = (_bit),                                      \
         .offset    = offsetof(_state, _field)                    \
             + type_check(uint32_t,typeof_field(_state, _field)), \
+        .set_default = true,                                     \
         .defval.u  = (bool)_defval,                              \
         }
 
@@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen;
         .info      = &(_prop),                                          \
         .offset    = offsetof(_state, _field)                           \
             + type_check(_type, typeof_field(_state, _field)),          \
+        .set_default = true,                                            \
         .defval.u  = (_type)_defval,                                    \
         }
 
+#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \
+        .name      = (_name),                                           \
+        .info      = &(_prop),                                          \
+        .offset    = offsetof(_state, _field)                           \
+            + type_check(_type, typeof_field(_state, _field)),          \
+        }
+
 #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
         .name      = (_name),                                           \
         .info      = &(qdev_prop_bit64),                                \
         .bitnr    = (_bit),                                             \
         .offset    = offsetof(_state, _field)                           \
             + type_check(uint64_t, typeof_field(_state, _field)),       \
+        .set_default = true,                                            \
         .defval.u  = (bool)_defval,                                     \
         }
 
@@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen;
         .info      = &(qdev_prop_bool),                          \
         .offset    = offsetof(_state, _field)                    \
             + type_check(bool, typeof_field(_state, _field)),    \
+        .set_default = true,                                     \
         .defval.u    = (bool)_defval,                            \
         }
 
@@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen;
                           _arrayfield, _arrayprop, _arraytype) {        \
         .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
         .info = &(qdev_prop_arraylen),                                  \
+        .set_default = true,                                            \
         .defval.u = 0,                                                  \
         .offset = offsetof(_state, _field)                              \
             + type_check(uint32_t, typeof_field(_state, _field)),       \
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 849952a..96965a7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
                                     prop->info->description,
                                     &error_abort);
 
-    if (prop->info->set_default_value) {
+    if (prop->set_default) {
         prop->info->set_default_value(obj, prop);
     }
 }
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

In some situations it's useful to have a qdev property which doesn't
automatically set its default value when qdev_property_add_static is
called (for instance when the default value is not constant).

Support this by adding a flag to the Property struct indicating
whether to set the default value.  This replaces the existing test
for whether the PorpertyInfo set_default_value function pointer is
NULL, and we set the .set_default field to true for all those cases
of struct Property which use a PropertyInfo with a non-NULL
set_default_value, so behaviour remains the same as before.

We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
of wanting to set an integer property with no default value.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Since the previous patch fixed the only case that was
using a default value but not explicitly setting .defval.[ui],
this patch is a bit easier to review: .set_default = true
is added in exactly the places that initialize .defval.[ui].
---
 include/hw/qdev-core.h       |  1 +
 include/hw/qdev-properties.h | 20 ++++++++++++++++++++
 hw/core/qdev.c               |  2 +-
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 9d7c1c0..d110163 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -226,6 +226,7 @@ struct Property {
     PropertyInfo *info;
     ptrdiff_t    offset;
     uint8_t      bitnr;
+    bool         set_default;
     union {
         int64_t i;
         uint64_t u;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 36d040c..66816a5 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
         .info      = &(_prop),                                          \
         .offset    = offsetof(_state, _field)                           \
             + type_check(_type,typeof_field(_state, _field)),           \
+        .set_default = true,                                            \
         .defval.i  = (_type)_defval,                                    \
         }
 
+#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \
+        .name      = (_name),                                           \
+        .info      = &(_prop),                                          \
+        .offset    = offsetof(_state, _field)                           \
+            + type_check(_type, typeof_field(_state, _field)),          \
+        }
+
 #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
         .name      = (_name),                                    \
         .info      = &(qdev_prop_bit),                           \
         .bitnr    = (_bit),                                      \
         .offset    = offsetof(_state, _field)                    \
             + type_check(uint32_t,typeof_field(_state, _field)), \
+        .set_default = true,                                     \
         .defval.u  = (bool)_defval,                              \
         }
 
@@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen;
         .info      = &(_prop),                                          \
         .offset    = offsetof(_state, _field)                           \
             + type_check(_type, typeof_field(_state, _field)),          \
+        .set_default = true,                                            \
         .defval.u  = (_type)_defval,                                    \
         }
 
+#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \
+        .name      = (_name),                                           \
+        .info      = &(_prop),                                          \
+        .offset    = offsetof(_state, _field)                           \
+            + type_check(_type, typeof_field(_state, _field)),          \
+        }
+
 #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
         .name      = (_name),                                           \
         .info      = &(qdev_prop_bit64),                                \
         .bitnr    = (_bit),                                             \
         .offset    = offsetof(_state, _field)                           \
             + type_check(uint64_t, typeof_field(_state, _field)),       \
+        .set_default = true,                                            \
         .defval.u  = (bool)_defval,                                     \
         }
 
@@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen;
         .info      = &(qdev_prop_bool),                          \
         .offset    = offsetof(_state, _field)                    \
             + type_check(bool, typeof_field(_state, _field)),    \
+        .set_default = true,                                     \
         .defval.u    = (bool)_defval,                            \
         }
 
@@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen;
                           _arrayfield, _arrayprop, _arraytype) {        \
         .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
         .info = &(qdev_prop_arraylen),                                  \
+        .set_default = true,                                            \
         .defval.u = 0,                                                  \
         .offset = offsetof(_state, _field)                              \
             + type_check(uint32_t, typeof_field(_state, _field)),       \
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 849952a..96965a7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
                                     prop->info->description,
                                     &error_abort);
 
-    if (prop->info->set_default_value) {
+    if (prop->set_default) {
         prop->info->set_default_value(obj, prop);
     }
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Marc-André Lureau]

Hi

----- Original Message -----
> In some situations it's useful to have a qdev property which doesn't
> automatically set its default value when qdev_property_add_static is
> called (for instance when the default value is not constant).
> 
> Support this by adding a flag to the Property struct indicating
> whether to set the default value.  This replaces the existing test
> for whether the PorpertyInfo set_default_value function pointer is
> NULL, and we set the .set_default field to true for all those cases
> of struct Property which use a PropertyInfo with a non-NULL
> set_default_value, so behaviour remains the same as before.
> 
> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
> of wanting to set an integer property with no default value.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

minor comment below

> ---
> Since the previous patch fixed the only case that was
> using a default value but not explicitly setting .defval.[ui],
> this patch is a bit easier to review: .set_default = true
> is added in exactly the places that initialize .defval.[ui].
> ---
>  include/hw/qdev-core.h       |  1 +
>  include/hw/qdev-properties.h | 20 ++++++++++++++++++++
>  hw/core/qdev.c               |  2 +-
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9d7c1c0..d110163 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -226,6 +226,7 @@ struct Property {
>      PropertyInfo *info;
>      ptrdiff_t    offset;
>      uint8_t      bitnr;
> +    bool         set_default;
>      union {
>          int64_t i;
>          uint64_t u;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 36d040c..66816a5 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type,typeof_field(_state, _field)),           \
> +        .set_default = true,                                            \
>          .defval.i  = (_type)_defval,                                    \
>          }
>  
> +#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) {
> \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
>          .name      = (_name),                                    \
>          .info      = &(qdev_prop_bit),                           \
>          .bitnr    = (_bit),                                      \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(uint32_t,typeof_field(_state, _field)), \
> +        .set_default = true,                                     \
>          .defval.u  = (bool)_defval,                              \
>          }
>  
> @@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type, typeof_field(_state, _field)),          \
> +        .set_default = true,                                            \
>          .defval.u  = (_type)_defval,                                    \
>          }
>  
> +#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type)
> { \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
>          .name      = (_name),                                           \
>          .info      = &(qdev_prop_bit64),                                \
>          .bitnr    = (_bit),                                             \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(uint64_t, typeof_field(_state, _field)),       \
> +        .set_default = true,                                            \
>          .defval.u  = (bool)_defval,                                     \
>          }
>  
> @@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(qdev_prop_bool),                          \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(bool, typeof_field(_state, _field)),    \
> +        .set_default = true,                                     \
>          .defval.u    = (bool)_defval,                            \
>          }
>  
> @@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .set_default = true,                                            \
>          .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 849952a..96965a7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property
> *prop,
>                                      prop->info->description,
>                                      &error_abort);
>  
> -    if (prop->info->set_default_value) {
> +    if (prop->set_default) {

Not sure if it's worth to have an assert(prop->info->set_default_value), probably not necessary.

>          prop->info->set_default_value(obj, prop);
>      }
>  }
> --
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Marc-André Lureau]

Hi

----- Original Message -----
> In some situations it's useful to have a qdev property which doesn't
> automatically set its default value when qdev_property_add_static is
> called (for instance when the default value is not constant).
> 
> Support this by adding a flag to the Property struct indicating
> whether to set the default value.  This replaces the existing test
> for whether the PorpertyInfo set_default_value function pointer is
> NULL, and we set the .set_default field to true for all those cases
> of struct Property which use a PropertyInfo with a non-NULL
> set_default_value, so behaviour remains the same as before.
> 
> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
> of wanting to set an integer property with no default value.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

minor comment below

> ---
> Since the previous patch fixed the only case that was
> using a default value but not explicitly setting .defval.[ui],
> this patch is a bit easier to review: .set_default = true
> is added in exactly the places that initialize .defval.[ui].
> ---
>  include/hw/qdev-core.h       |  1 +
>  include/hw/qdev-properties.h | 20 ++++++++++++++++++++
>  hw/core/qdev.c               |  2 +-
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9d7c1c0..d110163 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -226,6 +226,7 @@ struct Property {
>      PropertyInfo *info;
>      ptrdiff_t    offset;
>      uint8_t      bitnr;
> +    bool         set_default;
>      union {
>          int64_t i;
>          uint64_t u;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 36d040c..66816a5 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type,typeof_field(_state, _field)),           \
> +        .set_default = true,                                            \
>          .defval.i  = (_type)_defval,                                    \
>          }
>  
> +#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) {
> \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
>          .name      = (_name),                                    \
>          .info      = &(qdev_prop_bit),                           \
>          .bitnr    = (_bit),                                      \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(uint32_t,typeof_field(_state, _field)), \
> +        .set_default = true,                                     \
>          .defval.u  = (bool)_defval,                              \
>          }
>  
> @@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type, typeof_field(_state, _field)),          \
> +        .set_default = true,                                            \
>          .defval.u  = (_type)_defval,                                    \
>          }
>  
> +#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type)
> { \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
>          .name      = (_name),                                           \
>          .info      = &(qdev_prop_bit64),                                \
>          .bitnr    = (_bit),                                             \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(uint64_t, typeof_field(_state, _field)),       \
> +        .set_default = true,                                            \
>          .defval.u  = (bool)_defval,                                     \
>          }
>  
> @@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(qdev_prop_bool),                          \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(bool, typeof_field(_state, _field)),    \
> +        .set_default = true,                                     \
>          .defval.u    = (bool)_defval,                            \
>          }
>  
> @@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .set_default = true,                                            \
>          .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 849952a..96965a7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property
> *prop,
>                                      prop->info->description,
>                                      &error_abort);
>  
> -    if (prop->info->set_default_value) {
> +    if (prop->set_default) {

Not sure if it's worth to have an assert(prop->info->set_default_value), probably not necessary.

>          prop->info->set_default_value(obj, prop);
>      }
>  }
> --
> 2.7.4
> 
> 

Re: [Qemu-arm] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

On 11 July 2017 at 18:15, Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property
>> *prop,
>>                                      prop->info->description,
>>                                      &error_abort);
>>
>> -    if (prop->info->set_default_value) {
>> +    if (prop->set_default) {
>
> Not sure if it's worth to have an assert(prop->info->set_default_value), probably not necessary.
>
>>          prop->info->set_default_value(obj, prop);

Yes, we'll just segfault on the NULL pointer immediately anyway.
I tend to the view that assertions are for turning obscure
and delayed failures into clearer and more immediate
failures, and in this case the failure is already pretty
clear and immediate.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

On 11 July 2017 at 18:15, Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>> @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property
>> *prop,
>>                                      prop->info->description,
>>                                      &error_abort);
>>
>> -    if (prop->info->set_default_value) {
>> +    if (prop->set_default) {
>
> Not sure if it's worth to have an assert(prop->info->set_default_value), probably not necessary.
>
>>          prop->info->set_default_value(obj, prop);

Yes, we'll just segfault on the NULL pointer immediately anyway.
I tend to the view that assertions are for turning obscure
and delayed failures into clearer and more immediate
failures, and in this case the failure is already pretty
clear and immediate.

thanks
-- PMM

Re: [Qemu-arm] [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> In some situations it's useful to have a qdev property which doesn't
> automatically set its default value when qdev_property_add_static is
> called (for instance when the default value is not constant).
>
> Support this by adding a flag to the Property struct indicating
> whether to set the default value.  This replaces the existing test
> for whether the PorpertyInfo set_default_value function pointer is

PropertyInfo

> NULL, and we set the .set_default field to true for all those cases
> of struct Property which use a PropertyInfo with a non-NULL
> set_default_value, so behaviour remains the same as before.
>
> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
> of wanting to set an integer property with no default value.

Your text makes me wonder what is supposed to set the default value
then.  Glancing ahead to PATCH 3, it looks like method instance_init()
is.  Can you think of a scenario where something else sets it?

> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Since the previous patch fixed the only case that was
> using a default value but not explicitly setting .defval.[ui],
> this patch is a bit easier to review: .set_default = true
> is added in exactly the places that initialize .defval.[ui].
> ---
>  include/hw/qdev-core.h       |  1 +
>  include/hw/qdev-properties.h | 20 ++++++++++++++++++++
>  hw/core/qdev.c               |  2 +-
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9d7c1c0..d110163 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -226,6 +226,7 @@ struct Property {
>      PropertyInfo *info;
>      ptrdiff_t    offset;
>      uint8_t      bitnr;
> +    bool         set_default;

Your chance to add the very first comment to struct Property (its
existing undocumentedness is an embarrassment, but not this patch's
problem).  Let's write down that this flag governs where (integer)
default values come from, either from member devfal or from method
instance_init() or whatever.

>      union {
>          int64_t i;
>          uint64_t u;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 36d040c..66816a5 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type,typeof_field(_state, _field)),           \
> +        .set_default = true,                                            \
>          .defval.i  = (_type)_defval,                                    \
>          }

If you flip the sense of the flag, you don't need to mess with the
PropertyInfo, and don't need the note referring reviewers to the
previous patch.  However, the case "use .defval" already requires an
initializer, so adding one for .set_default seems fair.  Okay.

>  
> +#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
>          .name      = (_name),                                    \
>          .info      = &(qdev_prop_bit),                           \
>          .bitnr    = (_bit),                                      \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(uint32_t,typeof_field(_state, _field)), \
> +        .set_default = true,                                     \
>          .defval.u  = (bool)_defval,                              \
>          }
>  
> @@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type, typeof_field(_state, _field)),          \
> +        .set_default = true,                                            \
>          .defval.u  = (_type)_defval,                                    \
>          }
>  
> +#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
>          .name      = (_name),                                           \
>          .info      = &(qdev_prop_bit64),                                \
>          .bitnr    = (_bit),                                             \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(uint64_t, typeof_field(_state, _field)),       \
> +        .set_default = true,                                            \
>          .defval.u  = (bool)_defval,                                     \
>          }
>  
> @@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(qdev_prop_bool),                          \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(bool, typeof_field(_state, _field)),    \
> +        .set_default = true,                                     \
>          .defval.u    = (bool)_defval,                            \
>          }
>  
> @@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .set_default = true,                                            \
>          .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 849952a..96965a7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
>                                      prop->info->description,
>                                      &error_abort);
>  
> -    if (prop->info->set_default_value) {
> +    if (prop->set_default) {
>          prop->info->set_default_value(obj, prop);
>      }
>  }

Preferably with a suitable comment on member set_default and an improved
commit message:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> In some situations it's useful to have a qdev property which doesn't
> automatically set its default value when qdev_property_add_static is
> called (for instance when the default value is not constant).
>
> Support this by adding a flag to the Property struct indicating
> whether to set the default value.  This replaces the existing test
> for whether the PorpertyInfo set_default_value function pointer is

PropertyInfo

> NULL, and we set the .set_default field to true for all those cases
> of struct Property which use a PropertyInfo with a non-NULL
> set_default_value, so behaviour remains the same as before.
>
> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
> of wanting to set an integer property with no default value.

Your text makes me wonder what is supposed to set the default value
then.  Glancing ahead to PATCH 3, it looks like method instance_init()
is.  Can you think of a scenario where something else sets it?

> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Since the previous patch fixed the only case that was
> using a default value but not explicitly setting .defval.[ui],
> this patch is a bit easier to review: .set_default = true
> is added in exactly the places that initialize .defval.[ui].
> ---
>  include/hw/qdev-core.h       |  1 +
>  include/hw/qdev-properties.h | 20 ++++++++++++++++++++
>  hw/core/qdev.c               |  2 +-
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9d7c1c0..d110163 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -226,6 +226,7 @@ struct Property {
>      PropertyInfo *info;
>      ptrdiff_t    offset;
>      uint8_t      bitnr;
> +    bool         set_default;

Your chance to add the very first comment to struct Property (its
existing undocumentedness is an embarrassment, but not this patch's
problem).  Let's write down that this flag governs where (integer)
default values come from, either from member devfal or from method
instance_init() or whatever.

>      union {
>          int64_t i;
>          uint64_t u;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 36d040c..66816a5 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type,typeof_field(_state, _field)),           \
> +        .set_default = true,                                            \
>          .defval.i  = (_type)_defval,                                    \
>          }

If you flip the sense of the flag, you don't need to mess with the
PropertyInfo, and don't need the note referring reviewers to the
previous patch.  However, the case "use .defval" already requires an
initializer, so adding one for .set_default seems fair.  Okay.

>  
> +#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
>          .name      = (_name),                                    \
>          .info      = &(qdev_prop_bit),                           \
>          .bitnr    = (_bit),                                      \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(uint32_t,typeof_field(_state, _field)), \
> +        .set_default = true,                                     \
>          .defval.u  = (bool)_defval,                              \
>          }
>  
> @@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type, typeof_field(_state, _field)),          \
> +        .set_default = true,                                            \
>          .defval.u  = (_type)_defval,                                    \
>          }
>  
> +#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
>          .name      = (_name),                                           \
>          .info      = &(qdev_prop_bit64),                                \
>          .bitnr    = (_bit),                                             \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(uint64_t, typeof_field(_state, _field)),       \
> +        .set_default = true,                                            \
>          .defval.u  = (bool)_defval,                                     \
>          }
>  
> @@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(qdev_prop_bool),                          \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(bool, typeof_field(_state, _field)),    \
> +        .set_default = true,                                     \
>          .defval.u    = (bool)_defval,                            \
>          }
>  
> @@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .set_default = true,                                            \
>          .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 849952a..96965a7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
>                                      prop->info->description,
>                                      &error_abort);
>  
> -    if (prop->info->set_default_value) {
> +    if (prop->set_default) {
>          prop->info->set_default_value(obj, prop);
>      }
>  }

Preferably with a suitable comment on member set_default and an improved
commit message:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> In some situations it's useful to have a qdev property which doesn't
>> automatically set its default value when qdev_property_add_static is
>> called (for instance when the default value is not constant).
>>
>> Support this by adding a flag to the Property struct indicating
>> whether to set the default value.  This replaces the existing test
>> for whether the PorpertyInfo set_default_value function pointer is
>
> PropertyInfo
>
>> NULL, and we set the .set_default field to true for all those cases
>> of struct Property which use a PropertyInfo with a non-NULL
>> set_default_value, so behaviour remains the same as before.
>>
>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>> of wanting to set an integer property with no default value.
>
> Your text makes me wonder what is supposed to set the default value
> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
> is.  Can you think of a scenario where something else sets it?

Yes, instance_init is the obvious place (though in fact pretty
much anything that runs before the user of the device sets
properties will do).

>>      union {
>>          int64_t i;
>>          uint64_t u;
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 36d040c..66816a5 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
>>          .info      = &(_prop),                                          \
>>          .offset    = offsetof(_state, _field)                           \
>>              + type_check(_type,typeof_field(_state, _field)),           \
>> +        .set_default = true,                                            \
>>          .defval.i  = (_type)_defval,                                    \
>>          }
>
> If you flip the sense of the flag, you don't need to mess with the
> PropertyInfo, and don't need the note referring reviewers to the
> previous patch.  However, the case "use .defval" already requires an
> initializer, so adding one for .set_default seems fair.  Okay.

Having the flag this way round was your idea :-)
Putting it the other way round might have been a smaller change
in some sense (you'd have had to retain the check on
set_default_value being NULL), but I do think it's nicer
in the end...

> Preferably with a suitable comment on member set_default and an improved
> commit message:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> In some situations it's useful to have a qdev property which doesn't
>> automatically set its default value when qdev_property_add_static is
>> called (for instance when the default value is not constant).
>>
>> Support this by adding a flag to the Property struct indicating
>> whether to set the default value.  This replaces the existing test
>> for whether the PorpertyInfo set_default_value function pointer is
>
> PropertyInfo
>
>> NULL, and we set the .set_default field to true for all those cases
>> of struct Property which use a PropertyInfo with a non-NULL
>> set_default_value, so behaviour remains the same as before.
>>
>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>> of wanting to set an integer property with no default value.
>
> Your text makes me wonder what is supposed to set the default value
> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
> is.  Can you think of a scenario where something else sets it?

Yes, instance_init is the obvious place (though in fact pretty
much anything that runs before the user of the device sets
properties will do).

>>      union {
>>          int64_t i;
>>          uint64_t u;
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 36d040c..66816a5 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
>>          .info      = &(_prop),                                          \
>>          .offset    = offsetof(_state, _field)                           \
>>              + type_check(_type,typeof_field(_state, _field)),           \
>> +        .set_default = true,                                            \
>>          .defval.i  = (_type)_defval,                                    \
>>          }
>
> If you flip the sense of the flag, you don't need to mess with the
> PropertyInfo, and don't need the note referring reviewers to the
> previous patch.  However, the case "use .defval" already requires an
> initializer, so adding one for .set_default seems fair.  Okay.

Having the flag this way round was your idea :-)
Putting it the other way round might have been a smaller change
in some sense (you'd have had to retain the check on
set_default_value being NULL), but I do think it's nicer
in the end...

> Preferably with a suitable comment on member set_default and an improved
> commit message:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> In some situations it's useful to have a qdev property which doesn't
>> automatically set its default value when qdev_property_add_static is
>> called (for instance when the default value is not constant).
>>
>> Support this by adding a flag to the Property struct indicating
>> whether to set the default value.  This replaces the existing test
>> for whether the PorpertyInfo set_default_value function pointer is
>
> PropertyInfo
>
>> NULL, and we set the .set_default field to true for all those cases
>> of struct Property which use a PropertyInfo with a non-NULL
>> set_default_value, so behaviour remains the same as before.
>>
>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>> of wanting to set an integer property with no default value.
>
> Your text makes me wonder what is supposed to set the default value
> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
> is.  Can you think of a scenario where something else sets it?

OK, proposed extra paragraph for commit message:

This gives us the semantics of:
 * if .set_default is true and .info->set_default_value is not NULL,
   then .defval is used as the the default value of the property
 * otherwise, the property system does not set any default, and
   the field will retain whatever initial value it was given by
   the device's .instance_init method

(to go just before the "We define two new macros..." para.)

>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -226,6 +226,7 @@ struct Property {
>>      PropertyInfo *info;
>>      ptrdiff_t    offset;
>>      uint8_t      bitnr;
>> +    bool         set_default;
>
> Your chance to add the very first comment to struct Property (its
> existing undocumentedness is an embarrassment, but not this patch's
> problem).  Let's write down that this flag governs where (integer)
> default values come from, either from member devfal or from method
> instance_init() or whatever.

OK, how about
/**
 * Property:
 * @set_default: true if the default value should be set from @defval
 *    (if false then no default value is set by the property system
 *     and the field retains whatever value it was given by instance_init).
 * @defval: default value for the property. This is used only if @set_default
 *     is true and @info->set_default_value is not NULL.
 */

?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> In some situations it's useful to have a qdev property which doesn't
>> automatically set its default value when qdev_property_add_static is
>> called (for instance when the default value is not constant).
>>
>> Support this by adding a flag to the Property struct indicating
>> whether to set the default value.  This replaces the existing test
>> for whether the PorpertyInfo set_default_value function pointer is
>
> PropertyInfo
>
>> NULL, and we set the .set_default field to true for all those cases
>> of struct Property which use a PropertyInfo with a non-NULL
>> set_default_value, so behaviour remains the same as before.
>>
>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>> of wanting to set an integer property with no default value.
>
> Your text makes me wonder what is supposed to set the default value
> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
> is.  Can you think of a scenario where something else sets it?

OK, proposed extra paragraph for commit message:

This gives us the semantics of:
 * if .set_default is true and .info->set_default_value is not NULL,
   then .defval is used as the the default value of the property
 * otherwise, the property system does not set any default, and
   the field will retain whatever initial value it was given by
   the device's .instance_init method

(to go just before the "We define two new macros..." para.)

>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -226,6 +226,7 @@ struct Property {
>>      PropertyInfo *info;
>>      ptrdiff_t    offset;
>>      uint8_t      bitnr;
>> +    bool         set_default;
>
> Your chance to add the very first comment to struct Property (its
> existing undocumentedness is an embarrassment, but not this patch's
> problem).  Let's write down that this flag governs where (integer)
> default values come from, either from member devfal or from method
> instance_init() or whatever.

OK, how about
/**
 * Property:
 * @set_default: true if the default value should be set from @defval
 *    (if false then no default value is set by the property system
 *     and the field retains whatever value it was given by instance_init).
 * @defval: default value for the property. This is used only if @set_default
 *     is true and @info->set_default_value is not NULL.
 */

?

thanks
-- PMM

Re: [Qemu-arm] [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> In some situations it's useful to have a qdev property which doesn't
>>> automatically set its default value when qdev_property_add_static is
>>> called (for instance when the default value is not constant).
>>>
>>> Support this by adding a flag to the Property struct indicating
>>> whether to set the default value.  This replaces the existing test
>>> for whether the PorpertyInfo set_default_value function pointer is
>>
>> PropertyInfo
>>
>>> NULL, and we set the .set_default field to true for all those cases
>>> of struct Property which use a PropertyInfo with a non-NULL
>>> set_default_value, so behaviour remains the same as before.
>>>
>>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>>> of wanting to set an integer property with no default value.
>>
>> Your text makes me wonder what is supposed to set the default value
>> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
>> is.  Can you think of a scenario where something else sets it?
>
> OK, proposed extra paragraph for commit message:
>
> This gives us the semantics of:
>  * if .set_default is true and .info->set_default_value is not NULL,
>    then .defval is used as the the default value of the property

If I read your patch correctly, then this should be "if .set_default is
true, then .info->set_default_value() must not be null, and .defval is
used ..."

>  * otherwise, the property system does not set any default, and
>    the field will retain whatever initial value it was given by
>    the device's .instance_init method
>
> (to go just before the "We define two new macros..." para.)
>
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -226,6 +226,7 @@ struct Property {
>>>      PropertyInfo *info;
>>>      ptrdiff_t    offset;
>>>      uint8_t      bitnr;
>>> +    bool         set_default;
>>
>> Your chance to add the very first comment to struct Property (its
>> existing undocumentedness is an embarrassment, but not this patch's
>> problem).  Let's write down that this flag governs where (integer)
>> default values come from, either from member devfal or from method
>> instance_init() or whatever.
>
> OK, how about
> /**
>  * Property:
>  * @set_default: true if the default value should be set from @defval
>  *    (if false then no default value is set by the property system
>  *     and the field retains whatever value it was given by instance_init).
>  * @defval: default value for the property. This is used only if @set_default
>  *     is true and @info->set_default_value is not NULL.
>  */

Again, I believe ->set_default_value() must not be null when
->set_default is true.

>
> ?
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> In some situations it's useful to have a qdev property which doesn't
>>> automatically set its default value when qdev_property_add_static is
>>> called (for instance when the default value is not constant).
>>>
>>> Support this by adding a flag to the Property struct indicating
>>> whether to set the default value.  This replaces the existing test
>>> for whether the PorpertyInfo set_default_value function pointer is
>>
>> PropertyInfo
>>
>>> NULL, and we set the .set_default field to true for all those cases
>>> of struct Property which use a PropertyInfo with a non-NULL
>>> set_default_value, so behaviour remains the same as before.
>>>
>>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>>> of wanting to set an integer property with no default value.
>>
>> Your text makes me wonder what is supposed to set the default value
>> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
>> is.  Can you think of a scenario where something else sets it?
>
> OK, proposed extra paragraph for commit message:
>
> This gives us the semantics of:
>  * if .set_default is true and .info->set_default_value is not NULL,
>    then .defval is used as the the default value of the property

If I read your patch correctly, then this should be "if .set_default is
true, then .info->set_default_value() must not be null, and .defval is
used ..."

>  * otherwise, the property system does not set any default, and
>    the field will retain whatever initial value it was given by
>    the device's .instance_init method
>
> (to go just before the "We define two new macros..." para.)
>
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -226,6 +226,7 @@ struct Property {
>>>      PropertyInfo *info;
>>>      ptrdiff_t    offset;
>>>      uint8_t      bitnr;
>>> +    bool         set_default;
>>
>> Your chance to add the very first comment to struct Property (its
>> existing undocumentedness is an embarrassment, but not this patch's
>> problem).  Let's write down that this flag governs where (integer)
>> default values come from, either from member devfal or from method
>> instance_init() or whatever.
>
> OK, how about
> /**
>  * Property:
>  * @set_default: true if the default value should be set from @defval
>  *    (if false then no default value is set by the property system
>  *     and the field retains whatever value it was given by instance_init).
>  * @defval: default value for the property. This is used only if @set_default
>  *     is true and @info->set_default_value is not NULL.
>  */

Again, I believe ->set_default_value() must not be null when
->set_default is true.

>
> ?
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Peter Maydell]

On 13 July 2017 at 18:10, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> In some situations it's useful to have a qdev property which doesn't
>>>> automatically set its default value when qdev_property_add_static is
>>>> called (for instance when the default value is not constant).
>>>>
>>>> Support this by adding a flag to the Property struct indicating
>>>> whether to set the default value.  This replaces the existing test
>>>> for whether the PorpertyInfo set_default_value function pointer is
>>>
>>> PropertyInfo
>>>
>>>> NULL, and we set the .set_default field to true for all those cases
>>>> of struct Property which use a PropertyInfo with a non-NULL
>>>> set_default_value, so behaviour remains the same as before.
>>>>
>>>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>>>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>>>> of wanting to set an integer property with no default value.
>>>
>>> Your text makes me wonder what is supposed to set the default value
>>> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
>>> is.  Can you think of a scenario where something else sets it?
>>
>> OK, proposed extra paragraph for commit message:
>>
>> This gives us the semantics of:
>>  * if .set_default is true and .info->set_default_value is not NULL,
>>    then .defval is used as the the default value of the property
>
> If I read your patch correctly, then this should be "if .set_default is
> true, then .info->set_default_value() must not be null, and .defval is
> used ..."

Yes.

thanks
-- PMM

Re: [Qemu-arm] [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 July 2017 at 18:10, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> In some situations it's useful to have a qdev property which doesn't
>>>>> automatically set its default value when qdev_property_add_static is
>>>>> called (for instance when the default value is not constant).
>>>>>
>>>>> Support this by adding a flag to the Property struct indicating
>>>>> whether to set the default value.  This replaces the existing test
>>>>> for whether the PorpertyInfo set_default_value function pointer is
>>>>
>>>> PropertyInfo
>>>>
>>>>> NULL, and we set the .set_default field to true for all those cases
>>>>> of struct Property which use a PropertyInfo with a non-NULL
>>>>> set_default_value, so behaviour remains the same as before.
>>>>>
>>>>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>>>>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>>>>> of wanting to set an integer property with no default value.
>>>>
>>>> Your text makes me wonder what is supposed to set the default value
>>>> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
>>>> is.  Can you think of a scenario where something else sets it?
>>>
>>> OK, proposed extra paragraph for commit message:
>>>
>>> This gives us the semantics of:
>>>  * if .set_default is true and .info->set_default_value is not NULL,
>>>    then .defval is used as the the default value of the property
>>
>> If I read your patch correctly, then this should be "if .set_default is
>> true, then .info->set_default_value() must not be null, and .defval is
>> used ..."
>
> Yes.

Update your comment and commit message accordingly, and you may add
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 July 2017 at 18:10, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> In some situations it's useful to have a qdev property which doesn't
>>>>> automatically set its default value when qdev_property_add_static is
>>>>> called (for instance when the default value is not constant).
>>>>>
>>>>> Support this by adding a flag to the Property struct indicating
>>>>> whether to set the default value.  This replaces the existing test
>>>>> for whether the PorpertyInfo set_default_value function pointer is
>>>>
>>>> PropertyInfo
>>>>
>>>>> NULL, and we set the .set_default field to true for all those cases
>>>>> of struct Property which use a PropertyInfo with a non-NULL
>>>>> set_default_value, so behaviour remains the same as before.
>>>>>
>>>>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>>>>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>>>>> of wanting to set an integer property with no default value.
>>>>
>>>> Your text makes me wonder what is supposed to set the default value
>>>> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
>>>> is.  Can you think of a scenario where something else sets it?
>>>
>>> OK, proposed extra paragraph for commit message:
>>>
>>> This gives us the semantics of:
>>>  * if .set_default is true and .info->set_default_value is not NULL,
>>>    then .defval is used as the the default value of the property
>>
>> If I read your patch correctly, then this should be "if .set_default is
>> true, then .info->set_default_value() must not be null, and .defval is
>> used ..."
>
> Yes.

Update your comment and commit message accordingly, and you may add
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-arm] [PATCH 3/3] target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

[Peter Maydell]

The Cortex-M3 and M4 CPUs always have 8 PMSA MPU regions (this isn't
a configurable option for the hardware).  Make the default value of
the pmsav7-dregion property be set per-cpu, so we don't need to have
every user of these CPUs set it manually.  (The existing default of
16 is correct for the other PMSAv7 core, the Cortex-R5.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 28a9141..96d1f84 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -543,8 +543,15 @@ static Property arm_cpu_has_pmu_property =
 static Property arm_cpu_has_mpu_property =
             DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
 
+/* This is like DEFINE_PROP_UINT32 but it doesn't set the default value,
+ * because the CPU initfn will have already set cpu->pmsav7_dregion to
+ * the right value for that particular CPU type, and we don't want
+ * to override that with an incorrect constant value.
+ */
 static Property arm_cpu_pmsav7_dregion_property =
-            DEFINE_PROP_UINT32("pmsav7-dregion", ARMCPU, pmsav7_dregion, 16);
+            DEFINE_PROP_UNSIGNED_NODEFAULT("pmsav7-dregion", ARMCPU,
+                                           pmsav7_dregion,
+                                           qdev_prop_uint32, uint32_t);
 
 static void arm_cpu_post_init(Object *obj)
 {
@@ -1054,6 +1061,7 @@ static void cortex_m3_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V7);
     set_feature(&cpu->env, ARM_FEATURE_M);
     cpu->midr = 0x410fc231;
+    cpu->pmsav7_dregion = 8;
 }
 
 static void cortex_m4_initfn(Object *obj)
@@ -1064,6 +1072,7 @@ static void cortex_m4_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_M);
     set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
     cpu->midr = 0x410fc240; /* r0p0 */
+    cpu->pmsav7_dregion = 8;
 }
 static void arm_v7m_class_init(ObjectClass *oc, void *data)
 {
@@ -1112,6 +1121,7 @@ static void cortex_r5_initfn(Object *obj)
     cpu->id_isar4 = 0x0010142;
     cpu->id_isar5 = 0x0;
     cpu->mp_is_up = true;
+    cpu->pmsav7_dregion = 16;
     define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

[Peter Maydell]

The Cortex-M3 and M4 CPUs always have 8 PMSA MPU regions (this isn't
a configurable option for the hardware).  Make the default value of
the pmsav7-dregion property be set per-cpu, so we don't need to have
every user of these CPUs set it manually.  (The existing default of
16 is correct for the other PMSAv7 core, the Cortex-R5.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 28a9141..96d1f84 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -543,8 +543,15 @@ static Property arm_cpu_has_pmu_property =
 static Property arm_cpu_has_mpu_property =
             DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
 
+/* This is like DEFINE_PROP_UINT32 but it doesn't set the default value,
+ * because the CPU initfn will have already set cpu->pmsav7_dregion to
+ * the right value for that particular CPU type, and we don't want
+ * to override that with an incorrect constant value.
+ */
 static Property arm_cpu_pmsav7_dregion_property =
-            DEFINE_PROP_UINT32("pmsav7-dregion", ARMCPU, pmsav7_dregion, 16);
+            DEFINE_PROP_UNSIGNED_NODEFAULT("pmsav7-dregion", ARMCPU,
+                                           pmsav7_dregion,
+                                           qdev_prop_uint32, uint32_t);
 
 static void arm_cpu_post_init(Object *obj)
 {
@@ -1054,6 +1061,7 @@ static void cortex_m3_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V7);
     set_feature(&cpu->env, ARM_FEATURE_M);
     cpu->midr = 0x410fc231;
+    cpu->pmsav7_dregion = 8;
 }
 
 static void cortex_m4_initfn(Object *obj)
@@ -1064,6 +1072,7 @@ static void cortex_m4_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_M);
     set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
     cpu->midr = 0x410fc240; /* r0p0 */
+    cpu->pmsav7_dregion = 8;
 }
 static void arm_v7m_class_init(ObjectClass *oc, void *data)
 {
@@ -1112,6 +1121,7 @@ static void cortex_r5_initfn(Object *obj)
     cpu->id_isar4 = 0x0010142;
     cpu->id_isar5 = 0x0;
     cpu->mp_is_up = true;
+    cpu->pmsav7_dregion = 16;
     define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

[Marc-André Lureau]

Hi

----- Original Message -----
> The Cortex-M3 and M4 CPUs always have 8 PMSA MPU regions (this isn't
> a configurable option for the hardware).  Make the default value of
> the pmsav7-dregion property be set per-cpu, so we don't need to have
> every user of these CPUs set it manually.  (The existing default of
> 16 is correct for the other PMSAv7 core, the Cortex-R5.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

So until now that value was wrong for m3/m4 if I understand correctly.

> ---
>  target/arm/cpu.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 28a9141..96d1f84 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -543,8 +543,15 @@ static Property arm_cpu_has_pmu_property =
>  static Property arm_cpu_has_mpu_property =
>              DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
>  
> +/* This is like DEFINE_PROP_UINT32 but it doesn't set the default value,
> + * because the CPU initfn will have already set cpu->pmsav7_dregion to
> + * the right value for that particular CPU type, and we don't want
> + * to override that with an incorrect constant value.
> + */
>  static Property arm_cpu_pmsav7_dregion_property =
> -            DEFINE_PROP_UINT32("pmsav7-dregion", ARMCPU, pmsav7_dregion,
> 16);
> +            DEFINE_PROP_UNSIGNED_NODEFAULT("pmsav7-dregion", ARMCPU,
> +                                           pmsav7_dregion,
> +                                           qdev_prop_uint32, uint32_t);
>  
>  static void arm_cpu_post_init(Object *obj)
>  {
> @@ -1054,6 +1061,7 @@ static void cortex_m3_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V7);
>      set_feature(&cpu->env, ARM_FEATURE_M);
>      cpu->midr = 0x410fc231;
> +    cpu->pmsav7_dregion = 8;
>  }
>  
>  static void cortex_m4_initfn(Object *obj)
> @@ -1064,6 +1072,7 @@ static void cortex_m4_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_M);
>      set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
>      cpu->midr = 0x410fc240; /* r0p0 */
> +    cpu->pmsav7_dregion = 8;
>  }
>  static void arm_v7m_class_init(ObjectClass *oc, void *data)
>  {
> @@ -1112,6 +1121,7 @@ static void cortex_r5_initfn(Object *obj)
>      cpu->id_isar4 = 0x0010142;
>      cpu->id_isar5 = 0x0;
>      cpu->mp_is_up = true;
> +    cpu->pmsav7_dregion = 16;
>      define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
>  }
>  
> --
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 3/3] target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

[Marc-André Lureau]

Hi

----- Original Message -----
> The Cortex-M3 and M4 CPUs always have 8 PMSA MPU regions (this isn't
> a configurable option for the hardware).  Make the default value of
> the pmsav7-dregion property be set per-cpu, so we don't need to have
> every user of these CPUs set it manually.  (The existing default of
> 16 is correct for the other PMSAv7 core, the Cortex-R5.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

So until now that value was wrong for m3/m4 if I understand correctly.

> ---
>  target/arm/cpu.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 28a9141..96d1f84 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -543,8 +543,15 @@ static Property arm_cpu_has_pmu_property =
>  static Property arm_cpu_has_mpu_property =
>              DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
>  
> +/* This is like DEFINE_PROP_UINT32 but it doesn't set the default value,
> + * because the CPU initfn will have already set cpu->pmsav7_dregion to
> + * the right value for that particular CPU type, and we don't want
> + * to override that with an incorrect constant value.
> + */
>  static Property arm_cpu_pmsav7_dregion_property =
> -            DEFINE_PROP_UINT32("pmsav7-dregion", ARMCPU, pmsav7_dregion,
> 16);
> +            DEFINE_PROP_UNSIGNED_NODEFAULT("pmsav7-dregion", ARMCPU,
> +                                           pmsav7_dregion,
> +                                           qdev_prop_uint32, uint32_t);
>  
>  static void arm_cpu_post_init(Object *obj)
>  {
> @@ -1054,6 +1061,7 @@ static void cortex_m3_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V7);
>      set_feature(&cpu->env, ARM_FEATURE_M);
>      cpu->midr = 0x410fc231;
> +    cpu->pmsav7_dregion = 8;
>  }
>  
>  static void cortex_m4_initfn(Object *obj)
> @@ -1064,6 +1072,7 @@ static void cortex_m4_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_M);
>      set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
>      cpu->midr = 0x410fc240; /* r0p0 */
> +    cpu->pmsav7_dregion = 8;
>  }
>  static void arm_v7m_class_init(ObjectClass *oc, void *data)
>  {
> @@ -1112,6 +1121,7 @@ static void cortex_r5_initfn(Object *obj)
>      cpu->id_isar4 = 0x0010142;
>      cpu->id_isar5 = 0x0;
>      cpu->mp_is_up = true;
> +    cpu->pmsav7_dregion = 16;
>      define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
>  }
>  
> --
> 2.7.4
> 
> 

Re: [Qemu-arm] [PATCH 3/3] target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

[Peter Maydell]

On 11 July 2017 at 18:18, Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> ----- Original Message -----
>> The Cortex-M3 and M4 CPUs always have 8 PMSA MPU regions (this isn't
>> a configurable option for the hardware).  Make the default value of
>> the pmsav7-dregion property be set per-cpu, so we don't need to have
>> every user of these CPUs set it manually.  (The existing default of
>> 16 is correct for the other PMSAv7 core, the Cortex-R5.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> So until now that value was wrong for m3/m4 if I understand correctly.

Correct. (It was too high, so didn't cause a problem for
guests typically -- a guest assuming 8 memory regions would
just not use the registers associated with the extra
unexpected ones.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

[Peter Maydell]

On 11 July 2017 at 18:18, Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> ----- Original Message -----
>> The Cortex-M3 and M4 CPUs always have 8 PMSA MPU regions (this isn't
>> a configurable option for the hardware).  Make the default value of
>> the pmsav7-dregion property be set per-cpu, so we don't need to have
>> every user of these CPUs set it manually.  (The existing default of
>> 16 is correct for the other PMSAv7 core, the Cortex-R5.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> So until now that value was wrong for m3/m4 if I understand correctly.

Correct. (It was too high, so didn't cause a problem for
guests typically -- a guest assuming 8 memory regions would
just not use the registers associated with the extra
unexpected ones.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

[Marc-André Lureau]

hi

----- Original Message -----
> On 11 July 2017 at 18:18, Marc-André Lureau <marcandre.lureau@redhat.com>
> wrote:
> > ----- Original Message -----
> >> The Cortex-M3 and M4 CPUs always have 8 PMSA MPU regions (this isn't
> >> a configurable option for the hardware).  Make the default value of
> >> the pmsav7-dregion property be set per-cpu, so we don't need to have
> >> every user of these CPUs set it manually.  (The existing default of
> >> 16 is correct for the other PMSAv7 core, the Cortex-R5.)
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > So until now that value was wrong for m3/m4 if I understand correctly.
> 
> Correct. (It was too high, so didn't cause a problem for
> guests typically -- a guest assuming 8 memory regions would
> just not use the registers associated with the extra
> unexpected ones.)

Would be nice to add that to the commit message

in any case:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] target/arm: Make Cortex-M3 and M4 default to 8 PMSA regions

[Marc-André Lureau]

hi

----- Original Message -----
> On 11 July 2017 at 18:18, Marc-André Lureau <marcandre.lureau@redhat.com>
> wrote:
> > ----- Original Message -----
> >> The Cortex-M3 and M4 CPUs always have 8 PMSA MPU regions (this isn't
> >> a configurable option for the hardware).  Make the default value of
> >> the pmsav7-dregion property be set per-cpu, so we don't need to have
> >> every user of these CPUs set it manually.  (The existing default of
> >> 16 is correct for the other PMSAv7 core, the Cortex-R5.)
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > So until now that value was wrong for m3/m4 if I understand correctly.
> 
> Correct. (It was too high, so didn't cause a problem for
> guests typically -- a guest assuming 8 memory regions would
> just not use the registers associated with the extra
> unexpected ones.)

Would be nice to add that to the commit message

in any case:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>