[Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Peter Maydell]

The functions sysbus_add_memory and sysbus_del_memory are odd wrappers
around around memory_region_add/del_subregion, and their presence
is an encouragement to devices to try to map their own memory
regions into the system address space.

Since they're only used in a couple of places in the milkymist
and musicpal platforms, rewrite those uses to have the sysbus
devices expose the memory regions as sysbus mmio regions, and
then have the creator of the device (ie board code) map them
in the right places. Then we can remove the functions altogether.

The series includes a trivial patch to sysbus to make the init
method optional, since (as part of the move towards using only
instance_init and realize) it's now possible to have a simple
functional device which only needs an instance_init method
and no realize or init [the musicpal-misc device introduced
in patch 2 being one such example].

Tested on both musicpal and milkymist.

I rather suspect sysbus_add_io and sysbus_del_io should also be
removed, but since their users are in PPC and x86 platforms I'll
let somebody else do that part :-)

Changes v2->v3:
 * changed field name of parent obj in MusicPalMiscState to
   'parent_obj' as per convention
 * rebased on master
Changes v1->v2:
 * updated 'qdevify musicpal-misc' to drop unneeded typedef
   and QOM macros, as per review discussion


Peter Maydell (5):
  sysbus: make SysBusDeviceClass::init optional
  musicpal: qdevify musicpal-misc
  milkymist-minimac2: Just expose buffers as a sysbus mmio region
  milkymist-softusb: Don't map RAM memory regions in the device itself
  sysbus: Remove sysbus_add_memory and sysbus_del_memory

 hw/arm/musicpal.c       |   28 +++++++++++++++++++++++-----
 hw/milkymist-hw.h       |    6 +++---
 hw/milkymist-minimac2.c |    5 +----
 hw/milkymist-softusb.c  |   21 +++++++++++----------
 hw/sysbus.c             |   21 +++------------------
 hw/sysbus.h             |    5 -----
 6 files changed, 41 insertions(+), 45 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v3 1/5] sysbus: make SysBusDeviceClass::init optional

[Peter Maydell]

Make the SysBusDeviceClass::init optional, for devices which
genuinely don't need to do anything here. In particular, simple
devices which can do all their initialization in their
instance_init method don't need either a DeviceClass::realize
or SysBusDeviceClass::init method.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Andreas Färber <afaerber@suse.de>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/sysbus.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sysbus.c b/hw/sysbus.c
index 702fc72..9a19468 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -137,6 +137,9 @@ static int sysbus_device_init(DeviceState *dev)
     SysBusDevice *sd = SYS_BUS_DEVICE(dev);
     SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
 
+    if (!sbc->init) {
+        return 0;
+    }
     return sbc->init(sd);
 }
 
-- 
1.7.9.5

[Qemu-devel] [PATCH v3 2/5] musicpal: qdevify musicpal-misc

[Peter Maydell]

Make musicpal-misc into its own (trivial) qdev device, so we
can get rid of the abuse of sysbus_add_memory().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/arm/musicpal.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index a37dbd7..63d8e13 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1031,6 +1031,15 @@ static const TypeInfo mv88w8618_flashcfg_info = {
 
 #define MP_BOARD_REVISION       0x31
 
+typedef struct {
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+} MusicPalMiscState;
+
+#define TYPE_MUSICPAL_MISC "musicpal-misc"
+#define MUSICPAL_MISC(obj) \
+     OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC)
+
 static uint64_t musicpal_misc_read(void *opaque, hwaddr offset,
                                    unsigned size)
 {
@@ -1054,15 +1063,23 @@ static const MemoryRegionOps musicpal_misc_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void musicpal_misc_init(SysBusDevice *dev)
+static void musicpal_misc_init(Object *obj)
 {
-    MemoryRegion *iomem = g_new(MemoryRegion, 1);
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    MusicPalMiscState *s = MUSICPAL_MISC(obj);
 
-    memory_region_init_io(iomem, &musicpal_misc_ops, NULL,
+    memory_region_init_io(&s->iomem, &musicpal_misc_ops, NULL,
                           "musicpal-misc", MP_MISC_SIZE);
-    sysbus_add_memory(dev, MP_MISC_BASE, iomem);
+    sysbus_init_mmio(sd, &s->iomem);
 }
 
+static const TypeInfo musicpal_misc_info = {
+    .name = TYPE_MUSICPAL_MISC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_init = musicpal_misc_init,
+    .instance_size = sizeof(MusicPalMiscState),
+};
+
 /* WLAN register offsets */
 #define MP_WLAN_MAGIC1          0x11c
 #define MP_WLAN_MAGIC2          0x124
@@ -1612,7 +1629,7 @@ static void musicpal_init(QEMUMachineInitArgs *args)
 
     sysbus_create_simple("mv88w8618_wlan", MP_WLAN_BASE, NULL);
 
-    musicpal_misc_init(SYS_BUS_DEVICE(dev));
+    sysbus_create_simple(TYPE_MUSICPAL_MISC, MP_MISC_BASE, NULL);
 
     dev = sysbus_create_simple("musicpal_gpio", MP_GPIO_BASE, pic[MP_GPIO_IRQ]);
     i2c_dev = sysbus_create_simple("gpio_i2c", -1, NULL);
@@ -1692,6 +1709,7 @@ static void musicpal_register_types(void)
     type_register_static(&musicpal_lcd_info);
     type_register_static(&musicpal_gpio_info);
     type_register_static(&musicpal_key_info);
+    type_register_static(&musicpal_misc_info);
 }
 
 type_init(musicpal_register_types)
-- 
1.7.9.5

[Qemu-devel] [PATCH v3 3/5] milkymist-minimac2: Just expose buffers as a sysbus mmio region

[Peter Maydell]

Just expose the register buffers memory as a standard sysbus mmio
region which the creator of the device can map, rather than
providing a qdev property which the creator has to set to the
base address and then doing the mapping in the device's own
init function.

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       |    2 +-
 hw/milkymist-minimac2.c |    5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
index ced1c5f..aa0865f 100644
--- a/hw/milkymist-hw.h
+++ b/hw/milkymist-hw.h
@@ -177,10 +177,10 @@ static inline DeviceState *milkymist_minimac2_create(hwaddr base,
 
     qemu_check_nic_model(&nd_table[0], "minimac2");
     dev = qdev_create(NULL, "milkymist-minimac2");
-    qdev_prop_set_taddr(dev, "buffers_base", buffers_base);
     qdev_set_nic_properties(dev, &nd_table[0]);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, buffers_base);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, rx_irq);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 1, tx_irq);
 
diff --git a/hw/milkymist-minimac2.c b/hw/milkymist-minimac2.c
index c20ff90..29618e8 100644
--- a/hw/milkymist-minimac2.c
+++ b/hw/milkymist-minimac2.c
@@ -96,7 +96,6 @@ struct MilkymistMinimac2State {
     NICState *nic;
     NICConf conf;
     char *phy_model;
-    hwaddr buffers_base;
     MemoryRegion buffers;
     MemoryRegion regs_region;
 
@@ -475,7 +474,7 @@ static int milkymist_minimac2_init(SysBusDevice *dev)
     s->rx1_buf = s->rx0_buf + MINIMAC2_BUFFER_SIZE;
     s->tx_buf = s->rx1_buf + MINIMAC2_BUFFER_SIZE;
 
-    sysbus_add_memory(dev, s->buffers_base, &s->buffers);
+    sysbus_init_mmio(dev, &s->buffers);
 
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
     s->nic = qemu_new_nic(&net_milkymist_minimac2_info, &s->conf,
@@ -517,8 +516,6 @@ static const VMStateDescription vmstate_milkymist_minimac2 = {
 };
 
 static Property milkymist_minimac2_properties[] = {
-    DEFINE_PROP_TADDR("buffers_base", MilkymistMinimac2State,
-    buffers_base, 0),
     DEFINE_NIC_PROPERTIES(MilkymistMinimac2State, conf),
     DEFINE_PROP_STRING("phy_model", MilkymistMinimac2State, phy_model),
     DEFINE_PROP_END_OF_LIST(),
-- 
1.7.9.5

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

[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.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(-)

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

[Qemu-devel] [PATCH v3 5/5] sysbus: Remove sysbus_add_memory and sysbus_del_memory

[Peter Maydell]

Remove the sysbus_add_memory and sysbus_del_memory functions. These
are trivial wrappers for mapping a memory region into the system
memory space, and have no users now.  Sysbus devices should never map
their own memory regions anyway; the correct API for mapping an mmio
region is for the creator of the device to use sysbus_mmio_map.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/sysbus.c |   18 ------------------
 hw/sysbus.h |    5 -----
 2 files changed, 23 deletions(-)

diff --git a/hw/sysbus.c b/hw/sysbus.c
index 9a19468..9004d8c 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -236,24 +236,6 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
     return g_strdup(path);
 }
 
-void sysbus_add_memory(SysBusDevice *dev, hwaddr addr,
-                       MemoryRegion *mem)
-{
-    memory_region_add_subregion(get_system_memory(), addr, mem);
-}
-
-void sysbus_add_memory_overlap(SysBusDevice *dev, hwaddr addr,
-                               MemoryRegion *mem, unsigned priority)
-{
-    memory_region_add_subregion_overlap(get_system_memory(), addr, mem,
-                                        priority);
-}
-
-void sysbus_del_memory(SysBusDevice *dev, MemoryRegion *mem)
-{
-    memory_region_del_subregion(get_system_memory(), mem);
-}
-
 void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
                        MemoryRegion *mem)
 {
diff --git a/hw/sysbus.h b/hw/sysbus.h
index 5d90a52..7c2e316 100644
--- a/hw/sysbus.h
+++ b/hw/sysbus.h
@@ -58,11 +58,6 @@ void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
 void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
 void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
                              unsigned priority);
-void sysbus_add_memory(SysBusDevice *dev, hwaddr addr,
-                       MemoryRegion *mem);
-void sysbus_add_memory_overlap(SysBusDevice *dev, hwaddr addr,
-                               MemoryRegion *mem, unsigned priority);
-void sysbus_del_memory(SysBusDevice *dev, MemoryRegion *mem);
 void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
                    MemoryRegion *mem);
 void sysbus_del_io(SysBusDevice *dev, MemoryRegion *mem);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Paolo Bonzini]

Il 15/03/2013 15:34, Peter Maydell ha scritto:
> I rather suspect sysbus_add_io and sysbus_del_io should also be
> removed, but since their users are in PPC and x86 platforms I'll
> let somebody else do that part :-)

sysbus_add_io and sysbus_del_io are actually a good match for the I/O
address space of x86, because the model was to have "well-known" port
numbers standardized across all platforms.  So all the boards would have
to know those port addresses if we used sysbus_init_mmio.

Paolo

Re: [Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Peter Maydell]

On 15 March 2013 16:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 15/03/2013 15:34, Peter Maydell ha scritto:
>> I rather suspect sysbus_add_io and sysbus_del_io should also be
>> removed, but since their users are in PPC and x86 platforms I'll
>> let somebody else do that part :-)
>
> sysbus_add_io and sysbus_del_io are actually a good match for the I/O
> address space of x86, because the model was to have "well-known" port
> numbers standardized across all platforms.  So all the boards would have
> to know those port addresses if we used sysbus_init_mmio.

Maybe they should just call memory_region_add_subregion()
directly then? There's nothing sysbus-device-specific about
what these functions do, they just take a SysBusDevice* and
totally ignore it...

-- PMM

Re: [Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Paolo Bonzini]

Il 15/03/2013 17:09, Peter Maydell ha scritto:
> On 15 March 2013 16:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 15/03/2013 15:34, Peter Maydell ha scritto:
>>> I rather suspect sysbus_add_io and sysbus_del_io should also be
>>> removed, but since their users are in PPC and x86 platforms I'll
>>> let somebody else do that part :-)
>>
>> sysbus_add_io and sysbus_del_io are actually a good match for the I/O
>> address space of x86, because the model was to have "well-known" port
>> numbers standardized across all platforms.  So all the boards would have
>> to know those port addresses if we used sysbus_init_mmio.
> 
> Maybe they should just call memory_region_add_subregion()
> directly then? There's nothing sysbus-device-specific about
> what these functions do, they just take a SysBusDevice* and
> totally ignore it...

It affects the OpenFirmware path, but perhaps we can move the
get_fw_dev_path from the Bus to the Device class.

Paolo

Re: [Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Peter Maydell]

Ping!

-- PMM

On 15 March 2013 14:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> The functions sysbus_add_memory and sysbus_del_memory are odd wrappers
> around around memory_region_add/del_subregion, and their presence
> is an encouragement to devices to try to map their own memory
> regions into the system address space.
>
> Since they're only used in a couple of places in the milkymist
> and musicpal platforms, rewrite those uses to have the sysbus
> devices expose the memory regions as sysbus mmio regions, and
> then have the creator of the device (ie board code) map them
> in the right places. Then we can remove the functions altogether.
>
> The series includes a trivial patch to sysbus to make the init
> method optional, since (as part of the move towards using only
> instance_init and realize) it's now possible to have a simple
> functional device which only needs an instance_init method
> and no realize or init [the musicpal-misc device introduced
> in patch 2 being one such example].
>
> Tested on both musicpal and milkymist.
>
> I rather suspect sysbus_add_io and sysbus_del_io should also be
> removed, but since their users are in PPC and x86 platforms I'll
> let somebody else do that part :-)
>
> Changes v2->v3:
>  * changed field name of parent obj in MusicPalMiscState to
>    'parent_obj' as per convention
>  * rebased on master
> Changes v1->v2:
>  * updated 'qdevify musicpal-misc' to drop unneeded typedef
>    and QOM macros, as per review discussion
>
>
> Peter Maydell (5):
>   sysbus: make SysBusDeviceClass::init optional
>   musicpal: qdevify musicpal-misc
>   milkymist-minimac2: Just expose buffers as a sysbus mmio region
>   milkymist-softusb: Don't map RAM memory regions in the device itself
>   sysbus: Remove sysbus_add_memory and sysbus_del_memory
>
>  hw/arm/musicpal.c       |   28 +++++++++++++++++++++++-----
>  hw/milkymist-hw.h       |    6 +++---
>  hw/milkymist-minimac2.c |    5 +----
>  hw/milkymist-softusb.c  |   21 +++++++++++----------
>  hw/sysbus.c             |   21 +++------------------
>  hw/sysbus.h             |    5 -----
>  6 files changed, 41 insertions(+), 45 deletions(-)
>
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [PATCH v3 2/5] musicpal: qdevify musicpal-misc

[Andreas Färber]

Am 15.03.2013 15:34, schrieb Peter Maydell:
> Make musicpal-misc into its own (trivial) qdev device, so we
> can get rid of the abuse of sysbus_add_memory().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Andreas Färber <afaerber@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 v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Andreas Färber]

Am 28.03.2013 12:25, schrieb Peter Maydell:
> Ping!

All patches have at least one Reviewed-by or Acked-by including that of
the lm32 maintainer, so you could just apply these to arm-devs.next, no?

Andreas

> 
> -- PMM
> 
> On 15 March 2013 14:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The functions sysbus_add_memory and sysbus_del_memory are odd wrappers
>> around around memory_region_add/del_subregion, and their presence
>> is an encouragement to devices to try to map their own memory
>> regions into the system address space.
>>
>> Since they're only used in a couple of places in the milkymist
>> and musicpal platforms, rewrite those uses to have the sysbus
>> devices expose the memory regions as sysbus mmio regions, and
>> then have the creator of the device (ie board code) map them
>> in the right places. Then we can remove the functions altogether.
>>
>> The series includes a trivial patch to sysbus to make the init
>> method optional, since (as part of the move towards using only
>> instance_init and realize) it's now possible to have a simple
>> functional device which only needs an instance_init method
>> and no realize or init [the musicpal-misc device introduced
>> in patch 2 being one such example].
>>
>> Tested on both musicpal and milkymist.
>>
>> I rather suspect sysbus_add_io and sysbus_del_io should also be
>> removed, but since their users are in PPC and x86 platforms I'll
>> let somebody else do that part :-)
>>
>> Changes v2->v3:
>>  * changed field name of parent obj in MusicPalMiscState to
>>    'parent_obj' as per convention
>>  * rebased on master
>> Changes v1->v2:
>>  * updated 'qdevify musicpal-misc' to drop unneeded typedef
>>    and QOM macros, as per review discussion
>>
>>
>> Peter Maydell (5):
>>   sysbus: make SysBusDeviceClass::init optional
>>   musicpal: qdevify musicpal-misc
>>   milkymist-minimac2: Just expose buffers as a sysbus mmio region
>>   milkymist-softusb: Don't map RAM memory regions in the device itself
>>   sysbus: Remove sysbus_add_memory and sysbus_del_memory
>>
>>  hw/arm/musicpal.c       |   28 +++++++++++++++++++++++-----
>>  hw/milkymist-hw.h       |    6 +++---
>>  hw/milkymist-minimac2.c |    5 +----
>>  hw/milkymist-softusb.c  |   21 +++++++++++----------
>>  hw/sysbus.c             |   21 +++------------------
>>  hw/sysbus.h             |    5 -----
>>  6 files changed, 41 insertions(+), 45 deletions(-)
>>
>> --
>> 1.7.9.5
>>
>>
> 


-- 
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 v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Peter Maydell]

On 28 March 2013 15:15, Andreas Färber <afaerber@suse.de> wrote:
> Am 28.03.2013 12:25, schrieb Peter Maydell:
>> Ping!
>
> All patches have at least one Reviewed-by or Acked-by including that of
> the lm32 maintainer, so you could just apply these to arm-devs.next, no?

Well, I could, but they're not really arm patches and I kind of
feel like one of the reasons my pullreqs are accepted is because
I don't stuff unrelated patches into them.

-- PMM

Re: [Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Paolo Bonzini]

> On 28 March 2013 15:15, Andreas Färber <afaerber@suse.de> wrote:
> > Am 28.03.2013 12:25, schrieb Peter Maydell:
> >> Ping!
> >
> > All patches have at least one Reviewed-by or Acked-by including
> > that of
> > the lm32 maintainer, so you could just apply these to
> > arm-devs.next, no?
> 
> Well, I could, but they're not really arm patches and I kind of
> feel like one of the reasons my pullreqs are accepted is because
> I don't stuff unrelated patches into them.

If that is your worry, you could just request a pull from a different
branch than usual.

Paolo

Re: [Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 28 March 2013 15:15, Andreas Färber <afaerber@suse.de> wrote:
>> Am 28.03.2013 12:25, schrieb Peter Maydell:
>>> Ping!
>>
>> All patches have at least one Reviewed-by or Acked-by including that of
>> the lm32 maintainer, so you could just apply these to arm-devs.next, no?
>
> Well, I could, but they're not really arm patches and I kind of
> feel like one of the reasons my pullreqs are accepted is because
> I don't stuff unrelated patches into them.

I'll apply these..  In the future, please CC me when sending patches
like this.

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Peter Maydell]

On 28 March 2013 15:56, Anthony Liguori <aliguori@us.ibm.com> wrote:
> I'll apply these..  In the future, please CC me when sending patches
> like this.

Yes, I tend to forget to CC you on general type patches (as
opposed to ones where you're specifically a maintainer for
the area); hence the ping with you added to the cc list.

thanks
-- PMM

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

[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

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

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

[Michael Walle]

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.

-- 
michael

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

[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.

-- PMM

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

[Michael Walle]

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?

-- 
michael

Re: [Qemu-devel] [PATCH v3 0/5] Remove sysbus_add_memory and sysbus_del_memory

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori