Re: [PATCH v2 3/7] iommu/vt-d: Expose ISA direct mapping region via iommu_get_resv_regions

From: Lu Baolu
Date: Wed Apr 24 2019 - 21:25:46 EST


Hi Tom,

On 4/25/19 7:47 AM, Tom Murphy wrote:
I can see two potential problems with these patches that should be addressed:

The default domain of a group can be changed to type
IOMMU_DOMAIN_IDENTITY via the command line. With these patches we are
returning the si_domain for type IOMMU_DOMAIN_IDENTITY. There's a
chance the shared si_domain could be freed while it is still being
used when a group is freed. For example here in the
iommu_group_release:
https://github.com/torvalds/linux/blob/cd8dead0c39457e58ec1d36db93aedca811d48f1/drivers/iommu/iommu.c#L376
"if (group->default_domain)
iommu_domain_free(group->default_domain);"

Also now that a domain is attached to a device earlier we should
implement the is_attach_deferred call-back and use it to defer the
domain attach from iommu driver init to device driver init when iommu
is pre-enabled in kdump kernel. like in this commit:
https://github.com/torvalds/linux/commit/df3f7a6e8e855e4ff533508807cd7c3723faa51f


Both agreed and should be considered during development.

Best regards,
Lu Baolu


On Tue, Apr 16, 2019 at 3:24 AM Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:

Hi James,

On 4/15/19 10:16 PM, James Sewart wrote:
Hey Lu,

On 10 Apr 2019, at 06:22, Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:

Hi James,

On 4/6/19 2:02 AM, James Sewart wrote:
Hey Lu,
My bad, did some debugging on my end. The issue was swapping out
find_domain for iommu_get_domain_for_dev. It seems in some situations the
domain is not attached to the group but the device is expected to have the
domain still stored in its archdata.
Iâve attached the final patch with find_domain unremoved which seems to
work in my testing.

Just looked into your v3 patch set and some thoughts from my end posted
here just for your information.

Let me post the problems we want to address.

1. When allocating a new group for a device, how should we determine the
type of the default domain?

2. If we need to put a device into an existing group which uses a
different type of domain from what the device desires to use, we might
break the functionality of the device.

My new thought is letting the iommu generic code to determine the
default domain type (hence my proposed vendor specific default domain
type patches could be dropped).

If the default domain type is dynamical mapping, and later in iommu_no_mapping(), we determines that we must use an identity domain,
we then call iommu_request_dm_for_dev(dev).

If the default domain type is identity mapping, and later in
iommu_no_mapping(), we determined that we must use a dynamical domain,
we then call iommu_request_dma_domain_for_dev(dev).

We already have iommu_request_dm_for_dev() in iommu.c. We only need to
implement iommu_request_dma_domain_for_dev().

With this done, your patch titled "Create an IOMMU group for devices
that require an identity map" could also be dropped.

Any thoughts?

Theres a couple issues I can think of. iommu_request_dm_for_dev changes
the domain for all devices within the devices group, if we may have
devices with different domain requirements in the same group then only the
last initialised device will get the domain it wants. If we want to add
iommu_request_dma_domain_for_dev then we would still need another IOMMU
group for identity domain devices.

Current solution (a.k.a. v3) for this is to assign a new group to the
device which requires an identity mapped domain. This will break the
functionality of device direct pass-through (to user level). The iommu
group represents the minimum granularity of iommu isolation and
protection. The requirement of identity mapped domain should not be a
factor for a new group.

Both iomm_request_dma_domain_for_dev() or iommu_request_dm_for_dev()
only support groups with a single device in it.

The users could choose between domains of DMA type or IDENTITY type for
a group. If multiple devices share a single group and both don't work
for them, they have to disable the IOMMU. This isn't a valid
configuration from IOMMU's point of view.


Both with v3 and the proposed method here, iommu_should_identity_map is
determining whether the device requires an identity map. In v3 this is
called once up front by the generic code to determine for the IOMMU group
which domain type to use. In the proposed method I think this would happen
after initial domain allocation and would trigger the domain to be
replaced with a different domain.

I prefer the solution in v3. What do you think?

The only concern of solution in v3 from my point of view is some kind of
duplication. Even we can ask the Intel IOMMU driver to tell the default
domain type, we might change it after boot up. For example, for 32-bit
pci devices, we don't know whether it's 64-bit or 32-bit capable during
IOMMU initialization, so we might tell iommu.c to use identity mapped
domain. After boot up, we check it again, and find out that it's only
32-bit capable (hence only can access physical memory below 4G) and the
default identity domain doesn't work. And we have to change it to DMA
domain via iommu_request_dma_domain_for_dev().

So to make it simple and straight-forward, we can just let iommu.c to
determine the default domain type and after that the Intel IOMMU driver
could tweak it according to the quirk bits in the driver.

Best regards,
Lu Baolu