> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Friday, April 21, 2023 5:50 AM
>
> On Thu, 20 Apr 2023 17:55:22 +0100
> Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> > On 20/04/2023 3:49 pm, Alex Williamson wrote:
> > > On Thu, 20 Apr 2023 15:19:55 +0100
> > > Robin Murphy <robin.murphy@xxxxxxx> wrote:
> > >
> > >> On 2023-04-20 15:15, Alex Williamson wrote:
> > >>> On Thu, 20 Apr 2023 06:52:01 +0000
> > >>> "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
> > >>>
> > >>>> Hi, Alex,
> > >>>>
> > >>>> Happen to see that we may have inconsistent policy about RMRR
> devices cross
> > >>>> different vendors.
> > >>>>
> > >>>> Previously only Intel supports RMRR. Now both AMD/ARM have
> similar thing,
> > >>>> AMD IVMD and ARM RMR.
> > >>>
> > >>> Any similar requirement imposed by system firmware that the
> operating
> > >>> system must perpetually maintain a specific IOVA mapping for the
> device
> > >>> should impose similar restrictions as we've implemented for VT-d
> > >>> RMMR[1]. Thanks,
> > >>
> > >> Hmm, does that mean that vfio_iommu_resv_exclude() going to the
> trouble
> > >> of punching out all the reserved region holes isn't really needed?
> > >
> > > While "Reserved Memory Region Reporting", might suggest that the
> ranges
> > > are simply excluded, RMRR actually require that specific mappings are
> > > maintained for ongoing, side-band activity, which is not compatible
> > > with the ideas that userspace owns the IOVA address space for the
> > > device or separation of host vs userspace control of the device. Such
> > > mappings suggest things like system health monitoring where the
> > > influence of a user-owned device can easily extend to a system-wide
> > > scope if the user it able to manipulate the device to deny that
> > > interaction or report bad data.
> > >
> > > If these ARM and AMD tables impose similar requirements, we should
> > > really be restricting devices encumbered by such requirements from
> > > userspace access as well. Thanks,
> >
> > Indeed the primary use-case behind Arm's RMRs was certain devices like
> > big complex RAID controllers which have already been started by UEFI
> > firmware at boot and have live in-memory data which needs to be
> preserved.
> >
> > However, my point was more that if it's a VFIO policy that any device
> > with an IOMMU_RESV_DIRECT reservation is not suitable for userspace
> > assignment, then vfio_iommu_type1_attach_group() already has
> everything
> > it would need to robustly enforce that policy itself. It seems silly to
> > me for it to expect the IOMMU driver to fail the attach, then go ahead
> > and dutifully punch out direct regions if it happened not to. A couple
> > of obvious trivial tweaks and there could be no dependency on driver
> > behaviour at all, other than correctly reporting resv_regions to begin with.
> >
> > 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?
>
> It looks pretty easy to do as well, something like this (untested):
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 10db680acaed..521f9a731ce9 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2012,11 +2012,29 @@ static void
> __iommu_group_set_core_domain(struct iommu_group *group)
> static int __iommu_attach_device(struct iommu_domain *domain,
> struct device *dev)
> {
> - int ret;
> + int ret = 0;
>
> if (unlikely(domain->ops->attach_dev == NULL))
> return -ENODEV;
>
> + if (domain->type == IOMMU_DOMAIN_UNMANAGED) {
> + struct iommu_resv_region *region;
> + LIST_HEAD(resv_regions);
> +
> + iommu_get_resv_regions(dev, &resv_regions);
> + list_for_each_entry(region, &resv_regions, list) {
> + if (region->type == IOMMU_RESV_DIRECT) {
> + ret = -EPERM;
> + break;
> + }
> + }
> + iommu_put_resv_regions(dev, &resv_regions);
> + if (ret) {
> + dev_warn(dev, "Device may not be used with an
> unmanaged IOMMU domain due to reserved direct mapping
> requirement.\n");
> + return ret;
> + }
> + }
> +
> ret = domain->ops->attach_dev(domain, dev);
> if (ret)
> return ret;
>
> Restrictions in either type1 or iommufd would be pretty trivial as well,
> but centralizing it in core IOMMU code would do a better job of covering
> all use cases.
>
> This effectively makes the VT-d code further down the same path
> redundant, so no new restrictions there.
>
> What sort of fall-out should we expect on ARM or AMD? This was a pretty
> painful restriction to add on Intel. Thanks,
>
What about device_rmrr_is_relaxable()? Leave it in specific driver or
consolidate to be generic?
intel-iommu sets RELAXABLE for USB and GPU assuming their RMRR region
is not used post boot.
Presumably same policy can be applied to AMD too.
ARM RMR reports an explicit flag (ACPI_IORT_RMR_REMAP_PERMITTED) to
mark out whether a RMR region is relaxable. I'm not sure whether USB/GPU
is already covered.
