Hi Peter,
On 06/03/18 15:08, Peter Maydell wrote:
> On 17 February 2018 at 18:46, Eric Auger <eric.au...@redhat.com> 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 <eric.au...@redhat.com>
>>
>> ---
>> 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
>
