Re: RMRR device on non-Intel platform

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

On 2023-04-21 13:04, Jason Gunthorpe wrote:
> On Thu, Apr 20, 2023 at 03:49:33PM -0600, Alex Williamson wrote:
>>> If we think this policy deserves to go beyond VFIO and userspace, and
>>> it's reasonable that such devices should never be allowed to attach to
>>> any other kind of kernel-owned unmanaged domain either, then we can
>>> still trivially enforce that in core IOMMU code. I really see no need
>>> for it to be in drivers at all.
>>
>> It seems like a reasonable choice to me that any mixing of unmanaged
>> domains with IOMMU_RESV_DIRECT could be restricted globally.  Do we
>> even have infrastructure for a driver to honor the necessary mapping
>> requirements?
> 
> What we discussed about the definition of IOMMU_RESV_DIRECT was that
> an identity map needs to be present at all times. This is what is
> documented at least:
> 
> 	/* Memory regions which must be mapped 1:1 at all times */
> 	IOMMU_RESV_DIRECT,
> 
> Notably, this also means the device can never be attached to a
> blocking domain. I would also think that drivers asking for this
> should ideally implement the "atomic replace" we discussed already to
> change between identity and unmanaged without disturbing the FW doing
> DMA to these addresses..
> 
> I was looking at this when we talked about it earlier and we don't
> follow that guideline today for vfio/iommufd.
> 
> Since taking ownership immediately switches to a blocking domain
> restricting the use of blocking also restricts ownership thus vfio and
> iommufd will be prevented.
> 
> Other places using unmanaged domains must follow the
> iommu_get_resv_regions() and setup IOMMU_RESV_DIRECT - we should not
> restrict them in the core code.
> 
> It also slightly changes my prior remarks to Robin about error domain
> attach handling, since blocking domains are not available for these
> devices the "error state" for such a device should be the identity
> domain to preserve FW access.
> 
> Also, we have a functional gap, ARM would really like a
> IOMMU_RESV_DIRECT_RELAXABLE_SAFE which would have iommufd/vfio install
> the 1:1 map and allow the device to be used. This is necessary for the
> GIC ITS page hack to support MSI since we should enable VFIO inside a
> VM. It is always safe for hostile VFIO userspace to DMA to the ITS
> page.

Can you clarify why something other than IOMMU_RESV_SW_MSI would be 
needed? MSI regions already represent "safe" direct mappings, either as 
an inherent property of the hardware, or with an actual mapping 
maintained by software. Also RELAXABLE is meant to imply that it is only 
needed until a driver takes over the device, which at face value doesn't 
make much sense for interrupts.

> So, after my domain error handling series, the core fix is pretty
> simple and universal. We should also remove all the redundant code in
> drivers - drivers should report the regions each devices needs
> properly and leave enforcement to the core code.. Lu/Kevin do you want
> to take this?
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 19f8d28ff1323c..c15eb5e0ba761d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1059,6 +1059,9 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
>   		    entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
>   			continue;
>   
> +		if (entry->type == IOMMU_RESV_DIRECT)
> +			dev->iommu->requires_direct = 1;

We'll still need to set this when the default domain type is identity 
too - see the diff I posted (the other parts below I merely implied).

Thanks,
Robin.

> +
>   		for (addr = start; addr <= end; addr += pg_size) {
>   			phys_addr_t phys_addr;
>   
> @@ -2210,6 +2213,22 @@ static int __iommu_device_set_domain(struct iommu_group *group,
>   {
>   	int ret;
>   
> +	/*
> +	 * If the driver has requested IOMMU_RESV_DIRECT then we cannot allow
> +	 * the blocking domain to be attached as it does not contain the
> +	 * required 1:1 mapping. This test effectively exclusive the device from
> +	 * being used with iommu_group_claim_dma_owner() which will block vfio
> +	 * and iommufd as well.
> +	 */
> +	if (dev->iommu->requires_direct &&
> +	    (new_domain->type == IOMMU_DOMAIN_BLOCKED ||
> +	     new_domain == group->blocking_domain)) {
> +		dev_warn(
> +			dev,
> +			"Firmware has requested this device have a 1:1 IOMMU mapping, rejecting configuring the device without a 1:1 mapping. Contact your platform vendor.");
> +		return -EINVAL;
> +	}
> +
>   	if (dev->iommu->attach_deferred) {
>   		if (new_domain == group->default_domain)
>   			return 0;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 3ad14437487638..7729a07923faa6 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -407,6 +407,7 @@ struct iommu_fault_param {
>    * @priv:	 IOMMU Driver private data
>    * @max_pasids:  number of PASIDs this device can consume
>    * @attach_deferred: the dma domain attachment is deferred
> + * @requires_direct: The driver requested IOMMU_RESV_DIRECT
>    *
>    * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
>    *	struct iommu_group	*iommu_group;
> @@ -420,6 +421,7 @@ struct dev_iommu {
>   	void				*priv;
>   	u32				max_pasids;
>   	u32				attach_deferred:1;
> +	u32				requires_direct:1;
>   };
>   
>   int iommu_device_register(struct iommu_device *iommu,