Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-18 Thread Paolo Bonzini 
Il 15/02/2013 19:38, Peter Maydell ha scritto:
 But these memory regions belong to this device -- we own them and
  they won't go away until the device is destroyed [which is never,
  as it happens, for this device.] More generally, if it's valid
  for us to hold a MemoryRegion* and call memory_region_get_ram_ptr()
  in the read/write function, it's just as valid to keep the ram pointer:
  the two have exactly matching lifetimes, unless I'm missing something.
 
  No, you're not: In practice it should be handled just fine by reference
  counting, but I still find it harder to wrap my head around it.
 I'm still confused. Memory regions aren't reference counted.
 We're the device, we own the MemoryRegion, we can happily
 do whatever we like with it for the lifetime of the device.

Yes: reference counting of the device.

 I don't propose to change this patch for v2 of this series,
 since there doesn't seem to be any need to do so.

Fair enough.

Paolo

Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-17 Thread Michael Walle 
Am Freitag 15 Februar 2013, 12:45:05 schrieb Peter Maydell:
 Don't map the pmem and dmem RAM memory regions in the milkymist-softusb
 device itself. Instead just expose them as sysbus mmio regions which
 the device creator can map appropriately. This allows us to drop the
 pmem_base and dmem_base properties. Instead of going via
 cpu_physical_memory_read/_write when the device wants to access the
 RAMs, we just keep a host pointer to the memory and use that.
 
 Signed-off-by: Peter Maydell peter.mayd...@linaro.org

Acked-by: Michael Walle mich...@walle.cc

 ---
  hw/milkymist-hw.h  |4 ++--
  hw/milkymist-softusb.c |   21 +++--
  2 files changed, 13 insertions(+), 12 deletions(-)
 
 diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
 index 21e202b..47f6d50 100644
 --- a/hw/milkymist-hw.h
 +++ b/hw/milkymist-hw.h
 @@ -210,12 +210,12 @@ static inline DeviceState
 *milkymist_softusb_create(hwaddr base, DeviceState *dev;
 
  dev = qdev_create(NULL, milkymist-softusb);
 -qdev_prop_set_uint32(dev, pmem_base, pmem_base);
  qdev_prop_set_uint32(dev, pmem_size, pmem_size);
 -qdev_prop_set_uint32(dev, dmem_base, dmem_base);
  qdev_prop_set_uint32(dev, dmem_size, dmem_size);
  qdev_init_nofail(dev);
  sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
 +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, pmem_base);
 +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, dmem_base);
  sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
 
  return dev;
 diff --git a/hw/milkymist-softusb.c b/hw/milkymist-softusb.c
 index 01660be..a3e935f 100644
 --- a/hw/milkymist-softusb.c
 +++ b/hw/milkymist-softusb.c
 @@ -54,10 +54,11 @@ struct MilkymistSoftUsbState {
  MemoryRegion dmem;
  qemu_irq irq;
 
 +void *pmem_ptr;
 +void *dmem_ptr;
 +
  /* device properties */
 -uint32_t pmem_base;
  uint32_t pmem_size;
 -uint32_t dmem_base;
  uint32_t dmem_size;
 
  /* device registers */
 @@ -134,7 +135,7 @@ static inline void
 softusb_read_dmem(MilkymistSoftUsbState *s, return;
  }
 
 -cpu_physical_memory_read(s-dmem_base + offset, buf, len);
 +memcpy(buf, s-dmem_ptr + offset, len);
  }
 
  static inline void softusb_write_dmem(MilkymistSoftUsbState *s,
 @@ -146,7 +147,7 @@ static inline void
 softusb_write_dmem(MilkymistSoftUsbState *s, return;
  }
 
 -cpu_physical_memory_write(s-dmem_base + offset, buf, len);
 +memcpy(s-dmem_ptr + offset, buf, len);
  }
 
  static inline void softusb_read_pmem(MilkymistSoftUsbState *s,
 @@ -158,7 +159,7 @@ static inline void
 softusb_read_pmem(MilkymistSoftUsbState *s, return;
  }
 
 -cpu_physical_memory_read(s-pmem_base + offset, buf, len);
 +memcpy(buf, s-pmem_ptr + offset, len);
  }
 
  static inline void softusb_write_pmem(MilkymistSoftUsbState *s,
 @@ -170,7 +171,7 @@ static inline void
 softusb_write_pmem(MilkymistSoftUsbState *s, return;
  }
 
 -cpu_physical_memory_write(s-pmem_base + offset, buf, len);
 +memcpy(s-pmem_ptr + offset, buf, len);
  }
 
  static void softusb_mouse_changed(MilkymistSoftUsbState *s)
 @@ -270,11 +271,13 @@ static int milkymist_softusb_init(SysBusDevice *dev)
  memory_region_init_ram(s-pmem, milkymist-softusb.pmem,
 s-pmem_size);
  vmstate_register_ram_global(s-pmem);
 -sysbus_add_memory(dev, s-pmem_base, s-pmem);
 +s-pmem_ptr = memory_region_get_ram_ptr(s-pmem);
 +sysbus_init_mmio(dev, s-pmem);
  memory_region_init_ram(s-dmem, milkymist-softusb.dmem,
 s-dmem_size);
  vmstate_register_ram_global(s-dmem);
 -sysbus_add_memory(dev, s-dmem_base, s-dmem);
 +s-dmem_ptr = memory_region_get_ram_ptr(s-dmem);
 +sysbus_init_mmio(dev, s-dmem);
 
  hid_init(s-hid_kbd, HID_KEYBOARD, softusb_kbd_hid_datain);
  hid_init(s-hid_mouse, HID_MOUSE, softusb_mouse_hid_datain);
 @@ -298,9 +301,7 @@ static const VMStateDescription
 vmstate_milkymist_softusb = { };
 
  static Property milkymist_softusb_properties[] = {
 -DEFINE_PROP_UINT32(pmem_base, MilkymistSoftUsbState, pmem_base,
 0xa000), DEFINE_PROP_UINT32(pmem_size, MilkymistSoftUsbState,
 pmem_size, 0x1000), -DEFINE_PROP_UINT32(dmem_base,
 MilkymistSoftUsbState, dmem_base, 0xa002),
 DEFINE_PROP_UINT32(dmem_size, MilkymistSoftUsbState, dmem_size,
 0x2000), DEFINE_PROP_END_OF_LIST(),
  };

[Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-15 Thread Peter Maydell 
Don't map the pmem and dmem RAM memory regions in the milkymist-softusb
device itself. Instead just expose them as sysbus mmio regions which
the device creator can map appropriately. This allows us to drop the
pmem_base and dmem_base properties. Instead of going via
cpu_physical_memory_read/_write when the device wants to access the
RAMs, we just keep a host pointer to the memory and use that.

Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
 hw/milkymist-hw.h  |4 ++--
 hw/milkymist-softusb.c |   21 +++--
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
index 21e202b..47f6d50 100644
--- a/hw/milkymist-hw.h
+++ b/hw/milkymist-hw.h
@@ -210,12 +210,12 @@ static inline DeviceState 
*milkymist_softusb_create(hwaddr base,
 DeviceState *dev;
 
 dev = qdev_create(NULL, milkymist-softusb);
-qdev_prop_set_uint32(dev, pmem_base, pmem_base);
 qdev_prop_set_uint32(dev, pmem_size, pmem_size);
-qdev_prop_set_uint32(dev, dmem_base, dmem_base);
 qdev_prop_set_uint32(dev, dmem_size, dmem_size);
 qdev_init_nofail(dev);
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, pmem_base);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, dmem_base);
 sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
 
 return dev;
diff --git a/hw/milkymist-softusb.c b/hw/milkymist-softusb.c
index 01660be..a3e935f 100644
--- a/hw/milkymist-softusb.c
+++ b/hw/milkymist-softusb.c
@@ -54,10 +54,11 @@ struct MilkymistSoftUsbState {
 MemoryRegion dmem;
 qemu_irq irq;
 
+void *pmem_ptr;
+void *dmem_ptr;
+
 /* device properties */
-uint32_t pmem_base;
 uint32_t pmem_size;
-uint32_t dmem_base;
 uint32_t dmem_size;
 
 /* device registers */
@@ -134,7 +135,7 @@ static inline void softusb_read_dmem(MilkymistSoftUsbState 
*s,
 return;
 }
 
-cpu_physical_memory_read(s-dmem_base + offset, buf, len);
+memcpy(buf, s-dmem_ptr + offset, len);
 }
 
 static inline void softusb_write_dmem(MilkymistSoftUsbState *s,
@@ -146,7 +147,7 @@ static inline void softusb_write_dmem(MilkymistSoftUsbState 
*s,
 return;
 }
 
-cpu_physical_memory_write(s-dmem_base + offset, buf, len);
+memcpy(s-dmem_ptr + offset, buf, len);
 }
 
 static inline void softusb_read_pmem(MilkymistSoftUsbState *s,
@@ -158,7 +159,7 @@ static inline void softusb_read_pmem(MilkymistSoftUsbState 
*s,
 return;
 }
 
-cpu_physical_memory_read(s-pmem_base + offset, buf, len);
+memcpy(buf, s-pmem_ptr + offset, len);
 }
 
 static inline void softusb_write_pmem(MilkymistSoftUsbState *s,
@@ -170,7 +171,7 @@ static inline void softusb_write_pmem(MilkymistSoftUsbState 
*s,
 return;
 }
 
-cpu_physical_memory_write(s-pmem_base + offset, buf, len);
+memcpy(s-pmem_ptr + offset, buf, len);
 }
 
 static void softusb_mouse_changed(MilkymistSoftUsbState *s)
@@ -270,11 +271,13 @@ static int milkymist_softusb_init(SysBusDevice *dev)
 memory_region_init_ram(s-pmem, milkymist-softusb.pmem,
s-pmem_size);
 vmstate_register_ram_global(s-pmem);
-sysbus_add_memory(dev, s-pmem_base, s-pmem);
+s-pmem_ptr = memory_region_get_ram_ptr(s-pmem);
+sysbus_init_mmio(dev, s-pmem);
 memory_region_init_ram(s-dmem, milkymist-softusb.dmem,
s-dmem_size);
 vmstate_register_ram_global(s-dmem);
-sysbus_add_memory(dev, s-dmem_base, s-dmem);
+s-dmem_ptr = memory_region_get_ram_ptr(s-dmem);
+sysbus_init_mmio(dev, s-dmem);
 
 hid_init(s-hid_kbd, HID_KEYBOARD, softusb_kbd_hid_datain);
 hid_init(s-hid_mouse, HID_MOUSE, softusb_mouse_hid_datain);
@@ -298,9 +301,7 @@ static const VMStateDescription vmstate_milkymist_softusb = 
{
 };
 
 static Property milkymist_softusb_properties[] = {
-DEFINE_PROP_UINT32(pmem_base, MilkymistSoftUsbState, pmem_base, 
0xa000),
 DEFINE_PROP_UINT32(pmem_size, MilkymistSoftUsbState, pmem_size, 
0x1000),
-DEFINE_PROP_UINT32(dmem_base, MilkymistSoftUsbState, dmem_base, 
0xa002),
 DEFINE_PROP_UINT32(dmem_size, MilkymistSoftUsbState, dmem_size, 
0x2000),
 DEFINE_PROP_END_OF_LIST(),
 };
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-15 Thread Andreas Färber 
Am 15.02.2013 12:45, schrieb Peter Maydell:
 Don't map the pmem and dmem RAM memory regions in the milkymist-softusb
 device itself. Instead just expose them as sysbus mmio regions which
 the device creator can map appropriately. This allows us to drop the
 pmem_base and dmem_base properties. Instead of going via
 cpu_physical_memory_read/_write when the device wants to access the
 RAMs, we just keep a host pointer to the memory and use that.
 
 Signed-off-by: Peter Maydell peter.mayd...@linaro.org

Reviewed-by: Andreas Färber afaer...@suse.de

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-15 Thread Paolo Bonzini 
Il 15/02/2013 12:45, Peter Maydell ha scritto:
 Don't map the pmem and dmem RAM memory regions in the milkymist-softusb
 device itself. Instead just expose them as sysbus mmio regions which
 the device creator can map appropriately. This allows us to drop the
 pmem_base and dmem_base properties. Instead of going via
 cpu_physical_memory_read/_write when the device wants to access the
 RAMs, we just keep a host pointer to the memory and use that.

I think it's best to avoid storing long-lived host pointers.  Ideally we
should have no long-lived host pointers anytime the thread is quiescent
(for CPUs that means kvm_cpu_exec or the end of qemu_tcg_wait_io_event;
for I/O that means select/poll).

Can you call memory_region_get_ram_ptr from the read/write functions?

Paolo

 Signed-off-by: Peter Maydell peter.mayd...@linaro.org
 ---
  hw/milkymist-hw.h  |4 ++--
  hw/milkymist-softusb.c |   21 +++--
  2 files changed, 13 insertions(+), 12 deletions(-)
 
 diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
 index 21e202b..47f6d50 100644
 --- a/hw/milkymist-hw.h
 +++ b/hw/milkymist-hw.h
 @@ -210,12 +210,12 @@ static inline DeviceState 
 *milkymist_softusb_create(hwaddr base,
  DeviceState *dev;
  
  dev = qdev_create(NULL, milkymist-softusb);
 -qdev_prop_set_uint32(dev, pmem_base, pmem_base);
  qdev_prop_set_uint32(dev, pmem_size, pmem_size);
 -qdev_prop_set_uint32(dev, dmem_base, dmem_base);
  qdev_prop_set_uint32(dev, dmem_size, dmem_size);
  qdev_init_nofail(dev);
  sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
 +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, pmem_base);
 +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, dmem_base);
  sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
  
  return dev;
 diff --git a/hw/milkymist-softusb.c b/hw/milkymist-softusb.c
 index 01660be..a3e935f 100644
 --- a/hw/milkymist-softusb.c
 +++ b/hw/milkymist-softusb.c
 @@ -54,10 +54,11 @@ struct MilkymistSoftUsbState {
  MemoryRegion dmem;
  qemu_irq irq;
  
 +void *pmem_ptr;
 +void *dmem_ptr;
 +
  /* device properties */
 -uint32_t pmem_base;
  uint32_t pmem_size;
 -uint32_t dmem_base;
  uint32_t dmem_size;
  
  /* device registers */
 @@ -134,7 +135,7 @@ static inline void 
 softusb_read_dmem(MilkymistSoftUsbState *s,
  return;
  }
  
 -cpu_physical_memory_read(s-dmem_base + offset, buf, len);
 +memcpy(buf, s-dmem_ptr + offset, len);
  }
  
  static inline void softusb_write_dmem(MilkymistSoftUsbState *s,
 @@ -146,7 +147,7 @@ static inline void 
 softusb_write_dmem(MilkymistSoftUsbState *s,
  return;
  }
  
 -cpu_physical_memory_write(s-dmem_base + offset, buf, len);
 +memcpy(s-dmem_ptr + offset, buf, len);
  }
  
  static inline void softusb_read_pmem(MilkymistSoftUsbState *s,
 @@ -158,7 +159,7 @@ static inline void 
 softusb_read_pmem(MilkymistSoftUsbState *s,
  return;
  }
  
 -cpu_physical_memory_read(s-pmem_base + offset, buf, len);
 +memcpy(buf, s-pmem_ptr + offset, len);
  }
  
  static inline void softusb_write_pmem(MilkymistSoftUsbState *s,
 @@ -170,7 +171,7 @@ static inline void 
 softusb_write_pmem(MilkymistSoftUsbState *s,
  return;
  }
  
 -cpu_physical_memory_write(s-pmem_base + offset, buf, len);
 +memcpy(s-pmem_ptr + offset, buf, len);
  }
  
  static void softusb_mouse_changed(MilkymistSoftUsbState *s)
 @@ -270,11 +271,13 @@ static int milkymist_softusb_init(SysBusDevice *dev)
  memory_region_init_ram(s-pmem, milkymist-softusb.pmem,
 s-pmem_size);
  vmstate_register_ram_global(s-pmem);
 -sysbus_add_memory(dev, s-pmem_base, s-pmem);
 +s-pmem_ptr = memory_region_get_ram_ptr(s-pmem);
 +sysbus_init_mmio(dev, s-pmem);
  memory_region_init_ram(s-dmem, milkymist-softusb.dmem,
 s-dmem_size);
  vmstate_register_ram_global(s-dmem);
 -sysbus_add_memory(dev, s-dmem_base, s-dmem);
 +s-dmem_ptr = memory_region_get_ram_ptr(s-dmem);
 +sysbus_init_mmio(dev, s-dmem);
  
  hid_init(s-hid_kbd, HID_KEYBOARD, softusb_kbd_hid_datain);
  hid_init(s-hid_mouse, HID_MOUSE, softusb_mouse_hid_datain);
 @@ -298,9 +301,7 @@ static const VMStateDescription vmstate_milkymist_softusb 
 = {
  };
  
  static Property milkymist_softusb_properties[] = {
 -DEFINE_PROP_UINT32(pmem_base, MilkymistSoftUsbState, pmem_base, 
 0xa000),
  DEFINE_PROP_UINT32(pmem_size, MilkymistSoftUsbState, pmem_size, 
 0x1000),
 -DEFINE_PROP_UINT32(dmem_base, MilkymistSoftUsbState, dmem_base, 
 0xa002),
  DEFINE_PROP_UINT32(dmem_size, MilkymistSoftUsbState, dmem_size, 
 0x2000),
  DEFINE_PROP_END_OF_LIST(),
  };

Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-15 Thread Peter Maydell 
On 15 February 2013 15:21, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 15/02/2013 12:45, Peter Maydell ha scritto:
 Don't map the pmem and dmem RAM memory regions in the milkymist-softusb
 device itself. Instead just expose them as sysbus mmio regions which
 the device creator can map appropriately. This allows us to drop the
 pmem_base and dmem_base properties. Instead of going via
 cpu_physical_memory_read/_write when the device wants to access the
 RAMs, we just keep a host pointer to the memory and use that.

 I think it's best to avoid storing long-lived host pointers.  Ideally we
 should have no long-lived host pointers anytime the thread is quiescent
 (for CPUs that means kvm_cpu_exec or the end of qemu_tcg_wait_io_event;
 for I/O that means select/poll).

 Can you call memory_region_get_ram_ptr from the read/write functions?

I could, but does it buy us anything in particular? (alternatively,
what's the reason why we should avoid storing the host pointers?
We do it in a number of other devices...)

-- PMM

Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-15 Thread Paolo Bonzini 
Il 15/02/2013 16:35, Peter Maydell ha scritto:
  I think it's best to avoid storing long-lived host pointers.  Ideally we
  should have no long-lived host pointers anytime the thread is quiescent
  (for CPUs that means kvm_cpu_exec or the end of qemu_tcg_wait_io_event;
  for I/O that means select/poll).
 
  Can you call memory_region_get_ram_ptr from the read/write functions?
 I could, but does it buy us anything in particular? (alternatively,
 what's the reason why we should avoid storing the host pointers?
 We do it in a number of other devices...)

I find it more complex to reason about finer-grained locking
(particularly regarding the lifetimes) when you have object A storing
something into object B.  In practice it should be handled just fine by
reference counting, but I still find it harder to wrap my head around it.

Paolo

Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-15 Thread Peter Maydell 
On 15 February 2013 16:06, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 15/02/2013 16:35, Peter Maydell ha scritto:
  I think it's best to avoid storing long-lived host pointers.  Ideally we
  should have no long-lived host pointers anytime the thread is quiescent
  (for CPUs that means kvm_cpu_exec or the end of qemu_tcg_wait_io_event;
  for I/O that means select/poll).
 
  Can you call memory_region_get_ram_ptr from the read/write functions?
 I could, but does it buy us anything in particular? (alternatively,
 what's the reason why we should avoid storing the host pointers?
 We do it in a number of other devices...)

 I find it more complex to reason about finer-grained locking
 (particularly regarding the lifetimes) when you have object A storing
 something into object B.  In practice it should be handled just fine by
 reference counting, but I still find it harder to wrap my head around it.

But these memory regions belong to this device -- we own them and
they won't go away until the device is destroyed [which is never,
as it happens, for this device.] More generally, if it's valid
for us to hold a MemoryRegion* and call memory_region_get_ram_ptr()
in the read/write function, it's just as valid to keep the ram pointer:
the two have exactly matching lifetimes, unless I'm missing something.

(as an aside, memory_region_destroy() doesn't free the memory that
memory_region_init_ram() allocates.)

-- PMM

Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-15 Thread Paolo Bonzini 
Il 15/02/2013 17:17, Peter Maydell ha scritto:
 But these memory regions belong to this device -- we own them and
 they won't go away until the device is destroyed [which is never,
 as it happens, for this device.] More generally, if it's valid
 for us to hold a MemoryRegion* and call memory_region_get_ram_ptr()
 in the read/write function, it's just as valid to keep the ram pointer:
 the two have exactly matching lifetimes, unless I'm missing something.

No, you're not: In practice it should be handled just fine by reference
counting, but I still find it harder to wrap my head around it.

 (as an aside, memory_region_destroy() doesn't free the memory that
 memory_region_init_ram() allocates.)

Paolo

Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself

2013-02-15 Thread Peter Maydell 
On 15 February 2013 16:18, Paolo Bonzini pbonz...@redhat.com wrote:
 Il 15/02/2013 17:17, Peter Maydell ha scritto:
 But these memory regions belong to this device -- we own them and
 they won't go away until the device is destroyed [which is never,
 as it happens, for this device.] More generally, if it's valid
 for us to hold a MemoryRegion* and call memory_region_get_ram_ptr()
 in the read/write function, it's just as valid to keep the ram pointer:
 the two have exactly matching lifetimes, unless I'm missing something.

 No, you're not: In practice it should be handled just fine by reference
 counting, but I still find it harder to wrap my head around it.

I'm still confused. Memory regions aren't reference counted.
We're the device, we own the MemoryRegion, we can happily
do whatever we like with it for the lifetime of the device.

I don't propose to change this patch for v2 of this series,
since there doesn't seem to be any need to do so.

-- PMM