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".
> 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?
> +
> + 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?
> + }
> + 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.)
> + }
> +
> + 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?
> #endif /* HW_ARM_SMMU_COMMON */
> --
thanks
-- PMM
