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.mayd...@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();
>>
>
