Patchwork [06/10] vmmouse: convert to qdev

login
register
mail settings
Submitter Blue Swirl
Date Feb. 3, 2011, 9:01 p.m.
Message ID <AANLkTinmubBG2aVv302Z9N4ZThLqmkm_JHF01kAneyi9@mail.gmail.com>
Download mbox | patch
Permalink /patch/81777/
State New
Headers show

Comments

Blue Swirl - Feb. 3, 2011, 9:01 p.m.
Convert to qdev, also add a proper reset function.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/pc.c      |    5 +++--
 hw/pc.h      |    3 ---
 hw/vmmouse.c |   37 +++++++++++++++++++++++++++++--------
 3 files changed, 32 insertions(+), 13 deletions(-)
Markus Armbruster - Feb. 12, 2011, 5:03 p.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> Convert to qdev, also add a proper reset function.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  hw/pc.c      |    5 +++--
>  hw/pc.h      |    3 ---
>  hw/vmmouse.c |   37 +++++++++++++++++++++++++++++--------
>  3 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index fcee09a..f66ac5d 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1096,7 +1096,7 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>      PITState *pit;
>      qemu_irq rtc_irq = NULL;
>      qemu_irq *a20_line;
> -    ISADevice *i8042, *port92;
> +    ISADevice *i8042, *port92, *vmmouse;
>      qemu_irq *cpu_exit_irq;
>
>      register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
> @@ -1133,7 +1133,8 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>      a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
>      i8042 = isa_create_simple("i8042");
>      i8042_setup_a20_line(i8042, &a20_line[0]);
> -    vmmouse_init(i8042);
> +    vmmouse = isa_create("vmmouse");
> +    qdev_prop_set_ptr(&vmmouse->qdev, "ps2_mouse", i8042);
>      port92 = isa_create_simple("port92");
>      port92_init(port92, &a20_line[1]);
>
> diff --git a/hw/pc.h b/hw/pc.h
> index 603a2a3..ae83934 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -67,9 +67,6 @@ void hpet_pit_enable(void);
>  /* vmport.c */
>  void vmport_register(unsigned char command, IOPortReadFunc *func,
> void *opaque);
>
> -/* vmmouse.c */
> -void *vmmouse_init(void *m);
> -
>  /* pckbd.c */
>
>  void i8042_init(qemu_irq kbd_irq, qemu_irq mouse_irq, uint32_t io_base);
> diff --git a/hw/vmmouse.c b/hw/vmmouse.c
> index 2097119..3b39144 100644
> --- a/hw/vmmouse.c
> +++ b/hw/vmmouse.c
> @@ -25,6 +25,7 @@
>  #include "console.h"
>  #include "ps2.h"
>  #include "pc.h"
> +#include "qdev.h"
>
>  /* debug only vmmouse */
>  //#define DEBUG_VMMOUSE
> @@ -52,6 +53,7 @@
>
>  typedef struct _VMMouseState
>  {
> +    ISADevice dev;
>      uint32_t queue[VMMOUSE_QUEUE_SIZE];
>      int32_t queue_size;
>      uint16_t nb_queue;
> @@ -270,22 +272,41 @@ static const VMStateDescription vmstate_vmmouse = {
>      }
>  };
>
> -void *vmmouse_init(void *m)
> +static void vmmouse_reset(DeviceState *d)
>  {
> -    VMMouseState *s = NULL;
> +    VMMouseState *s = container_of(d, VMMouseState, dev.qdev);
>
> -    DPRINTF("vmmouse_init\n");
> +    s->status = 0xffff;
> +}
>
> -    s = qemu_mallocz(sizeof(VMMouseState));
> +static int vmmouse_initfn(ISADevice *dev)
> +{
> +    VMMouseState *s = DO_UPCAST(VMMouseState, dev, dev);
>
> -    s->status = 0xffff;
> -    s->ps2_mouse = m;
> -    s->queue_size = VMMOUSE_QUEUE_SIZE;

Where is member queue_size initialized in your new code?

> +    DPRINTF("vmmouse_init\n");
>
>      vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
>      vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
>      vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
>      vmstate_register(NULL, 0, &vmstate_vmmouse, s);
>
> -    return s;
> +    return 0;
> +}
> +
> +static ISADeviceInfo vmmouse_info = {
> +    .init          = vmmouse_initfn,
> +    .qdev.name     = "vmmouse",
> +    .qdev.size     = sizeof(VMMouseState),
> +    .qdev.no_user  = 1,
> +    .qdev.reset    = vmmouse_reset,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse),

Pointer properties are for dirty hacks only.  Is there really no better
solution?  Why does it have to be a property?

> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +static void vmmouse_dev_register(void)
> +{
> +    isa_qdev_register(&vmmouse_info);
>  }
> +device_init(vmmouse_dev_register)
Blue Swirl - Feb. 12, 2011, 5:17 p.m.
On Sat, Feb 12, 2011 at 7:03 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> Convert to qdev, also add a proper reset function.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  hw/pc.c      |    5 +++--
>>  hw/pc.h      |    3 ---
>>  hw/vmmouse.c |   37 +++++++++++++++++++++++++++++--------
>>  3 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index fcee09a..f66ac5d 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -1096,7 +1096,7 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>>      PITState *pit;
>>      qemu_irq rtc_irq = NULL;
>>      qemu_irq *a20_line;
>> -    ISADevice *i8042, *port92;
>> +    ISADevice *i8042, *port92, *vmmouse;
>>      qemu_irq *cpu_exit_irq;
>>
>>      register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
>> @@ -1133,7 +1133,8 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>>      a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
>>      i8042 = isa_create_simple("i8042");
>>      i8042_setup_a20_line(i8042, &a20_line[0]);
>> -    vmmouse_init(i8042);
>> +    vmmouse = isa_create("vmmouse");
>> +    qdev_prop_set_ptr(&vmmouse->qdev, "ps2_mouse", i8042);
>>      port92 = isa_create_simple("port92");
>>      port92_init(port92, &a20_line[1]);
>>
>> diff --git a/hw/pc.h b/hw/pc.h
>> index 603a2a3..ae83934 100644
>> --- a/hw/pc.h
>> +++ b/hw/pc.h
>> @@ -67,9 +67,6 @@ void hpet_pit_enable(void);
>>  /* vmport.c */
>>  void vmport_register(unsigned char command, IOPortReadFunc *func,
>> void *opaque);
>>
>> -/* vmmouse.c */
>> -void *vmmouse_init(void *m);
>> -
>>  /* pckbd.c */
>>
>>  void i8042_init(qemu_irq kbd_irq, qemu_irq mouse_irq, uint32_t io_base);
>> diff --git a/hw/vmmouse.c b/hw/vmmouse.c
>> index 2097119..3b39144 100644
>> --- a/hw/vmmouse.c
>> +++ b/hw/vmmouse.c
>> @@ -25,6 +25,7 @@
>>  #include "console.h"
>>  #include "ps2.h"
>>  #include "pc.h"
>> +#include "qdev.h"
>>
>>  /* debug only vmmouse */
>>  //#define DEBUG_VMMOUSE
>> @@ -52,6 +53,7 @@
>>
>>  typedef struct _VMMouseState
>>  {
>> +    ISADevice dev;
>>      uint32_t queue[VMMOUSE_QUEUE_SIZE];
>>      int32_t queue_size;
>>      uint16_t nb_queue;
>> @@ -270,22 +272,41 @@ static const VMStateDescription vmstate_vmmouse = {
>>      }
>>  };
>>
>> -void *vmmouse_init(void *m)
>> +static void vmmouse_reset(DeviceState *d)
>>  {
>> -    VMMouseState *s = NULL;
>> +    VMMouseState *s = container_of(d, VMMouseState, dev.qdev);
>>
>> -    DPRINTF("vmmouse_init\n");
>> +    s->status = 0xffff;
>> +}
>>
>> -    s = qemu_mallocz(sizeof(VMMouseState));
>> +static int vmmouse_initfn(ISADevice *dev)
>> +{
>> +    VMMouseState *s = DO_UPCAST(VMMouseState, dev, dev);
>>
>> -    s->status = 0xffff;
>> -    s->ps2_mouse = m;
>> -    s->queue_size = VMMOUSE_QUEUE_SIZE;
>
> Where is member queue_size initialized in your new code?

Good catch, nowhere. I'll fix it.

>> +    DPRINTF("vmmouse_init\n");
>>
>>      vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
>>      vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
>>      vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
>>      vmstate_register(NULL, 0, &vmstate_vmmouse, s);
>>
>> -    return s;
>> +    return 0;
>> +}
>> +
>> +static ISADeviceInfo vmmouse_info = {
>> +    .init          = vmmouse_initfn,
>> +    .qdev.name     = "vmmouse",
>> +    .qdev.size     = sizeof(VMMouseState),
>> +    .qdev.no_user  = 1,
>> +    .qdev.reset    = vmmouse_reset,
>> +    .qdev.props = (Property[]) {
>> +        DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse),
>
> Pointer properties are for dirty hacks only.  Is there really no better
> solution?  Why does it have to be a property?

So that it can be set using qdev interfaces only.
Anthony Liguori - Feb. 13, 2011, 3:42 p.m.
On 02/12/2011 11:03 AM, Markus Armbruster wrote:
> Blue Swirl<blauwirbel@gmail.com>  writes:
>
>    
>> Convert to qdev, also add a proper reset function.
>>
>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>
>> ---
>>   hw/pc.c      |    5 +++--
>>   hw/pc.h      |    3 ---
>>   hw/vmmouse.c |   37 +++++++++++++++++++++++++++++--------
>>   3 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index fcee09a..f66ac5d 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -1096,7 +1096,7 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>>       PITState *pit;
>>       qemu_irq rtc_irq = NULL;
>>       qemu_irq *a20_line;
>> -    ISADevice *i8042, *port92;
>> +    ISADevice *i8042, *port92, *vmmouse;
>>       qemu_irq *cpu_exit_irq;
>>
>>       register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
>> @@ -1133,7 +1133,8 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>>       a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
>>       i8042 = isa_create_simple("i8042");
>>       i8042_setup_a20_line(i8042,&a20_line[0]);
>> -    vmmouse_init(i8042);
>> +    vmmouse = isa_create("vmmouse");
>> +    qdev_prop_set_ptr(&vmmouse->qdev, "ps2_mouse", i8042);
>>       port92 = isa_create_simple("port92");
>>       port92_init(port92,&a20_line[1]);
>>
>> diff --git a/hw/pc.h b/hw/pc.h
>> index 603a2a3..ae83934 100644
>> --- a/hw/pc.h
>> +++ b/hw/pc.h
>> @@ -67,9 +67,6 @@ void hpet_pit_enable(void);
>>   /* vmport.c */
>>   void vmport_register(unsigned char command, IOPortReadFunc *func,
>> void *opaque);
>>
>> -/* vmmouse.c */
>> -void *vmmouse_init(void *m);
>> -
>>   /* pckbd.c */
>>
>>   void i8042_init(qemu_irq kbd_irq, qemu_irq mouse_irq, uint32_t io_base);
>> diff --git a/hw/vmmouse.c b/hw/vmmouse.c
>> index 2097119..3b39144 100644
>> --- a/hw/vmmouse.c
>> +++ b/hw/vmmouse.c
>> @@ -25,6 +25,7 @@
>>   #include "console.h"
>>   #include "ps2.h"
>>   #include "pc.h"
>> +#include "qdev.h"
>>
>>   /* debug only vmmouse */
>>   //#define DEBUG_VMMOUSE
>> @@ -52,6 +53,7 @@
>>
>>   typedef struct _VMMouseState
>>   {
>> +    ISADevice dev;
>>       uint32_t queue[VMMOUSE_QUEUE_SIZE];
>>       int32_t queue_size;
>>       uint16_t nb_queue;
>> @@ -270,22 +272,41 @@ static const VMStateDescription vmstate_vmmouse = {
>>       }
>>   };
>>
>> -void *vmmouse_init(void *m)
>> +static void vmmouse_reset(DeviceState *d)
>>   {
>> -    VMMouseState *s = NULL;
>> +    VMMouseState *s = container_of(d, VMMouseState, dev.qdev);
>>
>> -    DPRINTF("vmmouse_init\n");
>> +    s->status = 0xffff;
>> +}
>>
>> -    s = qemu_mallocz(sizeof(VMMouseState));
>> +static int vmmouse_initfn(ISADevice *dev)
>> +{
>> +    VMMouseState *s = DO_UPCAST(VMMouseState, dev, dev);
>>
>> -    s->status = 0xffff;
>> -    s->ps2_mouse = m;
>> -    s->queue_size = VMMOUSE_QUEUE_SIZE;
>>      
> Where is member queue_size initialized in your new code?
>
>    
>> +    DPRINTF("vmmouse_init\n");
>>
>>       vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
>>       vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
>>       vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
>>       vmstate_register(NULL, 0,&vmstate_vmmouse, s);
>>
>> -    return s;
>> +    return 0;
>> +}
>> +
>> +static ISADeviceInfo vmmouse_info = {
>> +    .init          = vmmouse_initfn,
>> +    .qdev.name     = "vmmouse",
>> +    .qdev.size     = sizeof(VMMouseState),
>> +    .qdev.no_user  = 1,
>> +    .qdev.reset    = vmmouse_reset,
>> +    .qdev.props = (Property[]) {
>> +        DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse),
>>      
> Pointer properties are for dirty hacks only.  Is there really no better
> solution?  Why does it have to be a property?
>    

vmmouse is really just an extension to the PS2 Mouse.  It's definitely 
not an ISA device.

In terms of qdev enablement, I would just make it a boolean option to 
the PS2Mouse and not expose it as a top level device at all.  It cannot 
exist without a PS2Mouse.

Regards,

Anthony Liguori

>> +        DEFINE_PROP_END_OF_LIST(),
>> +    }
>> +};
>> +
>> +static void vmmouse_dev_register(void)
>> +{
>> +    isa_qdev_register(&vmmouse_info);
>>   }
>> +device_init(vmmouse_dev_register)
>>      
>
Markus Armbruster - Feb. 15, 2011, 10:07 a.m.
Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/12/2011 11:03 AM, Markus Armbruster wrote:
>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>
>>    
>>> Convert to qdev, also add a proper reset function.
[...]
>> Pointer properties are for dirty hacks only.  Is there really no better
>> solution?  Why does it have to be a property?
>>    
>
> vmmouse is really just an extension to the PS2 Mouse.  It's definitely
> not an ISA device.
>
> In terms of qdev enablement, I would just make it a boolean option to
> the PS2Mouse and not expose it as a top level device at all.  It
> cannot exist without a PS2Mouse.

Which means making it a separate qdev is wrong.  That wrongness gave
rise to the dirty pointer property.  Pointer property serves as canary
again.

What now?


PS: Grumpy reviewer venting: review can keep such mistakes out of the
code, but it got committed less than two days after it was posted.
That, and the lack of proper reference headers bounced it several places
down my review queue.
Blue Swirl - Feb. 15, 2011, 5:22 p.m.
On Tue, Feb 15, 2011 at 12:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
>
>> On 02/12/2011 11:03 AM, Markus Armbruster wrote:
>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>
>>>
>>>> Convert to qdev, also add a proper reset function.
> [...]
>>> Pointer properties are for dirty hacks only.  Is there really no better
>>> solution?  Why does it have to be a property?
>>>
>>
>> vmmouse is really just an extension to the PS2 Mouse.  It's definitely
>> not an ISA device.
>>
>> In terms of qdev enablement, I would just make it a boolean option to
>> the PS2Mouse and not expose it as a top level device at all.  It
>> cannot exist without a PS2Mouse.
>
> Which means making it a separate qdev is wrong.  That wrongness gave
> rise to the dirty pointer property.  Pointer property serves as canary
> again.
>
> What now?

I don't find pointer property use so dirty, but I'll try to combine
the devices to see whether that makes sense.

> PS: Grumpy reviewer venting: review can keep such mistakes out of the
> code, but it got committed less than two days after it was posted.

Did not:
http://lists.nongnu.org/archive/html/qemu-devel/2011-02/msg00396.html
http://git.qemu.org/qemu.git/commit/?id=91c9e09147ba1f3604a3d5d29b4de7702082a33f

Thank you for reviewing.
Blue Swirl - Feb. 15, 2011, 6:32 p.m.
On Tue, Feb 15, 2011 at 7:22 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Feb 15, 2011 at 12:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Anthony Liguori <anthony@codemonkey.ws> writes:
>>
>>> On 02/12/2011 11:03 AM, Markus Armbruster wrote:
>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>>
>>>>
>>>>> Convert to qdev, also add a proper reset function.
>> [...]
>>>> Pointer properties are for dirty hacks only.  Is there really no better
>>>> solution?  Why does it have to be a property?
>>>>
>>>
>>> vmmouse is really just an extension to the PS2 Mouse.  It's definitely
>>> not an ISA device.
>>>
>>> In terms of qdev enablement, I would just make it a boolean option to
>>> the PS2Mouse and not expose it as a top level device at all.  It
>>> cannot exist without a PS2Mouse.
>>
>> Which means making it a separate qdev is wrong.  That wrongness gave
>> rise to the dirty pointer property.  Pointer property serves as canary
>> again.
>>
>> What now?
>
> I don't find pointer property use so dirty, but I'll try to combine
> the devices to see whether that makes sense.

ps2.c is actually a library which implements core parts of PS/2 mouse
and keyboard. It is used by pckbd.c (i8042) and pl050.c, so if we want
to get rid of it, all three should be merged. Perhaps instead the file
should be just renamed to libps2.c. As a side note, Makefile
dependencies are not optimal, the file should only be compiled when
either CONFIG_PCKBD is set (most architectures) or the target is ARM
(pl050).

vmport.c is only needed by vmmouse.c. It still implements some
unrelated functions. vmmouse.c does not register any ISA ports by
itself, so keeping it as a separate ISADevice does not make much
sense. Merging would let us get rid of the useless registration,
vmport.c is also compiled now even though it may be unused if there is
no vmmouse.

Merging pckbd.c with vmmouse.c: one problem is that pckbd.c is
compiled in hwlib, but vmmouse via vmport needs to access CPU
registers. Actually the interface between them is quite slim, only
i8042_isa_mouse_fake_event(). Maybe this can be replaced with a
qemu_irq, so we get rid of the pointer property.
Markus Armbruster - Feb. 16, 2011, 9:51 a.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> On Tue, Feb 15, 2011 at 12:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Anthony Liguori <anthony@codemonkey.ws> writes:
>>
>>> On 02/12/2011 11:03 AM, Markus Armbruster wrote:
>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>>
>>>>
>>>>> Convert to qdev, also add a proper reset function.
>> [...]
>>>> Pointer properties are for dirty hacks only.  Is there really no better
>>>> solution?  Why does it have to be a property?
>>>>
>>>
>>> vmmouse is really just an extension to the PS2 Mouse.  It's definitely
>>> not an ISA device.
>>>
>>> In terms of qdev enablement, I would just make it a boolean option to
>>> the PS2Mouse and not expose it as a top level device at all.  It
>>> cannot exist without a PS2Mouse.
>>
>> Which means making it a separate qdev is wrong.  That wrongness gave
>> rise to the dirty pointer property.  Pointer property serves as canary
>> again.
>>
>> What now?
>
> I don't find pointer property use so dirty,

See commit 036f7166.

>                                             but I'll try to combine
> the devices to see whether that makes sense.

Appreciated.

>> PS: Grumpy reviewer venting: review can keep such mistakes out of the
>> code, but it got committed less than two days after it was posted.
>
> Did not:
> http://lists.nongnu.org/archive/html/qemu-devel/2011-02/msg00396.html
> http://git.qemu.org/qemu.git/commit/?id=91c9e09147ba1f3604a3d5d29b4de7702082a33f

I must have misread the commit log.  Mea culpa, my sincere apologies.

> Thank you for reviewing.
Blue Swirl - Feb. 17, 2011, 7:52 p.m.
On Wed, Feb 16, 2011 at 11:51 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Tue, Feb 15, 2011 at 12:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Anthony Liguori <anthony@codemonkey.ws> writes:
>>>
>>>> On 02/12/2011 11:03 AM, Markus Armbruster wrote:
>>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>>>
>>>>>
>>>>>> Convert to qdev, also add a proper reset function.
>>> [...]
>>>>> Pointer properties are for dirty hacks only.  Is there really no better
>>>>> solution?  Why does it have to be a property?
>>>>>
>>>>
>>>> vmmouse is really just an extension to the PS2 Mouse.  It's definitely
>>>> not an ISA device.
>>>>
>>>> In terms of qdev enablement, I would just make it a boolean option to
>>>> the PS2Mouse and not expose it as a top level device at all.  It
>>>> cannot exist without a PS2Mouse.
>>>
>>> Which means making it a separate qdev is wrong.  That wrongness gave
>>> rise to the dirty pointer property.  Pointer property serves as canary
>>> again.
>>>
>>> What now?
>>
>> I don't find pointer property use so dirty,
>
> See commit 036f7166.
>
>>                                             but I'll try to combine
>> the devices to see whether that makes sense.
>
> Appreciated.

The attached patch would merge the devices, but I'm not so sure this
is the right approach. Merging seems to be OK, the registration could
be removed harder by adding a switch for known vmport values.

But vmmouse couldn't be left out of the build anymore since it would
be built per target (because of CPUState dependencies). That would be
a step backwards. Perhaps the register access helpers should be pushed
to board level.
Paolo Bonzini - Feb. 18, 2011, 12:34 p.m.
On 02/17/2011 08:52 PM, Blue Swirl wrote:
> On Wed, Feb 16, 2011 at 11:51 AM, Markus Armbruster<armbru@redhat.com>  wrote:
>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>
>>> On Tue, Feb 15, 2011 at 12:07 PM, Markus Armbruster<armbru@redhat.com>  wrote:
>>>> Anthony Liguori<anthony@codemonkey.ws>  writes:
>>>>
>>>>> On 02/12/2011 11:03 AM, Markus Armbruster wrote:
>>>>>> Blue Swirl<blauwirbel@gmail.com>    writes:
>>>>>>
>>>>>>
>>>>>>> Convert to qdev, also add a proper reset function.
>>>> [...]
>>>>>> Pointer properties are for dirty hacks only.  Is there really no better
>>>>>> solution?  Why does it have to be a property?
>>>>>>
>>>>>
>>>>> vmmouse is really just an extension to the PS2 Mouse.  It's definitely
>>>>> not an ISA device.
>>>>>
>>>>> In terms of qdev enablement, I would just make it a boolean option to
>>>>> the PS2Mouse and not expose it as a top level device at all.  It
>>>>> cannot exist without a PS2Mouse.
>>>>
>>>> Which means making it a separate qdev is wrong.  That wrongness gave
>>>> rise to the dirty pointer property.  Pointer property serves as canary
>>>> again.
>>>>
>>>> What now?
>>>
>>> I don't find pointer property use so dirty,
>>
>> See commit 036f7166.
>>
>>>                                              but I'll try to combine
>>> the devices to see whether that makes sense.
>>
>> Appreciated.
>
> The attached patch would merge the devices, but I'm not so sure this
> is the right approach. Merging seems to be OK, the registration could
> be removed harder by adding a switch for known vmport values.
>
> But vmmouse couldn't be left out of the build anymore since it would
> be built per target (because of CPUState dependencies). That would be
> a step backwards. Perhaps the register access helpers should be pushed
> to board level.

Is there any fundamental reason why obj-i386-$(CONFIG_VMMOUSE) doesn't work?

Paolo
Blue Swirl - Feb. 18, 2011, 6:04 p.m.
On Fri, Feb 18, 2011 at 2:34 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/17/2011 08:52 PM, Blue Swirl wrote:
>>
>> On Wed, Feb 16, 2011 at 11:51 AM, Markus Armbruster<armbru@redhat.com>
>>  wrote:
>>>
>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>
>>>> On Tue, Feb 15, 2011 at 12:07 PM, Markus Armbruster<armbru@redhat.com>
>>>>  wrote:
>>>>>
>>>>> Anthony Liguori<anthony@codemonkey.ws>  writes:
>>>>>
>>>>>> On 02/12/2011 11:03 AM, Markus Armbruster wrote:
>>>>>>>
>>>>>>> Blue Swirl<blauwirbel@gmail.com>    writes:
>>>>>>>
>>>>>>>
>>>>>>>> Convert to qdev, also add a proper reset function.
>>>>>
>>>>> [...]
>>>>>>>
>>>>>>> Pointer properties are for dirty hacks only.  Is there really no
>>>>>>> better
>>>>>>> solution?  Why does it have to be a property?
>>>>>>>
>>>>>>
>>>>>> vmmouse is really just an extension to the PS2 Mouse.  It's definitely
>>>>>> not an ISA device.
>>>>>>
>>>>>> In terms of qdev enablement, I would just make it a boolean option to
>>>>>> the PS2Mouse and not expose it as a top level device at all.  It
>>>>>> cannot exist without a PS2Mouse.
>>>>>
>>>>> Which means making it a separate qdev is wrong.  That wrongness gave
>>>>> rise to the dirty pointer property.  Pointer property serves as canary
>>>>> again.
>>>>>
>>>>> What now?
>>>>
>>>> I don't find pointer property use so dirty,
>>>
>>> See commit 036f7166.
>>>
>>>>                                             but I'll try to combine
>>>> the devices to see whether that makes sense.
>>>
>>> Appreciated.
>>
>> The attached patch would merge the devices, but I'm not so sure this
>> is the right approach. Merging seems to be OK, the registration could
>> be removed harder by adding a switch for known vmport values.
>>
>> But vmmouse couldn't be left out of the build anymore since it would
>> be built per target (because of CPUState dependencies). That would be
>> a step backwards. Perhaps the register access helpers should be pushed
>> to board level.
>
> Is there any fundamental reason why obj-i386-$(CONFIG_VMMOUSE) doesn't work?

Hm, actually nothing. There just aren't such devices except fulong
ones. I'll make a new patch.

Patch

diff --git a/hw/pc.c b/hw/pc.c
index fcee09a..f66ac5d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1096,7 +1096,7 @@  void pc_basic_device_init(qemu_irq *isa_irq,
     PITState *pit;
     qemu_irq rtc_irq = NULL;
     qemu_irq *a20_line;
-    ISADevice *i8042, *port92;
+    ISADevice *i8042, *port92, *vmmouse;
     qemu_irq *cpu_exit_irq;

     register_ioport_write(0x80, 1, 1, ioport80_write, NULL);
@@ -1133,7 +1133,8 @@  void pc_basic_device_init(qemu_irq *isa_irq,
     a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
     i8042 = isa_create_simple("i8042");
     i8042_setup_a20_line(i8042, &a20_line[0]);
-    vmmouse_init(i8042);
+    vmmouse = isa_create("vmmouse");
+    qdev_prop_set_ptr(&vmmouse->qdev, "ps2_mouse", i8042);
     port92 = isa_create_simple("port92");
     port92_init(port92, &a20_line[1]);

diff --git a/hw/pc.h b/hw/pc.h
index 603a2a3..ae83934 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -67,9 +67,6 @@  void hpet_pit_enable(void);
 /* vmport.c */
 void vmport_register(unsigned char command, IOPortReadFunc *func,
void *opaque);

-/* vmmouse.c */
-void *vmmouse_init(void *m);
-
 /* pckbd.c */

 void i8042_init(qemu_irq kbd_irq, qemu_irq mouse_irq, uint32_t io_base);
diff --git a/hw/vmmouse.c b/hw/vmmouse.c
index 2097119..3b39144 100644
--- a/hw/vmmouse.c
+++ b/hw/vmmouse.c
@@ -25,6 +25,7 @@ 
 #include "console.h"
 #include "ps2.h"
 #include "pc.h"
+#include "qdev.h"

 /* debug only vmmouse */
 //#define DEBUG_VMMOUSE
@@ -52,6 +53,7 @@ 

 typedef struct _VMMouseState
 {
+    ISADevice dev;
     uint32_t queue[VMMOUSE_QUEUE_SIZE];
     int32_t queue_size;
     uint16_t nb_queue;
@@ -270,22 +272,41 @@  static const VMStateDescription vmstate_vmmouse = {
     }
 };

-void *vmmouse_init(void *m)
+static void vmmouse_reset(DeviceState *d)
 {
-    VMMouseState *s = NULL;
+    VMMouseState *s = container_of(d, VMMouseState, dev.qdev);

-    DPRINTF("vmmouse_init\n");
+    s->status = 0xffff;
+}

-    s = qemu_mallocz(sizeof(VMMouseState));
+static int vmmouse_initfn(ISADevice *dev)
+{
+    VMMouseState *s = DO_UPCAST(VMMouseState, dev, dev);

-    s->status = 0xffff;
-    s->ps2_mouse = m;
-    s->queue_size = VMMOUSE_QUEUE_SIZE;
+    DPRINTF("vmmouse_init\n");

     vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
     vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
     vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
     vmstate_register(NULL, 0, &vmstate_vmmouse, s);

-    return s;
+    return 0;
+}
+
+static ISADeviceInfo vmmouse_info = {
+    .init          = vmmouse_initfn,
+    .qdev.name     = "vmmouse",
+    .qdev.size     = sizeof(VMMouseState),
+    .qdev.no_user  = 1,
+    .qdev.reset    = vmmouse_reset,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static void vmmouse_dev_register(void)
+{
+    isa_qdev_register(&vmmouse_info);
 }
+device_init(vmmouse_dev_register)