* [Qemu-devel] [PATCH 05/10] vmport: convert to qdev
@ 2011-02-03 21:00 Blue Swirl
2011-02-12 16:57 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Blue Swirl @ 2011-02-03 21:00 UTC (permalink / raw)
To: qemu-devel
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)
--
1.6.2.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] vmport: convert to qdev
2011-02-03 21:00 [Qemu-devel] [PATCH 05/10] vmport: convert to qdev Blue Swirl
@ 2011-02-12 16:57 ` Markus Armbruster
2011-02-12 17:12 ` Blue Swirl
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2011-02-12 16:57 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] vmport: convert to qdev
2011-02-12 16:57 ` Markus Armbruster
@ 2011-02-12 17:12 ` Blue Swirl
2011-02-15 10:22 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Blue Swirl @ 2011-02-12 17:12 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] vmport: convert to qdev
2011-02-12 17:12 ` Blue Swirl
@ 2011-02-15 10:22 ` Markus Armbruster
2011-02-15 17:34 ` Blue Swirl
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2011-02-15 10:22 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] vmport: convert to qdev
2011-02-15 10:22 ` Markus Armbruster
@ 2011-02-15 17:34 ` Blue Swirl
2011-02-16 9:54 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Blue Swirl @ 2011-02-15 17:34 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] vmport: convert to qdev
2011-02-15 17:34 ` Blue Swirl
@ 2011-02-16 9:54 ` Markus Armbruster
0 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2011-02-16 9:54 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-16 9:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-03 21:00 [Qemu-devel] [PATCH 05/10] vmport: convert to qdev Blue Swirl
2011-02-12 16:57 ` Markus Armbruster
2011-02-12 17:12 ` Blue Swirl
2011-02-15 10:22 ` Markus Armbruster
2011-02-15 17:34 ` Blue Swirl
2011-02-16 9:54 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).