[Qemu-devel] Re: [Qemu-commits] [COMMIT ee6847d] qdev: rework device properties.

[Qemu-devel] Re: [Qemu-commits] [COMMIT ee6847d] qdev: rework device properties.

[Blue Swirl]

On Fri, Jul 17, 2009 at 2:12 AM, Anthony Liguori<aliguori@us.ibm.com> wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
>
> This patch is a major overhaul of the device properties.  The properties
> are saved directly in the device state struct now, the linked list of
> property values is gone.

>     .qdev.name  = "fdc",
>     .qdev.size  = sizeof(fdctrl_t),
> -    .qdev.props = (DevicePropList[]) {
> -        {.name = "io_base", .type = PROP_TYPE_INT},
> -        {.name = "strict_io", .type = PROP_TYPE_INT},
> -        {.name = "mem_mapped", .type = PROP_TYPE_INT},
> -        {.name = "sun4m", .type = PROP_TYPE_INT},
> -        {.name = NULL}
> +    .qdev.props = (Property[]) {
> +        {
> +            .name = "io_base",
> +            .info = &qdev_prop_uint32,
> +            .offset = offsetof(fdctrl_t, io_base),
> +        },

This is broken, on SS-600MP, SS-10 and SS-20 fdc is located above 4G.
The correct type is target_phys_addr_t. I'll fix this.

> +typedef struct RamDevice
> +{
> +    SysBusDevice busdev;
> +    uint32_t size;
> +} RamDevice;
> +
>  /* System RAM */
>  static void ram_init1(SysBusDevice *dev)
>  {
>     ram_addr_t RAM_size, ram_offset;
> +    RamDevice *d = FROM_SYSBUS(RamDevice, dev);
>
> -    RAM_size = qdev_get_prop_int(&dev->qdev, "size", 0);
> +    RAM_size = d->size;
>
>     ram_offset = qemu_ram_alloc(RAM_size);
>     sysbus_init_mmio(dev, RAM_size, ram_offset);
> @@ -496,6 +499,7 @@ static void ram_init(target_phys_addr_t addr, ram_addr_t RAM_size,
>  {
>     DeviceState *dev;
>     SysBusDevice *s;
> +    RamDevice *d;
>
>     /* allocate RAM */
>     if ((uint64_t)RAM_size > max_mem) {
> @@ -506,20 +510,26 @@ static void ram_init(target_phys_addr_t addr, ram_addr_t RAM_size,
>         exit(1);
>     }
>     dev = qdev_create(NULL, "memory");
> -    qdev_set_prop_int(dev, "size", RAM_size);
>     qdev_init(dev);
>     s = sysbus_from_qdev(dev);
>
> +    d = FROM_SYSBUS(RamDevice, s);
> +    d->size = RAM_size;
> +
>     sysbus_mmio_map(s, 0, addr);
>  }

This is completely hosed because of the wrong order of setting
d->size. Now qemu_ram_alloc gets passed a zero.

Moreover, this limits the maximum memory to 4G. We would need a 64 bit
or ram_addr_t type, the memory could be specified in units of
mebibytes, or target_phys_addr_t could be misused.

I'm not sure how to fix this, but now sparc-softmmu just aborts at ram_init1.

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT ee6847d] qdev: rework device properties.

[Filip Navara]

On Fri, Jul 17, 2009 at 11:39 AM, Blue Swirl<blauwirbel@gmail.com> wrote:
> On Fri, Jul 17, 2009 at 2:12 AM, Anthony Liguori<aliguori@us.ibm.com> wrote:
>> From: Gerd Hoffmann <kraxel@redhat.com>
>>
>> This patch is a major overhaul of the device properties.  The properties
>> are saved directly in the device state struct now, the linked list of
>> property values is gone.
>
>>     .qdev.name  = "fdc",
>>     .qdev.size  = sizeof(fdctrl_t),
>> -    .qdev.props = (DevicePropList[]) {
>> -        {.name = "io_base", .type = PROP_TYPE_INT},
>> -        {.name = "strict_io", .type = PROP_TYPE_INT},
>> -        {.name = "mem_mapped", .type = PROP_TYPE_INT},
>> -        {.name = "sun4m", .type = PROP_TYPE_INT},
>> -        {.name = NULL}
>> +    .qdev.props = (Property[]) {
>> +        {
>> +            .name = "io_base",
>> +            .info = &qdev_prop_uint32,
>> +            .offset = offsetof(fdctrl_t, io_base),
>> +        },
>
> This is broken, on SS-600MP, SS-10 and SS-20 fdc is located above 4G.
> The correct type is target_phys_addr_t. I'll fix this.
>
>> +typedef struct RamDevice
>> +{
>> +    SysBusDevice busdev;
>> +    uint32_t size;
>> +} RamDevice;
>> +
>>  /* System RAM */
>>  static void ram_init1(SysBusDevice *dev)
>>  {
>>     ram_addr_t RAM_size, ram_offset;
>> +    RamDevice *d = FROM_SYSBUS(RamDevice, dev);
>>
>> -    RAM_size = qdev_get_prop_int(&dev->qdev, "size", 0);
>> +    RAM_size = d->size;
>>
>>     ram_offset = qemu_ram_alloc(RAM_size);
>>     sysbus_init_mmio(dev, RAM_size, ram_offset);
>> @@ -496,6 +499,7 @@ static void ram_init(target_phys_addr_t addr, ram_addr_t RAM_size,
>>  {
>>     DeviceState *dev;
>>     SysBusDevice *s;
>> +    RamDevice *d;
>>
>>     /* allocate RAM */
>>     if ((uint64_t)RAM_size > max_mem) {
>> @@ -506,20 +510,26 @@ static void ram_init(target_phys_addr_t addr, ram_addr_t RAM_size,
>>         exit(1);
>>     }
>>     dev = qdev_create(NULL, "memory");
>> -    qdev_set_prop_int(dev, "size", RAM_size);
>>     qdev_init(dev);
>>     s = sysbus_from_qdev(dev);
>>
>> +    d = FROM_SYSBUS(RamDevice, s);
>> +    d->size = RAM_size;
>> +
>>     sysbus_mmio_map(s, 0, addr);
>>  }
>
> This is completely hosed because of the wrong order of setting
> d->size. Now qemu_ram_alloc gets passed a zero.

Move the qdev_init(dev); call after the setting of d->size.

F.

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT ee6847d] qdev: rework device properties.

[Blue Swirl]

On Fri, Jul 17, 2009 at 1:05 PM, Filip Navara<filip.navara@gmail.com> wrote:
> On Fri, Jul 17, 2009 at 11:39 AM, Blue Swirl<blauwirbel@gmail.com> wrote:
>> On Fri, Jul 17, 2009 at 2:12 AM, Anthony Liguori<aliguori@us.ibm.com> wrote:
>>> From: Gerd Hoffmann <kraxel@redhat.com>
>>>
>>> This patch is a major overhaul of the device properties.  The properties
>>> are saved directly in the device state struct now, the linked list of
>>> property values is gone.
>>
>>>     .qdev.name  = "fdc",
>>>     .qdev.size  = sizeof(fdctrl_t),
>>> -    .qdev.props = (DevicePropList[]) {
>>> -        {.name = "io_base", .type = PROP_TYPE_INT},
>>> -        {.name = "strict_io", .type = PROP_TYPE_INT},
>>> -        {.name = "mem_mapped", .type = PROP_TYPE_INT},
>>> -        {.name = "sun4m", .type = PROP_TYPE_INT},
>>> -        {.name = NULL}
>>> +    .qdev.props = (Property[]) {
>>> +        {
>>> +            .name = "io_base",
>>> +            .info = &qdev_prop_uint32,
>>> +            .offset = offsetof(fdctrl_t, io_base),
>>> +        },
>>
>> This is broken, on SS-600MP, SS-10 and SS-20 fdc is located above 4G.
>> The correct type is target_phys_addr_t. I'll fix this.
>>
>>> +typedef struct RamDevice
>>> +{
>>> +    SysBusDevice busdev;
>>> +    uint32_t size;
>>> +} RamDevice;
>>> +
>>>  /* System RAM */
>>>  static void ram_init1(SysBusDevice *dev)
>>>  {
>>>     ram_addr_t RAM_size, ram_offset;
>>> +    RamDevice *d = FROM_SYSBUS(RamDevice, dev);
>>>
>>> -    RAM_size = qdev_get_prop_int(&dev->qdev, "size", 0);
>>> +    RAM_size = d->size;
>>>
>>>     ram_offset = qemu_ram_alloc(RAM_size);
>>>     sysbus_init_mmio(dev, RAM_size, ram_offset);
>>> @@ -496,6 +499,7 @@ static void ram_init(target_phys_addr_t addr, ram_addr_t RAM_size,
>>>  {
>>>     DeviceState *dev;
>>>     SysBusDevice *s;
>>> +    RamDevice *d;
>>>
>>>     /* allocate RAM */
>>>     if ((uint64_t)RAM_size > max_mem) {
>>> @@ -506,20 +510,26 @@ static void ram_init(target_phys_addr_t addr, ram_addr_t RAM_size,
>>>         exit(1);
>>>     }
>>>     dev = qdev_create(NULL, "memory");
>>> -    qdev_set_prop_int(dev, "size", RAM_size);
>>>     qdev_init(dev);
>>>     s = sysbus_from_qdev(dev);
>>>
>>> +    d = FROM_SYSBUS(RamDevice, s);
>>> +    d->size = RAM_size;
>>> +
>>>     sysbus_mmio_map(s, 0, addr);
>>>  }
>>
>> This is completely hosed because of the wrong order of setting
>> d->size. Now qemu_ram_alloc gets passed a zero.
>
> Move the qdev_init(dev); call after the setting of d->size.

Thanks, this fixes the zero memory size.