| Message ID | 20180426150901.30278-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] mmio-exec: Make device return MemoryRegion rather than host pointer | expand |
On 26/04/2018 17:09, Peter Maydell wrote: > > QUESTIONS: > * do we really need to postpone things from > memory_region_invalidate_mmio_ptr() to > memory_region_do_invalidate_mmio_ptr(), given that the deletion of > the memory region and ramblock is RCU-deferred anyway? > * should we add the subregion at elevated prio so it definitely hits > first? I've left it the way the existing code does for the > moment... > * is there any way to avoid the weird self-owning MemoryRegion > and corresponding need to pass a NULL name pointer? There would be, but the comment here says why it'd be an issue: /* Memory regions without an owner are supposed to never go away; * we do not ref/unref them because it slows down DMA sensibly. */ if (mr && mr->owner) { object_ref(mr->owner); } though perhaps we can remove that other hack in 2.13, which will have MemoryRegionCache again. Paolo
On 04/26/2018 05:09 PM, Peter Maydell wrote: > Our current interface for allowing a device to support execution from > MMIO regions has the device return a pointer into host memory > containing the contents to be used for execution. Unfortunately the > obvious implementation this encourages (and which our only in-tree > user uses) is to have a single buffer which is reused for each new > request_ptr call. This doesn't work, because RCU means that removal > of the old RAMBlock based on the host pointer may be deferred until a > point after a new RAMBlock with the same pointer has been added to > the list. The RAMBlock APIs assume that the host-pointer to ram_addr > mapping is unique, and mmio-exec is breaking that assumption. > > Instead, change the API so that the device is responsible for > allocating a new RAM MemoryRegion and populating it. The > MemoryRegion will be g_free()d when the region is eventually > invalidated or otherwise unneeded. > > HACKS: > * Note that in order for the refcounting to work correctly without > having to manufacture a device purely to be the owner of the > MemoryRegion, we must pass OBJECT(newmr) as the owner (so the MR is > its own owner!) and NULL as the name (or the property created on the > owner causes the refcount to go negative during finalization)... > > QUESTIONS: > * do we really need to postpone things from > memory_region_invalidate_mmio_ptr() to > memory_region_do_invalidate_mmio_ptr(), given that the deletion of > the memory region and ramblock is RCU-deferred anyway? > * should we add the subregion at elevated prio so it definitely hits > first? I've left it the way the existing code does for the > moment... > * is there any way to avoid the weird self-owning MemoryRegion > and corresponding need to pass a NULL name pointer? About refcounting and self-owning MemoryRegion: I put the MemoryRegion in the mmio_interface to avoid this issues and to passes the base address, size, etc. Fred > > NOTES: > * This change means that hw/misc/mmio_interface.c is no longer > used; if this gets beyond RFC stage then another patch to > delete it can follow. > * The "request_mmio_exec mustn't fail after it has called > memory_region_mmio_ptr_alloc()" requirement is a bit ugly, but > could probably be removed.
On 04/26/2018 05:09 PM, Peter Maydell wrote: > Our current interface for allowing a device to support execution from > MMIO regions has the device return a pointer into host memory > containing the contents to be used for execution. Unfortunately the > obvious implementation this encourages (and which our only in-tree > user uses) is to have a single buffer which is reused for each new > request_ptr call. This doesn't work, because RCU means that removal > of the old RAMBlock based on the host pointer may be deferred until a > point after a new RAMBlock with the same pointer has been added to > the list. The RAMBlock APIs assume that the host-pointer to ram_addr > mapping is unique, and mmio-exec is breaking that assumption. yes. I have run into the same problem while implementing mmio-exec for the aspeed fmc controllers. The FW (U-boot) in some configuration does a CRC calculation which jumps from one code section to another. This trashes the MMIO cache a very large number of times and the RAM block list can contain up to 40 occurrences of the same cache ... it is nearly impossible not to hit the bug you describe. This patch fixes the issue but maybe an another approach could be to introduce a cache allocator which would make sure that host-pointers are unique ? Thanks, C. > Instead, change the API so that the device is responsible for > allocating a new RAM MemoryRegion and populating it. The > MemoryRegion will be g_free()d when the region is eventually > invalidated or otherwise unneeded. > > HACKS: > * Note that in order for the refcounting to work correctly without > having to manufacture a device purely to be the owner of the > MemoryRegion, we must pass OBJECT(newmr) as the owner (so the MR is > its own owner!) and NULL as the name (or the property created on the > owner causes the refcount to go negative during finalization)... > > QUESTIONS: > * do we really need to postpone things from > memory_region_invalidate_mmio_ptr() to > memory_region_do_invalidate_mmio_ptr(), given that the deletion of > the memory region and ramblock is RCU-deferred anyway? > * should we add the subregion at elevated prio so it definitely hits > first? I've left it the way the existing code does for the > moment... > * is there any way to avoid the weird self-owning MemoryRegion > and corresponding need to pass a NULL name pointer? > > NOTES: > * This change means that hw/misc/mmio_interface.c is no longer > used; if this gets beyond RFC stage then another patch to > delete it can follow. > * The "request_mmio_exec mustn't fail after it has called > memory_region_mmio_ptr_alloc()" requirement is a bit ugly, but > could probably be removed. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This is decidedly RFC, but it is sufficient to get the xilinx-spips > mmio-execution test case to run without crashing. Mostly looking for > feedback on whether this is the right general direction or a load > of rubbish :-) > > include/exec/memory.h | 44 ++++++++++++++++++++---- > hw/ssi/xilinx_spips.c | 22 ++++++++---- > memory.c | 78 ++++++++++++++++++++++++++++--------------- > 3 files changed, 105 insertions(+), 39 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 31eae0a640..e55e06a166 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -139,14 +139,32 @@ struct MemoryRegionOps { > unsigned size, > MemTxAttrs attrs); > /* Instruction execution pre-callback: > - * @addr is the address of the access relative to the @mr. > - * @size is the size of the area returned by the callback. > - * @offset is the location of the pointer inside @mr. > + * @opaque is the opaque pointer for the MemoryRegion. > + * @alloc_token is a token to pass to memory_region_mmio_ptr_alloc() > + * @offset contains the address of the access relative to the @mr. > * > - * Returns a pointer to a location which contains guest code. > + * The request_mmio_exec callback must: > + * - adjust the offset downwards as desired and determine the size > + * of the region it wants to provide cached guest code for. > + * This must start on a guest page boundary and be a multiple > + * of a guest page in size. > + * - call memory_region_mmio_ptr_alloc(@alloc_token, size) > + * - fill in the contents of the RAM > + * Ownership of @newmr remains with the caller; it will be > + * automatically freed when no longer in use. > + * > + * If the callback cannot satisfy the request then it should > + * return false. Otherwise it must > + * - update @offset to the offset of the memory within the MemoryRegion > + * this is a callback for (ie the possibly rounded down value) > + * - return true > + * Note that if you return false then execution of the guest is going > + * to go wrong, either by QEMU delivering a bus abort exception to the > + * guest, or by simply exiting as unable to handle the situation, in > + * the same way that it would if you did not provide a request_mmio_exec > + * callback at all. > */ > - void *(*request_ptr)(void *opaque, hwaddr addr, unsigned *size, > - unsigned *offset); > + bool (*request_mmio_exec)(void *opaque, void *alloc_token, hwaddr *offset); > > enum device_endian endianness; > /* Guest-visible constraints: */ > @@ -1569,6 +1587,20 @@ bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr); > void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset, > unsigned size); > > +/** > + * memory_region_mmio_ptr_alloc: allocate memory from request_mmio_exec callback > + * > + * This function should be called only from a MemoryRegion's request_mmio_exec > + * callback function, in order to allocate the memory that should be filled > + * with the cached contents to be executed. > + * Note that once the request_mmio_exec callback calls this function it is > + * committed to handling the request, and may not return false. > + * > + * @alloc_token: the argument passed into the request_mmio_exec callback > + * @size: size in bytes to allocate > + */ > +void *memory_region_mmio_ptr_alloc(void *alloc_token, uint64_t size); > + > /** > * memory_region_dispatch_read: perform a read directly to the specified > * MemoryRegion. > diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c > index 03f5faee4b..8bb049f797 100644 > --- a/hw/ssi/xilinx_spips.c > +++ b/hw/ssi/xilinx_spips.c > @@ -1202,21 +1202,29 @@ static void lqspi_load_cache(void *opaque, hwaddr addr) > } > } > > -static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size, > - unsigned *offset) > +static bool lqspi_request_mmio_exec(void *opaque, void *alloc_token, > + hwaddr *offset) > { > XilinxQSPIPS *q = opaque; > hwaddr offset_within_the_region; > + void *hostptr; > > if (!q->mmio_execution_enabled) { > - return NULL; > + return false; > } > > - offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1); > + /* This could perhaps be cleverer and avoid the memcpy by > + * caching directly into the allocated area. > + */ > + offset_within_the_region = *offset & ~(LQSPI_CACHE_SIZE - 1); > lqspi_load_cache(opaque, offset_within_the_region); > - *size = LQSPI_CACHE_SIZE; > + > + hostptr = memory_region_mmio_ptr_alloc(alloc_token, LQSPI_CACHE_SIZE); > + > + memcpy(hostptr, q->lqspi_buf, LQSPI_CACHE_SIZE); > + > *offset = offset_within_the_region; > - return q->lqspi_buf; > + return true; > } > > static uint64_t > @@ -1240,7 +1248,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size) > > static const MemoryRegionOps lqspi_ops = { > .read = lqspi_read, > - .request_ptr = lqspi_request_mmio_ptr, > + .request_mmio_exec = lqspi_request_mmio_exec, > .endianness = DEVICE_NATIVE_ENDIAN, > .valid = { > .min_access_size = 1, > diff --git a/memory.c b/memory.c > index e70b64b8b9..71dec85156 100644 > --- a/memory.c > +++ b/memory.c > @@ -2653,41 +2653,74 @@ void memory_listener_unregister(MemoryListener *listener) > > bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr) > { > - void *host; > - unsigned size = 0; > - unsigned offset = 0; > - Object *new_interface; > + MemoryRegion *newmr; > + hwaddr offset; > > - if (!mr || !mr->ops->request_ptr) { > + if (!mr || !mr->ops->request_mmio_exec) { > return false; > } > > /* > - * Avoid an update if the request_ptr call > - * memory_region_invalidate_mmio_ptr which seems to be likely when we use > - * a cache. > + * Avoid an update if the request_mmio_exec hook calls > + * memory_region_invalidate_mmio_ptr, which it will do if the > + * device can only handle one outstanding MMIO exec region at once. > */ > memory_region_transaction_begin(); > > - host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset); > - > - if (!host || !size) { > + newmr = g_new0(MemoryRegion, 1); > + offset = addr - mr->addr; > + if (!mr->ops->request_mmio_exec(mr->opaque, newmr, &offset)) { > memory_region_transaction_commit(); > + g_free(newmr); > return false; > } > > - new_interface = object_new("mmio_interface"); > - qdev_prop_set_uint64(DEVICE(new_interface), "start", offset); > - qdev_prop_set_uint64(DEVICE(new_interface), "end", offset + size - 1); > - qdev_prop_set_bit(DEVICE(new_interface), "ro", true); > - qdev_prop_set_ptr(DEVICE(new_interface), "host_ptr", host); > - qdev_prop_set_ptr(DEVICE(new_interface), "subregion", mr); > - object_property_set_bool(OBJECT(new_interface), true, "realized", NULL); > + /* These requirements are because currently TCG needs a page sized > + * area to execute from. > + */ > + assert((offset & ~TARGET_PAGE_MASK) == 0); > + assert((memory_region_size(newmr) & ~TARGET_PAGE_MASK) == 0); > > + > + memory_region_add_subregion(mr, offset, newmr); > memory_region_transaction_commit(); > + > + /* Arrange that we automatically free this MemoryRegion when > + * its refcount goes to zero, which (once we've unrefed it here) > + * will happen when it is removed from the subregion. > + */ > + OBJECT(newmr)->free = g_free; > + object_unref(OBJECT(newmr)); > return true; > } > > +void *memory_region_mmio_ptr_alloc(void *alloc_token, uint64_t size) > +{ > + /* Allocate and return memory for an mmio_ptr request. This > + * is a function intended to be called from a memory region's > + * request_mmio_exec function, and the opaque pointer is the > + * newmr MemoryRegion allocated in memory_region_request_mmio_ptr(). > + */ > + MemoryRegion *newmr = alloc_token; > + Error *err = NULL; > + > + /* Note that in order for the refcounting to work correctly > + * without having to manufacture a device purely to be the > + * owner of the MemoryRegion, we must pass OBJECT(newmr) as > + * the owner (so the MR is its own owner!) and NULL as the name > + * (or the property created on the owner causes the refcount > + * to go negative during finalization)... > + */ > + memory_region_init_ram_nomigrate(newmr, OBJECT(newmr), NULL, > + size, &err); > + if (err) { > + error_free(err); > + return NULL; > + } > + > + return memory_region_get_ram_ptr(newmr); > +} > + > typedef struct MMIOPtrInvalidate { > MemoryRegion *mr; > hwaddr offset; > @@ -2714,15 +2747,8 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, > cpu_physical_memory_test_and_clear_dirty(offset, size, 1); > > if (section.mr != mr) { > - /* memory_region_find add a ref on section.mr */ > + memory_region_del_subregion(mr, section.mr); > memory_region_unref(section.mr); > - if (MMIO_INTERFACE(section.mr->owner)) { > - /* We found the interface just drop it. */ > - object_property_set_bool(section.mr->owner, false, "realized", > - NULL); > - object_unref(section.mr->owner); > - object_unparent(section.mr->owner); > - } > } > > qemu_mutex_unlock_iothread(); >
On 05/28/2018 07:22 AM, Cédric Le Goater wrote: > On 04/26/2018 05:09 PM, Peter Maydell wrote: >> Our current interface for allowing a device to support execution from >> MMIO regions has the device return a pointer into host memory >> containing the contents to be used for execution. Unfortunately the >> obvious implementation this encourages (and which our only in-tree >> user uses) is to have a single buffer which is reused for each new >> request_ptr call. This doesn't work, because RCU means that removal >> of the old RAMBlock based on the host pointer may be deferred until a >> point after a new RAMBlock with the same pointer has been added to >> the list. The RAMBlock APIs assume that the host-pointer to ram_addr >> mapping is unique, and mmio-exec is breaking that assumption. > > yes. I have run into the same problem while implementing mmio-exec > for the aspeed fmc controllers. > > The FW (U-boot) in some configuration does a CRC calculation which > jumps from one code section to another. This trashes the MMIO cache > a very large number of times and the RAM block list can contain up > to 40 occurrences of the same cache ... it is nearly impossible not > to hit the bug you describe. For the records, there are more than 31000 request/invalidate calls. > This patch fixes the issue There is still an issue (or a limitation) with the patch. It is not possible to reload the cache when a load is performed on the MMIO region from which QEMU is exec'ing. This breaks the TCG flow it seems. > but maybe an another approach could be to introduce a cache allocator > which would make sure that host-pointers are unique ? but freeing these cache lines is a problem. Same as of today ... Hmm, really a complex problem. Thanks, C. >> Instead, change the API so that the device is responsible for >> allocating a new RAM MemoryRegion and populating it. The >> MemoryRegion will be g_free()d when the region is eventually >> invalidated or otherwise unneeded. >> >> HACKS: >> * Note that in order for the refcounting to work correctly without >> having to manufacture a device purely to be the owner of the >> MemoryRegion, we must pass OBJECT(newmr) as the owner (so the MR is >> its own owner!) and NULL as the name (or the property created on the >> owner causes the refcount to go negative during finalization)... >> >> QUESTIONS: >> * do we really need to postpone things from >> memory_region_invalidate_mmio_ptr() to >> memory_region_do_invalidate_mmio_ptr(), given that the deletion of >> the memory region and ramblock is RCU-deferred anyway? >> * should we add the subregion at elevated prio so it definitely hits >> first? I've left it the way the existing code does for the >> moment... >> * is there any way to avoid the weird self-owning MemoryRegion >> and corresponding need to pass a NULL name pointer? >> >> NOTES: >> * This change means that hw/misc/mmio_interface.c is no longer >> used; if this gets beyond RFC stage then another patch to >> delete it can follow. >> * The "request_mmio_exec mustn't fail after it has called >> memory_region_mmio_ptr_alloc()" requirement is a bit ugly, but >> could probably be removed. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> This is decidedly RFC, but it is sufficient to get the xilinx-spips >> mmio-execution test case to run without crashing. Mostly looking for >> feedback on whether this is the right general direction or a load >> of rubbish :-) >> >> include/exec/memory.h | 44 ++++++++++++++++++++---- >> hw/ssi/xilinx_spips.c | 22 ++++++++---- >> memory.c | 78 ++++++++++++++++++++++++++++--------------- >> 3 files changed, 105 insertions(+), 39 deletions(-) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 31eae0a640..e55e06a166 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -139,14 +139,32 @@ struct MemoryRegionOps { >> unsigned size, >> MemTxAttrs attrs); >> /* Instruction execution pre-callback: >> - * @addr is the address of the access relative to the @mr. >> - * @size is the size of the area returned by the callback. >> - * @offset is the location of the pointer inside @mr. >> + * @opaque is the opaque pointer for the MemoryRegion. >> + * @alloc_token is a token to pass to memory_region_mmio_ptr_alloc() >> + * @offset contains the address of the access relative to the @mr. >> * >> - * Returns a pointer to a location which contains guest code. >> + * The request_mmio_exec callback must: >> + * - adjust the offset downwards as desired and determine the size >> + * of the region it wants to provide cached guest code for. >> + * This must start on a guest page boundary and be a multiple >> + * of a guest page in size. >> + * - call memory_region_mmio_ptr_alloc(@alloc_token, size) >> + * - fill in the contents of the RAM >> + * Ownership of @newmr remains with the caller; it will be >> + * automatically freed when no longer in use. >> + * >> + * If the callback cannot satisfy the request then it should >> + * return false. Otherwise it must >> + * - update @offset to the offset of the memory within the MemoryRegion >> + * this is a callback for (ie the possibly rounded down value) >> + * - return true >> + * Note that if you return false then execution of the guest is going >> + * to go wrong, either by QEMU delivering a bus abort exception to the >> + * guest, or by simply exiting as unable to handle the situation, in >> + * the same way that it would if you did not provide a request_mmio_exec >> + * callback at all. >> */ >> - void *(*request_ptr)(void *opaque, hwaddr addr, unsigned *size, >> - unsigned *offset); >> + bool (*request_mmio_exec)(void *opaque, void *alloc_token, hwaddr *offset); >> >> enum device_endian endianness; >> /* Guest-visible constraints: */ >> @@ -1569,6 +1587,20 @@ bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr); >> void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset, >> unsigned size); >> >> +/** >> + * memory_region_mmio_ptr_alloc: allocate memory from request_mmio_exec callback >> + * >> + * This function should be called only from a MemoryRegion's request_mmio_exec >> + * callback function, in order to allocate the memory that should be filled >> + * with the cached contents to be executed. >> + * Note that once the request_mmio_exec callback calls this function it is >> + * committed to handling the request, and may not return false. >> + * >> + * @alloc_token: the argument passed into the request_mmio_exec callback >> + * @size: size in bytes to allocate >> + */ >> +void *memory_region_mmio_ptr_alloc(void *alloc_token, uint64_t size); >> + >> /** >> * memory_region_dispatch_read: perform a read directly to the specified >> * MemoryRegion. >> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c >> index 03f5faee4b..8bb049f797 100644 >> --- a/hw/ssi/xilinx_spips.c >> +++ b/hw/ssi/xilinx_spips.c >> @@ -1202,21 +1202,29 @@ static void lqspi_load_cache(void *opaque, hwaddr addr) >> } >> } >> >> -static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size, >> - unsigned *offset) >> +static bool lqspi_request_mmio_exec(void *opaque, void *alloc_token, >> + hwaddr *offset) >> { >> XilinxQSPIPS *q = opaque; >> hwaddr offset_within_the_region; >> + void *hostptr; >> >> if (!q->mmio_execution_enabled) { >> - return NULL; >> + return false; >> } >> >> - offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1); >> + /* This could perhaps be cleverer and avoid the memcpy by >> + * caching directly into the allocated area. >> + */ >> + offset_within_the_region = *offset & ~(LQSPI_CACHE_SIZE - 1); >> lqspi_load_cache(opaque, offset_within_the_region); >> - *size = LQSPI_CACHE_SIZE; >> + >> + hostptr = memory_region_mmio_ptr_alloc(alloc_token, LQSPI_CACHE_SIZE); >> + >> + memcpy(hostptr, q->lqspi_buf, LQSPI_CACHE_SIZE); >> + >> *offset = offset_within_the_region; >> - return q->lqspi_buf; >> + return true; >> } >> >> static uint64_t >> @@ -1240,7 +1248,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size) >> >> static const MemoryRegionOps lqspi_ops = { >> .read = lqspi_read, >> - .request_ptr = lqspi_request_mmio_ptr, >> + .request_mmio_exec = lqspi_request_mmio_exec, >> .endianness = DEVICE_NATIVE_ENDIAN, >> .valid = { >> .min_access_size = 1, >> diff --git a/memory.c b/memory.c >> index e70b64b8b9..71dec85156 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -2653,41 +2653,74 @@ void memory_listener_unregister(MemoryListener *listener) >> >> bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr) >> { >> - void *host; >> - unsigned size = 0; >> - unsigned offset = 0; >> - Object *new_interface; >> + MemoryRegion *newmr; >> + hwaddr offset; >> >> - if (!mr || !mr->ops->request_ptr) { >> + if (!mr || !mr->ops->request_mmio_exec) { >> return false; >> } >> >> /* >> - * Avoid an update if the request_ptr call >> - * memory_region_invalidate_mmio_ptr which seems to be likely when we use >> - * a cache. >> + * Avoid an update if the request_mmio_exec hook calls >> + * memory_region_invalidate_mmio_ptr, which it will do if the >> + * device can only handle one outstanding MMIO exec region at once. >> */ >> memory_region_transaction_begin(); >> >> - host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset); >> - >> - if (!host || !size) { >> + newmr = g_new0(MemoryRegion, 1); >> + offset = addr - mr->addr; >> + if (!mr->ops->request_mmio_exec(mr->opaque, newmr, &offset)) { >> memory_region_transaction_commit(); >> + g_free(newmr); >> return false; >> } >> >> - new_interface = object_new("mmio_interface"); >> - qdev_prop_set_uint64(DEVICE(new_interface), "start", offset); >> - qdev_prop_set_uint64(DEVICE(new_interface), "end", offset + size - 1); >> - qdev_prop_set_bit(DEVICE(new_interface), "ro", true); >> - qdev_prop_set_ptr(DEVICE(new_interface), "host_ptr", host); >> - qdev_prop_set_ptr(DEVICE(new_interface), "subregion", mr); >> - object_property_set_bool(OBJECT(new_interface), true, "realized", NULL); >> + /* These requirements are because currently TCG needs a page sized >> + * area to execute from. >> + */ >> + assert((offset & ~TARGET_PAGE_MASK) == 0); >> + assert((memory_region_size(newmr) & ~TARGET_PAGE_MASK) == 0); >> >> + >> + memory_region_add_subregion(mr, offset, newmr); >> memory_region_transaction_commit(); >> + >> + /* Arrange that we automatically free this MemoryRegion when >> + * its refcount goes to zero, which (once we've unrefed it here) >> + * will happen when it is removed from the subregion. >> + */ >> + OBJECT(newmr)->free = g_free; >> + object_unref(OBJECT(newmr)); >> return true; >> } >> >> +void *memory_region_mmio_ptr_alloc(void *alloc_token, uint64_t size) >> +{ >> + /* Allocate and return memory for an mmio_ptr request. This >> + * is a function intended to be called from a memory region's >> + * request_mmio_exec function, and the opaque pointer is the >> + * newmr MemoryRegion allocated in memory_region_request_mmio_ptr(). >> + */ >> + MemoryRegion *newmr = alloc_token; >> + Error *err = NULL; >> + >> + /* Note that in order for the refcounting to work correctly >> + * without having to manufacture a device purely to be the >> + * owner of the MemoryRegion, we must pass OBJECT(newmr) as >> + * the owner (so the MR is its own owner!) and NULL as the name >> + * (or the property created on the owner causes the refcount >> + * to go negative during finalization)... >> + */ >> + memory_region_init_ram_nomigrate(newmr, OBJECT(newmr), NULL, >> + size, &err); >> + if (err) { >> + error_free(err); >> + return NULL; >> + } >> + >> + return memory_region_get_ram_ptr(newmr); >> +} >> + >> typedef struct MMIOPtrInvalidate { >> MemoryRegion *mr; >> hwaddr offset; >> @@ -2714,15 +2747,8 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, >> cpu_physical_memory_test_and_clear_dirty(offset, size, 1); >> >> if (section.mr != mr) { >> - /* memory_region_find add a ref on section.mr */ >> + memory_region_del_subregion(mr, section.mr); >> memory_region_unref(section.mr); >> - if (MMIO_INTERFACE(section.mr->owner)) { >> - /* We found the interface just drop it. */ >> - object_property_set_bool(section.mr->owner, false, "realized", >> - NULL); >> - object_unref(section.mr->owner); >> - object_unparent(section.mr->owner); >> - } >> } >> >> qemu_mutex_unlock_iothread(); >> >
diff --git a/include/exec/memory.h b/include/exec/memory.h index 31eae0a640..e55e06a166 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -139,14 +139,32 @@ struct MemoryRegionOps { unsigned size, MemTxAttrs attrs); /* Instruction execution pre-callback: - * @addr is the address of the access relative to the @mr. - * @size is the size of the area returned by the callback. - * @offset is the location of the pointer inside @mr. + * @opaque is the opaque pointer for the MemoryRegion. + * @alloc_token is a token to pass to memory_region_mmio_ptr_alloc() + * @offset contains the address of the access relative to the @mr. * - * Returns a pointer to a location which contains guest code. + * The request_mmio_exec callback must: + * - adjust the offset downwards as desired and determine the size + * of the region it wants to provide cached guest code for. + * This must start on a guest page boundary and be a multiple + * of a guest page in size. + * - call memory_region_mmio_ptr_alloc(@alloc_token, size) + * - fill in the contents of the RAM + * Ownership of @newmr remains with the caller; it will be + * automatically freed when no longer in use. + * + * If the callback cannot satisfy the request then it should + * return false. Otherwise it must + * - update @offset to the offset of the memory within the MemoryRegion + * this is a callback for (ie the possibly rounded down value) + * - return true + * Note that if you return false then execution of the guest is going + * to go wrong, either by QEMU delivering a bus abort exception to the + * guest, or by simply exiting as unable to handle the situation, in + * the same way that it would if you did not provide a request_mmio_exec + * callback at all. */ - void *(*request_ptr)(void *opaque, hwaddr addr, unsigned *size, - unsigned *offset); + bool (*request_mmio_exec)(void *opaque, void *alloc_token, hwaddr *offset); enum device_endian endianness; /* Guest-visible constraints: */ @@ -1569,6 +1587,20 @@ bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr); void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset, unsigned size); +/** + * memory_region_mmio_ptr_alloc: allocate memory from request_mmio_exec callback + * + * This function should be called only from a MemoryRegion's request_mmio_exec + * callback function, in order to allocate the memory that should be filled + * with the cached contents to be executed. + * Note that once the request_mmio_exec callback calls this function it is + * committed to handling the request, and may not return false. + * + * @alloc_token: the argument passed into the request_mmio_exec callback + * @size: size in bytes to allocate + */ +void *memory_region_mmio_ptr_alloc(void *alloc_token, uint64_t size); + /** * memory_region_dispatch_read: perform a read directly to the specified * MemoryRegion. diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c index 03f5faee4b..8bb049f797 100644 --- a/hw/ssi/xilinx_spips.c +++ b/hw/ssi/xilinx_spips.c @@ -1202,21 +1202,29 @@ static void lqspi_load_cache(void *opaque, hwaddr addr) } } -static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size, - unsigned *offset) +static bool lqspi_request_mmio_exec(void *opaque, void *alloc_token, + hwaddr *offset) { XilinxQSPIPS *q = opaque; hwaddr offset_within_the_region; + void *hostptr; if (!q->mmio_execution_enabled) { - return NULL; + return false; } - offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1); + /* This could perhaps be cleverer and avoid the memcpy by + * caching directly into the allocated area. + */ + offset_within_the_region = *offset & ~(LQSPI_CACHE_SIZE - 1); lqspi_load_cache(opaque, offset_within_the_region); - *size = LQSPI_CACHE_SIZE; + + hostptr = memory_region_mmio_ptr_alloc(alloc_token, LQSPI_CACHE_SIZE); + + memcpy(hostptr, q->lqspi_buf, LQSPI_CACHE_SIZE); + *offset = offset_within_the_region; - return q->lqspi_buf; + return true; } static uint64_t @@ -1240,7 +1248,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size) static const MemoryRegionOps lqspi_ops = { .read = lqspi_read, - .request_ptr = lqspi_request_mmio_ptr, + .request_mmio_exec = lqspi_request_mmio_exec, .endianness = DEVICE_NATIVE_ENDIAN, .valid = { .min_access_size = 1, diff --git a/memory.c b/memory.c index e70b64b8b9..71dec85156 100644 --- a/memory.c +++ b/memory.c @@ -2653,41 +2653,74 @@ void memory_listener_unregister(MemoryListener *listener) bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr) { - void *host; - unsigned size = 0; - unsigned offset = 0; - Object *new_interface; + MemoryRegion *newmr; + hwaddr offset; - if (!mr || !mr->ops->request_ptr) { + if (!mr || !mr->ops->request_mmio_exec) { return false; } /* - * Avoid an update if the request_ptr call - * memory_region_invalidate_mmio_ptr which seems to be likely when we use - * a cache. + * Avoid an update if the request_mmio_exec hook calls + * memory_region_invalidate_mmio_ptr, which it will do if the + * device can only handle one outstanding MMIO exec region at once. */ memory_region_transaction_begin(); - host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset); - - if (!host || !size) { + newmr = g_new0(MemoryRegion, 1); + offset = addr - mr->addr; + if (!mr->ops->request_mmio_exec(mr->opaque, newmr, &offset)) { memory_region_transaction_commit(); + g_free(newmr); return false; } - new_interface = object_new("mmio_interface"); - qdev_prop_set_uint64(DEVICE(new_interface), "start", offset); - qdev_prop_set_uint64(DEVICE(new_interface), "end", offset + size - 1); - qdev_prop_set_bit(DEVICE(new_interface), "ro", true); - qdev_prop_set_ptr(DEVICE(new_interface), "host_ptr", host); - qdev_prop_set_ptr(DEVICE(new_interface), "subregion", mr); - object_property_set_bool(OBJECT(new_interface), true, "realized", NULL); + /* These requirements are because currently TCG needs a page sized + * area to execute from. + */ + assert((offset & ~TARGET_PAGE_MASK) == 0); + assert((memory_region_size(newmr) & ~TARGET_PAGE_MASK) == 0); + + memory_region_add_subregion(mr, offset, newmr); memory_region_transaction_commit(); + + /* Arrange that we automatically free this MemoryRegion when + * its refcount goes to zero, which (once we've unrefed it here) + * will happen when it is removed from the subregion. + */ + OBJECT(newmr)->free = g_free; + object_unref(OBJECT(newmr)); return true; } +void *memory_region_mmio_ptr_alloc(void *alloc_token, uint64_t size) +{ + /* Allocate and return memory for an mmio_ptr request. This + * is a function intended to be called from a memory region's + * request_mmio_exec function, and the opaque pointer is the + * newmr MemoryRegion allocated in memory_region_request_mmio_ptr(). + */ + MemoryRegion *newmr = alloc_token; + Error *err = NULL; + + /* Note that in order for the refcounting to work correctly + * without having to manufacture a device purely to be the + * owner of the MemoryRegion, we must pass OBJECT(newmr) as + * the owner (so the MR is its own owner!) and NULL as the name + * (or the property created on the owner causes the refcount + * to go negative during finalization)... + */ + memory_region_init_ram_nomigrate(newmr, OBJECT(newmr), NULL, + size, &err); + if (err) { + error_free(err); + return NULL; + } + + return memory_region_get_ram_ptr(newmr); +} + typedef struct MMIOPtrInvalidate { MemoryRegion *mr; hwaddr offset; @@ -2714,15 +2747,8 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, cpu_physical_memory_test_and_clear_dirty(offset, size, 1); if (section.mr != mr) { - /* memory_region_find add a ref on section.mr */ + memory_region_del_subregion(mr, section.mr); memory_region_unref(section.mr); - if (MMIO_INTERFACE(section.mr->owner)) { - /* We found the interface just drop it. */ - object_property_set_bool(section.mr->owner, false, "realized", - NULL); - object_unref(section.mr->owner); - object_unparent(section.mr->owner); - } } qemu_mutex_unlock_iothread();
Our current interface for allowing a device to support execution from MMIO regions has the device return a pointer into host memory containing the contents to be used for execution. Unfortunately the obvious implementation this encourages (and which our only in-tree user uses) is to have a single buffer which is reused for each new request_ptr call. This doesn't work, because RCU means that removal of the old RAMBlock based on the host pointer may be deferred until a point after a new RAMBlock with the same pointer has been added to the list. The RAMBlock APIs assume that the host-pointer to ram_addr mapping is unique, and mmio-exec is breaking that assumption. Instead, change the API so that the device is responsible for allocating a new RAM MemoryRegion and populating it. The MemoryRegion will be g_free()d when the region is eventually invalidated or otherwise unneeded. HACKS: * Note that in order for the refcounting to work correctly without having to manufacture a device purely to be the owner of the MemoryRegion, we must pass OBJECT(newmr) as the owner (so the MR is its own owner!) and NULL as the name (or the property created on the owner causes the refcount to go negative during finalization)... QUESTIONS: * do we really need to postpone things from memory_region_invalidate_mmio_ptr() to memory_region_do_invalidate_mmio_ptr(), given that the deletion of the memory region and ramblock is RCU-deferred anyway? * should we add the subregion at elevated prio so it definitely hits first? I've left it the way the existing code does for the moment... * is there any way to avoid the weird self-owning MemoryRegion and corresponding need to pass a NULL name pointer? NOTES: * This change means that hw/misc/mmio_interface.c is no longer used; if this gets beyond RFC stage then another patch to delete it can follow. * The "request_mmio_exec mustn't fail after it has called memory_region_mmio_ptr_alloc()" requirement is a bit ugly, but could probably be removed. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This is decidedly RFC, but it is sufficient to get the xilinx-spips mmio-execution test case to run without crashing. Mostly looking for feedback on whether this is the right general direction or a load of rubbish :-) include/exec/memory.h | 44 ++++++++++++++++++++---- hw/ssi/xilinx_spips.c | 22 ++++++++---- memory.c | 78 ++++++++++++++++++++++++++++--------------- 3 files changed, 105 insertions(+), 39 deletions(-) -- 2.17.0