Hi Peter,
On 04/17/2018 01:02 PM, Peter Maydell wrote:
> On 12 April 2018 at 08:38, Eric Auger <eric.au...@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.au...@redhat.com>
>> Signed-off-by: Bharat Bhushan <bharat.bhus...@nxp.com>
>>
>> ---
>> v9 -> v10:
>> - use address_space_translate
>>
>> v5 -> v6:
>> - use IOMMUMemoryRegionClass API
>> ---
>> 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 ecc39ac..c6f6aa8 100644
>> --- a/target/arm/kvm.c
>> +++ b/target/arm/kvm.c
>> @@ -20,8 +20,10 @@
>> #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 "exec/memattrs.h"
>> #include "exec/address-spaces.h"
>> #include "hw/boards.h"
>> @@ -649,6 +651,31 @@ 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);
>> + hwaddr xlat, len, doorbell_gpa;
>> + MemoryRegionSection mrs;
>> + MemoryRegion *mr;
>> +
>> + if (as == &address_space_memory) {
>> + return 0;
>> + }
>> +
>> + /* MSI doorbell address is translated by an IOMMU */
>> +
>> + rcu_read_lock();
>> + mr = address_space_translate(as, address, &xlat, &len, true);
>> + if (!mr) {
>> + return 1;
>
> This early-return code path fails to release the rcu_read_lock.
ough, fixed.
>
>> + }
>> + mrs = memory_region_find(mr, xlat, 0);
>
> Is it really valid to pass a size of 0 to memory_region_find() ?
> We should probably use however big the doorbell write is.
Using size=1 as most of the callers do.
>
> Do we need to do something here to handle the case where
> memory_region_find() doesn't find a a region, or does that
> just work automatically?
Added a check on mrs.mr.
Also released the ref on mrs.mr after usage.
Thanks
Eric
>
>> + doorbell_gpa = mrs.offset_within_address_space;
>> + rcu_read_unlock();
>> +
>> + route->u.msi.address_lo = doorbell_gpa;
>> + route->u.msi.address_hi = doorbell_gpa >> 32;
>> +
>> + trace_kvm_arm_fixup_msi_route(address, doorbell_gpa);
>> +
>> return 0;
>> }
>>
>> diff --git a/target/arm/trace-events b/target/arm/trace-events
>> index 9e37131..6b759f9 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, uint64_t gpa) "MSI iova =
>> 0x%"PRIx64" is translated into 0x%"PRIx64
>> --
>> 2.5.5
>
> thanks
> -- PMM
>
