Re: [RFC PATCH] iommu/vt-d: Exclude known RMRRs from reserved ranges

[Alex Williamson]

On Wed, 6 Jun 2018 05:29:58 +0000
"Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:

> > From: Alex Williamson
> > Sent: Wednesday, June 6, 2018 3:07 AM
> > 
> > device_is_rmrr_locked() allows graphics and USB devices to participate
> > in the IOMMU API despite, and ignoring their RMRR association, however
> > intel_iommu_get_resv_regions() still includes the RMRRs as unavailable
> > IOVA space for the device.  Are we ignoring the RMRR for these devices
> > or are we not?  If vfio starts consuming reserved regions, perhaps we
> > no longer need to consider devices with RMRRs excluded from the IOMMU
> > API interface, but we have a transitional problem that these allowed
> > devices still impose incompatible IOVA restrictions per the reserved
> > region reporting.  Dive further down the rabbit hole by also ignoring
> > RMRRs for "known" devices in the reserved region reporting.  
> 
> intel_iommu_get_resv_regions is used not just for IOMMU API. I'm
> afraid doing so will make RMRR completely ignored, even in normal
> DMA API path...

Well, I'm a bit stuck then, we have existing IOMMU API users that
ignore these ranges and in fact conflict with these ranges blocking us
from restricting mappings within these ranges.  My impression is that
IOMMU reserved ranges should only be ranges which have some fundamental
limitation in the IOMMU, not simply mappings for which firmware has
requested an identity mapped range.  The latter should simply be a
pre-allocation of the IOVA space, for the cases where we choose to
honor the RMRR.  Thanks,

Alex

> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > ---
> >  drivers/iommu/intel-iommu.c |   35 +++++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> > 
> > If this is the approach we want to take, I could pull this in via the
> > vfio tree, along with Shameer's patches which expose an IOVA list and
> > enforce it to userspace, otherwise I'm afraid Shameer's patches will
> > be blocked a while longer.  Thanks,
> > 
> > Alex
> > 
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 749d8f235346..f312f93199c5 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -2864,19 +2864,24 @@ static bool device_has_rmrr(struct device *dev)
> >   * any use of the RMRR regions will be torn down before assigning the
> > device
> >   * to a guest.
> >   */
> > -static bool device_is_rmrr_locked(struct device *dev)
> > +static bool rmrr_is_ignored(struct device *dev)
> >  {
> > -	if (!device_has_rmrr(dev))
> > -		return false;
> > -
> >  	if (dev_is_pci(dev)) {
> >  		struct pci_dev *pdev = to_pci_dev(dev);
> > 
> >  		if (IS_USB_DEVICE(pdev) || IS_GFX_DEVICE(pdev))
> > -			return false;
> > +			return true;
> >  	}
> > 
> > -	return true;
> > +	return false;
> > +}
> > +
> > +static bool device_is_rmrr_locked(struct device *dev)
> > +{
> > +	if (!device_has_rmrr(dev))
> > +		return false;
> > +
> > +	return !rmrr_is_ignored(dev);
> >  }
> > 
> >  static int iommu_should_identity_map(struct device *dev, int startup)
> > @@ -5141,17 +5146,19 @@ static void
> > intel_iommu_get_resv_regions(struct device *device,
> >  	struct device *i_dev;
> >  	int i;
> > 
> > -	rcu_read_lock();
> > -	for_each_rmrr_units(rmrr) {
> > -		for_each_active_dev_scope(rmrr->devices, rmrr-  
> > >devices_cnt,  
> > -					  i, i_dev) {
> > -			if (i_dev != device)
> > -				continue;
> > +	if (!rmrr_is_ignored(device)) {
> > +		rcu_read_lock();
> > +		for_each_rmrr_units(rmrr) {
> > +			for_each_active_dev_scope(rmrr->devices,
> > +						  rmrr->devices_cnt, i, i_dev)
> > {
> > +				if (i_dev != device)
> > +					continue;
> > 
> > -			list_add_tail(&rmrr->resv->list, head);
> > +				list_add_tail(&rmrr->resv->list, head);
> > +			}
> >  		}
> > +		rcu_read_unlock();
> >  	}
> > -	rcu_read_unlock();
> > 
> >  	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
> >  				      IOAPIC_RANGE_END -
> > IOAPIC_RANGE_START + 1,  
>