Hi,
On 3/31/20 12:51 PM, André Przywara wrote:
> On 26/03/2020 15:24, 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.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>> ---
>> include/kvm/ioport.h | 2 +
>> include/kvm/rbtree-interval.h | 4 +-
>> ioport.c | 64 +++++++++++++++++-------
>> mmio.c | 91 +++++++++++++++++++++++++----------
>> 4 files changed, 118 insertions(+), 43 deletions(-)
>>
>> diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h
>> index 62a719327e3f..039633f76bdd 100644
>> --- a/include/kvm/ioport.h
>> +++ b/include/kvm/ioport.h
>> @@ -22,6 +22,8 @@ struct ioport {
>> struct ioport_operations *ops;
>> void *priv;
>> struct device_header dev_hdr;
>> + u32 refcount;
>> + bool remove;
> The use of this extra "remove" variable seems somehow odd. I think
> normally you would initialise the refcount to 1, and let the unregister
> operation do a put as well, with the removal code triggered if the count
> reaches zero. At least this is what kref does, can we do the same here?
> Or is there anything that would prevent it? I think it's a good idea to
> stick to existing design patterns for things like refcounts.
>
> Cheers,
> Andre.
>
You're totally right, it didn't cross my mind to initialize refcount to 1, it's a
great idea, I'll do it like that.
Thanks,
Alex
