Re: [Qemu-devel] [PATCH for-2.10] xlnx-qspi: add a property for mmio-execution

[Peter Maydell <peter.maydell@linaro.org>]

On 10 August 2017 at 14:08, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>
>
> On 08/10/2017 02:43 PM, Peter Maydell wrote:
>>
>> On 10 August 2017 at 13:21, KONRAD Frederic <frederic.konrad@adacore.com>
>> wrote:
>>>

>> Looking at the implementation it seems like this will work in
>> practice (because the invalidate code in memory.c checks that
>> the MR it's about to drop really is an MMIO_INTERFACE), but
>> if so we should document this. However, I'm not totally convinced
>> that it really will work in complicated cases like where you
>> have device A part of whose memory range is a container which
>> holds a device B which is also using the mmio_pointer API:
>> in that case if device A calls invalidate_mmio_ptr with an
>> address that's in the part of A's memory region that is the
>> device B container it could end up invalidating an mmio-interface
>> that's actually inside device B. So it would be safer to say
>> "the caller may only invalidate pointers it's actually told
>> the memory system about".
>>
>
> I see what you mean but I'm not sure this will happen because the
> mmio-interface is mapped on @mr which is passed to invalidate.
>
> So if device A doesn't have any mmio-interface mapped it can't
> find the device B mmio-interface as we pass device A
> MemoryRegion.

I think what you're saying is (1) you can't create an mmio-ptr
for a container region (2) when you call invalidate_mmio_ptr
you have to pass exactly the MR that has the memory-region-ops
that the have the request_ptr set. This isn't entirely clear
from the docs, but OTOH it seems reasonable and I agree that
in that case you can't get the situation I suggested above.

> But I agree the doc is a little confusing about that.
>
>> (Conversely if we're convinced that passing bogus pointers
>> to memory_region_invalidate_mmio_ptr() is fine then we
>> don't need to add the q->mmio_execution_enabled flag check.)
>
>
> True but this will trigger an async_safe_work with all the
> overhead.

If we care about that then we should have a "have we used the
cached region as an mmio_ptr" flag separately anyway, so that
we don't trigger async_safe_work for the case of "not executing
from the memory, but we had to call lqspi_load_cache() because
the guest did a read from some address that was outside the
cached section".

thanks
-- PMM