| Message ID | AANLkTinmubBG2aVv302Z9N4ZThLqmkm_JHF01kAneyi9@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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)
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.
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) >> >
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.
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.
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.
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.
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.
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
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.
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)
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(-)