Re: [PATCH v9 12/13] KVM: PPC: Add support for IOMMU in-kernel handling

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 09/06/2013 04:10 AM, Gleb Natapov wrote:
> On Wed, Sep 04, 2013 at 02:01:28AM +1000, Alexey Kardashevskiy wrote:
>> On 09/03/2013 08:53 PM, Gleb Natapov wrote:
>>> On Mon, Sep 02, 2013 at 01:14:29PM +1000, Alexey Kardashevskiy wrote:
>>>> On 09/01/2013 10:06 PM, Gleb Natapov wrote:
>>>>> On Wed, Aug 28, 2013 at 06:50:41PM +1000, Alexey Kardashevskiy wrote:
>>>>>> This allows the host kernel to handle H_PUT_TCE, H_PUT_TCE_INDIRECT
>>>>>> and H_STUFF_TCE requests targeted an IOMMU TCE table without passing
>>>>>> them to user space which saves time on switching to user space and back.
>>>>>>
>>>>>> Both real and virtual modes are supported. The kernel tries to
>>>>>> handle a TCE request in the real mode, if fails it passes the request
>>>>>> to the virtual mode to complete the operation. If it a virtual mode
>>>>>> handler fails, the request is passed to user space.
>>>>>>
>>>>>> The first user of this is VFIO on POWER. Trampolines to the VFIO external
>>>>>> user API functions are required for this patch.
>>>>>>
>>>>>> This adds a "SPAPR TCE IOMMU" KVM device to associate a logical bus
>>>>>> number (LIOBN) with an VFIO IOMMU group fd and enable in-kernel handling
>>>>>> of map/unmap requests. The device supports a single attribute which is
>>>>>> a struct with LIOBN and IOMMU fd. When the attribute is set, the device
>>>>>> establishes the connection between KVM and VFIO.
>>>>>>
>>>>>> Tests show that this patch increases transmission speed from 220MB/s
>>>>>> to 750..1020MB/s on 10Gb network (Chelsea CXGB3 10Gb ethernet card).
>>>>>>
>>>>>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Changes:
>>>>>> v9:
>>>>>> * KVM_CAP_SPAPR_TCE_IOMMU ioctl to KVM replaced with "SPAPR TCE IOMMU"
>>>>>> KVM device
>>>>>> * release_spapr_tce_table() is not shared between different TCE types
>>>>>> * reduced the patch size by moving VFIO external API
>>>>>> trampolines to separate patche
>>>>>> * moved documentation from Documentation/virtual/kvm/api.txt to
>>>>>> Documentation/virtual/kvm/devices/spapr_tce_iommu.txt
>>>>>>
>>>>>> v8:
>>>>>> * fixed warnings from check_patch.pl
>>>>>>
>>>>>> 2013/07/11:
>>>>>> * removed multiple #ifdef IOMMU_API as IOMMU_API is always enabled
>>>>>> for KVM_BOOK3S_64
>>>>>> * kvmppc_gpa_to_hva_and_get also returns host phys address. Not much sense
>>>>>> for this here but the next patch for hugepages support will use it more.
>>>>>>
>>>>>> 2013/07/06:
>>>>>> * added realmode arch_spin_lock to protect TCE table from races
>>>>>> in real and virtual modes
>>>>>> * POWERPC IOMMU API is changed to support real mode
>>>>>> * iommu_take_ownership and iommu_release_ownership are protected by
>>>>>> iommu_table's locks
>>>>>> * VFIO external user API use rewritten
>>>>>> * multiple small fixes
>>>>>>
>>>>>> 2013/06/27:
>>>>>> * tce_list page is referenced now in order to protect it from accident
>>>>>> invalidation during H_PUT_TCE_INDIRECT execution
>>>>>> * added use of the external user VFIO API
>>>>>>
>>>>>> 2013/06/05:
>>>>>> * changed capability number
>>>>>> * changed ioctl number
>>>>>> * update the doc article number
>>>>>>
>>>>>> 2013/05/20:
>>>>>> * removed get_user() from real mode handlers
>>>>>> * kvm_vcpu_arch::tce_tmp usage extended. Now real mode handler puts there
>>>>>> translated TCEs, tries realmode_get_page() on those and if it fails, it
>>>>>> passes control over the virtual mode handler which tries to finish
>>>>>> the request handling
>>>>>> * kvmppc_lookup_pte() now does realmode_get_page() protected by BUSY bit
>>>>>> on a page
>>>>>> * The only reason to pass the request to user mode now is when the user mode
>>>>>> did not register TCE table in the kernel, in all other cases the virtual mode
>>>>>> handler is expected to do the job
>>>>>> ---
>>>>>>  .../virtual/kvm/devices/spapr_tce_iommu.txt        |  37 +++
>>>>>>  arch/powerpc/include/asm/kvm_host.h                |   4 +
>>>>>>  arch/powerpc/kvm/book3s_64_vio.c                   | 310 ++++++++++++++++++++-
>>>>>>  arch/powerpc/kvm/book3s_64_vio_hv.c                | 122 ++++++++
>>>>>>  arch/powerpc/kvm/powerpc.c                         |   1 +
>>>>>>  include/linux/kvm_host.h                           |   1 +
>>>>>>  virt/kvm/kvm_main.c                                |   5 +
>>>>>>  7 files changed, 477 insertions(+), 3 deletions(-)
>>>>>>  create mode 100644 Documentation/virtual/kvm/devices/spapr_tce_iommu.txt
>>>>>>
>>>>>> diff --git a/Documentation/virtual/kvm/devices/spapr_tce_iommu.txt b/Documentation/virtual/kvm/devices/spapr_tce_iommu.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..4bc8fc3
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/virtual/kvm/devices/spapr_tce_iommu.txt
>>>>>> @@ -0,0 +1,37 @@
>>>>>> +SPAPR TCE IOMMU device
>>>>>> +
>>>>>> +Capability: KVM_CAP_SPAPR_TCE_IOMMU
>>>>>> +Architectures: powerpc
>>>>>> +
>>>>>> +Device type supported: KVM_DEV_TYPE_SPAPR_TCE_IOMMU
>>>>>> +
>>>>>> +Groups:
>>>>>> +  KVM_DEV_SPAPR_TCE_IOMMU_ATTR_LINKAGE
>>>>>> +  Attributes: single attribute with pair { LIOBN, IOMMU fd}
>>>>>> +
>>>>>> +This is completely made up device which provides API to link
>>>>>> +logical bus number (LIOBN) and IOMMU group. The user space has
>>>>>> +to create a new SPAPR TCE IOMMU device per a logical bus.
>>>>>> +
>>>>> Why not have one device that can handle multimple links?
>>>>
>>>>
>>>> I can do that. If I make it so, it won't even look as a device at all, just
>>>> some weird interface to KVM but ok. What bothers me is it is just a
>>> May be I do not understand usage pattern here. Why do you feel that device
>>> that can handle multiple links is worse than device per link? How many logical
>>> buses is there usually? How often they created/destroyed? I am not insisting
>>> on the change, just trying to understand why you do not like it.
>>
>>
>> Is it usually one PCI host bus adapter per IOMMU group which is usually
>> one PCI card or 2-3 cards if it is a legacy PCI-X, and they are created
>> when QEMU-KVM starts. Not many. And they live till KVM ends.
>>
>> My point is why would I want to put all links to one device? It all is just
>> a matter of taste and nothing more. Or I am missing something but I do not
>> see what. If it is all about making thing to be kosher/halal/orthodox, then
>> I have more stuff to do, like reworking the emulated TCEs. But if is it for
>> (I do not know, just guessing) performance or something like that - then
>> I'll fix it, I just need to know what I am fixing.
>>
> Each device creates an fd, if you can have a lot of them eventually this
> will be a bottleneck. You are saying this is not the case, so lets go
> with proposed interface.


Did you decide not to answer the email which Ben sent yesterday or you just
did not see it? Just checking :)



-- 
Alexey