Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops

["Suthikulpanit, Suravee via iommu" <iommu@lists.linux-foundation.org>]

Robin,

On 6/14/2022 4:51 PM, Robin Murphy wrote:
> On 2022-06-13 15:38, Suthikulpanit, Suravee wrote:
>> Robin,
>>
>> On 6/13/2022 4:31 PM, Robin Murphy wrote:
>>>
>>>> Introducing check_domain_type_supported() callback in iommu_ops,
>>>> which allows IOMMU generic layer to check with vendor-specific IOMMU driver
>>>> whether the requested type is supported. This allows user to request
>>>> types other than the default type.
>>>
>>> Note also that you're only adding this in the sysfs path - what about the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH?
>>
>> For SNP case, we cannot enable SNP if iommu=off or iommu=pt or iommu.passthrough=1 or CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y.
>> So, when another driver tries to enable SNP, the IOMMU driver prevents it (see iommu_sev_snp_supported() in patch 3).
> 
> Ugh, I hadn't looked too closely at the other patches, but an interface that looks like a simple "is this feature supported?" check with a secret side-effect of changing global behaviour as well? Yuck :(
> 
> What external drivers are expected to have the authority to affect the entire system and call that? The fact that you're exporting it suggests they could be loaded from modules *after* v2 features have been enabled and/or the user has configured a non-default identity domain for a group via sysfs... Fun!

I see your point.

Currently, the function to enable SNP will be called from SEV driver when it tries to enable SNP support globally on the system.
This is done during fs_initcall(), which is early in the boot process. I can also add a guard code to make sure that this won't
be done after a certain phase.

>> Instead, if we boot with iommu.passhthrough=0, when another driver tries to enable SNP, the IOMMU driver allows this and switch to SNP enable mode.
>> Subsequently, if user tries to switch a domain (via sysfs) to IOMMU_DOMAIN_IDENTITY, the IOMMU needs to prevent this because it has already switch
>> to SNP-enabled mode.
>>
>>> AFAICS there shouldn't need to be any core-level changes to support this. We already have drivers which don't support passthrough at all, so conditionally not supporting it should be no big deal. What should happen currently is that def_domain_type returns 0 for "don't care", then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA.
>>
>> Technically, we can do it the way you suggest. But isn't this confusing? At first, def_domain_type() returns 0 for "don't care",
>> but then it rejects the request to change to IOMMU_DOMAIN_IDENTITY when trying to call domain_alloc().
> 
> Yes, that's how it works; def_domain_type is responsible for quirking individual *devices* that need to have a specific domain type (in practice, devices which need identity mapping), while domain_alloc is responsible for saying which domain types the driver supports as a whole, by allocating them or not as appropriate.
> 
> We don't have a particularly neat way to achieve the negative of def_domain_type - i.e. saying that a specific device *can't* use a specific otherwise-supported domain type - other than subsequently failing in attach_dev, but so far we've not needed such a thing. And if SNP is expected to be mutually exclusive with identity domain support globally, then we still shouldn't need it.

Thanks for your feedback.

Suravee
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu