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().
---
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;
}
diff --git a/target/arm/trace-events b/target/arm/trace-events
index 9e37131..8b3c220 100644
--- a/target/arm/trace-events
+++ b/target/arm/trace-events
@@ -8,3 +8,6 @@ arm_gt_tval_write(int timer, uint64_t value) "gt_tval_write: timer %d value 0x%"
arm_gt_ctl_write(int timer, uint64_t value) "gt_ctl_write: timer %d value 0x%" PRIx64
arm_gt_imask_toggle(int timer, int irqstate) "gt_ctl_write: timer %d IMASK toggle, new irqstate %d"
arm_gt_cntvoff_write(uint64_t value) "gt_cntvoff_write: value 0x%" PRIx64
+
+# target/arm/kvm.c
+kvm_arm_fixup_msi_route(uint64_t iova, uint32_t devid, const char *name, uint64_t gpa) "MSI addr = 0x%"PRIx64" is translated for devfn=%d through %s into 0x%"PRIx64
--
2.5.5
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
* the code here needs to know specifically that this is an SMMU
and not some other kind of IOMMU
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.
thanks
-- PMM
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
>
On 12/03/2018 16:16, Auger Eric wrote: >> 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 ? Yes, x86 is wrong in this respect (it wouldn't use an IOMMUNotifier, but it still should process Interrupt Entry Cache invalidations). >> 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 More precisely, Intel IOMMU implements interrupt remapping through an MMIO region instead of an IOMMU region, because on x86 interrupt remapping can also change the MSI value and not just the address. >> * 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. Probably something like address_space_translate can be used? Paolo
Hi Paolo, On 13/03/18 14:37, Paolo Bonzini wrote: > On 12/03/2018 16:16, Auger Eric wrote: >>> 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 ? > > Yes, x86 is wrong in this respect (it wouldn't use an IOMMUNotifier, but > it still should process Interrupt Entry Cache invalidations). I am not sure anymore of the issue we want to fix. The concen I had in mind was: what if the MSI doorbell gets unmapped. We would need to be notified in some way. As the doorbell lies in MMIO space, no IOMMUNotifier is attached to it at the moment. On the other hand, if the doorbell gets unmapped, can't we expect some kind of tear down of the route? Paolo, is it the use case you are talking about here? > >>> 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 > > More precisely, Intel IOMMU implements interrupt remapping through an > MMIO region instead of an IOMMU region, because on x86 interrupt > remapping can also change the MSI value and not just the address. thank you for the clarification. Yes I too much focused on IOMMU DMA translation and forgot the x86 interrupt remapping stuff which corresponds to what is done here. > >>> * 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. > > Probably something like address_space_translate can be used? Yes achieved with address_space_translate + memory_region_find Thanks Eric > > Paolo >
© 2016 - 2025 Red Hat, Inc.