| Message ID | AANLkTikGH72TXx=oLevM6nNnirSO7TWj+Jun+rmTC-gr@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Blue Swirl <blauwirbel@gmail.com> writes: > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > --- > hw/pc.h | 1 - > hw/pc_piix.c | 2 -- > hw/vmport.c | 24 +++++++++++++++++++++--- > 3 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/hw/pc.h b/hw/pc.h > index a048768..603a2a3 100644 > --- a/hw/pc.h > +++ b/hw/pc.h > @@ -65,7 +65,6 @@ void hpet_pit_disable(void); > void hpet_pit_enable(void); > > /* vmport.c */ > -void vmport_init(void); > void vmport_register(unsigned char command, IOPortReadFunc *func, > void *opaque); > > /* vmmouse.c */ > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 7b74473..d0bd0cd 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -86,8 +86,6 @@ static void pc_init1(ram_addr_t ram_size, > > pc_cpus_init(cpu_model); > > - vmport_init(); > - > /* allocate ram and load rom/bios */ > pc_memory_init(ram_size, kernel_filename, kernel_cmdline, initrd_filename, > &below_4g_mem_size, &above_4g_mem_size); > diff --git a/hw/vmport.c b/hw/vmport.c > index 6c9d7c9..4432be0 100644 > --- a/hw/vmport.c > +++ b/hw/vmport.c > @@ -26,6 +26,7 @@ > #include "pc.h" > #include "sysemu.h" > #include "kvm.h" > +#include "qdev.h" > > //#define VMPORT_DEBUG > > @@ -37,6 +38,7 @@ > > typedef struct _VMPortState > { > + ISADevice dev; > IOPortReadFunc *func[VMPORT_ENTRIES]; > void *opaque[VMPORT_ENTRIES]; > } VMPortState; > @@ -100,12 +102,28 @@ static uint32_t vmport_cmd_ram_size(void > *opaque, uint32_t addr) > return ram_size; > } > > -void vmport_init(void) > +static int vmport_initfn(ISADevice *dev) > { > - register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &port_state); > - register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &port_state); > + VMPortState *s = DO_UPCAST(VMPortState, dev, dev); > > + register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &s); > + register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &s); > + isa_init_ioport(dev, 0x5658); > /* Register some generic port commands */ > vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL); > vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL); > + return 0; > } > + > +static ISADeviceInfo vmport_info = { > + .qdev.name = "vmport", > + .qdev.size = sizeof(VMPortState), > + .qdev.no_user = 1, > + .init = vmport_initfn, > +}; > + > +static void vmport_dev_register(void) > +{ > + isa_qdev_register(&vmport_info); > +} > +device_init(vmport_dev_register) Old code has pc_init1() call vmport_init(). Where does your code create qdev "vmport"? And what's happening with port_state? It's still used by vmport_register(), but no longer connected to the I/O ports. Can't see how vmport_register() has any effect anymore. Have you tested this?
On Sat, Feb 12, 2011 at 6:57 PM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >> --- >> hw/pc.h | 1 - >> hw/pc_piix.c | 2 -- >> hw/vmport.c | 24 +++++++++++++++++++++--- >> 3 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/hw/pc.h b/hw/pc.h >> index a048768..603a2a3 100644 >> --- a/hw/pc.h >> +++ b/hw/pc.h >> @@ -65,7 +65,6 @@ void hpet_pit_disable(void); >> void hpet_pit_enable(void); >> >> /* vmport.c */ >> -void vmport_init(void); >> void vmport_register(unsigned char command, IOPortReadFunc *func, >> void *opaque); >> >> /* vmmouse.c */ >> diff --git a/hw/pc_piix.c b/hw/pc_piix.c >> index 7b74473..d0bd0cd 100644 >> --- a/hw/pc_piix.c >> +++ b/hw/pc_piix.c >> @@ -86,8 +86,6 @@ static void pc_init1(ram_addr_t ram_size, >> >> pc_cpus_init(cpu_model); >> >> - vmport_init(); >> - >> /* allocate ram and load rom/bios */ >> pc_memory_init(ram_size, kernel_filename, kernel_cmdline, initrd_filename, >> &below_4g_mem_size, &above_4g_mem_size); >> diff --git a/hw/vmport.c b/hw/vmport.c >> index 6c9d7c9..4432be0 100644 >> --- a/hw/vmport.c >> +++ b/hw/vmport.c >> @@ -26,6 +26,7 @@ >> #include "pc.h" >> #include "sysemu.h" >> #include "kvm.h" >> +#include "qdev.h" >> >> //#define VMPORT_DEBUG >> >> @@ -37,6 +38,7 @@ >> >> typedef struct _VMPortState >> { >> + ISADevice dev; >> IOPortReadFunc *func[VMPORT_ENTRIES]; >> void *opaque[VMPORT_ENTRIES]; >> } VMPortState; >> @@ -100,12 +102,28 @@ static uint32_t vmport_cmd_ram_size(void >> *opaque, uint32_t addr) >> return ram_size; >> } >> >> -void vmport_init(void) >> +static int vmport_initfn(ISADevice *dev) >> { >> - register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &port_state); >> - register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &port_state); >> + VMPortState *s = DO_UPCAST(VMPortState, dev, dev); >> >> + register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &s); >> + register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &s); >> + isa_init_ioport(dev, 0x5658); >> /* Register some generic port commands */ >> vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL); >> vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL); >> + return 0; >> } >> + >> +static ISADeviceInfo vmport_info = { >> + .qdev.name = "vmport", >> + .qdev.size = sizeof(VMPortState), >> + .qdev.no_user = 1, >> + .init = vmport_initfn, >> +}; >> + >> +static void vmport_dev_register(void) >> +{ >> + isa_qdev_register(&vmport_info); >> +} >> +device_init(vmport_dev_register) > > Old code has pc_init1() call vmport_init(). Where does your code create > qdev "vmport"? And what's happening with port_state? It's still used > by vmport_register(), but no longer connected to the I/O ports. Can't > see how vmport_register() has any effect anymore. I fixed it in the committed version. > Have you tested this? Sure.
Blue Swirl <blauwirbel@gmail.com> writes: > On Sat, Feb 12, 2011 at 6:57 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Blue Swirl <blauwirbel@gmail.com> writes: >> >>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>> --- >>> hw/pc.h | 1 - >>> hw/pc_piix.c | 2 -- >>> hw/vmport.c | 24 +++++++++++++++++++++--- >>> 3 files changed, 21 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/pc.h b/hw/pc.h >>> index a048768..603a2a3 100644 >>> --- a/hw/pc.h >>> +++ b/hw/pc.h >>> @@ -65,7 +65,6 @@ void hpet_pit_disable(void); >>> void hpet_pit_enable(void); >>> >>> /* vmport.c */ >>> -void vmport_init(void); >>> void vmport_register(unsigned char command, IOPortReadFunc *func, >>> void *opaque); >>> >>> /* vmmouse.c */ >>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c >>> index 7b74473..d0bd0cd 100644 >>> --- a/hw/pc_piix.c >>> +++ b/hw/pc_piix.c >>> @@ -86,8 +86,6 @@ static void pc_init1(ram_addr_t ram_size, >>> >>> pc_cpus_init(cpu_model); >>> >>> - vmport_init(); >>> - >>> /* allocate ram and load rom/bios */ >>> pc_memory_init(ram_size, kernel_filename, kernel_cmdline, initrd_filename, >>> &below_4g_mem_size, &above_4g_mem_size); >>> diff --git a/hw/vmport.c b/hw/vmport.c >>> index 6c9d7c9..4432be0 100644 >>> --- a/hw/vmport.c >>> +++ b/hw/vmport.c >>> @@ -26,6 +26,7 @@ >>> #include "pc.h" >>> #include "sysemu.h" >>> #include "kvm.h" >>> +#include "qdev.h" >>> >>> //#define VMPORT_DEBUG >>> >>> @@ -37,6 +38,7 @@ >>> >>> typedef struct _VMPortState >>> { >>> + ISADevice dev; >>> IOPortReadFunc *func[VMPORT_ENTRIES]; >>> void *opaque[VMPORT_ENTRIES]; >>> } VMPortState; >>> @@ -100,12 +102,28 @@ static uint32_t vmport_cmd_ram_size(void >>> *opaque, uint32_t addr) >>> return ram_size; >>> } >>> >>> -void vmport_init(void) >>> +static int vmport_initfn(ISADevice *dev) >>> { >>> - register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &port_state); >>> - register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &port_state); >>> + VMPortState *s = DO_UPCAST(VMPortState, dev, dev); >>> >>> + register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &s); >>> + register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &s); >>> + isa_init_ioport(dev, 0x5658); >>> /* Register some generic port commands */ >>> vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL); >>> vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL); >>> + return 0; >>> } >>> + >>> +static ISADeviceInfo vmport_info = { >>> + .qdev.name = "vmport", >>> + .qdev.size = sizeof(VMPortState), >>> + .qdev.no_user = 1, >>> + .init = vmport_initfn, >>> +}; >>> + >>> +static void vmport_dev_register(void) >>> +{ >>> + isa_qdev_register(&vmport_info); >>> +} >>> +device_init(vmport_dev_register) >> >> Old code has pc_init1() call vmport_init(). Where does your code create >> qdev "vmport"? And what's happening with port_state? It's still used >> by vmport_register(), but no longer connected to the I/O ports. Can't >> see how vmport_register() has any effect anymore. > > I fixed it in the committed version. Did you post v2 to the list for review? >> Have you tested this? > > Sure. Here's how your v2 creates and initializes the qdev: diff --git a/hw/pc.c b/hw/pc.c index fcee09a..c698161 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -1133,6 +1133,7 @@ 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]); + vmport_init(); vmmouse_init(i8042); port92 = isa_create_simple("port92"); port92_init(port92, &a20_line[1]); This allocates a new VMPortState, and registers callbacks for port 5658: @@ -100,12 +102,28 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) return ram_size; } -void vmport_init(void) +static int vmport_initfn(ISADevice *dev) { - register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &port_state); - register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &port_state); + VMPortState *s = DO_UPCAST(VMPortState, dev, dev); + register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &s); + register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &s); + isa_init_ioport(dev, 0x5658); /* Register some generic port commands */ vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL); vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL); + return 0; } The callbacks receive the qdev VMPortState as argument. vmport_register() still updates global port_state. I.e. it no longer has any effect whatsoever on what the ports do. Maybe I'm dense, but I can't see how this can work.
On Tue, Feb 15, 2011 at 12:22 PM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> On Sat, Feb 12, 2011 at 6:57 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Blue Swirl <blauwirbel@gmail.com> writes: >>> >>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>>> --- >>>> hw/pc.h | 1 - >>>> hw/pc_piix.c | 2 -- >>>> hw/vmport.c | 24 +++++++++++++++++++++--- >>>> 3 files changed, 21 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hw/pc.h b/hw/pc.h >>>> index a048768..603a2a3 100644 >>>> --- a/hw/pc.h >>>> +++ b/hw/pc.h >>>> @@ -65,7 +65,6 @@ void hpet_pit_disable(void); >>>> void hpet_pit_enable(void); >>>> >>>> /* vmport.c */ >>>> -void vmport_init(void); >>>> void vmport_register(unsigned char command, IOPortReadFunc *func, >>>> void *opaque); >>>> >>>> /* vmmouse.c */ >>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c >>>> index 7b74473..d0bd0cd 100644 >>>> --- a/hw/pc_piix.c >>>> +++ b/hw/pc_piix.c >>>> @@ -86,8 +86,6 @@ static void pc_init1(ram_addr_t ram_size, >>>> >>>> pc_cpus_init(cpu_model); >>>> >>>> - vmport_init(); >>>> - >>>> /* allocate ram and load rom/bios */ >>>> pc_memory_init(ram_size, kernel_filename, kernel_cmdline, initrd_filename, >>>> &below_4g_mem_size, &above_4g_mem_size); >>>> diff --git a/hw/vmport.c b/hw/vmport.c >>>> index 6c9d7c9..4432be0 100644 >>>> --- a/hw/vmport.c >>>> +++ b/hw/vmport.c >>>> @@ -26,6 +26,7 @@ >>>> #include "pc.h" >>>> #include "sysemu.h" >>>> #include "kvm.h" >>>> +#include "qdev.h" >>>> >>>> //#define VMPORT_DEBUG >>>> >>>> @@ -37,6 +38,7 @@ >>>> >>>> typedef struct _VMPortState >>>> { >>>> + ISADevice dev; >>>> IOPortReadFunc *func[VMPORT_ENTRIES]; >>>> void *opaque[VMPORT_ENTRIES]; >>>> } VMPortState; >>>> @@ -100,12 +102,28 @@ static uint32_t vmport_cmd_ram_size(void >>>> *opaque, uint32_t addr) >>>> return ram_size; >>>> } >>>> >>>> -void vmport_init(void) >>>> +static int vmport_initfn(ISADevice *dev) >>>> { >>>> - register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &port_state); >>>> - register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &port_state); >>>> + VMPortState *s = DO_UPCAST(VMPortState, dev, dev); >>>> >>>> + register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &s); >>>> + register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &s); >>>> + isa_init_ioport(dev, 0x5658); >>>> /* Register some generic port commands */ >>>> vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL); >>>> vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL); >>>> + return 0; >>>> } >>>> + >>>> +static ISADeviceInfo vmport_info = { >>>> + .qdev.name = "vmport", >>>> + .qdev.size = sizeof(VMPortState), >>>> + .qdev.no_user = 1, >>>> + .init = vmport_initfn, >>>> +}; >>>> + >>>> +static void vmport_dev_register(void) >>>> +{ >>>> + isa_qdev_register(&vmport_info); >>>> +} >>>> +device_init(vmport_dev_register) >>> >>> Old code has pc_init1() call vmport_init(). Where does your code create >>> qdev "vmport"? And what's happening with port_state? It's still used >>> by vmport_register(), but no longer connected to the I/O ports. Can't >>> see how vmport_register() has any effect anymore. >> >> I fixed it in the committed version. > > Did you post v2 to the list for review? No, since v1 got no review. >>> Have you tested this? >> >> Sure. > > Here's how your v2 creates and initializes the qdev: > > diff --git a/hw/pc.c b/hw/pc.c > index fcee09a..c698161 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -1133,6 +1133,7 @@ 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]); > + vmport_init(); > vmmouse_init(i8042); > port92 = isa_create_simple("port92"); > port92_init(port92, &a20_line[1]); > > This allocates a new VMPortState, and registers callbacks for port 5658: > > @@ -100,12 +102,28 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) > return ram_size; > } > > -void vmport_init(void) > +static int vmport_initfn(ISADevice *dev) > { > - register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &port_state); > - register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &port_state); > + VMPortState *s = DO_UPCAST(VMPortState, dev, dev); > > + register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &s); > + register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &s); > + isa_init_ioport(dev, 0x5658); > /* Register some generic port commands */ > vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL); > vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL); > + return 0; > } > > The callbacks receive the qdev VMPortState as argument. > > vmport_register() still updates global port_state. I.e. it no longer > has any effect whatsoever on what the ports do. > > Maybe I'm dense, but I can't see how this can work. Good catch, it doesn't. Probably vmport_register() should take ISADevice* parameter to provide the state, instead of using static state (which would be easy one-line change). But if all this is going to be thrown into ps2.c, it may not be necessary. The whole concept of registration may become useless.
Blue Swirl <blauwirbel@gmail.com> writes: > On Tue, Feb 15, 2011 at 12:22 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Blue Swirl <blauwirbel@gmail.com> writes: >> >>> On Sat, Feb 12, 2011 at 6:57 PM, Markus Armbruster <armbru@redhat.com> wrote: [...] >>>> Old code has pc_init1() call vmport_init(). Where does your code create >>>> qdev "vmport"? And what's happening with port_state? It's still used >>>> by vmport_register(), but no longer connected to the I/O ports. Can't >>>> see how vmport_register() has any effect anymore. >>> >>> I fixed it in the committed version. >> >> Did you post v2 to the list for review? > > No, since v1 got no review. *Please* don't do that. >>>> Have you tested this? >>> >>> Sure. >> >> Here's how your v2 creates and initializes the qdev: [...] >> Maybe I'm dense, but I can't see how this can work. > > Good catch, it doesn't. Probably vmport_register() should take > ISADevice* parameter to provide the state, instead of using static > state (which would be easy one-line change). > > But if all this is going to be thrown into ps2.c, it may not be > necessary. The whole concept of registration may become useless. Yes.
diff --git a/hw/pc.h b/hw/pc.h index a048768..603a2a3 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -65,7 +65,6 @@ void hpet_pit_disable(void); void hpet_pit_enable(void); /* vmport.c */ -void vmport_init(void); void vmport_register(unsigned char command, IOPortReadFunc *func, void *opaque); /* vmmouse.c */ diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 7b74473..d0bd0cd 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -86,8 +86,6 @@ static void pc_init1(ram_addr_t ram_size, pc_cpus_init(cpu_model); - vmport_init(); - /* allocate ram and load rom/bios */ pc_memory_init(ram_size, kernel_filename, kernel_cmdline, initrd_filename, &below_4g_mem_size, &above_4g_mem_size); diff --git a/hw/vmport.c b/hw/vmport.c index 6c9d7c9..4432be0 100644 --- a/hw/vmport.c +++ b/hw/vmport.c @@ -26,6 +26,7 @@ #include "pc.h" #include "sysemu.h" #include "kvm.h" +#include "qdev.h" //#define VMPORT_DEBUG @@ -37,6 +38,7 @@ typedef struct _VMPortState { + ISADevice dev; IOPortReadFunc *func[VMPORT_ENTRIES]; void *opaque[VMPORT_ENTRIES]; } VMPortState; @@ -100,12 +102,28 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) return ram_size; } -void vmport_init(void) +static int vmport_initfn(ISADevice *dev) { - register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &port_state); - register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &port_state); + VMPortState *s = DO_UPCAST(VMPortState, dev, dev); + register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &s); + register_ioport_write(0x5658, 1, 4, vmport_ioport_write, &s); + isa_init_ioport(dev, 0x5658); /* Register some generic port commands */ vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL); vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL); + return 0; } + +static ISADeviceInfo vmport_info = { + .qdev.name = "vmport", + .qdev.size = sizeof(VMPortState), + .qdev.no_user = 1, + .init = vmport_initfn, +}; + +static void vmport_dev_register(void) +{ + isa_qdev_register(&vmport_info); +}
Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- hw/pc.h | 1 - hw/pc_piix.c | 2 -- hw/vmport.c | 24 +++++++++++++++++++++--- 3 files changed, 21 insertions(+), 6 deletions(-) +device_init(vmport_dev_register)