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

[Peter Maydell <peter.maydell@linaro.org>]

On 12 April 2018 at 08:37, Eric Auger <eric.auger@redhat.com> wrote:
> We set up the infrastructure to enumerate all the PCI devices
> attached to the SMMU and create an associated IOMMU memory
> region and address space.
>
> 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 pointer 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>
> Signed-off-by: Prem Mallappa <prem.mallappa@broadcom.com>
>
> ---
> v9 -> v10:
> - comment functions added to the header
> - g_free(name)
> - renamed smmu_find_as_from_bus_num into smmu_find_smmu_pcibus
> - add a comment about lazy init in smmu_find_smmu_pcibus
> - add a trace event when creating the smmu iommu mr
>
> 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         | 69 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/trace-events          |  3 ++
>  include/hw/arm/smmu-common.h |  8 +++++
>  3 files changed, 80 insertions(+)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index e086ff5..9a966bb 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -28,8 +28,69 @@
>  #include "qemu/error-report.h"
>  #include "hw/arm/smmu-common.h"
>
> +/**
> + * The bus number is used for lookup when SID based invalidatation occurs.

"invalidation"

> + * In that case we lazily populate the SMMUPciBus array from the bus hash
> + * table. At the time the SMMUPciBus is created (smmu_find_add_as), the bus
> + * numbers may not be always initialized yet.
> + */


> +/* Return the stream ID of an SMMU device */
> +static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
> +{
> +    return  ((pci_bus_num(sdev->bus) & 0xff) << 8) | sdev->devfn;

I think this is PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn)
and I suspect the mask with 0xff is unneeded (compare pci_get_bdf()
and amdvi_do_translate(), which just assume the bus number is sane).

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM