| Message ID | 4cf1beb7dafb9143c261d266557d3173bf160524.1598376594.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] core/register: Specify instance_size in the TypeInfo | expand |
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 > > >
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!
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 >
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 >> >>
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 >
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)
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(-)