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

[Auger Eric]

Hi Peter,

On 04/17/2018 01:02 PM, Peter Maydell wrote:
> On 12 April 2018 at 08:38, Eric Auger <address@hidden> 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 <address@hidden>
>> Signed-off-by: Bharat Bhushan <address@hidden>
>>
>> ---
>> 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
>