Hi Lu, On 5/29/19 4:34 AM, Lu Baolu wrote: > Hi, > > On 5/28/19 7:50 PM, Eric Auger wrote: >> Now we have a new IOMMU_RESV_DIRECT_RELAXABLE reserved memory >> region type, let's report USB and GFX RMRRs as relaxable ones. >> >> We introduce a new device_rmrr_is_relaxable() helper to check >> whether the rmrr belongs to the relaxable category. >> >> This allows to have a finer reporting at IOMMU API level of >> reserved memory regions. This will be exploitable by VFIO to >> define the usable IOVA range and detect potential conflicts >> between the guest physical address space and host reserved >> regions. >> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> >> --- >> >> v3 -> v4: >> - introduce device_rmrr_is_relaxable and reshuffle the comments >> --- >> drivers/iommu/intel-iommu.c | 55 +++++++++++++++++++++++++++---------- >> 1 file changed, 40 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c >> index 9302351818ab..01c82f848470 100644 >> --- a/drivers/iommu/intel-iommu.c >> +++ b/drivers/iommu/intel-iommu.c >> @@ -2920,6 +2920,36 @@ static bool device_has_rmrr(struct device *dev) >> return false; >> } >> +/* >> + * device_rmrr_is_relaxable - Test whether the RMRR of this device >> + * is relaxable (ie. is allowed to be not enforced under some >> conditions) >> + * >> + * @dev: device handle >> + * >> + * We assume that PCI USB devices with RMRRs have them largely >> + * for historical reasons and that the RMRR space is not actively >> used post >> + * boot. This exclusion may change if vendors begin to abuse it. >> + * >> + * The same exception is made for graphics devices, with the >> requirement that >> + * any use of the RMRR regions will be torn down before assigning the >> device >> + * to a guest. >> + * >> + * Return: true if the RMRR is relaxable >> + */ >> +static bool device_rmrr_is_relaxable(struct device *dev) >> +{ >> + struct pci_dev *pdev; >> + >> + if (!dev_is_pci(dev)) >> + return false; >> + >> + pdev = to_pci_dev(dev); >> + if (IS_USB_DEVICE(pdev) || IS_GFX_DEVICE(pdev)) >> + return true; >> + else >> + return false; >> +} > > I know this is only code refactoring. But strictly speaking, the rmrr of > any USB host device is ignorable only if quirk_usb_early_handoff() has > been called. There, the control of USB host controller will be handed > over from BIOS to OS and the corresponding SMI are disabled. > > This function is registered in drivers/usb/host/pci-quirks.c > > DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID, > PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff); > > and only get compiled if CONFIG_USB_PCI is enabled. > > Hence, it's safer to say: > > +#ifdef CONFIG_USB_PCI > + if (IS_USB_DEVICE(pdev)) > + return true; > +#endif /* CONFIG_USB_PCI */ > > I am okay if we keep this untouched and make this change within a > separated patch.
As we first checked whether the device was a pci device, isn't it sufficient to guarantee the quirk is setup? As you suggested, I am inclined to keep it as a separate patch anyway. Thank you for the review! Best Regards Eric > >> + >> /* >> * There are a couple cases where we need to restrict the >> functionality of >> * devices associated with RMRRs. The first is when evaluating a >> device for >> @@ -2934,25 +2964,16 @@ static bool device_has_rmrr(struct device *dev) >> * We therefore prevent devices associated with an RMRR from >> participating in >> * the IOMMU API, which eliminates them from device assignment. >> * >> - * In both cases we assume that PCI USB devices with RMRRs have them >> largely >> - * for historical reasons and that the RMRR space is not actively >> used post >> - * boot. This exclusion may change if vendors begin to abuse it. >> - * >> - * The same exception is made for graphics devices, with the >> requirement that >> - * any use of the RMRR regions will be torn down before assigning the >> device >> - * to a guest. >> + * In both cases, devices which have relaxable RMRRs are not >> concerned by this >> + * restriction. See device_rmrr_is_relaxable comment. >> */ >> static bool device_is_rmrr_locked(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; >> - } >> + if (device_rmrr_is_relaxable(dev)) >> + return false; >> return true; >> } >> @@ -5494,6 +5515,7 @@ static void intel_iommu_get_resv_regions(struct >> device *device, >> for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt, >> i, i_dev) { >> struct iommu_resv_region *resv; >> + enum iommu_resv_type type; >> size_t length; >> if (i_dev != device && >> @@ -5501,9 +5523,12 @@ static void intel_iommu_get_resv_regions(struct >> device *device, >> continue; >> length = rmrr->end_address - rmrr->base_address + 1; >> + >> + type = device_rmrr_is_relaxable(device) ? >> + IOMMU_RESV_DIRECT_RELAXABLE : IOMMU_RESV_DIRECT; >> + >> resv = iommu_alloc_resv_region(rmrr->base_address, >> - length, prot, >> - IOMMU_RESV_DIRECT); >> + length, prot, type); >> if (!resv) >> break; >> > > Other looks good to me. > > Reviewed-by: Lu Baolu <baolu...@linux.intel.com> > > Best regards, > Baolu