Re: [PATCH v5 2/8] iommu/vt-d: Add per-device IOMMU feature ops entries
Hi Lu Baolu, On Thu, Jan 24, 2019 at 02:47:39PM +0800, Lu Baolu wrote: > bool iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)? Looks good. Having a function to check for enabled features is certainly a good thing. Regards, Joerg
Re: [PATCH v5 2/8] iommu/vt-d: Add per-device IOMMU feature ops entries
Hi Joerg, On 1/11/19 7:16 PM, Joerg Roedel wrote: + +static bool +intel_iommu_dev_has_feat(struct device *dev, enum iommu_dev_features feat) +{ + struct device_domain_info *info = dev->archdata.iommu; + + if (feat == IOMMU_DEV_FEAT_AUX) + return scalable_mode_support() && info && info->auxd_enabled; + + return false; +} Why is this checking the auxd_enabled flag? The function should just return whether the device_supports_ scalable mode, not whether it is enabled. Yes, as the API name implies, it should return the device capability instead of enable/disable status. I misused this API in the IOMMU driver. Since we already have iommu_dev_enable/disable_feature() to enable and disable an iommu specific feature, is it possible to add another API to query whether a specific feature has been enabled? How about bool iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)? This is necessary for the third party drivers (like vfio) to determine which domain attach interface it should use: if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) iommmu_aux_attach_device(domain, dev) else iommu_attach_device(domain, dev) Best regards, Lu Baolu
Re: [PATCH v5 2/8] iommu/vt-d: Add per-device IOMMU feature ops entries
Hi Joerg, Thanks for reviewing my patch. On 1/11/19 7:16 PM, Joerg Roedel wrote: Hi, this looks a bit confusing to me because I can see no checking whether the device actually supports scalable mode. Yes. I should put some checking there. Device scalable mode capability is exposed in PCI extended capability list. More below: On Thu, Jan 10, 2019 at 11:00:21AM +0800, Lu Baolu wrote: +static int intel_iommu_enable_auxd(struct device *dev) +{ + struct device_domain_info *info; + struct dmar_domain *domain; + unsigned long flags; + + if (!scalable_mode_support()) + return -ENODEV; + + domain = get_valid_domain_for_dev(dev); + if (!domain) + return -ENODEV; + + spin_lock_irqsave(_domain_lock, flags); + info = dev->archdata.iommu; + info->auxd_enabled = 1; + spin_unlock_irqrestore(_domain_lock, flags); + + return 0; +} This code sets a flag to mark scalable mode enabled. Doesn't the device need some handling too, like enabling the PASID capability and all? Yes. My design was rough. We should prepare the device for scalable mode instead of assuming that everything is ready. + +static bool +intel_iommu_dev_has_feat(struct device *dev, enum iommu_dev_features feat) +{ + struct device_domain_info *info = dev->archdata.iommu; + + if (feat == IOMMU_DEV_FEAT_AUX) + return scalable_mode_support() && info && info->auxd_enabled; + + return false; +} Why is this checking the auxd_enabled flag? We need an API to check whether this feature is enabled. In vfio, it is used like below, if (iommu_dev_has_feat(dev, FEAT_AUX_DOMAIN)) iommu_aux_attach_device(dev, domain) else iommu_attach_device(dev, domain) The function should just return whether the device _supports_ scalable mode, not whether it is enabled. Do we want to have an API to tell whether device has aux-domain feature? It could be included in the enable API. The enable API returns failure if device doesn't support aux-domain. Regards, Joerg Best regards, Lu Baolu
Re: [PATCH v5 2/8] iommu/vt-d: Add per-device IOMMU feature ops entries
Hi, this looks a bit confusing to me because I can see no checking whether the device actually supports scalable mode. More below: On Thu, Jan 10, 2019 at 11:00:21AM +0800, Lu Baolu wrote: > +static int intel_iommu_enable_auxd(struct device *dev) > +{ > + struct device_domain_info *info; > + struct dmar_domain *domain; > + unsigned long flags; > + > + if (!scalable_mode_support()) > + return -ENODEV; > + > + domain = get_valid_domain_for_dev(dev); > + if (!domain) > + return -ENODEV; > + > + spin_lock_irqsave(_domain_lock, flags); > + info = dev->archdata.iommu; > + info->auxd_enabled = 1; > + spin_unlock_irqrestore(_domain_lock, flags); > + > + return 0; > +} This code sets a flag to mark scalable mode enabled. Doesn't the device need some handling too, like enabling the PASID capability and all? > + > +static bool > +intel_iommu_dev_has_feat(struct device *dev, enum iommu_dev_features feat) > +{ > + struct device_domain_info *info = dev->archdata.iommu; > + > + if (feat == IOMMU_DEV_FEAT_AUX) > + return scalable_mode_support() && info && info->auxd_enabled; > + > + return false; > +} Why is this checking the auxd_enabled flag? The function should just return whether the device _supports_ scalable mode, not whether it is enabled. Regards, Joerg