Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
On Wed, Aug 26, 2020 at 12:49 PM Eduardo Habkost wrote:
>
> On Tue, Aug 25, 2020 at 10:30:59AM -0700, Alistair Francis wrote:
> > Reported-by: Eduardo Habkost
> > Signed-off-by: Alistair Francis
> > ---
> > hw/core/register.c | 31 +--
> > 1 file changed, 13 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/core/register.c b/hw/core/register.c
> > index ddf91eb445..31038bd7cc 100644
> > --- a/hw/core/register.c
> > +++ b/hw/core/register.c
> > @@ -176,17 +176,6 @@ void register_reset(RegisterInfo *reg)
> > }
> > }
> >
> > -void register_init(RegisterInfo *reg)
> > -{
> > -assert(reg);
> > -
> > -if (!reg->data || !reg->access) {
> > -return;
> > -}
> > -
> > -object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
> > -}
> > -
> > void register_write_memory(void *opaque, hwaddr addr,
> > uint64_t value, unsigned size)
> > {
> > @@ -269,13 +258,18 @@ static RegisterInfoArray
> > *register_init_block(DeviceState *owner,
> > int index = rae[i].addr / data_size;
> > RegisterInfo *r = [index];
> >
> > -*r = (RegisterInfo) {
> > -.data = data + data_size * index,
> > -.data_size = data_size,
> > -.access = [i],
> > -.opaque = owner,
> > -};
> > -register_init(r);
> > +if (data + data_size * index == 0 || ![i]) {
>
> Do you know what's the goal of this check?
>
> Can `data` or `rae` be NULL? If not, it seems impossible for
> this condition to be true. If they can, this seems to be a weird
> and fragile way of checking for NULL arguments.
I think the idea is that some sections in rae could be NULL or parts
of the data array could be NULL and we are checking for that here.
It seems unlikely that will be the case but I don't think the check
really hurts us.
Alistair
>
> > +continue;
> > +}
> > +
> > +/* Init the register, this will zero it. */
> > +object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
> > +
> > +/* Set the properties of the register */
> > +r->data = data + data_size * index;
> > +r->data_size = data_size;
> > +r->access = [i];
> > +r->opaque = owner;
> >
> > r_array->r[i] = r;
> > }
> > @@ -329,6 +323,7 @@ static const TypeInfo register_info = {
> > .name = TYPE_REGISTER,
> > .parent = TYPE_DEVICE,
> > .class_init = register_class_init,
> > +.instance_size = sizeof(RegisterInfo),
> > };
> >
> > static void register_register_types(void)
> > --
> > 2.28.0
> >
>
> --
> Eduardo
>
Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
On Wed, Aug 26, 2020 at 2:40 AM Philippe Mathieu-Daudé wrote:
>
> Le mar. 25 août 2020 19:42, Alistair Francis a
> écrit :
>>
>> Reported-by: Eduardo Habkost
>> Signed-off-by: Alistair Francis
>> ---
>> hw/core/register.c | 31 +--
>> 1 file changed, 13 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index ddf91eb445..31038bd7cc 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -176,17 +176,6 @@ void register_reset(RegisterInfo *reg)
>> }
>> }
>>
>> -void register_init(RegisterInfo *reg)
>> -{
>> -assert(reg);
>> -
>> -if (!reg->data || !reg->access) {
>> -return;
>> -}
>> -
>> -object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
>> -}
>> -
>> void register_write_memory(void *opaque, hwaddr addr,
>> uint64_t value, unsigned size)
>> {
>> @@ -269,13 +258,18 @@ static RegisterInfoArray
>> *register_init_block(DeviceState *owner,
>> int index = rae[i].addr / data_size;
>> RegisterInfo *r = [index];
>>
>> -*r = (RegisterInfo) {
>> -.data = data + data_size * index,
>> -.data_size = data_size,
>> -.access = [i],
>> -.opaque = owner,
>> -};
>> -register_init(r);
>> +if (data + data_size * index == 0 || ![i]) {
>> +continue;
>> +}
>> +
>> +/* Init the register, this will zero it. */
>> +object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
>
>
> Easier to review [index] than that void* cast IMO.
I find that more complex as then we aren't using the local variable we
created. I'm going to leave it as is, I hope that's ok.
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé
Thanks
Alistair
>
>> +
>> +/* Set the properties of the register */
>> +r->data = data + data_size * index;
>> +r->data_size = data_size;
>> +r->access = [i];
>> +r->opaque = owner;
>>
>> r_array->r[i] = r;
>> }
>> @@ -329,6 +323,7 @@ static const TypeInfo register_info = {
>> .name = TYPE_REGISTER,
>> .parent = TYPE_DEVICE,
>> .class_init = register_class_init,
>> +.instance_size = sizeof(RegisterInfo),
>> };
>>
>> static void register_register_types(void)
>> --
>> 2.28.0
>>
>>
Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
On Tue, Aug 25, 2020 at 10:30:59AM -0700, Alistair Francis wrote:
> Reported-by: Eduardo Habkost
> Signed-off-by: Alistair Francis
> ---
> hw/core/register.c | 31 +--
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index ddf91eb445..31038bd7cc 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -176,17 +176,6 @@ void register_reset(RegisterInfo *reg)
> }
> }
>
> -void register_init(RegisterInfo *reg)
> -{
> -assert(reg);
> -
> -if (!reg->data || !reg->access) {
> -return;
> -}
> -
> -object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
> -}
> -
> void register_write_memory(void *opaque, hwaddr addr,
> uint64_t value, unsigned size)
> {
> @@ -269,13 +258,18 @@ static RegisterInfoArray
> *register_init_block(DeviceState *owner,
> int index = rae[i].addr / data_size;
> RegisterInfo *r = [index];
>
> -*r = (RegisterInfo) {
> -.data = data + data_size * index,
> -.data_size = data_size,
> -.access = [i],
> -.opaque = owner,
> -};
> -register_init(r);
> +if (data + data_size * index == 0 || ![i]) {
Do you know what's the goal of this check?
Can `data` or `rae` be NULL? If not, it seems impossible for
this condition to be true. If they can, this seems to be a weird
and fragile way of checking for NULL arguments.
> +continue;
> +}
> +
> +/* Init the register, this will zero it. */
> +object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
> +
> +/* Set the properties of the register */
> +r->data = data + data_size * index;
> +r->data_size = data_size;
> +r->access = [i];
> +r->opaque = owner;
>
> r_array->r[i] = r;
> }
> @@ -329,6 +323,7 @@ static const TypeInfo register_info = {
> .name = TYPE_REGISTER,
> .parent = TYPE_DEVICE,
> .class_init = register_class_init,
> +.instance_size = sizeof(RegisterInfo),
> };
>
> static void register_register_types(void)
> --
> 2.28.0
>
--
Eduardo
Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
On Wed, Aug 26, 2020 at 11:40:28AM +0200, Philippe Mathieu-Daudé wrote:
> Le mar. 25 août 2020 19:42, Alistair Francis a
> écrit :
>
> > Reported-by: Eduardo Habkost
> > Signed-off-by: Alistair Francis
> > ---
> > hw/core/register.c | 31 +--
> > 1 file changed, 13 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/core/register.c b/hw/core/register.c
> > index ddf91eb445..31038bd7cc 100644
> > --- a/hw/core/register.c
> > +++ b/hw/core/register.c
> > @@ -176,17 +176,6 @@ void register_reset(RegisterInfo *reg)
> > }
> > }
> >
> > -void register_init(RegisterInfo *reg)
> > -{
> > -assert(reg);
> > -
> > -if (!reg->data || !reg->access) {
> > -return;
> > -}
> > -
> > -object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
> > -}
> > -
> > void register_write_memory(void *opaque, hwaddr addr,
> > uint64_t value, unsigned size)
> > {
> > @@ -269,13 +258,18 @@ static RegisterInfoArray
> > *register_init_block(DeviceState *owner,
> > int index = rae[i].addr / data_size;
> > RegisterInfo *r = [index];
> >
> > -*r = (RegisterInfo) {
> > -.data = data + data_size * index,
> > -.data_size = data_size,
> > -.access = [i],
> > -.opaque = owner,
> > -};
> > -register_init(r);
> > +if (data + data_size * index == 0 || ![i]) {
> > +continue;
> > +}
> > +
> > +/* Init the register, this will zero it. */
> > +object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
> >
>
> Easier to review [index] than that void* cast IMO.
BTW, I plan to make DECLARE_INSTANCE_CHECKER provide a
object_initialize() wrapper, e.g. in the case of TYPE_REGISTER,
the following function would be auto-generated:
static void REGISTER_INIT(RegisterInfo *obj) {
object_initialize((void *)obj, sizeof(*obj), TYPE_REGISTER);
}
so the line above could be simply rewritten as:
REGISTER_INIT(r);
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé
Thanks!
--
Eduardo
Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
Le mar. 25 août 2020 19:42, Alistair Francis a
écrit :
> Reported-by: Eduardo Habkost
> Signed-off-by: Alistair Francis
> ---
> hw/core/register.c | 31 +--
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index ddf91eb445..31038bd7cc 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -176,17 +176,6 @@ void register_reset(RegisterInfo *reg)
> }
> }
>
> -void register_init(RegisterInfo *reg)
> -{
> -assert(reg);
> -
> -if (!reg->data || !reg->access) {
> -return;
> -}
> -
> -object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
> -}
> -
> void register_write_memory(void *opaque, hwaddr addr,
> uint64_t value, unsigned size)
> {
> @@ -269,13 +258,18 @@ static RegisterInfoArray
> *register_init_block(DeviceState *owner,
> int index = rae[i].addr / data_size;
> RegisterInfo *r = [index];
>
> -*r = (RegisterInfo) {
> -.data = data + data_size * index,
> -.data_size = data_size,
> -.access = [i],
> -.opaque = owner,
> -};
> -register_init(r);
> +if (data + data_size * index == 0 || ![i]) {
> +continue;
> +}
> +
> +/* Init the register, this will zero it. */
> +object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
>
Easier to review [index] than that void* cast IMO.
Otherwise:
Reviewed-by: Philippe Mathieu-Daudé
+
> +/* Set the properties of the register */
> +r->data = data + data_size * index;
> +r->data_size = data_size;
> +r->access = [i];
> +r->opaque = owner;
>
> r_array->r[i] = r;
> }
> @@ -329,6 +323,7 @@ static const TypeInfo register_info = {
> .name = TYPE_REGISTER,
> .parent = TYPE_DEVICE,
> .class_init = register_class_init,
> +.instance_size = sizeof(RegisterInfo),
> };
>
> static void register_register_types(void)
> --
> 2.28.0
>
>
>
[PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
Reported-by: Eduardo Habkost
Signed-off-by: Alistair Francis
---
hw/core/register.c | 31 +--
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/hw/core/register.c b/hw/core/register.c
index ddf91eb445..31038bd7cc 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -176,17 +176,6 @@ void register_reset(RegisterInfo *reg)
}
}
-void register_init(RegisterInfo *reg)
-{
-assert(reg);
-
-if (!reg->data || !reg->access) {
-return;
-}
-
-object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
-}
-
void register_write_memory(void *opaque, hwaddr addr,
uint64_t value, unsigned size)
{
@@ -269,13 +258,18 @@ static RegisterInfoArray *register_init_block(DeviceState
*owner,
int index = rae[i].addr / data_size;
RegisterInfo *r = [index];
-*r = (RegisterInfo) {
-.data = data + data_size * index,
-.data_size = data_size,
-.access = [i],
-.opaque = owner,
-};
-register_init(r);
+if (data + data_size * index == 0 || ![i]) {
+continue;
+}
+
+/* Init the register, this will zero it. */
+object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
+
+/* Set the properties of the register */
+r->data = data + data_size * index;
+r->data_size = data_size;
+r->access = [i];
+r->opaque = owner;
r_array->r[i] = r;
}
@@ -329,6 +323,7 @@ static const TypeInfo register_info = {
.name = TYPE_REGISTER,
.parent = TYPE_DEVICE,
.class_init = register_class_init,
+.instance_size = sizeof(RegisterInfo),
};
static void register_register_types(void)
--
2.28.0
