* [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
@ 2020-08-25 17:30 Alistair Francis
2020-08-26 9:40 ` Philippe Mathieu-Daudé
2020-08-26 19:49 ` Eduardo Habkost
0 siblings, 2 replies; 6+ messages in thread
From: Alistair Francis @ 2020-08-25 17:30 UTC (permalink / raw)
To: qemu-devel, ehabkost; +Cc: alistair23, alistair.francis, f4bug
Reported-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
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 = &ri[index];
- *r = (RegisterInfo) {
- .data = data + data_size * index,
- .data_size = data_size,
- .access = &rae[i],
- .opaque = owner,
- };
- register_init(r);
+ if (data + data_size * index == 0 || !&rae[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 = &rae[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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
2020-08-25 17:30 [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo Alistair Francis
@ 2020-08-26 9:40 ` Philippe Mathieu-Daudé
2020-08-26 14:46 ` Eduardo Habkost
2020-08-26 21:20 ` Alistair Francis
2020-08-26 19:49 ` Eduardo Habkost
1 sibling, 2 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-26 9:40 UTC (permalink / raw)
To: Alistair Francis
Cc: alistair23, qemu-devel@nongnu.org Developers, Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]
Le mar. 25 août 2020 19:42, Alistair Francis <alistair.francis@wdc.com> a
écrit :
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> 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 = &ri[index];
>
> - *r = (RegisterInfo) {
> - .data = data + data_size * index,
> - .data_size = data_size,
> - .access = &rae[i],
> - .opaque = owner,
> - };
> - register_init(r);
> + if (data + data_size * index == 0 || !&rae[i]) {
> + continue;
> + }
> +
> + /* Init the register, this will zero it. */
> + object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
>
Easier to review &ri[index] than that void* cast IMO.
Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
+
> + /* Set the properties of the register */
> + r->data = data + data_size * index;
> + r->data_size = data_size;
> + r->access = &rae[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
>
>
>
[-- Attachment #2: Type: text/html, Size: 3612 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
2020-08-26 9:40 ` Philippe Mathieu-Daudé
@ 2020-08-26 14:46 ` Eduardo Habkost
2020-08-26 21:20 ` Alistair Francis
1 sibling, 0 replies; 6+ messages in thread
From: Eduardo Habkost @ 2020-08-26 14:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: alistair23, Alistair Francis, qemu-devel@nongnu.org Developers
On Wed, Aug 26, 2020 at 11:40:28AM +0200, Philippe Mathieu-Daudé wrote:
> Le mar. 25 août 2020 19:42, Alistair Francis <alistair.francis@wdc.com> a
> écrit :
>
> > Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > 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 = &ri[index];
> >
> > - *r = (RegisterInfo) {
> > - .data = data + data_size * index,
> > - .data_size = data_size,
> > - .access = &rae[i],
> > - .opaque = owner,
> > - };
> > - register_init(r);
> > + if (data + data_size * index == 0 || !&rae[i]) {
> > + continue;
> > + }
> > +
> > + /* Init the register, this will zero it. */
> > + object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
> >
>
> Easier to review &ri[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é <f4bug@amsat.org>
Thanks!
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
2020-08-25 17:30 [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo Alistair Francis
2020-08-26 9:40 ` Philippe Mathieu-Daudé
@ 2020-08-26 19:49 ` Eduardo Habkost
2020-08-26 21:52 ` Alistair Francis
1 sibling, 1 reply; 6+ messages in thread
From: Eduardo Habkost @ 2020-08-26 19:49 UTC (permalink / raw)
To: Alistair Francis; +Cc: alistair23, qemu-devel, f4bug
On Tue, Aug 25, 2020 at 10:30:59AM -0700, Alistair Francis wrote:
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> 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 = &ri[index];
>
> - *r = (RegisterInfo) {
> - .data = data + data_size * index,
> - .data_size = data_size,
> - .access = &rae[i],
> - .opaque = owner,
> - };
> - register_init(r);
> + if (data + data_size * index == 0 || !&rae[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 = &rae[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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
2020-08-26 9:40 ` Philippe Mathieu-Daudé
2020-08-26 14:46 ` Eduardo Habkost
@ 2020-08-26 21:20 ` Alistair Francis
1 sibling, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2020-08-26 21:20 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, Eduardo Habkost
On Wed, Aug 26, 2020 at 2:40 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Le mar. 25 août 2020 19:42, Alistair Francis <alistair.francis@wdc.com> a écrit :
>>
>> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>> 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 = &ri[index];
>>
>> - *r = (RegisterInfo) {
>> - .data = data + data_size * index,
>> - .data_size = data_size,
>> - .access = &rae[i],
>> - .opaque = owner,
>> - };
>> - register_init(r);
>> + if (data + data_size * index == 0 || !&rae[i]) {
>> + continue;
>> + }
>> +
>> + /* Init the register, this will zero it. */
>> + object_initialize((void *)r, sizeof(*r), TYPE_REGISTER);
>
>
> Easier to review &ri[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é <f4bug@amsat.org>
Thanks
Alistair
>
>> +
>> + /* Set the properties of the register */
>> + r->data = data + data_size * index;
>> + r->data_size = data_size;
>> + r->access = &rae[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
>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo
2020-08-26 19:49 ` Eduardo Habkost
@ 2020-08-26 21:52 ` Alistair Francis
0 siblings, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2020-08-26 21:52 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
Philippe Mathieu-Daudé
On Wed, Aug 26, 2020 at 12:49 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Tue, Aug 25, 2020 at 10:30:59AM -0700, Alistair Francis wrote:
> > Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > 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 = &ri[index];
> >
> > - *r = (RegisterInfo) {
> > - .data = data + data_size * index,
> > - .data_size = data_size,
> > - .access = &rae[i],
> > - .opaque = owner,
> > - };
> > - register_init(r);
> > + if (data + data_size * index == 0 || !&rae[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 = &rae[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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-26 22:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-25 17:30 [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo Alistair Francis
2020-08-26 9:40 ` Philippe Mathieu-Daudé
2020-08-26 14:46 ` Eduardo Habkost
2020-08-26 21:20 ` Alistair Francis
2020-08-26 19:49 ` Eduardo Habkost
2020-08-26 21:52 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).