Hi,
On 5/1/20 4:30 PM, Alexandru Elisei wrote:
> 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.
I take that back, I've tested your proposal and it's not working. Let's consider
the scenario with 3 threads where the a mmio node is created with a refcount of 1,
as you suggested (please excuse any bad formatting):
1 | 2 | 3
1. mmio_get() | |
2. refcount++(== 2)| |
| 1. deactivate BAR |
| 2. BAR active |
| | 1. deactivate BAR
| | 2. BAR active
| | 3. refcount--(==1)
| 3. refcount--(==0) |
| 4. remove mmio node|
3. mmio->mmio_fn() | |
When Thread 1 calls mmio->mmio_fn(), the mmio node has been free'd. My original
version works because the only place where I decrement refcount is after the call
to mmio_fn. I really don't want to use locking when calling pci_deactivate_bar. Do
you have any ideas?
Thanks,
Alex
