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

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,

On 07/05/2018 02:56 PM, Peter Maydell wrote:
> On 5 July 2018 at 08:27, Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>> ---
>>  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];

I don't see any reason either. This code is inherited from
hw/i386.intel_iommu.c vtd_find_add_as() which does the same allocation
and I cannot find any reason out there either. This is not a
justification though ;-)

Can I fix it in 3.1 or do you want me to fix it for 3.0?

Thanks

Eric
> ?
> 
> thanks
> -- PMM
>