[PATCH v2 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

[Ryan Roberts]

On 23/04/2024 17:39, Jason Gunthorpe wrote:
> On Tue, Apr 23, 2024 at 05:27:09PM +0100, Ryan Roberts wrote:
> 
>>>  	switch (smmu_domain->stage) {
>>>  	case ARM_SMMU_DOMAIN_S1:
>>>  		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
>>> -		ias = min_t(unsigned long, ias, VA_BITS);
>>> -		oas = smmu->ias;
>>> +		pgtbl_cfg.ias = min_t(unsigned long, ias, VA_BITS);
>>
>> I know this isn't changed by this patch, but do we really mean VA_BITS here?
>> Don't we want vabits_actual? I'm guessing we are intending to limit ias to the
>> size the kernel is using.
> 
> I think the intention here is to allow CONFIG_ARM64_VA_BITS to choose
> the number of page table levels in the iommu. 

Hmm ok, got it. But it seems odd...

> Perhaps a SMMU specific
> config option would be clearer.

Yes, it would certainly be clearer. Anyway, I don't think there is any required
action here. Thanks for explaining.

> 
> There is no relationship to the configuration of the S1 paging domain
> and what the MM is doing, there should be no crossing between those
> two worlds.
> 
> vabits_actual is used in the SVA code to match the SMMU SVA S1 to the
> mm_struct.
> 
>>> @@ -3206,6 +3212,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>>>  	if (user_data)
>>>  		return ERR_PTR(-EINVAL);
>>>  
>>> +	if (enforce_dirty &&
>>> +	    !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING))
>>> +		return ERR_PTR(-EOPNOTSUPP);
>>
>> I'm guessing the intention is that only a stage 1 will ever be marked with
>> IOMMU_CAP_DIRTY_TRACKING (there are a few places that assume/check we are
>> dealing with S1)? But is there a reason why stage 2 can't be supported as well?
> 
> Stage 2 really should ultimately be supported..
> 
> I'd say this is a first step as iommufd will naturally prefer the S1
> configuration when not using nesting. After the nesting support is
> merged the vSMMU will require the S2 to do the dirty tracking.

Yep, ok, so we should plan to support S2 as a later step. Sounds good.

> 
> Jason