Re: [v2 PATCH] iommu/arm-smmu-v3: Fix L1 stream table index calculation for 32-bit sid size

[Yang Shi <yang@os.amperecomputing.com>]

On 10/2/24 12:40 PM, Jason Gunthorpe wrote:
> On Wed, Oct 02, 2024 at 12:22:48PM -0700, Nicolin Chen wrote:
>> On Wed, Oct 02, 2024 at 12:04:32PM -0700, Yang Shi wrote:
>>>> On Wed, Oct 02, 2024 at 10:55:14AM -0700, Yang Shi wrote:
>>>>> +static inline unsigned int arm_smmu_strtab_max_sid(struct arm_smmu_device *smmu)
>>>>> +{
>>>>> +       return (1ULL << smmu->sid_bits);
>>>>> +}
>>>>> +
>>>> Hmm, why ULL gets truncated to unsigned int here?
>>> No particular reason, but it should be better to not truncate here. Will
>>> fix it.
>> Yea, and looks like we are going to do with:
>> static inline u64 arm_smmu_strtab_num_sids(struct arm_smmu_device *smmu);
>>
>> Then let's be careful at those return-value holders too:
>> -----------------------------------------------------------
>> static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
>> {
>> 	u32 size;
>> 	struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
>>
>> 	size = (1 << smmu->sid_bits) * sizeof(struct arm_smmu_ste);
>>          ^^^^
>>          overflow?
>> [...]
>> 	cfg->linear.num_ents = 1 << smmu->sid_bits;
>>                      ^^^^^^^^
>>                      This is u32
>> -----------------------------------------------------------

Instead of relying in separate emails, I'd prefer reply in one single place.

This is linear stream table code, IIUC nobody actually implements such 
large sid size with linear table per the earlier discussions.

> It would make some sense to have something like:
>
>   u64 size = arm_smmu_strtab_max_sid()
>
>   /* Would require too much memory */
>   if (size > SZ_512M)
>      return -EINVAL;

This extra check is definitely fine to me and makes sense. SZ_512M is 1 
<< 29, so it means no hardware actually has 29 bit sid size with linear 
stream table. It makes sense to me. But I'm not smmu expert, so just 
would like to double check such configuration is not existing.

>
> Just to reject bad configuration rather than truncate the allocation
> and overflow STE array memory or something. Having drivers be robust
> to this kind of stuff is a confidential compute topic :\
>
> Jason