Re: [PATCH v2 1/1] core/register: Specify instance_size in the TypeInfo

[Alistair Francis]

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
>