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
