Hi,
On 5/15/20 11:13 AM, André Przywara wrote:
> On 14/05/2020 16:38, Alexandru Elisei wrote:
>
> Hi,
>
>> kvmtool uses brlock for protecting accesses to the ioport and mmio
>> red-black trees. brlock allows concurrent reads, but only one writer, which
>> is assumed not to be a VCPU thread (for more information see commit
>> 0b907ed2eaec ("kvm tools: Add a brlock)). This is done by issuing a
>> compiler barrier on read and pausing the entire virtual machine on writes.
>> When KVM_BRLOCK_DEBUG is defined, brlock uses instead a pthread read/write
>> lock.
>>
>> When we will implement reassignable BARs, the mmio or ioport mapping will
>> be done as a result of a VCPU mmio access. When brlock is a pthread
>> read/write lock, it means that we will try to acquire a write lock with the
>> read lock already held by the same VCPU and we will deadlock. When it's
>> not, a VCPU will have to call kvm__pause, which means the virtual machine
>> will stay paused forever.
>>
>> Let's avoid all this by using a mutex and reference counting the red-black
>> tree entries. This way we can guarantee that we won't unregister a node
>> that another thread is currently using for emulation.
> It's a pity that we can't use the traditional refcount pattern
> (initialise to 1) here, but I trust your analysis and testing that this
> is needed.
If it helps, the pattern is somewhat similar to kobject, which has a kobject_del
function (kref doesn't). kboject_del decrements the reference count only if the
parent is not NULL, then sets parent to NULL. The patch does what kobject_del
would have done if it didn't have a kref embedded in it.
>
> As far as my brain can process, this looks alright now:
>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
>
> Just a note: the usage of return and goto seems now somewhat
> inconsistent here: there are cases where we do "unlock;return;" in the
> middle of the function (kvm__deregister_mmio), and other cases where we
> use a "goto out", even though there is just a return at that label
> (kvm__emulate_mmio). In ioport__unregister() you seem to switch the scheme.
> But this is just cosmetical, and we can fix this up later, should we
> care, so this doesn't hold back this patch.
There are a number of inconsistencies in both files: mmio doesn't have its own
header file and ioport has, kvm__deregister_mmio should use unregister (like
ioport), it returns bool instead of an int (like ioport), when unregistering a
mmio region we call KVM_UNREGISTER_COALESCED_MMIO unconditionally, ioport
unregisters all ioports on exit and mmio doesn't, ioport creates a new device for
each call to ioport__register, etc, etc. Not to mention the fact that ioport is
used for architectures that don't have the notion of I/O ports. I think these
issues deserve their own cleanup series.
Thanks,
Alex
