On Tue, Feb 15, 2011 at 12:22 PM, Markus Armbruster <arm...@redhat.com> wrote: > Blue Swirl <blauwir...@gmail.com> writes: > >> On Sat, Feb 12, 2011 at 6:57 PM, Markus Armbruster <arm...@redhat.com> wrote: >>> Blue Swirl <blauwir...@gmail.com> writes: >>> >>>> Signed-off-by: Blue Swirl <blauwir...@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.