| Message ID | 20210223210625.604517-4-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMUv3 Nested Stage Setup (VFIO part) | expand |
Hi Eric,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.11]
[cannot apply to vfio/next next-20210223]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Eric-Auger/SMMUv3-Nested-Stage-Setup-VFIO-part/20210224-051641
base: f40ddce88593482919761f74910f42f4b84c004b
config: arm64-randconfig-r003-20210223 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/ec823a68d862693dc787422f168409996f43b10a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Eric-Auger/SMMUv3-Nested-Stage-Setup-VFIO-part/20210224-051641
git checkout ec823a68d862693dc787422f168409996f43b10a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/vfio/vfio_iommu_type1.c:36:
In file included from include/linux/vfio.h:16:
include/uapi/linux/vfio.h:1198:34: error: field has incomplete type 'struct iommu_pasid_table_config'
struct iommu_pasid_table_config config; /* used on SET */
^
include/uapi/linux/vfio.h:1198:9: note: forward declaration of 'struct iommu_pasid_table_config'
struct iommu_pasid_table_config config; /* used on SET */
^
drivers/vfio/vfio_iommu_type1.c:2625:3: error: implicit declaration of function 'iommu_detach_pasid_table' [-Werror,-Wimplicit-function-declaration]
iommu_detach_pasid_table(d->domain);
^
drivers/vfio/vfio_iommu_type1.c:2625:3: note: did you mean 'vfio_detach_pasid_table'?
drivers/vfio/vfio_iommu_type1.c:2619:1: note: 'vfio_detach_pasid_table' declared here
vfio_detach_pasid_table(struct vfio_iommu *iommu)
^
drivers/vfio/vfio_iommu_type1.c:2639:9: error: implicit declaration of function 'iommu_uapi_attach_pasid_table' [-Werror,-Wimplicit-function-declaration]
ret = iommu_uapi_attach_pasid_table(d->domain, (void __user *)arg);
^
drivers/vfio/vfio_iommu_type1.c:2642:5: error: implicit declaration of function 'iommu_detach_pasid_table' [-Werror,-Wimplicit-function-declaration]
iommu_detach_pasid_table(d->domain);
^
>> drivers/vfio/vfio_iommu_type1.c:2668:9: error: implicit declaration of function 'iommu_bind_guest_msi' [-Werror,-Wimplicit-function-declaration]
ret = iommu_bind_guest_msi(d->domain, giova, gpa, size);
^
>> drivers/vfio/vfio_iommu_type1.c:2675:3: error: implicit declaration of function 'iommu_unbind_guest_msi' [-Werror,-Wimplicit-function-declaration]
iommu_unbind_guest_msi(d->domain, giova);
^
drivers/vfio/vfio_iommu_type1.c:2689:3: error: implicit declaration of function 'iommu_unbind_guest_msi' [-Werror,-Wimplicit-function-declaration]
iommu_unbind_guest_msi(d->domain, giova);
^
7 errors generated.
vim +/iommu_bind_guest_msi +2668 drivers/vfio/vfio_iommu_type1.c
2617
2618 static void
> 2619 vfio_detach_pasid_table(struct vfio_iommu *iommu)
2620 {
2621 struct vfio_domain *d;
2622
2623 mutex_lock(&iommu->lock);
2624 list_for_each_entry(d, &iommu->domain_list, next)
2625 iommu_detach_pasid_table(d->domain);
2626
2627 mutex_unlock(&iommu->lock);
2628 }
2629
2630 static int
2631 vfio_attach_pasid_table(struct vfio_iommu *iommu, unsigned long arg)
2632 {
2633 struct vfio_domain *d;
2634 int ret = 0;
2635
2636 mutex_lock(&iommu->lock);
2637
2638 list_for_each_entry(d, &iommu->domain_list, next) {
> 2639 ret = iommu_uapi_attach_pasid_table(d->domain, (void __user *)arg);
2640 if (ret) {
2641 list_for_each_entry_continue_reverse(d, &iommu->domain_list, next)
2642 iommu_detach_pasid_table(d->domain);
2643 break;
2644 }
2645 }
2646
2647 mutex_unlock(&iommu->lock);
2648 return ret;
2649 }
2650 static int vfio_cache_inv_fn(struct device *dev, void *data)
2651 {
2652 struct domain_capsule *dc = (struct domain_capsule *)data;
2653 unsigned long arg = *(unsigned long *)dc->data;
2654
2655 return iommu_uapi_cache_invalidate(dc->domain, dev, (void __user *)arg);
2656 }
2657
2658 static int
2659 vfio_bind_msi(struct vfio_iommu *iommu,
2660 dma_addr_t giova, phys_addr_t gpa, size_t size)
2661 {
2662 struct vfio_domain *d;
2663 int ret = 0;
2664
2665 mutex_lock(&iommu->lock);
2666
2667 list_for_each_entry(d, &iommu->domain_list, next) {
> 2668 ret = iommu_bind_guest_msi(d->domain, giova, gpa, size);
2669 if (ret)
2670 goto unwind;
2671 }
2672 goto unlock;
2673 unwind:
2674 list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) {
> 2675 iommu_unbind_guest_msi(d->domain, giova);
2676 }
2677 unlock:
2678 mutex_unlock(&iommu->lock);
2679 return ret;
2680 }
2681
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi, On Tue, Feb 23, 2021 at 10:06:15PM +0100, Eric Auger wrote: > This patch adds the VFIO_IOMMU_SET_MSI_BINDING ioctl which aim > to (un)register the guest MSI binding to the host. This latter > then can use those stage 1 bindings to build a nested stage > binding targeting the physical MSIs. Now that RMR is in the IORT spec, could it be used for the nested MSI problem? For virtio-iommu tables I was planning to do it like this: MSI is mapped at stage-2 with an arbitrary IPA->doorbell PA. We report this IPA to userspace through iommu_groups/X/reserved_regions. No change there. Then to the guest we report a reserved identity mapping at IPA (using RMR, an equivalent DT binding, or probed RESV_MEM for virtio-iommu). The guest creates that mapping at stage-1, and that's it. Unless I overlooked something we'd only reuse existing infrastructure and avoid the SET_MSI_BINDING interface. Thanks, Jean
Hi Jean, On 3/5/21 11:45 AM, Jean-Philippe Brucker wrote: > Hi, > > On Tue, Feb 23, 2021 at 10:06:15PM +0100, Eric Auger wrote: >> This patch adds the VFIO_IOMMU_SET_MSI_BINDING ioctl which aim >> to (un)register the guest MSI binding to the host. This latter >> then can use those stage 1 bindings to build a nested stage >> binding targeting the physical MSIs. > > Now that RMR is in the IORT spec, could it be used for the nested MSI > problem? For virtio-iommu tables I was planning to do it like this: > > MSI is mapped at stage-2 with an arbitrary IPA->doorbell PA. We report > this IPA to userspace through iommu_groups/X/reserved_regions. No change > there. Then to the guest we report a reserved identity mapping at IPA > (using RMR, an equivalent DT binding, or probed RESV_MEM for > virtio-iommu). Is there any DT binding equivalent? The guest creates that mapping at stage-1, and that's it. > Unless I overlooked something we'd only reuse existing infrastructure and > avoid the SET_MSI_BINDING interface. Yes at first glance I think this should work. The guest SMMU driver will continue allocating IOVA for MSIs but I think that's not an issue as they won't be used. For the SMMU case this makes the guest behavior different from the baremetal one though. Typically you will never get any S1 fault. Also the S1 mapping is static and direct. I will prototype this too. Thanks Eric > > Thanks, > Jean >
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index b4057ce809b0..0e6af4fe8c3d 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -2655,6 +2655,41 @@ static int vfio_cache_inv_fn(struct device *dev, void *data) return iommu_uapi_cache_invalidate(dc->domain, dev, (void __user *)arg); } +static int +vfio_bind_msi(struct vfio_iommu *iommu, + dma_addr_t giova, phys_addr_t gpa, size_t size) +{ + struct vfio_domain *d; + int ret = 0; + + mutex_lock(&iommu->lock); + + list_for_each_entry(d, &iommu->domain_list, next) { + ret = iommu_bind_guest_msi(d->domain, giova, gpa, size); + if (ret) + goto unwind; + } + goto unlock; +unwind: + list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) { + iommu_unbind_guest_msi(d->domain, giova); + } +unlock: + mutex_unlock(&iommu->lock); + return ret; +} + +static void +vfio_unbind_msi(struct vfio_iommu *iommu, dma_addr_t giova) +{ + struct vfio_domain *d; + + mutex_lock(&iommu->lock); + list_for_each_entry(d, &iommu->domain_list, next) + iommu_unbind_guest_msi(d->domain, giova); + mutex_unlock(&iommu->lock); +} + static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu, struct vfio_info_cap *caps) { @@ -2859,6 +2894,31 @@ static int vfio_iommu_type1_cache_invalidate(struct vfio_iommu *iommu, return ret; } +static int vfio_iommu_type1_set_msi_binding(struct vfio_iommu *iommu, + unsigned long arg) +{ + struct vfio_iommu_type1_set_msi_binding msi_binding; + unsigned long minsz; + + minsz = offsetofend(struct vfio_iommu_type1_set_msi_binding, + size); + + if (copy_from_user(&msi_binding, (void __user *)arg, minsz)) + return -EFAULT; + + if (msi_binding.argsz < minsz) + return -EINVAL; + + if (msi_binding.flags == VFIO_IOMMU_UNBIND_MSI) { + vfio_unbind_msi(iommu, msi_binding.iova); + return 0; + } else if (msi_binding.flags == VFIO_IOMMU_BIND_MSI) { + return vfio_bind_msi(iommu, msi_binding.iova, + msi_binding.gpa, msi_binding.size); + } + return -EINVAL; +} + static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu, unsigned long arg) { @@ -2983,6 +3043,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data, return vfio_iommu_type1_set_pasid_table(iommu, arg); case VFIO_IOMMU_CACHE_INVALIDATE: return vfio_iommu_type1_cache_invalidate(iommu, arg); + case VFIO_IOMMU_SET_MSI_BINDING: + return vfio_iommu_type1_set_msi_binding(iommu, arg); default: return -ENOTTY; } diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index ee6747ff8006..4c82e8f21618 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -1213,6 +1213,26 @@ struct vfio_iommu_type1_cache_invalidate { }; #define VFIO_IOMMU_CACHE_INVALIDATE _IO(VFIO_TYPE, VFIO_BASE + 19) +/** + * VFIO_IOMMU_SET_MSI_BINDING - _IOWR(VFIO_TYPE, VFIO_BASE + 20, + * struct vfio_iommu_type1_set_msi_binding) + * + * Pass a stage 1 MSI doorbell mapping to the host so that this + * latter can build a nested stage2 mapping. Or conversely tear + * down a previously bound stage 1 MSI binding. + */ +struct vfio_iommu_type1_set_msi_binding { + __u32 argsz; + __u32 flags; +#define VFIO_IOMMU_BIND_MSI (1 << 0) +#define VFIO_IOMMU_UNBIND_MSI (1 << 1) + __u64 iova; /* MSI guest IOVA */ + /* Fields below are used on BIND */ + __u64 gpa; /* MSI guest physical address */ + __u64 size; /* size of stage1 mapping (bytes) */ +}; +#define VFIO_IOMMU_SET_MSI_BINDING _IO(VFIO_TYPE, VFIO_BASE + 20) + /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */ /*
This patch adds the VFIO_IOMMU_SET_MSI_BINDING ioctl which aim to (un)register the guest MSI binding to the host. This latter then can use those stage 1 bindings to build a nested stage binding targeting the physical MSIs. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v11 -> v12: - Share VFIO_BASE + 20 with VFIO_IOMMU_SPAPR_TCE_REMOVE - rework returned values v10 -> v11: - renamed ustruct into msi_binding - return 0 on unbind v8 -> v9: - merge VFIO_IOMMU_BIND_MSI/VFIO_IOMMU_UNBIND_MSI into a single VFIO_IOMMU_SET_MSI_BINDING ioctl - ioctl id changed v6 -> v7: - removed the dev arg v3 -> v4: - add UNBIND - unwind on BIND error v2 -> v3: - adapt to new proto of bind_guest_msi - directly use vfio_iommu_for_each_dev v1 -> v2: - s/vfio_iommu_type1_guest_msi_binding/vfio_iommu_type1_bind_guest_msi --- drivers/vfio/vfio_iommu_type1.c | 62 +++++++++++++++++++++++++++++++++ include/uapi/linux/vfio.h | 20 +++++++++++ 2 files changed, 82 insertions(+)