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

[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 <address@hidden>
> ---
>  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, 
> 0xa0000000),
>      DEFINE_PROP_UINT32("pmem_size", MilkymistSoftUsbState, pmem_size, 
> 0x00001000),
> -    DEFINE_PROP_UINT32("dmem_base", MilkymistSoftUsbState, dmem_base, 
> 0xa0020000),
>      DEFINE_PROP_UINT32("dmem_size", MilkymistSoftUsbState, dmem_size, 
> 0x00002000),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>