* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support
@ 2020-05-16 4:59 kbuild test robot
0 siblings, 0 replies; 11+ messages in thread
From: kbuild test robot @ 2020-05-16 4:59 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 13200 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <1589410909-38925-5-git-send-email-jacob.jun.pan@linux.intel.com>
References: <1589410909-38925-5-git-send-email-jacob.jun.pan@linux.intel.com>
TO: Jacob Pan <jacob.jun.pan@linux.intel.com>
Hi Jacob,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.7-rc5 next-20200515]
[cannot apply to iommu/next linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jacob-Pan/Nested-Shared-Virtual-Address-SVA-VT-d-support/20200514-070150
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 24085f70a6e1b0cb647ec92623284641d8270637
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-193-gb8fad4bc-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/iommu/intel-svm.c:342:49: sparse: sparse: non size-preserving integer to pointer cast
# https://github.com/0day-ci/linux/commit/1503f7f9276292f51f303aae7bd50cb57fb16249
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 1503f7f9276292f51f303aae7bd50cb57fb16249
vim +342 drivers/iommu/intel-svm.c
2f26e0a9c9860db David Woodhouse 2015-09-09 224
034d473109e9078 Jacob Pan 2020-01-02 225 #define for_each_svm_dev(sdev, svm, d) \
034d473109e9078 Jacob Pan 2020-01-02 226 list_for_each_entry((sdev), &(svm)->devs, list) \
034d473109e9078 Jacob Pan 2020-01-02 227 if ((d) != (sdev)->dev) {} else
034d473109e9078 Jacob Pan 2020-01-02 228
1503f7f9276292f Jacob Pan 2020-05-13 229 int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
1503f7f9276292f Jacob Pan 2020-05-13 230 struct iommu_gpasid_bind_data *data)
1503f7f9276292f Jacob Pan 2020-05-13 231 {
1503f7f9276292f Jacob Pan 2020-05-13 232 struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
1503f7f9276292f Jacob Pan 2020-05-13 233 struct dmar_domain *dmar_domain;
1503f7f9276292f Jacob Pan 2020-05-13 234 struct intel_svm_dev *sdev;
1503f7f9276292f Jacob Pan 2020-05-13 235 struct intel_svm *svm;
1503f7f9276292f Jacob Pan 2020-05-13 236 int ret = 0;
1503f7f9276292f Jacob Pan 2020-05-13 237
1503f7f9276292f Jacob Pan 2020-05-13 238 if (WARN_ON(!iommu) || !data)
1503f7f9276292f Jacob Pan 2020-05-13 239 return -EINVAL;
1503f7f9276292f Jacob Pan 2020-05-13 240
1503f7f9276292f Jacob Pan 2020-05-13 241 if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||
1503f7f9276292f Jacob Pan 2020-05-13 242 data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
1503f7f9276292f Jacob Pan 2020-05-13 243 return -EINVAL;
1503f7f9276292f Jacob Pan 2020-05-13 244
1503f7f9276292f Jacob Pan 2020-05-13 245 if (dev_is_pci(dev)) {
1503f7f9276292f Jacob Pan 2020-05-13 246 /* VT-d supports devices with full 20 bit PASIDs only */
1503f7f9276292f Jacob Pan 2020-05-13 247 if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX)
1503f7f9276292f Jacob Pan 2020-05-13 248 return -EINVAL;
1503f7f9276292f Jacob Pan 2020-05-13 249 } else {
1503f7f9276292f Jacob Pan 2020-05-13 250 return -ENOTSUPP;
1503f7f9276292f Jacob Pan 2020-05-13 251 }
1503f7f9276292f Jacob Pan 2020-05-13 252
1503f7f9276292f Jacob Pan 2020-05-13 253 /*
1503f7f9276292f Jacob Pan 2020-05-13 254 * We only check host PASID range, we have no knowledge to check
1503f7f9276292f Jacob Pan 2020-05-13 255 * guest PASID range.
1503f7f9276292f Jacob Pan 2020-05-13 256 */
1503f7f9276292f Jacob Pan 2020-05-13 257 if (data->hpasid <= 0 || data->hpasid >= PASID_MAX)
1503f7f9276292f Jacob Pan 2020-05-13 258 return -EINVAL;
1503f7f9276292f Jacob Pan 2020-05-13 259
1503f7f9276292f Jacob Pan 2020-05-13 260 dmar_domain = to_dmar_domain(domain);
1503f7f9276292f Jacob Pan 2020-05-13 261
1503f7f9276292f Jacob Pan 2020-05-13 262 mutex_lock(&pasid_mutex);
1503f7f9276292f Jacob Pan 2020-05-13 263 svm = ioasid_find(NULL, data->hpasid, NULL);
1503f7f9276292f Jacob Pan 2020-05-13 264 if (IS_ERR(svm)) {
1503f7f9276292f Jacob Pan 2020-05-13 265 ret = PTR_ERR(svm);
1503f7f9276292f Jacob Pan 2020-05-13 266 goto out;
1503f7f9276292f Jacob Pan 2020-05-13 267 }
1503f7f9276292f Jacob Pan 2020-05-13 268
1503f7f9276292f Jacob Pan 2020-05-13 269 if (svm) {
1503f7f9276292f Jacob Pan 2020-05-13 270 /*
1503f7f9276292f Jacob Pan 2020-05-13 271 * If we found svm for the PASID, there must be at
1503f7f9276292f Jacob Pan 2020-05-13 272 * least one device bond, otherwise svm should be freed.
1503f7f9276292f Jacob Pan 2020-05-13 273 */
1503f7f9276292f Jacob Pan 2020-05-13 274 if (WARN_ON(list_empty(&svm->devs))) {
1503f7f9276292f Jacob Pan 2020-05-13 275 ret = -EINVAL;
1503f7f9276292f Jacob Pan 2020-05-13 276 goto out;
1503f7f9276292f Jacob Pan 2020-05-13 277 }
1503f7f9276292f Jacob Pan 2020-05-13 278
1503f7f9276292f Jacob Pan 2020-05-13 279 for_each_svm_dev(sdev, svm, dev) {
1503f7f9276292f Jacob Pan 2020-05-13 280 /*
1503f7f9276292f Jacob Pan 2020-05-13 281 * For devices with aux domains, we should allow multiple
1503f7f9276292f Jacob Pan 2020-05-13 282 * bind calls with the same PASID and pdev.
1503f7f9276292f Jacob Pan 2020-05-13 283 */
1503f7f9276292f Jacob Pan 2020-05-13 284 if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) {
1503f7f9276292f Jacob Pan 2020-05-13 285 sdev->users++;
1503f7f9276292f Jacob Pan 2020-05-13 286 } else {
1503f7f9276292f Jacob Pan 2020-05-13 287 dev_warn_ratelimited(dev, "Already bound with PASID %u\n",
1503f7f9276292f Jacob Pan 2020-05-13 288 svm->pasid);
1503f7f9276292f Jacob Pan 2020-05-13 289 ret = -EBUSY;
1503f7f9276292f Jacob Pan 2020-05-13 290 }
1503f7f9276292f Jacob Pan 2020-05-13 291 goto out;
1503f7f9276292f Jacob Pan 2020-05-13 292 }
1503f7f9276292f Jacob Pan 2020-05-13 293 } else {
1503f7f9276292f Jacob Pan 2020-05-13 294 /* We come here when PASID has never been bond to a device. */
1503f7f9276292f Jacob Pan 2020-05-13 295 svm = kzalloc(sizeof(*svm), GFP_KERNEL);
1503f7f9276292f Jacob Pan 2020-05-13 296 if (!svm) {
1503f7f9276292f Jacob Pan 2020-05-13 297 ret = -ENOMEM;
1503f7f9276292f Jacob Pan 2020-05-13 298 goto out;
1503f7f9276292f Jacob Pan 2020-05-13 299 }
1503f7f9276292f Jacob Pan 2020-05-13 300 /* REVISIT: upper layer/VFIO can track host process that bind
1503f7f9276292f Jacob Pan 2020-05-13 301 * the PASID. ioasid_set = mm might be sufficient for vfio to
1503f7f9276292f Jacob Pan 2020-05-13 302 * check pasid VMM ownership. We can drop the following line
1503f7f9276292f Jacob Pan 2020-05-13 303 * once VFIO and IOASID set check is in place.
1503f7f9276292f Jacob Pan 2020-05-13 304 */
1503f7f9276292f Jacob Pan 2020-05-13 305 svm->mm = get_task_mm(current);
1503f7f9276292f Jacob Pan 2020-05-13 306 svm->pasid = data->hpasid;
1503f7f9276292f Jacob Pan 2020-05-13 307 if (data->flags & IOMMU_SVA_GPASID_VAL) {
1503f7f9276292f Jacob Pan 2020-05-13 308 svm->gpasid = data->gpasid;
1503f7f9276292f Jacob Pan 2020-05-13 309 svm->flags |= SVM_FLAG_GUEST_PASID;
1503f7f9276292f Jacob Pan 2020-05-13 310 }
1503f7f9276292f Jacob Pan 2020-05-13 311 ioasid_set_data(data->hpasid, svm);
1503f7f9276292f Jacob Pan 2020-05-13 312 INIT_LIST_HEAD_RCU(&svm->devs);
1503f7f9276292f Jacob Pan 2020-05-13 313 mmput(svm->mm);
1503f7f9276292f Jacob Pan 2020-05-13 314 }
1503f7f9276292f Jacob Pan 2020-05-13 315 sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
1503f7f9276292f Jacob Pan 2020-05-13 316 if (!sdev) {
1503f7f9276292f Jacob Pan 2020-05-13 317 ret = -ENOMEM;
1503f7f9276292f Jacob Pan 2020-05-13 318 goto out;
1503f7f9276292f Jacob Pan 2020-05-13 319 }
1503f7f9276292f Jacob Pan 2020-05-13 320 sdev->dev = dev;
1503f7f9276292f Jacob Pan 2020-05-13 321
1503f7f9276292f Jacob Pan 2020-05-13 322 /* Only count users if device has aux domains */
1503f7f9276292f Jacob Pan 2020-05-13 323 if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
1503f7f9276292f Jacob Pan 2020-05-13 324 sdev->users = 1;
1503f7f9276292f Jacob Pan 2020-05-13 325
1503f7f9276292f Jacob Pan 2020-05-13 326 /* Set up device context entry for PASID if not enabled already */
1503f7f9276292f Jacob Pan 2020-05-13 327 ret = intel_iommu_enable_pasid(iommu, sdev->dev);
1503f7f9276292f Jacob Pan 2020-05-13 328 if (ret) {
1503f7f9276292f Jacob Pan 2020-05-13 329 dev_err_ratelimited(dev, "Failed to enable PASID capability\n");
1503f7f9276292f Jacob Pan 2020-05-13 330 kfree(sdev);
1503f7f9276292f Jacob Pan 2020-05-13 331 goto out;
1503f7f9276292f Jacob Pan 2020-05-13 332 }
1503f7f9276292f Jacob Pan 2020-05-13 333
1503f7f9276292f Jacob Pan 2020-05-13 334 /*
1503f7f9276292f Jacob Pan 2020-05-13 335 * PASID table is per device for better security. Therefore, for
1503f7f9276292f Jacob Pan 2020-05-13 336 * each bind of a new device even with an existing PASID, we need to
1503f7f9276292f Jacob Pan 2020-05-13 337 * call the nested mode setup function here.
1503f7f9276292f Jacob Pan 2020-05-13 338 */
1503f7f9276292f Jacob Pan 2020-05-13 339 spin_lock(&iommu->lock);
1503f7f9276292f Jacob Pan 2020-05-13 340 ret = intel_pasid_setup_nested(iommu,
1503f7f9276292f Jacob Pan 2020-05-13 341 dev,
1503f7f9276292f Jacob Pan 2020-05-13 @342 (pgd_t *)data->gpgd,
1503f7f9276292f Jacob Pan 2020-05-13 343 data->hpasid,
1503f7f9276292f Jacob Pan 2020-05-13 344 &data->vtd,
1503f7f9276292f Jacob Pan 2020-05-13 345 dmar_domain,
1503f7f9276292f Jacob Pan 2020-05-13 346 data->addr_width);
1503f7f9276292f Jacob Pan 2020-05-13 347 spin_unlock(&iommu->lock);
1503f7f9276292f Jacob Pan 2020-05-13 348 if (ret) {
1503f7f9276292f Jacob Pan 2020-05-13 349 dev_err_ratelimited(dev, "Failed to set up PASID %llu in nested mode, Err %d\n",
1503f7f9276292f Jacob Pan 2020-05-13 350 data->hpasid, ret);
1503f7f9276292f Jacob Pan 2020-05-13 351 /*
1503f7f9276292f Jacob Pan 2020-05-13 352 * PASID entry should be in cleared state if nested mode
1503f7f9276292f Jacob Pan 2020-05-13 353 * set up failed. So we only need to clear IOASID tracking
1503f7f9276292f Jacob Pan 2020-05-13 354 * data such that free call will succeed.
1503f7f9276292f Jacob Pan 2020-05-13 355 */
1503f7f9276292f Jacob Pan 2020-05-13 356 kfree(sdev);
1503f7f9276292f Jacob Pan 2020-05-13 357 goto out;
1503f7f9276292f Jacob Pan 2020-05-13 358 }
1503f7f9276292f Jacob Pan 2020-05-13 359
1503f7f9276292f Jacob Pan 2020-05-13 360 svm->flags |= SVM_FLAG_GUEST_MODE;
1503f7f9276292f Jacob Pan 2020-05-13 361
1503f7f9276292f Jacob Pan 2020-05-13 362 init_rcu_head(&sdev->rcu);
1503f7f9276292f Jacob Pan 2020-05-13 363 list_add_rcu(&sdev->list, &svm->devs);
1503f7f9276292f Jacob Pan 2020-05-13 364 out:
1503f7f9276292f Jacob Pan 2020-05-13 365 if (list_empty(&svm->devs)) {
1503f7f9276292f Jacob Pan 2020-05-13 366 ioasid_set_data(data->hpasid, NULL);
1503f7f9276292f Jacob Pan 2020-05-13 367 kfree(svm);
1503f7f9276292f Jacob Pan 2020-05-13 368 }
1503f7f9276292f Jacob Pan 2020-05-13 369
1503f7f9276292f Jacob Pan 2020-05-13 370 mutex_unlock(&pasid_mutex);
1503f7f9276292f Jacob Pan 2020-05-13 371 return ret;
1503f7f9276292f Jacob Pan 2020-05-13 372 }
1503f7f9276292f Jacob Pan 2020-05-13 373
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v13 0/8] Nested Shared Virtual Address (SVA) VT-d support @ 2020-05-13 23:01 Jacob Pan 2020-05-13 23:01 ` Jacob Pan 0 siblings, 1 reply; 11+ messages in thread From: Jacob Pan @ 2020-05-13 23:01 UTC (permalink / raw) To: Lu Baolu, iommu, LKML, Joerg Roedel, David Woodhouse, Jean-Philippe Brucker, Eric Auger Cc: Yi Liu, Tian, Kevin, Raj Ashok, Alex Williamson, Christoph Hellwig, Jonathan Cameron, Jacob Pan Shared virtual address (SVA), a.k.a, Shared virtual memory (SVM) on Intel platforms allow address space sharing between device DMA and applications. SVA can reduce programming complexity and enhance security. This series is intended to enable SVA virtualization, i.e. enable use of SVA within a guest user application. This is the remaining portion of the original patchset that is based on Joerg's x86/vt-d branch. The preparatory and cleanup patches are merged here. (git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git) Only IOMMU portion of the changes are included in this series. Additional support is needed in VFIO and QEMU (will be submitted separately) to complete this functionality. To make incremental changes and reduce the size of each patchset. This series does not inlcude support for page request services. In VT-d implementation, PASID table is per device and maintained in the host. Guest PASID table is shadowed in VMM where virtual IOMMU is emulated. .-------------. .---------------------------. | vIOMMU | | Guest process CR3, FL only| | | '---------------------------' .----------------/ | PASID Entry |--- PASID cache flush - '-------------' | | | V | | CR3 in GPA '-------------' Guest ------| Shadow |--------------------------|-------- v v v Host .-------------. .----------------------. | pIOMMU | | Bind FL for GVA-GPA | | | '----------------------' .----------------/ | | PASID Entry | V (Nested xlate) '----------------\.------------------------------. | | |SL for GPA-HPA, default domain| | | '------------------------------' '-------------' Where: - FL = First level/stage one page tables - SL = Second level/stage two page tables This is the remaining VT-d only portion of V5 since the uAPIs and IOASID common code have been applied to Joerg's IOMMU core branch. (https://lkml.org/lkml/2019/10/2/833) The complete set with VFIO patches are here: https://github.com/jacobpan/linux.git:siov_sva The complete nested SVA upstream patches are divided into three phases: 1. Common APIs and PCI device direct assignment 2. Page Request Services (PRS) support 3. Mediated device assignment With this set and the accompanied VFIO code, we will achieve phase #1. Thanks, Jacob ChangeLog: - v13 - Dropped memory type support (MTS) in guest PASID bind - Do not support multiple bind gpasid if device has no aux domain - Removed extra error msgs in pasid_setup_bind_data() - Replaced SVM device list free function with combined out label - v12 - Fixed IA64 cross compile error - Squashed two patches that add macros with its users - Use ratelimited prints for all user called APIs - Check domain nesting attr for vSVA APIs. - Misc style improvements - v11 Misc fixes and improvements based on review by Kevin & Eric - Fixed devTLB granularity conversion - Simplified VT-d granulairy lookup by replacing 2D map array with invalid entries. - Fixed locking in bind guest PASID - Added nesting domain attr check - Squashed agaw checking patch with user - Use rate limitted error message for all user originated calls - v10 - Addressed Eric's review in v7 and v9. Most fixes are in 3/10 and 6/10. Extra condition checks and consolidation of duplicated codes. - v9 - Addressed Baolu's comments for v8 for IOTLB flush consolidation, bug fixes - Removed IOASID notifier code which will be submitted separately to address PASID life cycle management with multiple users. - v8 - Extracted cleanup patches from V7 and accepted into maintainer's tree (https://lkml.org/lkml/2019/12/2/514). - Added IOASID notifier and VT-d handler for termination of PASID IOMMU context upon free. This will ensure success of VFIO IOASID free API regardless PASID is in use. (https://lore.kernel.org/linux-iommu/1571919983-3231-1-git-send-email-yi.l.liu@intel.com/) - V7 - Respect vIOMMU PASID range in virtual command PASID/IOASID allocator - Caching virtual command capabilities to avoid runtime checks that could cause vmexits. - V6 - Rebased on top of Joerg's core branch (git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git core) - Adapt to new uAPIs and IOASID allocators - V5 Rebased on v5.3-rc4 which has some of the IOMMU fault APIs merged. Addressed v4 review comments from Eric Auger, Baolu Lu, and Jonathan Cameron. Specific changes are as follows: - Refined custom IOASID allocator to support multiple vIOMMU, hotplug cases. - Extracted vendor data from IOMMU guest PASID bind data, for VT-d will support all necessary guest PASID entry fields for PASID bind. - Support non-identity host-guest PASID mapping - Exception handling in various cases - V4 - Redesigned IOASID allocator such that it can support custom allocators with shared helper functions. Use separate XArray to store IOASIDs per allocator. Took advice from Eric Auger to have default allocator use the generic allocator structure. Combined into one patch in that the default allocator is just "another" allocator now. Can be built as a module in case of driver use without IOMMU. - Extended bind guest PASID data to support SMMU and non-identity guest to host PASID mapping https://lkml.org/lkml/2019/5/21/802 - Rebased on Jean's sva/api common tree, new patches starts with [PATCH v4 10/22] - V3 - Addressed thorough review comments from Eric Auger (Thank you!) - Moved IOASID allocator from driver core to IOMMU code per suggestion by Christoph Hellwig (https://lkml.org/lkml/2019/4/26/462) - Rebased on top of Jean's SVA API branch and Eric's v7[1] (git://linux-arm.org/linux-jpb.git sva/api) - All IOMMU APIs are unmodified (except the new bind guest PASID call in patch 9/16) - V2 - Rebased on Joerg's IOMMU x86/vt-d branch v5.1-rc4 - Integrated with Eric Auger's new v7 series for common APIs (https://github.com/eauger/linux/tree/v5.1-rc3-2stage-v7) - Addressed review comments from Andy Shevchenko and Alex Williamson on IOASID custom allocator. - Support multiple custom IOASID allocators (vIOMMUs) and dynamic registration. Jacob Pan (7): iommu/vt-d: Move domain helper to header iommu/vt-d: Use a helper function to skip agaw for SL iommu/vt-d: Add nested translation helper function iommu/vt-d: Add bind guest PASID support iommu/vt-d: Support flushing more translation cache types iommu/vt-d: Add svm/sva invalidate function iommu/vt-d: Add custom allocator for IOASID Lu Baolu (1): iommu/vt-d: Enlightened PASID allocation drivers/iommu/dmar.c | 40 ++++++ drivers/iommu/intel-iommu.c | 291 +++++++++++++++++++++++++++++++++++++++----- drivers/iommu/intel-pasid.c | 266 +++++++++++++++++++++++++++++++++++++--- drivers/iommu/intel-pasid.h | 23 +++- drivers/iommu/intel-svm.c | 203 ++++++++++++++++++++++++++++++ include/linux/intel-iommu.h | 69 ++++++++++- include/linux/intel-svm.h | 12 ++ include/uapi/linux/iommu.h | 5 + 8 files changed, 858 insertions(+), 51 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support 2020-05-13 23:01 [PATCH v13 0/8] Nested Shared Virtual Address (SVA) VT-d support Jacob Pan @ 2020-05-13 23:01 ` Jacob Pan 0 siblings, 0 replies; 11+ messages in thread From: Jacob Pan @ 2020-05-13 23:01 UTC (permalink / raw) To: Lu Baolu, iommu, LKML, Joerg Roedel, David Woodhouse, Jean-Philippe Brucker, Eric Auger Cc: Yi Liu, Tian, Kevin, Raj Ashok, Alex Williamson, Christoph Hellwig, Jonathan Cameron, Jacob Pan When supporting guest SVA with emulated IOMMU, the guest PASID table is shadowed in VMM. Updates to guest vIOMMU PASID table will result in PASID cache flush which will be passed down to the host as bind guest PASID calls. For the SL page tables, it will be harvested from device's default domain (request w/o PASID), or aux domain in case of mediated device. .-------------. .---------------------------. | vIOMMU | | Guest process CR3, FL only| | | '---------------------------' .----------------/ | PASID Entry |--- PASID cache flush - '-------------' | | | V | | CR3 in GPA '-------------' Guest ------| Shadow |--------------------------|-------- v v v Host .-------------. .----------------------. | pIOMMU | | Bind FL for GVA-GPA | | | '----------------------' .----------------/ | | PASID Entry | V (Nested xlate) '----------------\.------------------------------. | | |SL for GPA-HPA, default domain| | | '------------------------------' '-------------' Where: - FL = First level/stage one page tables - SL = Second level/stage two page tables Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> Signed-off-by: Liu Yi L <yi.l.liu@intel.com> --- v13 Only allow multiple bind if device has aux domain --- --- drivers/iommu/intel-iommu.c | 4 + drivers/iommu/intel-svm.c | 203 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/intel-iommu.h | 6 +- include/linux/intel-svm.h | 12 +++ 4 files changed, 224 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 9c01e391a931..8862d6b0ef21 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -6179,6 +6179,10 @@ const struct iommu_ops intel_iommu_ops = { .dev_disable_feat = intel_iommu_dev_disable_feat, .is_attach_deferred = intel_iommu_is_attach_deferred, .pgsize_bitmap = INTEL_IOMMU_PGSIZES, +#ifdef CONFIG_INTEL_IOMMU_SVM + .sva_bind_gpasid = intel_svm_bind_gpasid, + .sva_unbind_gpasid = intel_svm_unbind_gpasid, +#endif }; static void quirk_iommu_igfx(struct pci_dev *dev) diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c index 2998418f0a38..46819c5c13ee 100644 --- a/drivers/iommu/intel-svm.c +++ b/drivers/iommu/intel-svm.c @@ -226,6 +226,209 @@ static LIST_HEAD(global_svm_list); list_for_each_entry((sdev), &(svm)->devs, list) \ if ((d) != (sdev)->dev) {} else +int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev, + struct iommu_gpasid_bind_data *data) +{ + struct intel_iommu *iommu = intel_svm_device_to_iommu(dev); + struct dmar_domain *dmar_domain; + struct intel_svm_dev *sdev; + struct intel_svm *svm; + int ret = 0; + + if (WARN_ON(!iommu) || !data) + return -EINVAL; + + if (data->version != IOMMU_GPASID_BIND_VERSION_1 || + data->format != IOMMU_PASID_FORMAT_INTEL_VTD) + return -EINVAL; + + if (dev_is_pci(dev)) { + /* VT-d supports devices with full 20 bit PASIDs only */ + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) + return -EINVAL; + } else { + return -ENOTSUPP; + } + + /* + * We only check host PASID range, we have no knowledge to check + * guest PASID range. + */ + if (data->hpasid <= 0 || data->hpasid >= PASID_MAX) + return -EINVAL; + + dmar_domain = to_dmar_domain(domain); + + mutex_lock(&pasid_mutex); + svm = ioasid_find(NULL, data->hpasid, NULL); + if (IS_ERR(svm)) { + ret = PTR_ERR(svm); + goto out; + } + + if (svm) { + /* + * If we found svm for the PASID, there must be at + * least one device bond, otherwise svm should be freed. + */ + if (WARN_ON(list_empty(&svm->devs))) { + ret = -EINVAL; + goto out; + } + + for_each_svm_dev(sdev, svm, dev) { + /* + * For devices with aux domains, we should allow multiple + * bind calls with the same PASID and pdev. + */ + if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) { + sdev->users++; + } else { + dev_warn_ratelimited(dev, "Already bound with PASID %u\n", + svm->pasid); + ret = -EBUSY; + } + goto out; + } + } else { + /* We come here when PASID has never been bond to a device. */ + svm = kzalloc(sizeof(*svm), GFP_KERNEL); + if (!svm) { + ret = -ENOMEM; + goto out; + } + /* REVISIT: upper layer/VFIO can track host process that bind + * the PASID. ioasid_set = mm might be sufficient for vfio to + * check pasid VMM ownership. We can drop the following line + * once VFIO and IOASID set check is in place. + */ + svm->mm = get_task_mm(current); + svm->pasid = data->hpasid; + if (data->flags & IOMMU_SVA_GPASID_VAL) { + svm->gpasid = data->gpasid; + svm->flags |= SVM_FLAG_GUEST_PASID; + } + ioasid_set_data(data->hpasid, svm); + INIT_LIST_HEAD_RCU(&svm->devs); + mmput(svm->mm); + } + sdev = kzalloc(sizeof(*sdev), GFP_KERNEL); + if (!sdev) { + ret = -ENOMEM; + goto out; + } + sdev->dev = dev; + + /* Only count users if device has aux domains */ + if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) + sdev->users = 1; + + /* Set up device context entry for PASID if not enabled already */ + ret = intel_iommu_enable_pasid(iommu, sdev->dev); + if (ret) { + dev_err_ratelimited(dev, "Failed to enable PASID capability\n"); + kfree(sdev); + goto out; + } + + /* + * PASID table is per device for better security. Therefore, for + * each bind of a new device even with an existing PASID, we need to + * call the nested mode setup function here. + */ + spin_lock(&iommu->lock); + ret = intel_pasid_setup_nested(iommu, + dev, + (pgd_t *)data->gpgd, + data->hpasid, + &data->vtd, + dmar_domain, + data->addr_width); + spin_unlock(&iommu->lock); + if (ret) { + dev_err_ratelimited(dev, "Failed to set up PASID %llu in nested mode, Err %d\n", + data->hpasid, ret); + /* + * PASID entry should be in cleared state if nested mode + * set up failed. So we only need to clear IOASID tracking + * data such that free call will succeed. + */ + kfree(sdev); + goto out; + } + + svm->flags |= SVM_FLAG_GUEST_MODE; + + init_rcu_head(&sdev->rcu); + list_add_rcu(&sdev->list, &svm->devs); + out: + if (list_empty(&svm->devs)) { + ioasid_set_data(data->hpasid, NULL); + kfree(svm); + } + + mutex_unlock(&pasid_mutex); + return ret; +} + +int intel_svm_unbind_gpasid(struct device *dev, int pasid) +{ + struct intel_iommu *iommu = intel_svm_device_to_iommu(dev); + struct intel_svm_dev *sdev; + struct intel_svm *svm; + int ret = -EINVAL; + + if (WARN_ON(!iommu)) + return -EINVAL; + + mutex_lock(&pasid_mutex); + svm = ioasid_find(NULL, pasid, NULL); + if (!svm) { + ret = -EINVAL; + goto out; + } + + if (IS_ERR(svm)) { + ret = PTR_ERR(svm); + goto out; + } + + for_each_svm_dev(sdev, svm, dev) { + ret = 0; + if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) + sdev->users--; + if (!sdev->users) { + list_del_rcu(&sdev->list); + intel_pasid_tear_down_entry(iommu, dev, svm->pasid); + intel_flush_svm_range_dev(svm, sdev, 0, -1, 0); + /* TODO: Drain in flight PRQ for the PASID since it + * may get reused soon, we don't want to + * confuse with its previous life. + * intel_svm_drain_prq(dev, pasid); + */ + kfree_rcu(sdev, rcu); + + if (list_empty(&svm->devs)) { + /* + * We do not free the IOASID here in that + * IOMMU driver did not allocate it. + * Unlike native SVM, IOASID for guest use was + * allocated prior to the bind call. + * In any case, if the free call comes before + * the unbind, IOMMU driver will get notified + * and perform cleanup. + */ + ioasid_set_data(pasid, NULL); + kfree(svm); + } + } + break; + } +out: + mutex_unlock(&pasid_mutex); + return ret; +} + int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_ops *ops) { struct intel_iommu *iommu = intel_svm_device_to_iommu(dev); diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h index 6da03f627ba3..878534939ed8 100644 --- a/include/linux/intel-iommu.h +++ b/include/linux/intel-iommu.h @@ -706,7 +706,9 @@ struct dmar_domain *find_domain(struct device *dev); extern void intel_svm_check(struct intel_iommu *iommu); extern int intel_svm_enable_prq(struct intel_iommu *iommu); extern int intel_svm_finish_prq(struct intel_iommu *iommu); - +int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev, + struct iommu_gpasid_bind_data *data); +int intel_svm_unbind_gpasid(struct device *dev, int pasid); struct svm_dev_ops; struct intel_svm_dev { @@ -723,9 +725,11 @@ struct intel_svm_dev { struct intel_svm { struct mmu_notifier notifier; struct mm_struct *mm; + struct intel_iommu *iommu; int flags; int pasid; + int gpasid; /* In case that guest PASID is different from host PASID */ struct list_head devs; struct list_head list; }; diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h index d7c403d0dd27..11bdb968e263 100644 --- a/include/linux/intel-svm.h +++ b/include/linux/intel-svm.h @@ -44,6 +44,18 @@ struct svm_dev_ops { * do such IOTLB flushes automatically. */ #define SVM_FLAG_SUPERVISOR_MODE (1<<1) +/* + * The SVM_FLAG_GUEST_MODE flag is used when a PASID bind is for guest + * processes. Compared to the host bind, the primary differences are: + * 1. mm life cycle management + * 2. fault reporting + */ +#define SVM_FLAG_GUEST_MODE (1<<2) +/* + * The SVM_FLAG_GUEST_PASID flag is used when a guest has its own PASID space, + * which requires guest and host PASID translation at both directions. + */ +#define SVM_FLAG_GUEST_PASID (1<<3) #ifdef CONFIG_INTEL_IOMMU_SVM -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support @ 2020-05-13 23:01 ` Jacob Pan 0 siblings, 0 replies; 11+ messages in thread From: Jacob Pan @ 2020-05-13 23:01 UTC (permalink / raw) To: Lu Baolu, iommu, LKML, Joerg Roedel, David Woodhouse, Jean-Philippe Brucker, Eric Auger Cc: Tian, Kevin, Raj Ashok, Christoph Hellwig, Alex Williamson, Jonathan Cameron When supporting guest SVA with emulated IOMMU, the guest PASID table is shadowed in VMM. Updates to guest vIOMMU PASID table will result in PASID cache flush which will be passed down to the host as bind guest PASID calls. For the SL page tables, it will be harvested from device's default domain (request w/o PASID), or aux domain in case of mediated device. .-------------. .---------------------------. | vIOMMU | | Guest process CR3, FL only| | | '---------------------------' .----------------/ | PASID Entry |--- PASID cache flush - '-------------' | | | V | | CR3 in GPA '-------------' Guest ------| Shadow |--------------------------|-------- v v v Host .-------------. .----------------------. | pIOMMU | | Bind FL for GVA-GPA | | | '----------------------' .----------------/ | | PASID Entry | V (Nested xlate) '----------------\.------------------------------. | | |SL for GPA-HPA, default domain| | | '------------------------------' '-------------' Where: - FL = First level/stage one page tables - SL = Second level/stage two page tables Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> Signed-off-by: Liu Yi L <yi.l.liu@intel.com> --- v13 Only allow multiple bind if device has aux domain --- --- drivers/iommu/intel-iommu.c | 4 + drivers/iommu/intel-svm.c | 203 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/intel-iommu.h | 6 +- include/linux/intel-svm.h | 12 +++ 4 files changed, 224 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 9c01e391a931..8862d6b0ef21 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -6179,6 +6179,10 @@ const struct iommu_ops intel_iommu_ops = { .dev_disable_feat = intel_iommu_dev_disable_feat, .is_attach_deferred = intel_iommu_is_attach_deferred, .pgsize_bitmap = INTEL_IOMMU_PGSIZES, +#ifdef CONFIG_INTEL_IOMMU_SVM + .sva_bind_gpasid = intel_svm_bind_gpasid, + .sva_unbind_gpasid = intel_svm_unbind_gpasid, +#endif }; static void quirk_iommu_igfx(struct pci_dev *dev) diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c index 2998418f0a38..46819c5c13ee 100644 --- a/drivers/iommu/intel-svm.c +++ b/drivers/iommu/intel-svm.c @@ -226,6 +226,209 @@ static LIST_HEAD(global_svm_list); list_for_each_entry((sdev), &(svm)->devs, list) \ if ((d) != (sdev)->dev) {} else +int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev, + struct iommu_gpasid_bind_data *data) +{ + struct intel_iommu *iommu = intel_svm_device_to_iommu(dev); + struct dmar_domain *dmar_domain; + struct intel_svm_dev *sdev; + struct intel_svm *svm; + int ret = 0; + + if (WARN_ON(!iommu) || !data) + return -EINVAL; + + if (data->version != IOMMU_GPASID_BIND_VERSION_1 || + data->format != IOMMU_PASID_FORMAT_INTEL_VTD) + return -EINVAL; + + if (dev_is_pci(dev)) { + /* VT-d supports devices with full 20 bit PASIDs only */ + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) + return -EINVAL; + } else { + return -ENOTSUPP; + } + + /* + * We only check host PASID range, we have no knowledge to check + * guest PASID range. + */ + if (data->hpasid <= 0 || data->hpasid >= PASID_MAX) + return -EINVAL; + + dmar_domain = to_dmar_domain(domain); + + mutex_lock(&pasid_mutex); + svm = ioasid_find(NULL, data->hpasid, NULL); + if (IS_ERR(svm)) { + ret = PTR_ERR(svm); + goto out; + } + + if (svm) { + /* + * If we found svm for the PASID, there must be at + * least one device bond, otherwise svm should be freed. + */ + if (WARN_ON(list_empty(&svm->devs))) { + ret = -EINVAL; + goto out; + } + + for_each_svm_dev(sdev, svm, dev) { + /* + * For devices with aux domains, we should allow multiple + * bind calls with the same PASID and pdev. + */ + if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) { + sdev->users++; + } else { + dev_warn_ratelimited(dev, "Already bound with PASID %u\n", + svm->pasid); + ret = -EBUSY; + } + goto out; + } + } else { + /* We come here when PASID has never been bond to a device. */ + svm = kzalloc(sizeof(*svm), GFP_KERNEL); + if (!svm) { + ret = -ENOMEM; + goto out; + } + /* REVISIT: upper layer/VFIO can track host process that bind + * the PASID. ioasid_set = mm might be sufficient for vfio to + * check pasid VMM ownership. We can drop the following line + * once VFIO and IOASID set check is in place. + */ + svm->mm = get_task_mm(current); + svm->pasid = data->hpasid; + if (data->flags & IOMMU_SVA_GPASID_VAL) { + svm->gpasid = data->gpasid; + svm->flags |= SVM_FLAG_GUEST_PASID; + } + ioasid_set_data(data->hpasid, svm); + INIT_LIST_HEAD_RCU(&svm->devs); + mmput(svm->mm); + } + sdev = kzalloc(sizeof(*sdev), GFP_KERNEL); + if (!sdev) { + ret = -ENOMEM; + goto out; + } + sdev->dev = dev; + + /* Only count users if device has aux domains */ + if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) + sdev->users = 1; + + /* Set up device context entry for PASID if not enabled already */ + ret = intel_iommu_enable_pasid(iommu, sdev->dev); + if (ret) { + dev_err_ratelimited(dev, "Failed to enable PASID capability\n"); + kfree(sdev); + goto out; + } + + /* + * PASID table is per device for better security. Therefore, for + * each bind of a new device even with an existing PASID, we need to + * call the nested mode setup function here. + */ + spin_lock(&iommu->lock); + ret = intel_pasid_setup_nested(iommu, + dev, + (pgd_t *)data->gpgd, + data->hpasid, + &data->vtd, + dmar_domain, + data->addr_width); + spin_unlock(&iommu->lock); + if (ret) { + dev_err_ratelimited(dev, "Failed to set up PASID %llu in nested mode, Err %d\n", + data->hpasid, ret); + /* + * PASID entry should be in cleared state if nested mode + * set up failed. So we only need to clear IOASID tracking + * data such that free call will succeed. + */ + kfree(sdev); + goto out; + } + + svm->flags |= SVM_FLAG_GUEST_MODE; + + init_rcu_head(&sdev->rcu); + list_add_rcu(&sdev->list, &svm->devs); + out: + if (list_empty(&svm->devs)) { + ioasid_set_data(data->hpasid, NULL); + kfree(svm); + } + + mutex_unlock(&pasid_mutex); + return ret; +} + +int intel_svm_unbind_gpasid(struct device *dev, int pasid) +{ + struct intel_iommu *iommu = intel_svm_device_to_iommu(dev); + struct intel_svm_dev *sdev; + struct intel_svm *svm; + int ret = -EINVAL; + + if (WARN_ON(!iommu)) + return -EINVAL; + + mutex_lock(&pasid_mutex); + svm = ioasid_find(NULL, pasid, NULL); + if (!svm) { + ret = -EINVAL; + goto out; + } + + if (IS_ERR(svm)) { + ret = PTR_ERR(svm); + goto out; + } + + for_each_svm_dev(sdev, svm, dev) { + ret = 0; + if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) + sdev->users--; + if (!sdev->users) { + list_del_rcu(&sdev->list); + intel_pasid_tear_down_entry(iommu, dev, svm->pasid); + intel_flush_svm_range_dev(svm, sdev, 0, -1, 0); + /* TODO: Drain in flight PRQ for the PASID since it + * may get reused soon, we don't want to + * confuse with its previous life. + * intel_svm_drain_prq(dev, pasid); + */ + kfree_rcu(sdev, rcu); + + if (list_empty(&svm->devs)) { + /* + * We do not free the IOASID here in that + * IOMMU driver did not allocate it. + * Unlike native SVM, IOASID for guest use was + * allocated prior to the bind call. + * In any case, if the free call comes before + * the unbind, IOMMU driver will get notified + * and perform cleanup. + */ + ioasid_set_data(pasid, NULL); + kfree(svm); + } + } + break; + } +out: + mutex_unlock(&pasid_mutex); + return ret; +} + int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_ops *ops) { struct intel_iommu *iommu = intel_svm_device_to_iommu(dev); diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h index 6da03f627ba3..878534939ed8 100644 --- a/include/linux/intel-iommu.h +++ b/include/linux/intel-iommu.h @@ -706,7 +706,9 @@ struct dmar_domain *find_domain(struct device *dev); extern void intel_svm_check(struct intel_iommu *iommu); extern int intel_svm_enable_prq(struct intel_iommu *iommu); extern int intel_svm_finish_prq(struct intel_iommu *iommu); - +int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev, + struct iommu_gpasid_bind_data *data); +int intel_svm_unbind_gpasid(struct device *dev, int pasid); struct svm_dev_ops; struct intel_svm_dev { @@ -723,9 +725,11 @@ struct intel_svm_dev { struct intel_svm { struct mmu_notifier notifier; struct mm_struct *mm; + struct intel_iommu *iommu; int flags; int pasid; + int gpasid; /* In case that guest PASID is different from host PASID */ struct list_head devs; struct list_head list; }; diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h index d7c403d0dd27..11bdb968e263 100644 --- a/include/linux/intel-svm.h +++ b/include/linux/intel-svm.h @@ -44,6 +44,18 @@ struct svm_dev_ops { * do such IOTLB flushes automatically. */ #define SVM_FLAG_SUPERVISOR_MODE (1<<1) +/* + * The SVM_FLAG_GUEST_MODE flag is used when a PASID bind is for guest + * processes. Compared to the host bind, the primary differences are: + * 1. mm life cycle management + * 2. fault reporting + */ +#define SVM_FLAG_GUEST_MODE (1<<2) +/* + * The SVM_FLAG_GUEST_PASID flag is used when a guest has its own PASID space, + * which requires guest and host PASID translation at both directions. + */ +#define SVM_FLAG_GUEST_PASID (1<<3) #ifdef CONFIG_INTEL_IOMMU_SVM -- 2.7.4 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support 2020-05-13 23:01 ` Jacob Pan @ 2020-05-14 5:59 ` Christoph Hellwig -1 siblings, 0 replies; 11+ messages in thread From: Christoph Hellwig @ 2020-05-14 5:59 UTC (permalink / raw) To: Jacob Pan Cc: Lu Baolu, iommu, LKML, Joerg Roedel, David Woodhouse, Jean-Philippe Brucker, Eric Auger, Yi Liu, Tian, Kevin, Raj Ashok, Alex Williamson, Christoph Hellwig, Jonathan Cameron > + if (dev_is_pci(dev)) { > + /* VT-d supports devices with full 20 bit PASIDs only */ > + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) > + return -EINVAL; > + } else { > + return -ENOTSUPP; > + } This looks strange. Why not: if (!dev_is_pci(dev)) { return -ENOTSUPP; /* VT-d supports devices with full 20 bit PASIDs only */ if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) return -EINVAL; > + for_each_svm_dev(sdev, svm, dev) { > + /* > + * For devices with aux domains, we should allow multiple > + * bind calls with the same PASID and pdev. > + */ > + if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) { > + sdev->users++; > + } else { > + dev_warn_ratelimited(dev, "Already bound with PASID %u\n", > + svm->pasid); > + ret = -EBUSY; > + } > + goto out; Is this intentionally a for loop that jumps out of the loop after the first device? > + /* > + * PASID table is per device for better security. Therefore, for > + * each bind of a new device even with an existing PASID, we need to > + * call the nested mode setup function here. > + */ > + spin_lock(&iommu->lock); > + ret = intel_pasid_setup_nested(iommu, > + dev, > + (pgd_t *)data->gpgd, > + data->hpasid, > + &data->vtd, > + dmar_domain, > + data->addr_width); Why not: et = intel_pasid_setup_nested(iommu, dev, (pgd_t *)data->gpgd, data->hpasid, &data->vtd, dmar_domain, data->addr_width); ? > + for_each_svm_dev(sdev, svm, dev) { > + ret = 0; ... > + break; > + } Same only looks at the first device style. Why dos it only care about the first device? That needs at least a comment, and probably a first_svm_dev or so heper to make it explicit. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support @ 2020-05-14 5:59 ` Christoph Hellwig 0 siblings, 0 replies; 11+ messages in thread From: Christoph Hellwig @ 2020-05-14 5:59 UTC (permalink / raw) To: Jacob Pan Cc: Tian, Kevin, Alex Williamson, Raj Ashok, David Woodhouse, LKML, Christoph Hellwig, iommu, Jean-Philippe Brucker, Jonathan Cameron > + if (dev_is_pci(dev)) { > + /* VT-d supports devices with full 20 bit PASIDs only */ > + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) > + return -EINVAL; > + } else { > + return -ENOTSUPP; > + } This looks strange. Why not: if (!dev_is_pci(dev)) { return -ENOTSUPP; /* VT-d supports devices with full 20 bit PASIDs only */ if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) return -EINVAL; > + for_each_svm_dev(sdev, svm, dev) { > + /* > + * For devices with aux domains, we should allow multiple > + * bind calls with the same PASID and pdev. > + */ > + if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX)) { > + sdev->users++; > + } else { > + dev_warn_ratelimited(dev, "Already bound with PASID %u\n", > + svm->pasid); > + ret = -EBUSY; > + } > + goto out; Is this intentionally a for loop that jumps out of the loop after the first device? > + /* > + * PASID table is per device for better security. Therefore, for > + * each bind of a new device even with an existing PASID, we need to > + * call the nested mode setup function here. > + */ > + spin_lock(&iommu->lock); > + ret = intel_pasid_setup_nested(iommu, > + dev, > + (pgd_t *)data->gpgd, > + data->hpasid, > + &data->vtd, > + dmar_domain, > + data->addr_width); Why not: et = intel_pasid_setup_nested(iommu, dev, (pgd_t *)data->gpgd, data->hpasid, &data->vtd, dmar_domain, data->addr_width); ? > + for_each_svm_dev(sdev, svm, dev) { > + ret = 0; ... > + break; > + } Same only looks at the first device style. Why dos it only care about the first device? That needs at least a comment, and probably a first_svm_dev or so heper to make it explicit. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support 2020-05-14 5:59 ` Christoph Hellwig @ 2020-05-14 15:57 ` Jacob Pan -1 siblings, 0 replies; 11+ messages in thread From: Jacob Pan @ 2020-05-14 15:57 UTC (permalink / raw) To: Christoph Hellwig Cc: Lu Baolu, iommu, LKML, Joerg Roedel, David Woodhouse, Jean-Philippe Brucker, Eric Auger, Yi Liu, Tian, Kevin, Raj Ashok, Alex Williamson, Jonathan Cameron, jacob.jun.pan Hi Christoph, Thanks a lot for the reviews, comments below. Jacob On Wed, 13 May 2020 22:59:30 -0700 Christoph Hellwig <hch@infradead.org> wrote: > > + if (dev_is_pci(dev)) { > > + /* VT-d supports devices with full 20 bit PASIDs > > only */ > > + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) > > + return -EINVAL; > > + } else { > > + return -ENOTSUPP; > > + } > > This looks strange. Why not: > > if (!dev_is_pci(dev)) { > return -ENOTSUPP; > > /* VT-d supports devices with full 20 bit PASIDs only */ > if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) > return -EINVAL; > That is better, will do. > > + for_each_svm_dev(sdev, svm, dev) { > > + /* > > + * For devices with aux domains, we should > > allow multiple > > + * bind calls with the same PASID and pdev. > > + */ > > + if (iommu_dev_feature_enabled(dev, > > IOMMU_DEV_FEAT_AUX)) { > > + sdev->users++; > > + } else { > > + dev_warn_ratelimited(dev, "Already > > bound with PASID %u\n", > > + svm->pasid); > > + ret = -EBUSY; > > + } > > + goto out; > > Is this intentionally a for loop that jumps out of the loop after > the first device? > The name is confusing, it is not a loop. I will change it to find_svm_dev() and comments like this? /* * Find the matching device in a given SVM. The bind code ensures that * each device can only be added to the SVM list once. */ #define find_svm_dev(sdev, svm, d) \ list_for_each_entry((sdev), &(svm)->devs, list) \ if ((d) != (sdev)->dev) {} else > > + /* > > + * PASID table is per device for better security. > > Therefore, for > > + * each bind of a new device even with an existing PASID, > > we need to > > + * call the nested mode setup function here. > > + */ > > + spin_lock(&iommu->lock); > > + ret = intel_pasid_setup_nested(iommu, > > + dev, > > + (pgd_t *)data->gpgd, > > + data->hpasid, > > + &data->vtd, > > + dmar_domain, > > + data->addr_width); > > Why not: > > et = intel_pasid_setup_nested(iommu, dev, (pgd_t *)data->gpgd, > data->hpasid, &data->vtd, dmar_domain, > data->addr_width); > > ? > I thought we want to align the parentheses? Either way is fine. Baolu? > > + for_each_svm_dev(sdev, svm, dev) { > > + ret = 0; > > ... > > > + break; > > + } > > Same only looks at the first device style. Why dos it only care about > the first device? That needs at least a comment, and probably a > first_svm_dev or so heper to make it explicit. Yes, same as above. change to find_svm_dev() since there should be at most one matching device in the svm list. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support @ 2020-05-14 15:57 ` Jacob Pan 0 siblings, 0 replies; 11+ messages in thread From: Jacob Pan @ 2020-05-14 15:57 UTC (permalink / raw) To: Christoph Hellwig Cc: Tian, Kevin, Alex Williamson, Raj Ashok, David Woodhouse, LKML, iommu, Jean-Philippe Brucker, Jonathan Cameron Hi Christoph, Thanks a lot for the reviews, comments below. Jacob On Wed, 13 May 2020 22:59:30 -0700 Christoph Hellwig <hch@infradead.org> wrote: > > + if (dev_is_pci(dev)) { > > + /* VT-d supports devices with full 20 bit PASIDs > > only */ > > + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) > > + return -EINVAL; > > + } else { > > + return -ENOTSUPP; > > + } > > This looks strange. Why not: > > if (!dev_is_pci(dev)) { > return -ENOTSUPP; > > /* VT-d supports devices with full 20 bit PASIDs only */ > if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) > return -EINVAL; > That is better, will do. > > + for_each_svm_dev(sdev, svm, dev) { > > + /* > > + * For devices with aux domains, we should > > allow multiple > > + * bind calls with the same PASID and pdev. > > + */ > > + if (iommu_dev_feature_enabled(dev, > > IOMMU_DEV_FEAT_AUX)) { > > + sdev->users++; > > + } else { > > + dev_warn_ratelimited(dev, "Already > > bound with PASID %u\n", > > + svm->pasid); > > + ret = -EBUSY; > > + } > > + goto out; > > Is this intentionally a for loop that jumps out of the loop after > the first device? > The name is confusing, it is not a loop. I will change it to find_svm_dev() and comments like this? /* * Find the matching device in a given SVM. The bind code ensures that * each device can only be added to the SVM list once. */ #define find_svm_dev(sdev, svm, d) \ list_for_each_entry((sdev), &(svm)->devs, list) \ if ((d) != (sdev)->dev) {} else > > + /* > > + * PASID table is per device for better security. > > Therefore, for > > + * each bind of a new device even with an existing PASID, > > we need to > > + * call the nested mode setup function here. > > + */ > > + spin_lock(&iommu->lock); > > + ret = intel_pasid_setup_nested(iommu, > > + dev, > > + (pgd_t *)data->gpgd, > > + data->hpasid, > > + &data->vtd, > > + dmar_domain, > > + data->addr_width); > > Why not: > > et = intel_pasid_setup_nested(iommu, dev, (pgd_t *)data->gpgd, > data->hpasid, &data->vtd, dmar_domain, > data->addr_width); > > ? > I thought we want to align the parentheses? Either way is fine. Baolu? > > + for_each_svm_dev(sdev, svm, dev) { > > + ret = 0; > > ... > > > + break; > > + } > > Same only looks at the first device style. Why dos it only care about > the first device? That needs at least a comment, and probably a > first_svm_dev or so heper to make it explicit. Yes, same as above. change to find_svm_dev() since there should be at most one matching device in the svm list. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support 2020-05-14 15:57 ` Jacob Pan @ 2020-05-15 1:01 ` Lu Baolu -1 siblings, 0 replies; 11+ messages in thread From: Lu Baolu @ 2020-05-15 1:01 UTC (permalink / raw) To: Jacob Pan, Christoph Hellwig Cc: baolu.lu, iommu, LKML, Joerg Roedel, David Woodhouse, Jean-Philippe Brucker, Eric Auger, Yi Liu, Tian, Kevin, Raj Ashok, Alex Williamson, Jonathan Cameron Hi Jacob, On 5/14/20 11:57 PM, Jacob Pan wrote: >>> + /* >>> + * PASID table is per device for better security. >>> Therefore, for >>> + * each bind of a new device even with an existing PASID, >>> we need to >>> + * call the nested mode setup function here. >>> + */ >>> + spin_lock(&iommu->lock); >>> + ret = intel_pasid_setup_nested(iommu, >>> + dev, >>> + (pgd_t *)data->gpgd, >>> + data->hpasid, >>> + &data->vtd, >>> + dmar_domain, >>> + data->addr_width); >> Why not: >> >> et = intel_pasid_setup_nested(iommu, dev, (pgd_t *)data->gpgd, >> data->hpasid, &data->vtd, dmar_domain, >> data->addr_width); >> >> ? >> > I thought we want to align the parentheses? Either way is fine. > Baolu? > Let's keep the code style consistent in this file. Best regards, baolu ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support @ 2020-05-15 1:01 ` Lu Baolu 0 siblings, 0 replies; 11+ messages in thread From: Lu Baolu @ 2020-05-15 1:01 UTC (permalink / raw) To: Jacob Pan, Christoph Hellwig Cc: Tian, Kevin, Alex Williamson, Raj Ashok, David Woodhouse, LKML, iommu, Jean-Philippe Brucker, Jonathan Cameron Hi Jacob, On 5/14/20 11:57 PM, Jacob Pan wrote: >>> + /* >>> + * PASID table is per device for better security. >>> Therefore, for >>> + * each bind of a new device even with an existing PASID, >>> we need to >>> + * call the nested mode setup function here. >>> + */ >>> + spin_lock(&iommu->lock); >>> + ret = intel_pasid_setup_nested(iommu, >>> + dev, >>> + (pgd_t *)data->gpgd, >>> + data->hpasid, >>> + &data->vtd, >>> + dmar_domain, >>> + data->addr_width); >> Why not: >> >> et = intel_pasid_setup_nested(iommu, dev, (pgd_t *)data->gpgd, >> data->hpasid, &data->vtd, dmar_domain, >> data->addr_width); >> >> ? >> > I thought we want to align the parentheses? Either way is fine. > Baolu? > Let's keep the code style consistent in this file. Best regards, baolu _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support 2020-05-14 15:57 ` Jacob Pan @ 2020-05-16 6:02 ` Lu Baolu -1 siblings, 0 replies; 11+ messages in thread From: Lu Baolu @ 2020-05-16 6:02 UTC (permalink / raw) To: Jacob Pan, Christoph Hellwig Cc: baolu.lu, iommu, LKML, Joerg Roedel, David Woodhouse, Jean-Philippe Brucker, Eric Auger, Yi Liu, Tian, Kevin, Raj Ashok, Alex Williamson, Jonathan Cameron Hi Jacob, On 2020/5/14 23:57, Jacob Pan wrote: > Hi Christoph, > > Thanks a lot for the reviews, comments below. > > Jacob > > On Wed, 13 May 2020 22:59:30 -0700 > Christoph Hellwig<hch@infradead.org> wrote: > >>> + if (dev_is_pci(dev)) { >>> + /* VT-d supports devices with full 20 bit PASIDs >>> only */ >>> + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) >>> + return -EINVAL; >>> + } else { >>> + return -ENOTSUPP; >>> + } >> This looks strange. Why not: >> >> if (!dev_is_pci(dev)) { >> return -ENOTSUPP; >> >> /* VT-d supports devices with full 20 bit PASIDs only */ >> if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) >> return -EINVAL; >> > That is better, will do. > >>> + for_each_svm_dev(sdev, svm, dev) { >>> + /* >>> + * For devices with aux domains, we should >>> allow multiple >>> + * bind calls with the same PASID and pdev. >>> + */ >>> + if (iommu_dev_feature_enabled(dev, >>> IOMMU_DEV_FEAT_AUX)) { >>> + sdev->users++; >>> + } else { >>> + dev_warn_ratelimited(dev, "Already >>> bound with PASID %u\n", >>> + svm->pasid); >>> + ret = -EBUSY; >>> + } >>> + goto out; >> Is this intentionally a for loop that jumps out of the loop after >> the first device? >> > The name is confusing, it is not a loop. I will change it to > find_svm_dev() and comments like this? > > /* > * Find the matching device in a given SVM. The bind code ensures that > * each device can only be added to the SVM list once. > */ > #define find_svm_dev(sdev, svm, d) \ > list_for_each_entry((sdev), &(svm)->devs, list) \ > if ((d) != (sdev)->dev) {} else > The for_each_svm_dev() is not added by this series and is also used by other functions. How about changing it in a separated patch? Best regards, baolu ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support @ 2020-05-16 6:02 ` Lu Baolu 0 siblings, 0 replies; 11+ messages in thread From: Lu Baolu @ 2020-05-16 6:02 UTC (permalink / raw) To: Jacob Pan, Christoph Hellwig Cc: Tian, Kevin, Alex Williamson, Raj Ashok, David Woodhouse, LKML, iommu, Jean-Philippe Brucker, Jonathan Cameron Hi Jacob, On 2020/5/14 23:57, Jacob Pan wrote: > Hi Christoph, > > Thanks a lot for the reviews, comments below. > > Jacob > > On Wed, 13 May 2020 22:59:30 -0700 > Christoph Hellwig<hch@infradead.org> wrote: > >>> + if (dev_is_pci(dev)) { >>> + /* VT-d supports devices with full 20 bit PASIDs >>> only */ >>> + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) >>> + return -EINVAL; >>> + } else { >>> + return -ENOTSUPP; >>> + } >> This looks strange. Why not: >> >> if (!dev_is_pci(dev)) { >> return -ENOTSUPP; >> >> /* VT-d supports devices with full 20 bit PASIDs only */ >> if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) >> return -EINVAL; >> > That is better, will do. > >>> + for_each_svm_dev(sdev, svm, dev) { >>> + /* >>> + * For devices with aux domains, we should >>> allow multiple >>> + * bind calls with the same PASID and pdev. >>> + */ >>> + if (iommu_dev_feature_enabled(dev, >>> IOMMU_DEV_FEAT_AUX)) { >>> + sdev->users++; >>> + } else { >>> + dev_warn_ratelimited(dev, "Already >>> bound with PASID %u\n", >>> + svm->pasid); >>> + ret = -EBUSY; >>> + } >>> + goto out; >> Is this intentionally a for loop that jumps out of the loop after >> the first device? >> > The name is confusing, it is not a loop. I will change it to > find_svm_dev() and comments like this? > > /* > * Find the matching device in a given SVM. The bind code ensures that > * each device can only be added to the SVM list once. > */ > #define find_svm_dev(sdev, svm, d) \ > list_for_each_entry((sdev), &(svm)->devs, list) \ > if ((d) != (sdev)->dev) {} else > The for_each_svm_dev() is not added by this series and is also used by other functions. How about changing it in a separated patch? Best regards, baolu _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-16 6:02 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-16 4:59 [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support kbuild test robot -- strict thread matches above, loose matches on Subject: below -- 2020-05-13 23:01 [PATCH v13 0/8] Nested Shared Virtual Address (SVA) VT-d support Jacob Pan 2020-05-13 23:01 ` [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support Jacob Pan 2020-05-13 23:01 ` Jacob Pan 2020-05-14 5:59 ` Christoph Hellwig 2020-05-14 5:59 ` Christoph Hellwig 2020-05-14 15:57 ` Jacob Pan 2020-05-14 15:57 ` Jacob Pan 2020-05-15 1:01 ` Lu Baolu 2020-05-15 1:01 ` Lu Baolu 2020-05-16 6:02 ` Lu Baolu 2020-05-16 6:02 ` Lu Baolu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.