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

[Robin Murphy <robin.murphy@arm.com>]

On 2022-06-13 15:38, Suthikulpanit, Suravee wrote:
> Robin,
> 
> On 6/13/2022 4:31 PM, Robin Murphy wrote:
>> On 2022-06-13 02:25, Suravee Suthikulpanit wrote:
>>> When user requests to change IOMMU domain to a new type, IOMMU generic
>>> layer checks the requested type against the default domain type returned
>>> by vendor-specific IOMMU driver.
>>>
>>> However, there is only one default domain type, and current mechanism
>>> does not allow if the requested type does not match the default type.
>>
>> I don't really follow the reasoning here. If a driver's 
>> def_domain_type callback returns a specific type, it's saying that the 
>> device *has* to have that specific domain type for 
>> driver/platform-specific reasons. 
> 
> Agree, and I understand this part.
> 
>> If 
>> that's not the case, then the driver shouldn't say so in the first place.
> 
> Considering the case:
> 1. Boot w/ default domain = IOMMU_DOMAIN_DMA_FQ
> 2. User wants to change to IOMMU_DOMAIN_IDENTITY, which is not supported 
> by IOMMU driver. In this case, IOMMU driver can return 
> IOMMU_DOMAIN_DMA_FQ and prevent the mode change.
> 3. However, if user want to change to IOMMU_DOMAIN_DMA. The driver can 
> support this. However, since the def_domain_type() returns 
> IOMMU_DOMAIN_DMA_FQ, it ends up prevent the mode change.

Why would a driver be forcing IOMMU_DOMAIN_DMA_FQ for a device though? 
Nobody's doing that today, and semantically it wouldn't really make 
sense - forcing translation to deny passthrough on a device-specific 
basis (beyond the common handling of untrusted devices) *might* be a 
thing, but the performance/strictness tradeoff of using a flush queue or 
not is surely a subjective user decision, not an objective platform one.

> IIUC, we should support step 3 above. Basically, with the newly proposed 
> interface, it allows us to check with IOMMU driver if it can support 
> certain domain types before trying
> to allocate the domain.

Indeed we could do that - as a much more comprehensive change to the 
internal domain_alloc interfaces - but do we really need to? If we 
succeed at allocating a domain then we know it's supported; if it fails 
then we can't give the user what they asked for, regardless of the exact 
reason why - what do we gain from doubling the number of potential 
failure paths that we have to handle?

>>> 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!

> 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,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu