diff mbox

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

Message ID 1363358063-23973-5-git-send-email-peter.maydell@linaro.org
State Accepted
Commit c34e120554c31d45bdfbac08a5c1d9ef92a62020
Headers show

Commit Message

Peter Maydell March 15, 2013, 2:34 p.m. UTC
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.maydell@linaro.org>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Acked-by: Michael Walle <michael@walle.cc>
---
 hw/milkymist-hw.h      |    4 ++--
 hw/milkymist-softusb.c |   21 +++++++++++----------
 2 files changed, 13 insertions(+), 12 deletions(-)

Comments

Anthony Liguori March 28, 2013, 5:55 p.m. UTC | #1
Peter Maydell <peter.maydell@linaro.org> writes:

> 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.maydell@linaro.org>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> Acked-by: Michael Walle <michael@walle.cc>

Breaks the build:

[aliguori@ccnode4 qemu]$ make
  CC    lm32-softmmu/hw/lm32/../milkymist-softusb.o
/home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c: In function ‘softusb_mouse_hid_datain’:
/home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:183:38: error: ‘m’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
/home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:179:13: note: ‘m’ was declared here
/home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c: In function ‘softusb_kbd_hid_datain’:
/home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:197:38: error: ‘m’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
/home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:193:13: note: ‘m’ was declared here
cc1: all warnings being treated as errors
make[1]: *** [hw/lm32/../milkymist-softusb.o] Error 1

Regards,

Anthony Liguori

> ---
>  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 aa0865f..5782e1a 100644
> --- a/hw/milkymist-hw.h
> +++ b/hw/milkymist-hw.h
> @@ -194,12 +194,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 d911686..b279d4e 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(),
>  };
> -- 
> 1.7.9.5
Michael Walle March 28, 2013, 6:31 p.m. UTC | #2
Am Donnerstag 28 März 2013, 18:55:59 schrieb Anthony Liguori:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > 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.maydell@linaro.org>
> > Reviewed-by: Andreas Färber <afaerber@suse.de>
> > Acked-by: Michael Walle <michael@walle.cc>
> 
> Breaks the build:
> 
> [aliguori@ccnode4 qemu]$ make
>   CC    lm32-softmmu/hw/lm32/../milkymist-softusb.o
> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c: In function
> ‘softusb_mouse_hid_datain’:
> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:183:38: error: ‘m’
> may be used uninitialized in this function [-Werror=maybe-uninitialized]
> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:179:13: note: ‘m’
> was declared here /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:
> In function ‘softusb_kbd_hid_datain’:
> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:197:38: error: ‘m’
> may be used uninitialized in this function [-Werror=maybe-uninitialized]
> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:193:13: note: ‘m’
> was declared here cc1: all warnings being treated as errors
> make[1]: *** [hw/lm32/../milkymist-softusb.o] Error 1

are you sure, this patch breaks the build, or was is broken before?

i'll send a patch soon.
Peter Maydell March 28, 2013, 6:33 p.m. UTC | #3
On 28 March 2013 18:31, Michael Walle <michael@walle.cc> wrote:
> Am Donnerstag 28 März 2013, 18:55:59 schrieb Anthony Liguori:cc>
>>
>> Breaks the build:
>>
>> [aliguori@ccnode4 qemu]$ make
>>   CC    lm32-softmmu/hw/lm32/../milkymist-softusb.o
>> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c: In function
>> ‘softusb_mouse_hid_datain’:
>> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:183:38: error: ‘m’
>> may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:179:13: note: ‘m’
>> was declared here /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:
>> In function ‘softusb_kbd_hid_datain’:
>> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:197:38: error: ‘m’
>> may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:193:13: note: ‘m’
>> was declared here cc1: all warnings being treated as errors
>> make[1]: *** [hw/lm32/../milkymist-softusb.o] Error 1
>
> are you sure, this patch breaks the build, or was is broken before?
>
> i'll send a patch soon.

My compiler doesn't complain. I suspect you may be right and
the problem was already there. Anyway I think that adding
a 'memset(buf, 0, len);' after the error_report() calls
in softusb_read_pmem()/softusb_read_dmem() will fix it.

-- PMM
Michael Walle March 28, 2013, 6:37 p.m. UTC | #4
Am Donnerstag 28 März 2013, 19:33:48 schrieb Peter Maydell:
> On 28 March 2013 18:31, Michael Walle <michael@walle.cc> wrote:
> > Am Donnerstag 28 März 2013, 18:55:59 schrieb Anthony Liguori:cc>
> > 
> >> Breaks the build:
> >> 
> >> [aliguori@ccnode4 qemu]$ make
> >> 
> >>   CC    lm32-softmmu/hw/lm32/../milkymist-softusb.o
> >> 
> >> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c: In function
> >> ‘softusb_mouse_hid_datain’:
> >> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:183:38: error:
> >> ‘m’ may be used uninitialized in this function
> >> [-Werror=maybe-uninitialized]
> >> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:179:13: note:
> >> ‘m’ was declared here
> >> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c: In function
> >> ‘softusb_kbd_hid_datain’:
> >> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:197:38: error:
> >> ‘m’ may be used uninitialized in this function
> >> [-Werror=maybe-uninitialized]
> >> /home/aliguori/git/qemu/hw/lm32/../milkymist-softusb.c:193:13: note:
> >> ‘m’ was declared here cc1: all warnings being treated as errors
> >> make[1]: *** [hw/lm32/../milkymist-softusb.o] Error 1
> > 
> > are you sure, this patch breaks the build, or was is broken before?
> > 
> > i'll send a patch soon.
> 
> My compiler doesn't complain. I suspect you may be right and
> the problem was already there. Anyway I think that adding
> a 'memset(buf, 0, len);' after the error_report() calls
> in softusb_read_pmem()/softusb_read_dmem() will fix it.

Sounds good. Are you fixing it, or should i send a patch?
diff mbox

Patch

diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
index aa0865f..5782e1a 100644
--- a/hw/milkymist-hw.h
+++ b/hw/milkymist-hw.h
@@ -194,12 +194,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 d911686..b279d4e 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(),
 };