Hey Suravee,
On 17/10/2023 10:54, Joao Martins wrote:
> On 17/10/2023 09:18, Suthikulpanit, Suravee wrote:
>> On 9/23/2023 8:25 AM, Joao Martins wrote:
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * ----------------------------------------------------
>>> */
>>> @@ -527,6 +610,7 @@ static struct io_pgtable *v1_alloc_pgtable(struct
>>> io_pgtable_cfg *cfg, void *coo
>>> pgtable->iop.ops.map_pages = iommu_v1_map_pages;
>>> pgtable->iop.ops.unmap_pages = iommu_v1_unmap_pages;
>>> pgtable->iop.ops.iova_to_phys = iommu_v1_iova_to_phys;
>>> + pgtable->iop.ops.read_and_clear_dirty = iommu_v1_read_and_clear_dirty;
>>> return &pgtable->iop;
>>> }
>>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>>> index af36c627022f..31b333cc6fe1 100644
>>> --- a/drivers/iommu/amd/iommu.c
>>> +++ b/drivers/iommu/amd/iommu.c
>>> ....
>>> @@ -2156,11 +2160,17 @@ static inline u64 dma_max_address(void)
>>> return ((1ULL << PM_LEVEL_SHIFT(amd_iommu_gpt_level)) - 1);
>>> }
>>> +static bool amd_iommu_hd_support(struct amd_iommu *iommu)
>>> +{
>>> + return iommu && (iommu->features & FEATURE_HDSUP);
>>> +}
>>> +
>>
>> You can use the newly introduced check_feature(u64 mask) to check the HD support.
>>
>
> It appears that the check_feature() is logically equivalent to
> check_feature_on_all_iommus(); where this check is per-device/per-iommu check to
> support potentially nature of different IOMMUs with different features. Being
> per-IOMMU would allow you to have firmware to not advertise certain IOMMU
> features on some devices while still supporting for others. I understand this is
> not a thing in x86, but the UAPI supports it. Having said that, you still want
> me to switch to check_feature() ?
>
Do you have a strong preference here between current code and switching to
global check via check_feature() ?
> I think iommufd tree next branch is still in v6.6-rc2, so I am not sure I can
> really use check_feature() yet without leading Jason individual branch into
> compile errors. This all eventually gets merged into linux-next daily, but my
> impression is that individual maintainer's next is compilable? Worst case I
> submit a follow-up post merge cleanup to switch to check_feature()? [I can't use
> use check_feature_on_all_iommus() as that's removed by this commit below]
>
Jason, how do we usually handle this cross trees? check_feature() doesn't exist
in your tree, but it does in Joerg's tree; meanwhile
check_feature_on_all_iommus() gets renamed to check_feature(). Should I need to
go with it, do I rebase against linux-next? I have been assuming that your tree
must compile; or worst-case different maintainer pull each other's trees.
Alternatively: I can check the counter directly to replicate the amd_iommu_efr
check under the current helper I made (amd_iommu_hd_support) and then change it
after the fact... That should lead to less dependencies?
>> (See
>> https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git/commit/?h=next&id=7b7563a93437ef945c829538da28f0095f1603ec)
>>
>>> ...
>>> @@ -2252,6 +2268,9 @@ static int amd_iommu_attach_device(struct iommu_domain
>>> *dom,
>>> return 0;
>>> dev_data->defer_attach = false;
>>> + if (dom->dirty_ops && iommu &&
>>> + !(iommu->features & FEATURE_HDSUP))
>>
>> if (dom->dirty_ops && !check_feature(FEATURE_HDSUP))
>>
> OK -- will switch depending on above paragraph
>
>>> + return -EINVAL;
>>> if (dev_data->domain)
>>> detach_device(dev);
>>> @@ -2371,6 +2390,11 @@ static bool amd_iommu_capable(struct device *dev, enum
>>> iommu_cap cap)
>>> return true;
>>> case IOMMU_CAP_DEFERRED_FLUSH:
>>> return true;
>>> + case IOMMU_CAP_DIRTY: {
>>> + struct amd_iommu *iommu = rlookup_amd_iommu(dev);
>>> +
>>> + return amd_iommu_hd_support(iommu);
>>
>> return check_feature(FEATURE_HDSUP);
>>
> Likewise
