On 09/05/2012 03:02 PM, Jan Kiszka wrote:
> On 2012-09-05 13:25, Avi Kivity wrote:
>> On 09/05/2012 02:11 PM, Jan Kiszka wrote:
>>> On 2012-09-05 12:53, Avi Kivity wrote:
>>>> On 09/05/2012 01:36 PM, Jan Kiszka wrote:
>>>>>>
>>>>>> My current preference is MemoryRegionOps::ref(MemoryRegion *mr) (and a
>>>>>> corresponding unref), which has the following requirements:
>>>>>>
>>>>>> - if the refcount is nonzero, MemoryRegion::opaque is safe to use
>>>>>> - if the refcount is nonzero, the MemoryRegion itself is stable.
>>>>>
>>>>> The second point means that the memory subsystem will cache the region
>>>>> state and apply changes only after leaving a handler that performed them?
>>>>
>>>> No. I/O callbacks may be called after a region has been disabled.
>>>>
>>>> I guess we can restrict this to converted regions, so we don't introduce
>>>> regressions.
>>>
>>> s/can/have to/. This property change will require some special care,
>>> hopefully mostly at the memory layer. A simple scenario from recent patches:
>>>
>>> if (<enable>) {
>>> memory_region_set_address(&s->pm_io, pm_io_base);
>>> memory_region_set_enabled(&s->pm_io, true);
>>> } else {
>>> memory_region_set_enabled(&s->pm_io, false);
>>> }
>>
>> I am unable to avoid pointing out that this can be collapsed to
>>
>> memory_region_set_address(&s->pm_io, pm_io_base);
>> memory_region_set_enabled(&s->pm_io, <enable>);
>>
>> as the address is meaningless when disabled. Sorry.
>>
>>>
>>> We will have to ensure that set_enabled(..., true) will never cause a
>>> dispatch using an outdated base address.
>>
>> No, this is entirely safe. If the guest changes a region offset
>> concurrently with issuing mmio on it, then it must expect either the old
>> or new offset to be used during dispatch.
>
I forgot to mention that my clever rewrite above actually breaks this -
it needs to be wrapped in a transaction, otherwise we have a
move-then-disable pattern.
> You assume disable, reprogram, enable, ignoring that there could be
> multiple, invalid states between disable and enable. Real hardware takes
> care of the ordering.
It's possible of course, but the snippet above isn't susceptible on its own.
I don't think it's solvable. To serialize, you must hold the device
lock, but we don't want to take the device lock during dispatch.
Users can protect against them by checking for ->enabled:
void foo_io_read(...)
{
FooState *s = container_of(mr, ...);
qemu_mutex_lock(&s->lock);
if (!memory_region_enabled(mr)) {
*data = ~(uint64_t)0;
goto out;
}
...
out:
qemu_mutex_unlock(&s->lock);
}
Non-converted users will be naturally protected since we will take the
bql on their behalf.
--
error compiling committee.c: too many arguments to function
