| Message ID | 20180605190400.22732.2998.stgit@gimli.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 05 Jun 2018 13:06:54 -0600 Alex Williamson <alex.williamson@redhat.com> wrote: > 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. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > 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, > I think this patch makes sense in that it makes IOVA reserved range check consistent. Thanks, Jacob > 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, > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu [Jacob Pan]
> 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... > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > 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,
On Wed, 6 Jun 2018 05:29:58 +0000 "Tian, Kevin" <kevin.tian@intel.com> 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@redhat.com> > > --- > > 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, >
> From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Thursday, June 7, 2018 11:02 PM > > On Wed, 6 Jun 2018 05:29:58 +0000 > "Tian, Kevin" <kevin.tian@intel.com> 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, > Then possibly need introduce a different interface for pre-allocation scenario, if above definition of reserved ranges is agreed. Currently two categories are both called reserved resources, e.g. IOMMU_RESV _DIRECT for rmrr and IOMMU_RESV_MSI for MSI... Thanks Kevin
Hi Alex, > -----Original Message----- > From: Tian, Kevin [mailto:kevin.tian@intel.com] > Sent: 08 June 2018 03:56 > To: Alex Williamson <alex.williamson@redhat.com> > Cc: dwmw2@infradead.org; iommu@lists.linux-foundation.org; > kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi@huawei.com> > Subject: RE: [RFC PATCH] iommu/vt-d: Exclude known RMRRs from reserved > ranges > > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > Sent: Thursday, June 7, 2018 11:02 PM > > > > On Wed, 6 Jun 2018 05:29:58 +0000 > > "Tian, Kevin" <kevin.tian@intel.com> 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, > > > > Then possibly need introduce a different interface for pre-allocation > scenario, if above definition of reserved ranges is agreed. Currently > two categories are both called reserved resources, e.g. IOMMU_RESV > _DIRECT for rmrr and IOMMU_RESV_MSI for MSI... Sorry, but I just thought of checking on the future plans/directions for solving this issue(Unfortunately vfio iova list series depends on this). Please let me know if there is any plans for a v2 soon. Much appreciated, Shameer
On Thu, 2018-06-07 at 09:01 -0600, Alex Williamson wrote: > > > 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, Unfortunately, Intel likes to encourage vendors to do batshit insane things in firmware, at runtime, using DMA. Perhaps the way to handle this is for the device drivers to cancel any RMRR for their devices, as they take it over. So USB and graphics drivers would clear the corresponding RMRRs as they take ownership of the device... but anything without a driver, or things like the SCSI host controllers which know that there might be some HP insanity going on in SMM in parallel with what they're doing, would not (and hence would not be assignable, etc.).
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,
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. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- 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