[Qemu-devel] [PATCH 0/3] Remove PROP_PTR, part 2/4

[Qemu-devel] [PATCH 0/3] Remove PROP_PTR, part 2/4

[Paolo Bonzini]

Part 1 is OMAP and is included in the qdev series.  This one covers
devices that should never have used PROP_PTR in the first place.
The other parts will have to wait for QOMification of buses and CPUs.

Paolo Bonzini (3):
  smbus: fix writes
  smbus_eeprom: remove PTR property
  vmmouse: replace PROP_PTR property with gpio pin to i8042

 hw/pc.c           |    7 +++----
 hw/pc.h           |    1 -
 hw/pckbd.c        |    7 +++++--
 hw/smbus.c        |   11 +++++++----
 hw/smbus.h        |    4 ++--
 hw/smbus_eeprom.c |   39 +++++++++++++++++++++------------------
 hw/vmmouse.c      |   11 +++--------
 7 files changed, 41 insertions(+), 39 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/3] smbus: fix writes

[Paolo Bonzini]

SMBus protocol sends offset and length before the actual data that
is transferred.  So we need to skip two bytes rather than one.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/smbus.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/smbus.c b/hw/smbus.c
index 77626f3..4ff2342 100644
--- a/hw/smbus.c
+++ b/hw/smbus.c
@@ -59,9 +59,12 @@ static void smbus_do_write(SMBusDevice *dev)
     } else {
         dev->command = dev->data_buf[0];
         DPRINTF("Command %d len %d\n", dev->command, dev->data_len - 1);
+        if (dev->data_buf[1] > dev->data_len - 2) {
+            fprintf(stderr, "SMBus data transfer overrun!\n");
+        }
         if (sc->write_data) {
-            sc->write_data(dev, dev->command, dev->data_buf + 1,
-                           dev->data_len - 1);
+            sc->write_data(dev, dev->command, dev->data_buf + 2,
+                           MIN(dev->data_buf[1], dev->data_len - 2));
         }
     }
 }
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/3] smbus_eeprom: remove PTR property

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/smbus.c        |    4 ++--
 hw/smbus.h        |    4 ++--
 hw/smbus_eeprom.c |   39 +++++++++++++++++++++------------------
 3 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/hw/smbus.c b/hw/smbus.c
index 4ff2342..969d46d 100644
--- a/hw/smbus.c
+++ b/hw/smbus.c
@@ -295,8 +295,8 @@ int smbus_read_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data)
     return len;
 }
 
-void smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                       int len)
+void smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command,
+                       const uint8_t *data, int len)
 {
     int i;
 
diff --git a/hw/smbus.h b/hw/smbus.h
index 6ed45bd..3dee2d1 100644
--- a/hw/smbus.h
+++ b/hw/smbus.h
@@ -74,8 +74,8 @@ void smbus_write_byte(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t data)
 uint16_t smbus_read_word(i2c_bus *bus, uint8_t addr, uint8_t command);
 void smbus_write_word(i2c_bus *bus, uint8_t addr, uint8_t command, uint16_t data);
 int smbus_read_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data);
-void smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                       int len);
+void smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command,
+                       const uint8_t *data, int len);
 
 void smbus_eeprom_init(i2c_bus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int size);
diff --git a/hw/smbus_eeprom.c b/hw/smbus_eeprom.c
index 9d96cbe..8e42c41 100644
--- a/hw/smbus_eeprom.c
+++ b/hw/smbus_eeprom.c
@@ -27,10 +27,11 @@
 #include "smbus.h"
 
 //#define DEBUG
+#define SMBUS_EEPROM_SIZE 256
 
 typedef struct SMBusEEPROMDevice {
     SMBusDevice smbusdev;
-    void *data;
+    uint8_t data[SMBUS_EEPROM_SIZE];
     uint8_t offset;
 } SMBusEEPROMDevice;
 
@@ -54,8 +55,7 @@ static void eeprom_send_byte(SMBusDevice *dev, uint8_t val)
 static uint8_t eeprom_receive_byte(SMBusDevice *dev)
 {
     SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
-    uint8_t *data = eeprom->data;
-    uint8_t val = data[eeprom->offset++];
+    uint8_t val = eeprom->data[eeprom->offset++];
 #ifdef DEBUG
     printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
            dev->i2c.address, val);
@@ -75,8 +75,8 @@ static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int l
        It is a block write without a length byte.  Fortunately we
        get the full block anyway.  */
     /* TODO: Should this set the current location?  */
-    if (cmd + len > 256)
-        n = 256 - cmd;
+    if (cmd + len > SMBUS_EEPROM_SIZE)
+        n = SMBUS_EEPROM_SIZE - cmd;
     else
         n = len;
     memcpy(eeprom->data + cmd, buf, n);
@@ -104,14 +104,8 @@ static int smbus_eeprom_initfn(SMBusDevice *dev)
     return 0;
 }
 
-static Property smbus_eeprom_properties[] = {
-    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
     SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
 
     sc->init = smbus_eeprom_initfn;
@@ -120,7 +114,6 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     sc->receive_byte = eeprom_receive_byte;
     sc->write_data = eeprom_write_data;
     sc->read_data = eeprom_read_data;
-    dc->props = smbus_eeprom_properties;
 }
 
 static TypeInfo smbus_eeprom_info = {
@@ -141,16 +134,26 @@ void smbus_eeprom_init(i2c_bus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int eeprom_spd_size)
 {
     int i;
-    uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent */
-    if (eeprom_spd_size > 0) {
-        memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size);
-    }
 
     for (i = 0; i < nb_eeprom; i++) {
         DeviceState *eeprom;
+        int address = 0x50 + i;
+
         eeprom = qdev_create((BusState *)smbus, "smbus-eeprom");
-        qdev_prop_set_uint8(eeprom, "address", 0x50 + i);
-        qdev_prop_set_ptr(eeprom, "data", eeprom_buf + (i * 256));
+        qdev_prop_set_uint8(eeprom, "address", address);
         qdev_init_nofail(eeprom);
+
+        if (eeprom_spd_size > 0) {
+            int len = MIN(eeprom_spd_size, SMBUS_EEPROM_SIZE);
+            int offset = 0;
+            while (len > 0) {
+                int xfer_len = MIN(len, 32);
+                smbus_write_block(smbus, address, offset, eeprom_spd, xfer_len);
+                len -= xfer_len;
+                eeprom_spd_size -= xfer_len;
+                eeprom_spd += xfer_len;
+                offset += xfer_len;
+            }
+        }
     }
 }
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/3] vmmouse: replace PROP_PTR property with gpio pin to i8042

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pc.c      |    7 +++----
 hw/pc.h      |    1 -
 hw/pckbd.c   |    7 +++++--
 hw/vmmouse.c |   11 +++--------
 4 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 7f3aa65..76787c7 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1181,13 +1181,12 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
     if (!no_vmport) {
         vmport_init(isa_bus);
         vmmouse = isa_try_create(isa_bus, "vmmouse");
+        qdev_init_nofail(&vmmouse->qdev);
+        qdev_connect_gpio_out(DEVICE(vmmouse), 0,
+                              qdev_get_gpio_in(DEVICE(i8042), 0));
     } else {
         vmmouse = NULL;
     }
-    if (vmmouse) {
-        qdev_prop_set_ptr(&vmmouse->qdev, "ps2_mouse", i8042);
-        qdev_init_nofail(&vmmouse->qdev);
-    }
     port92 = isa_create_simple(isa_bus, "port92");
     port92_init(port92, &a20_line[1]);
 
diff --git a/hw/pc.h b/hw/pc.h
index c666ec9..5602549 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -121,7 +121,6 @@ void i8042_init(qemu_irq kbd_irq, qemu_irq mouse_irq, uint32_t io_base);
 void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
                    MemoryRegion *region, ram_addr_t size,
                    target_phys_addr_t mask);
-void i8042_isa_mouse_fake_event(void *opaque);
 void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
 
 /* pc.c */
diff --git a/hw/pckbd.c b/hw/pckbd.c
index b4c53be..ea8ff73 100644
--- a/hw/pckbd.c
+++ b/hw/pckbd.c
@@ -433,12 +433,14 @@ typedef struct ISAKBDState {
     MemoryRegion io[2];
 } ISAKBDState;
 
-void i8042_isa_mouse_fake_event(void *opaque)
+static void i8042_isa_mouse_fake_event(void *opaque, int n, int level)
 {
     ISADevice *dev = opaque;
     KBDState *s = &(DO_UPCAST(ISAKBDState, dev, dev)->kbd);
 
-    ps2_mouse_fake_event(s->mouse);
+    if (level) {
+        ps2_mouse_fake_event(s->mouse);
+    }
 }
 
 void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out)
@@ -482,6 +484,7 @@ static int i8042_initfn(ISADevice *dev)
     ISAKBDState *isa_s = DO_UPCAST(ISAKBDState, dev, dev);
     KBDState *s = &isa_s->kbd;
 
+    qdev_init_gpio_in(DEVICE(dev), i8042_isa_mouse_fake_event, 1);
     isa_init_irq(dev, &s->irq_kbd, 1);
     isa_init_irq(dev, &s->irq_mouse, 12);
 
diff --git a/hw/vmmouse.c b/hw/vmmouse.c
index fda4f89..0272e7e 100644
--- a/hw/vmmouse.c
+++ b/hw/vmmouse.c
@@ -60,7 +60,7 @@ typedef struct _VMMouseState
     uint16_t status;
     uint8_t absolute;
     QEMUPutMouseEntry *entry;
-    void *ps2_mouse;
+    qemu_irq ps2_mouse_event;
 } VMMouseState;
 
 static uint32_t vmmouse_get_status(VMMouseState *s)
@@ -99,7 +99,7 @@ static void vmmouse_mouse_event(void *opaque, int x, int y, int dz, int buttons_
 
     /* need to still generate PS2 events to notify driver to
        read from queue */
-    i8042_isa_mouse_fake_event(s->ps2_mouse);
+    qemu_set_irq(s->ps2_mouse_event, 1);
 }
 
 static void vmmouse_remove_handler(VMMouseState *s)
@@ -264,6 +264,7 @@ static int vmmouse_initfn(ISADevice *dev)
 
     DPRINTF("vmmouse_init\n");
 
+    qdev_init_gpio_out(DEVICE(dev), &s->ps2_mouse_event, 1);
     vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
     vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
     vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
@@ -271,11 +272,6 @@ static int vmmouse_initfn(ISADevice *dev)
     return 0;
 }
 
-static Property vmmouse_properties[] = {
-    DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void vmmouse_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -284,7 +280,6 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
     dc->no_user = 1;
     dc->reset = vmmouse_reset;
     dc->vmsd = &vmstate_vmmouse;
-    dc->props = vmmouse_properties;
 }
 
 static TypeInfo vmmouse_info = {
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 3/3] vmmouse: replace PROP_PTR property with gpio pin to i8042

[Gerhard Wiesinger]

Hello Paolo,

What will be fixed/enhanced by this patch?
Scenario?

Can you also add a more detailed commit message.

Thnx.

Ciao,
Gerhard

--
http://www.wiesinger.com/


On Sat, 4 Feb 2012, Paolo Bonzini wrote:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/pc.c      |    7 +++----
> hw/pc.h      |    1 -
> hw/pckbd.c   |    7 +++++--
> hw/vmmouse.c |   11 +++--------
> 4 files changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 7f3aa65..76787c7 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1181,13 +1181,12 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>     if (!no_vmport) {
>         vmport_init(isa_bus);
>         vmmouse = isa_try_create(isa_bus, "vmmouse");
> +        qdev_init_nofail(&vmmouse->qdev);
> +        qdev_connect_gpio_out(DEVICE(vmmouse), 0,
> +                              qdev_get_gpio_in(DEVICE(i8042), 0));
>     } else {
>         vmmouse = NULL;
>     }
> -    if (vmmouse) {
> -        qdev_prop_set_ptr(&vmmouse->qdev, "ps2_mouse", i8042);
> -        qdev_init_nofail(&vmmouse->qdev);
> -    }
>     port92 = isa_create_simple(isa_bus, "port92");
>     port92_init(port92, &a20_line[1]);
>
> diff --git a/hw/pc.h b/hw/pc.h
> index c666ec9..5602549 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -121,7 +121,6 @@ void i8042_init(qemu_irq kbd_irq, qemu_irq mouse_irq, uint32_t io_base);
> void i8042_mm_init(qemu_irq kbd_irq, qemu_irq mouse_irq,
>                    MemoryRegion *region, ram_addr_t size,
>                    target_phys_addr_t mask);
> -void i8042_isa_mouse_fake_event(void *opaque);
> void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
>
> /* pc.c */
> diff --git a/hw/pckbd.c b/hw/pckbd.c
> index b4c53be..ea8ff73 100644
> --- a/hw/pckbd.c
> +++ b/hw/pckbd.c
> @@ -433,12 +433,14 @@ typedef struct ISAKBDState {
>     MemoryRegion io[2];
> } ISAKBDState;
>
> -void i8042_isa_mouse_fake_event(void *opaque)
> +static void i8042_isa_mouse_fake_event(void *opaque, int n, int level)
> {
>     ISADevice *dev = opaque;
>     KBDState *s = &(DO_UPCAST(ISAKBDState, dev, dev)->kbd);
>
> -    ps2_mouse_fake_event(s->mouse);
> +    if (level) {
> +        ps2_mouse_fake_event(s->mouse);
> +    }
> }
>
> void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out)
> @@ -482,6 +484,7 @@ static int i8042_initfn(ISADevice *dev)
>     ISAKBDState *isa_s = DO_UPCAST(ISAKBDState, dev, dev);
>     KBDState *s = &isa_s->kbd;
>
> +    qdev_init_gpio_in(DEVICE(dev), i8042_isa_mouse_fake_event, 1);
>     isa_init_irq(dev, &s->irq_kbd, 1);
>     isa_init_irq(dev, &s->irq_mouse, 12);
>
> diff --git a/hw/vmmouse.c b/hw/vmmouse.c
> index fda4f89..0272e7e 100644
> --- a/hw/vmmouse.c
> +++ b/hw/vmmouse.c
> @@ -60,7 +60,7 @@ typedef struct _VMMouseState
>     uint16_t status;
>     uint8_t absolute;
>     QEMUPutMouseEntry *entry;
> -    void *ps2_mouse;
> +    qemu_irq ps2_mouse_event;
> } VMMouseState;
>
> static uint32_t vmmouse_get_status(VMMouseState *s)
> @@ -99,7 +99,7 @@ static void vmmouse_mouse_event(void *opaque, int x, int y, int dz, int buttons_
>
>     /* need to still generate PS2 events to notify driver to
>        read from queue */
> -    i8042_isa_mouse_fake_event(s->ps2_mouse);
> +    qemu_set_irq(s->ps2_mouse_event, 1);
> }
>
> static void vmmouse_remove_handler(VMMouseState *s)
> @@ -264,6 +264,7 @@ static int vmmouse_initfn(ISADevice *dev)
>
>     DPRINTF("vmmouse_init\n");
>
> +    qdev_init_gpio_out(DEVICE(dev), &s->ps2_mouse_event, 1);
>     vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
>     vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
>     vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
> @@ -271,11 +272,6 @@ static int vmmouse_initfn(ISADevice *dev)
>     return 0;
> }
>
> -static Property vmmouse_properties[] = {
> -    DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> static void vmmouse_class_initfn(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -284,7 +280,6 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
>     dc->no_user = 1;
>     dc->reset = vmmouse_reset;
>     dc->vmsd = &vmstate_vmmouse;
> -    dc->props = vmmouse_properties;
> }
>
> static TypeInfo vmmouse_info = {
> -- 
> 1.7.7.6
>
>
>

Re: [Qemu-devel] [PATCH 3/3] vmmouse: replace PROP_PTR property with gpio pin to i8042

[Paolo Bonzini]

On 02/04/2012 02:03 PM, Gerhard Wiesinger wrote:
> Hello Paolo,
>
> What will be fixed/enhanced by this patch? Scenario?

Nothing, it's just a cleanup.  It's a step towards removing PROP_PTR.

Paolo

Re: [Qemu-devel] [PATCH 3/3] vmmouse: replace PROP_PTR property with gpio pin to i8042

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/pc.c      |    7 +++----
>  hw/pc.h      |    1 -
>  hw/pckbd.c   |    7 +++++--
>  hw/vmmouse.c |   11 +++--------
>  4 files changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 7f3aa65..76787c7 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1181,13 +1181,12 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>      if (!no_vmport) {
>          vmport_init(isa_bus);
>          vmmouse = isa_try_create(isa_bus, "vmmouse");
> +        qdev_init_nofail(&vmmouse->qdev);
> +        qdev_connect_gpio_out(DEVICE(vmmouse), 0,
> +                              qdev_get_gpio_in(DEVICE(i8042), 0));

Are you sue that vmmouse is always non-NULL?  My understanding is that
all callers check if is_try_create() return NULL.

Could you clarify?

Thanks, Juan.

Re: [Qemu-devel] [PATCH 3/3] vmmouse: replace PROP_PTR property with gpio pin to i8042

[Paolo Bonzini]

On 02/07/2012 01:39 PM, Juan Quintela wrote:
>> >            vmmouse = isa_try_create(isa_bus, "vmmouse");
>> >  +        qdev_init_nofail(&vmmouse->qdev);
>> >  +        qdev_connect_gpio_out(DEVICE(vmmouse), 0,
>> >  +                              qdev_get_gpio_in(DEVICE(i8042), 0));
> Are you sue that vmmouse is always non-NULL?  My understanding is that
> all callers check if is_try_create() return NULL.
>
> Could you clarify?

No, I couldn't. :)  You're right.

Paolo

Re: [Qemu-devel] [PATCH 3/3] vmmouse: replace PROP_PTR property with gpio pin to i8042

[Paul Brook]

> +static void i8042_isa_mouse_fake_event(void *opaque, int n, int level)
>  {
>      ISADevice *dev = opaque;
>      KBDState *s = &(DO_UPCAST(ISAKBDState, dev, dev)->kbd);
>  
> -    ps2_mouse_fake_event(s->mouse);
> +    if (level) {
> +        ps2_mouse_fake_event(s->mouse);
> +    }
>  }
>...  
> @@ -99,7 +99,7 @@ static void vmmouse_mouse_event(void *opaque, int x, int
>      /* need to still generate PS2 events to notify driver to
>         read from queue */
> -    i8042_isa_mouse_fake_event(s->ps2_mouse);
> +    qemu_set_irq(s->ps2_mouse_event, 1);
>  }

Both of these are wrong. qemu_irq represents a state. It is not an messge 
passing API.  Currently we don't discard duplicate sets, but that may change 
in the future.

If you want to convey an event via an IRQ line you need to actually cause and 
detect state changes.  i.e. i8042_isa_mouse_fake_event needs to remember the 
previous state, and only act on a low-high transition.  vmmouse_mouse_event 
should use qemu_irq_pulse.

Paul