Re: RMRR device on non-Intel platform

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

On 2023-04-21 13:45, Jason Gunthorpe wrote:
> On Fri, Apr 21, 2023 at 01:29:46PM +0100, Robin Murphy wrote:
> 
>> Can you clarify why something other than IOMMU_RESV_SW_MSI would be
>> needed?
> 
> We need iommufd to setup a 1:1 map for the reserved space.
> 
> So, of the reserved spaces we have these:
> 
> 	/* Memory regions which must be mapped 1:1 at all times */
> 	IOMMU_RESV_DIRECT,
> 
>             Block iommufd
> 
> 	/*
> 	 * Memory regions which are advertised to be 1:1 but are
> 	 * commonly considered relaxable in some conditions,
> 	 * for instance in device assignment use case (USB, Graphics)
> 	 */
> 	IOMMU_RESV_DIRECT_RELAXABLE,
> 
>             iommufd ignores this one
> 
> 	/* Arbitrary "never map this or give it to a device" address ranges */
> 	IOMMU_RESV_RESERVED,
> 
> 	   iommufd prevents using this IOVA range
> 
> 	/* Hardware MSI region (untranslated) */
> 	IOMMU_RESV_MSI,
> 
> 	   iommufd treats this the same as IOMMU_RESV_RESERVED
> 
> 	/* Software-managed MSI translation window */
> 	IOMMU_RESV_SW_MSI,
> 
> 	   iommufd treats this the same as IOMMU_RESV_RESERVED, also
> 	   it passes the start to iommu_get_msi_cookie() which
> 	   eventually maps something, but not 1:1.
> 
> I don't think it is a compatible change for IOMMU_RESV_SW_MSI to also
> mean 1:1 map?

Bleh, my bad, the nested MSI stuff is right on the limit of the amount 
of horribleness I can hold in my head at once even at the best of times, 
let alone when my head is still half-full of PMUs.

I think a slightly more considered and slightly less wrong version of 
that idea is to mark it as IOMMU_RESV_MSI, and special-case 
direct-mapping those on Arm (I believe it would technically be benign to 
do on x86 too, but might annoy people with its pointlessness). However...

> On baremetal we have no idea what the platform put under that
> hardcoded address?
> 
> On VM we don't use the iommu_get_msi_cookie() flow because the GIC in
> the VM pretends it doesn't have an ITS page?  (did I get that right?)

No, that can't be right - PCIe devices have to support MSI or MSI-X, and 
many of them won't support INTx at all, so if the guest wants to use 
interrupts in general it must surely need to believe it has some kind of 
MSI controller, which for practical purposes in this context means an 
ITS. That was the next thing I started wondering after the above - if 
the aim is to direct-map the host's SW_MSI region to effectively pass 
through the S2 MSI cookie, but you have the same Linux SMMU driver in 
the guest, isn't that guest driver still going to add a conflicting 
SW_MSI region for the same IOVA and confuse things?

Ideally for nesting, the VMM would just tell us the IPA of where it's 
going to claim the given device's associated MSI doorbell is, we map 
that to the real underlying address at S2, then the guest can use its S1 
cookie as normal if it wants to, and the host doesn't have to rewrite 
addresses either way. The set_dev_data thing starts to look tempting for 
this too... Given that the nesting usage model inherently constrains the 
VMM's options for emulating the IOMMU, would it be unreasonable to make 
our lives a lot easier with some similar constraints around interrupts, 
and just not attempt to support the full gamut of "emulate any kind of 
IRQ with any other kind of IRQ" irqfd hilarity?

>> 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.
> 
> I used "relxable" to suggest it is safe for userspace.

I know, but the subtlety is the reason *why* it's safe for userspace. 
Namely that a VFIO driver has bound and reset (or at least taken control 
of) the device, and thus it is assumed to no longer be doing whatever 
the boot firmware left it doing, therefore the reserved region is 
assumed to no longer be relevant, and from then on the requirement to 
preserve it can be relaxed.

>> 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).
> 
> Right, I missed that!
> 
> I suggest like this to avoid the double loop:
> 
> @@ -1037,9 +1037,6 @@ static int iommu_create_device_direct_mappings(struct iom>
>          unsigned long pg_size;
>          int ret = 0;
>   
> -       if (!iommu_is_dma_domain(domain))
> -               return 0;
> -
>          BUG_ON(!domain->pgsize_bitmap);
>   
>          pg_size = 1UL << __ffs(domain->pgsize_bitmap);

But then you realise that you also need to juggle this around since 
identity domains aren't required to have a valid pgsize_bitmap either, 
give up on the idea and go straight to writing a dedicated loop as the 
clearer and tidier option because hey this is hardly a fast path anyway. 
At least, you do if you're me :)

Cheers,
Robin.

> @@ -1052,13 +1049,18 @@ static int iommu_create_device_direct_mappings(struct i>
>                  dma_addr_t start, end, addr;
>                  size_t map_size = 0;
>   
> -               start = ALIGN(entry->start, pg_size);
> -               end   = ALIGN(entry->start + entry->length, pg_size);
> -
>                  if (entry->type != IOMMU_RESV_DIRECT &&
>                      entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
>                          continue;
>   
> +               if (entry->type == IOMMU_RESV_DIRECT)
> +                       dev->iommu->requires_direct = 1;
> +
> +               if (!iommu_is_dma_domain(domain))
> +                       continue;
> +
> +               start = ALIGN(entry->start, pg_size);
> +               end   = ALIGN(entry->start + entry->length, pg_size);
>                  for (addr = start; addr <= end; addr += pg_size) {
>                          phys_addr_t phys_addr;
> 
> Jason