Re: [Qemu-devel] [PATCH for 3.0] hw/arm/smmu-common: Fix devfn computation in smmu_iommu_mr

[Peter Maydell]

On 5 July 2018 at 08:27, Eric Auger <address@hidden> wrote:
> smmu_iommu_mr() aims at returning the IOMMUMemoryRegion corresponding
> to a given sid. The function extracts both the PCIe bus number and
> the devfn to return this data. Current computation of devfn is wrong
> as it only returns the PCIe function instead of slot | function.
>
> Fixes 32cfd7f39e08 ("hw/arm/smmuv3: Cache/invalidate config data")
>
> Signed-off-by: Eric Auger <address@hidden>
> ---
>  hw/arm/smmu-common.c         | 2 +-
>  include/hw/arm/smmu-common.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 3098915..55c75d6 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -351,7 +351,7 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t 
> sid)
>      bus_n = PCI_BUS_NUM(sid);
>      smmu_bus = smmu_find_smmu_pcibus(s, bus_n);
>      if (smmu_bus) {
> -        devfn = sid & 0x7;
> +        devfn = SMMU_PCI_DEVFN(sid);
>          smmu = smmu_bus->pbdev[devfn];
>          if (smmu) {
>              return &smmu->iommu;
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 50e2912..b07cadd 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -24,6 +24,7 @@
>
>  #define SMMU_PCI_BUS_MAX      256
>  #define SMMU_PCI_DEVFN_MAX    256
> +#define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
>
>  #define SMMU_MAX_VA_BITS      48

Applied to target-arm.next, thanks.

As I was reviewing this, I checked where we allocate the pbdev array
to confirm that it's big enough (which it is), and I noticed an oddity:
in include/hw/arm/smmu-common.h we define the SMMUPciBus struct like this:

typedef struct SMMUPciBus {
    PCIBus       *bus;
    SMMUDevice   *pbdev[0]; /* Parent array is sparse, so dynamically alloc */
} SMMUPciBus;

but in fact we don't ever have local variables of this type and the
only place we dynamically allocate them is in smmu_find_add_as(),
which does
        sbus = g_malloc0(sizeof(SMMUPciBus) +
                         sizeof(SMMUDevice *) * SMMU_PCI_DEVFN_MAX);

Is there a reason I missed why we don't just define the struct
to have
  SMMUDevice *pbdev[SMMU_PCI_DEVFN_MAX];
?

thanks
-- PMM