| Message ID | 1518893216-9983-3-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM SMMUv3 Emulation Support | expand |
On 17 February 2018 at 18:46, Eric Auger <eric.auger@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.auger@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
Hi Peter, On 06/03/18 15:08, Peter Maydell wrote: > On 17 February 2018 at 18:46, Eric Auger <eric.auger@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.auger@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 >
On 6 March 2018 at 14:47, Auger Eric <eric.auger@redhat.com> wrote: > Hi Peter, > > On 06/03/18 15:08, Peter Maydell wrote: >> On 17 February 2018 at 18:46, Eric Auger <eric.auger@redhat.com> wrote: >>> + 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. No, it's fine here. You should just have a comment about why it makes sense to lazily populate the hashtable (it's overall going to be slower than if we populated it up front), to document the rationale for the design decision. thanks -- PMM
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]; + + 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; + } + } + } + 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); + } + + 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; +} #endif /* HW_ARM_SMMU_COMMON */
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 the bus number allows to find the list of SMMUDevices. Signed-off-by: Eric Auger <eric.auger@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(+)