| Message ID | 1360928706-13041-5-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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.maydell@linaro.org> Reviewed-by: Andreas Färber <afaerber@suse.de> Andreas
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.maydell@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, 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(), > }; >
On 15 February 2013 15:21, Paolo Bonzini <pbonzini@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
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
On 15 February 2013 16:06, Paolo Bonzini <pbonzini@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
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
On 15 February 2013 16:18, Paolo Bonzini <pbonzini@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
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.maydell@linaro.org> 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 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(), > };
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
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(), };
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> --- hw/milkymist-hw.h | 4 ++-- hw/milkymist-softusb.c | 21 +++++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-)