diff mbox series

[02/10] kvm/vfio: Reduce the scope of PPC #ifdefs

Message ID 2-v1-33906a626da1+16b0-vfio_kvm_no_group_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Remove vfio_group from the struct file facing VFIO API | expand

Commit Message

Jason Gunthorpe April 14, 2022, 6:46 p.m. UTC 
Use IS_ENABLED and static inlines instead of just ifdef'ing away all the
PPC code. This allows it to be compile tested on all platforms and makes
it easier to maintain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 virt/kvm/vfio.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

I'm not a big fan if #ifdefs around #include, but lack a better idea - these
weird spapr things should not be part of the generic kvm arch API at least,
IMHO.

Comments

Christoph Hellwig April 15, 2022, 4:47 a.m. UTC | #1 
On Thu, Apr 14, 2022 at 03:46:01PM -0300, Jason Gunthorpe wrote:
> Use IS_ENABLED and static inlines instead of just ifdef'ing away all the
> PPC code. This allows it to be compile tested on all platforms and makes
> it easier to maintain.

That's even uglier than what we had.  I'd rather have a new vfio_ppc.c
to implement it, then you can stubs for it in the header (or IS_ENABLED
if you really want) but we don't need stubs for ppc-specific arch
functionality in a consumer of it.
Jason Gunthorpe April 15, 2022, 12:13 p.m. UTC | #2 
On Fri, Apr 15, 2022 at 06:47:31AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 14, 2022 at 03:46:01PM -0300, Jason Gunthorpe wrote:
> > Use IS_ENABLED and static inlines instead of just ifdef'ing away all the
> > PPC code. This allows it to be compile tested on all platforms and makes
> > it easier to maintain.
> 
> That's even uglier than what we had.  I'd rather have a new vfio_ppc.c
> to implement it, then you can stubs for it in the header (or IS_ENABLED
> if you really want) but we don't need stubs for ppc-specific arch
> functionality in a consumer of it.

That seems like a good approach, I will try it

Thanks,
Jason
Jason Gunthorpe April 15, 2022, 12:35 p.m. UTC | #3 
On Fri, Apr 15, 2022 at 09:13:43AM -0300, Jason Gunthorpe wrote:
> On Fri, Apr 15, 2022 at 06:47:31AM +0200, Christoph Hellwig wrote:
> > On Thu, Apr 14, 2022 at 03:46:01PM -0300, Jason Gunthorpe wrote:
> > > Use IS_ENABLED and static inlines instead of just ifdef'ing away all the
> > > PPC code. This allows it to be compile tested on all platforms and makes
> > > it easier to maintain.
> > 
> > That's even uglier than what we had.  I'd rather have a new vfio_ppc.c
> > to implement it, then you can stubs for it in the header (or IS_ENABLED
> > if you really want) but we don't need stubs for ppc-specific arch
> > functionality in a consumer of it.
> 
> That seems like a good approach, I will try it

Actually, it defeats the whole point of this patch. I wrote it so I
could compile test all this stuff on x86 - if I shift it into a
vfio_ppc.c and make some kconfig stuff then it still won't compile
test on x86.

At that point I'd rather leave the ifdefs as-is and drop this patch.

Yes, the #ifdef is ugly, but this whole PPC thing is ugly :\

Jason
Christoph Hellwig April 15, 2022, 2:36 p.m. UTC | #4 
On Fri, Apr 15, 2022 at 09:35:04AM -0300, Jason Gunthorpe wrote:
> Actually, it defeats the whole point of this patch. I wrote it so I
> could compile test all this stuff on x86 - if I shift it into a
> vfio_ppc.c and make some kconfig stuff then it still won't compile
> test on x86.
> 
> At that point I'd rather leave the ifdefs as-is and drop this patch.
> 
> Yes, the #ifdef is ugly, but this whole PPC thing is ugly :\

I'd rather leave it as is then.
diff mbox series

Patch

diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index a1167ab7a2246f..9b942f447df79d 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -19,6 +19,16 @@ 
 
 #ifdef CONFIG_SPAPR_TCE_IOMMU
 #include <asm/kvm_ppc.h>
+#else
+static long kvm_spapr_tce_attach_iommu_group(struct kvm *kvm, int tablefd,
+		struct iommu_group *grp)
+{
+	return -EINVAL;
+}
+static void kvm_spapr_tce_release_iommu_group(struct kvm *kvm,
+					      struct iommu_group *grp)
+{
+}
 #endif
 
 struct kvm_vfio_group {
@@ -106,7 +116,6 @@  static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group)
 	return ret > 0;
 }
 
-#ifdef CONFIG_SPAPR_TCE_IOMMU
 static int kvm_vfio_external_user_iommu_id(struct vfio_group *vfio_group)
 {
 	int (*fn)(struct vfio_group *);
@@ -137,15 +146,18 @@  static struct iommu_group *kvm_vfio_group_get_iommu_group(
 static void kvm_spapr_tce_release_vfio_group(struct kvm *kvm,
 		struct vfio_group *vfio_group)
 {
-	struct iommu_group *grp = kvm_vfio_group_get_iommu_group(vfio_group);
+	struct iommu_group *grp;
 
+	if (!IS_ENABLED(CONFIG_SPAPR_TCE_IOMMU))
+		return;
+
+	grp = kvm_vfio_group_get_iommu_group(vfio_group);
 	if (WARN_ON_ONCE(!grp))
 		return;
 
 	kvm_spapr_tce_release_iommu_group(kvm, grp);
 	iommu_group_put(grp);
 }
-#endif
 
 /*
  * Groups can use the same or different IOMMU domains.  If the same then
@@ -253,9 +265,7 @@  static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd)
 
 		list_del(&kvg->node);
 		kvm_arch_end_assignment(dev->kvm);
-#ifdef CONFIG_SPAPR_TCE_IOMMU
 		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg->vfio_group);
-#endif
 		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
 		kvm_vfio_group_put_external_user(kvg->vfio_group);
 		kfree(kvg);
@@ -272,7 +282,6 @@  static int kvm_vfio_group_del(struct kvm_device *dev, unsigned int fd)
 	return ret;
 }
 
-#ifdef CONFIG_SPAPR_TCE_IOMMU
 static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev,
 					void __user *arg)
 {
@@ -284,6 +293,9 @@  static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev,
 	struct iommu_group *grp;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_SPAPR_TCE_IOMMU))
+		return -ENXIO;
+
 	if (copy_from_user(&param, arg, sizeof(struct kvm_vfio_spapr_tce)))
 		return -EFAULT;
 
@@ -323,7 +335,6 @@  static int kvm_vfio_group_set_spapr_tce(struct kvm_device *dev,
 	kvm_vfio_group_put_external_user(vfio_group);
 	return ret;
 }
-#endif
 
 static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
 {
@@ -341,10 +352,8 @@  static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
 			return -EFAULT;
 		return kvm_vfio_group_del(dev, fd);
 
-#ifdef CONFIG_SPAPR_TCE_IOMMU
 	case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE:
 		return kvm_vfio_group_set_spapr_tce(dev, (void __user *)arg);
-#endif
 	}
 
 	return -ENXIO;
@@ -369,9 +378,10 @@  static int kvm_vfio_has_attr(struct kvm_device *dev,
 		switch (attr->attr) {
 		case KVM_DEV_VFIO_GROUP_ADD:
 		case KVM_DEV_VFIO_GROUP_DEL:
-#ifdef CONFIG_SPAPR_TCE_IOMMU
+			return 0;
 		case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE:
-#endif
+			if (!IS_ENABLED(CONFIG_SPAPR_TCE_IOMMU))
+				break;
 			return 0;
 		}
 
@@ -387,9 +397,7 @@  static void kvm_vfio_destroy(struct kvm_device *dev)
 	struct kvm_vfio_group *kvg, *tmp;
 
 	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
-#ifdef CONFIG_SPAPR_TCE_IOMMU
 		kvm_spapr_tce_release_vfio_group(dev->kvm, kvg->vfio_group);
-#endif
 		kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
 		kvm_vfio_group_put_external_user(kvg->vfio_group);
 		list_del(&kvg->node);