Re: [Qemu-devel] [PATCH v9 11/14] target/arm/kvm: Translate the MSI doorbell in kvm_arch_fixup_msi_route

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,
On 12/03/18 12:59, Peter Maydell wrote:
> On 17 February 2018 at 18:46, Eric Auger <eric.auger@redhat.com> wrote:
>> In case the MSI is translated by an IOMMU we need to fixup the
>> MSI route with the translated address.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v5 -> v6:
>> - use IOMMUMemoryRegionClass API
>>
>> It is still unclear to me if we need to register an IOMMUNotifier
>> to handle any change in the MSI doorbell which would occur behind
>> the scene and would not lead to any call to kvm_arch_fixup_msi_route().
> 
> Paolo, do you know the answer to this question ?
> 
>> ---
>>  target/arm/kvm.c        | 27 +++++++++++++++++++++++++++
>>  target/arm/trace-events |  3 +++
>>  2 files changed, 30 insertions(+)
>>
>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>> index 1219d00..9f5976a 100644
>> --- a/target/arm/kvm.c
>> +++ b/target/arm/kvm.c
>> @@ -20,8 +20,13 @@
>>  #include "sysemu/kvm.h"
>>  #include "kvm_arm.h"
>>  #include "cpu.h"
>> +#include "trace.h"
>>  #include "internals.h"
>>  #include "hw/arm/arm.h"
>> +#include "hw/pci/pci.h"
>> +#include "hw/pci/msi.h"
>> +#include "hw/arm/smmu-common.h"
>> +#include "hw/arm/smmuv3.h"
>>  #include "exec/memattrs.h"
>>  #include "exec/address-spaces.h"
>>  #include "hw/boards.h"
>> @@ -666,6 +671,28 @@ int kvm_arm_vgic_probe(void)
>>  int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>>                               uint64_t address, uint32_t data, PCIDevice *dev)
>>  {
>> +    AddressSpace *as = pci_device_iommu_address_space(dev);
>> +    IOMMUMemoryRegionClass *imrc;
>> +    IOMMUTLBEntry entry;
>> +    SMMUDevice *sdev;
>> +
>> +    if (as == &address_space_memory) {
>> +        return 0;
>> +    }
>> +
>> +    /* MSI doorbell address is translated by an IOMMU */
>> +    sdev = container_of(as, SMMUDevice, as);
>> +    imrc = IOMMU_MEMORY_REGION_GET_CLASS(&sdev->iommu);
>> +
>> +    entry = imrc->translate(&sdev->iommu, address, IOMMU_WO);
>> +
>> +    route->u.msi.address_lo = entry.translated_addr;
>> +    route->u.msi.address_hi = entry.translated_addr >> 32;
>> +
>> +    trace_kvm_arm_fixup_msi_route(address, sdev->devfn,
>> +                                  sdev->iommu.parent_obj.name,
>> +                                  entry.translated_addr);
>> +
>>      return 0;
>>  }
> 
> It seems a bit odd that:
>  * the code for arm for "PCI devices behind IOMMU need to have
>    the MSI doorbell writes go through the IOMMU" looks rather
>    different from the code for x86 for the same thing
ARM SMMU translates MSIs whereas Intel/AMD IOMMU do not translate them.
Hence this implementation
>  * the code here needs to know specifically that this is an SMMU
>    and not some other kind of IOMMU
Yes when introducing virtio-iommu we will need to get this fixed. We
need a way to retrieve the iommu mr from the as. I will work on this
concurrently.
> 
> I would have expected this to be more generic-to-all-IOMMU
> APIs and maybe even implemented in the generic KVM support code...
> 
> The x86 code seems to be similarly x86-specific though, so
> this is more of a "perhaps there is a cleanup opportunity here"
> observation I guess.

OK

Thanks

Eric
> 
> thanks
> -- PMM
>