Re: [Qemu-devel] [PATCH 05/10] vmport: convert to qdev

[Blue Swirl]

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