Re: [Qemu-devel] [PATCH v9 02/14] hw/arm/smmu-common: IOMMU memory region and address space setup

[Auger Eric]

Hi Peter,

On 06/03/18 15:08, Peter Maydell wrote:
> On 17 February 2018 at 18:46, Eric Auger <address@hidden> wrote:
>> We enumerate all the PCI devices attached to the SMMU and
>> initialize an associated IOMMU memory region and address space.
>> This happens on SMMU base instance init.
>>
>> Those info are stored in SMMUDevice objects. The devices are
>> grouped according to the PCIBus they belong to. A hash table
>> indexed by the PCIBus poinet is used. Also an array indexed by
> 
> "pointer".
OK
> 
>> the bus number allows to find the list of SMMUDevices.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>> v8 -> v9:
>> - fix key value for lookup
>>
>> v7 -> v8:
>> - introduce SMMU_MAX_VA_BITS
>> - use PCI bus handle as a key
>> - do not clear s->smmu_as_by_bus_num
>> - use g_new0 instead of g_malloc0
>> - use primary_bus field
>> ---
>>  hw/arm/smmu-common.c         | 59 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/arm/smmu-common.h |  6 +++++
>>  2 files changed, 65 insertions(+)
>>
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index 86a5aab..d0516dc 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -28,12 +28,71 @@
>>  #include "qemu/error-report.h"
>>  #include "hw/arm/smmu-common.h"
>>
>> +SMMUPciBus *smmu_find_as_from_bus_num(SMMUState *s, uint8_t bus_num)
>> +{
>> +    SMMUPciBus *smmu_pci_bus = s->smmu_as_by_bus_num[bus_num];
> 
> Variable name suggests this is a table of AddressSpaces indexed by
> bus number, but the code says we're getting SMMUPCIBus objects from it?
Yes I can rename this variable. It stems from x86 naming (see
hw/intel_iommu.c vtd_find_as_from_bus_num). The code here does the same
functionality with arm smmu datatypes.
SMMUPciBus ~ VTDBus and smmu_as_by_bus_num ~ vtd_as_by_bus_num

purpose is to find the SMUPciBus which matches the bus number passed in
parameter.

> 
>> +
>> +    if (!smmu_pci_bus) {
>> +        GHashTableIter iter;
>> +
>> +        g_hash_table_iter_init(&iter, s->smmu_as_by_busptr);
>> +        while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) 
>> {
>> +            if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
>> +                s->smmu_as_by_bus_num[bus_num] = smmu_pci_bus;
>> +                return smmu_pci_bus;
>> +            }
>> +        }
> 
> Why do we populate this hashtable lazily rather than when we
> put the SMMUPciBus in the smmu_as_by_busptr table? Do we
> expect this function not to ordinarily be called?

This function only is used when handling invalidations (vhost/vfio). I
can even remove it from this series at this stage and re-introduce
latter on. From the SID, you retrieve bus_num, retrieve the SMMUPciBus.
Andd from the function number you can then retrieve the IOMMU mr at
smmu_bus->pbdev[devfn]

That's the purpose of ths function. But I will remove it.
> 
>> +    }
>> +    return smmu_pci_bus;
>> +}
>> +
>> +static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
>> +{
>> +    SMMUState *s = opaque;
>> +    SMMUPciBus *sbus = g_hash_table_lookup(s->smmu_as_by_busptr, bus);
>> +    SMMUDevice *sdev;
>> +
>> +    if (!sbus) {
>> +        sbus = g_malloc0(sizeof(SMMUPciBus) +
>> +                         sizeof(SMMUDevice *) * SMMU_PCI_DEVFN_MAX);
>> +        sbus->bus = bus;
>> +        g_hash_table_insert(s->smmu_as_by_busptr, bus, sbus);
>> +    }
>> +
>> +    sdev = sbus->pbdev[devfn];
>> +    if (!sdev) {
>> +        char *name = g_strdup_printf("%s-%d-%d",
>> +                                     s->mrtypename,
>> +                                     pci_bus_num(bus), devfn);
>> +        sdev = sbus->pbdev[devfn] = g_new0(SMMUDevice, 1);
>> +
>> +        sdev->smmu = s;
>> +        sdev->bus = bus;
>> +        sdev->devfn = devfn;
>> +
>> +        memory_region_init_iommu(&sdev->iommu, sizeof(sdev->iommu),
>> +                                 s->mrtypename,
>> +                                 OBJECT(s), name, 1ULL << SMMU_MAX_VA_BITS);
>> +        address_space_init(&sdev->as,
>> +                           MEMORY_REGION(&sdev->iommu), name);
> 
> This leaks the memory pointed to by name, doesn't it?
> (address_space_init() takes a copy of the name string, so you want
> to g_free(name) here.)
Hum yes. Will free it.
> 
>> +    }
>> +
>> +    return &sdev->as;
>> +}
>> +
>>  static void smmu_base_realize(DeviceState *dev, Error **errp)
>>  {
>>      SMMUState *s = ARM_SMMU(dev);
>>
>>      s->configs = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>>      s->iotlb = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>> +    s->smmu_as_by_busptr = g_hash_table_new(NULL, NULL);
>> +
>> +    if (s->primary_bus) {
>> +        pci_setup_iommu(s->primary_bus, smmu_find_add_as, s);
>> +    } else {
>> +        error_setg(errp, "SMMU is not attached to any PCI bus!");
>> +    }
>>  }
>>
>>  static void smmu_base_reset(DeviceState *dev)
>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>> index 8a9d931..aee96c2 100644
>> --- a/include/hw/arm/smmu-common.h
>> +++ b/include/hw/arm/smmu-common.h
>> @@ -121,4 +121,10 @@ typedef struct {
>>  #define ARM_SMMU_GET_CLASS(obj)                              \
>>      OBJECT_GET_CLASS(SMMUBaseClass, (obj), TYPE_ARM_SMMU)
>>
>> +SMMUPciBus *smmu_find_as_from_bus_num(SMMUState *s, uint8_t bus_num);
>> +
>> +static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
>> +{
>> +    return  ((pci_bus_num(sdev->bus) & 0xff) << 8) | sdev->devfn;
>> +}
> 
> Can we have at least brief documentation comments for any
> new functions or prototypes added to header files, please?
Sure.

Thanks

Eric
> 
>>  #endif  /* HW_ARM_SMMU_COMMON */
>> --
> 
> thanks
> -- PMM
>