[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87ac9686-cffb-43ac-b8f0-ccd3632fe5cd@os.amperecomputing.com>
Date: Fri, 4 Oct 2024 14:47:48 -0700
From: Yang Shi <yang@...amperecomputing.com>
To: Daniel Mentz <danielmentz@...gle.com>
Cc: jgg@...pe.ca, nicolinc@...dia.com, james.morse@....com, will@...nel.org,
robin.murphy@....com, linux-arm-kernel@...ts.infradead.org,
iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [v3 PATCH] iommu/arm-smmu-v3: Fix L1 stream table index
calculation for 32-bit sid size
On 10/4/24 2:14 PM, Daniel Mentz wrote:
> On Fri, Oct 4, 2024 at 11:04 AM Yang Shi <yang@...amperecomputing.com> wrote:
>> static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
>> {
>> - u32 size;
>> + u64 size;
>> struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
>> + u64 num_sids = arm_smmu_strtab_num_sids(smmu);
>> +
>> + size = num_sids * sizeof(struct arm_smmu_ste);
>> + /* The max size for dmam_alloc_coherent() is 32-bit */
> I'd remove this comment. I assume the intent here was to say that the
> maximum size is 4GB (not 32 bit). I also can't find any reference to
> this limitation. Where does dmam_alloc_coherent() limit the size of an
> allocation to 4GB? Also, this comment might not be applicable to 64
> bit platforms.
The "size" parameter passed to dmam_alloc_coherent() is size_t type
which is unsigned int.
>
>> + if (size > SIZE_MAX)
>> + return -EINVAL;
> I'm assuming this is for platforms where the range of a u64 is larger
> than that of a size_t type? If we're printing an error message if an
> allocation fails (i.e. "failed to allocate linear stream table (%llu
> bytes)\n"), then we might also want to print an error message here.
Thanks for the suggestion. Yes, we can if it really helps.
>
>> - cfg->linear.num_ents = 1 << smmu->sid_bits;
>> + cfg->linear.num_ents = num_sids;
> If you're worried about 32 bit platforms, then I'm wondering if this
> also needs some attention. cfg->linear.num_ents is defined as an
> unsigned int and num_sids could potentially be outside the range of an
> unsigned int on 32 bit platforms.
The (size > SIZE_MAX) check can guarantee excessively large num_sids
won't reach here.
>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> index 1e9952ca989f..c8ceddc5e8ef 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> @@ -853,6 +853,11 @@ struct arm_smmu_master_domain {
>> ioasid_t ssid;
>> };
>>
>> +static inline u64 arm_smmu_strtab_num_sids(struct arm_smmu_device *smmu)
>> +{
>> + return (1ULL << smmu->sid_bits);
>> +}
>> +
> I'm wondering if it makes sense to move this up and put it right
> before arm_smmu_strtab_l1_idx(). That way, all the arm_smmu_strtab_*
> functions are in one place.
I did it. But the function uses struct arm_smmu_device which is defined
after those arm_smmu_strtab_* helpers. I have to put the helper after
struct arm_smmu_device definition to avoid compile error. We may
consider re-organize the header file to group them better, but I don't
think it is urgent enough and it seems out of the scope of the bug fix
patch. I really want to have the bug fix landed in upstream ASAP.
>
> On a related note, in arm_smmu_init_strtab_2lvl() we're capping the
> number of l1 entries at STRTAB_MAX_L1_ENTRIES for 2 level stream
> tables. I'm thinking it would make sense to limit the size of linear
> stream tables for the same reasons.
Yes, this also works. But I don't know what value should be used. Jason
actually suggested (size > SIZE_512M) in v2 review, but I thought the
value is a magic number. Why 512M? Just because it is too large for
allocation. So I picked up SIZE_MAX, just because it is the largest size
supported by size_t type.
Powered by blists - more mailing lists