diff mbox series

[v12,20/24] vfio: Only check group->type for noiommu test

Message ID 20230602121653.80017-21-yi.l.liu@intel.com (mailing list archive)
State New, archived
Headers show
Series Add vfio_device cdev for iommufd support | expand

Commit Message

Yi Liu June 2, 2023, 12:16 p.m. UTC 
group->type can be VFIO_NO_IOMMU only when vfio_noiommu option is true.
And vfio_noiommu option can only be true if CONFIG_VFIO_NOIOMMU is enabled.
So checking group->type is enough when testing noiommu.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/vfio/group.c | 3 +--
 drivers/vfio/vfio.h  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

Comments

Alex Williamson June 12, 2023, 10:37 p.m. UTC | #1 
On Fri,  2 Jun 2023 05:16:49 -0700
Yi Liu <yi.l.liu@intel.com> wrote:

> group->type can be VFIO_NO_IOMMU only when vfio_noiommu option is true.
> And vfio_noiommu option can only be true if CONFIG_VFIO_NOIOMMU is enabled.
> So checking group->type is enough when testing noiommu.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/vfio/group.c | 3 +--
>  drivers/vfio/vfio.h  | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index 41a09a2df690..653b62f93474 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -133,8 +133,7 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
>  
>  	iommufd = iommufd_ctx_from_file(f.file);
>  	if (!IS_ERR(iommufd)) {
> -		if (IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> -		    group->type == VFIO_NO_IOMMU)
> +		if (group->type == VFIO_NO_IOMMU)
>  			ret = iommufd_vfio_compat_set_no_iommu(iommufd);
>  		else
>  			ret = iommufd_vfio_compat_ioas_create(iommufd);
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 5835c74e97ce..1b89e8bc8571 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -108,8 +108,7 @@ void vfio_group_cleanup(void);
>  
>  static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
>  {
> -	return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> -	       vdev->group->type == VFIO_NO_IOMMU;
> +	return vdev->group->type == VFIO_NO_IOMMU;
>  }
>  
>  #if IS_ENABLED(CONFIG_VFIO_CONTAINER)

This patch should be dropped.  It's logically correct, but ignores that
the config option can be determined at compile time and therefore the
code can be better optimized based on that test.  I think there was a
specific case where I questioned it, but this drops an otherwise valid
compiler optimization.  Thanks,

Alex
Yi Liu June 13, 2023, 9:20 a.m. UTC | #2 
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Tuesday, June 13, 2023 6:38 AM
> On Fri,  2 Jun 2023 05:16:49 -0700
> Yi Liu <yi.l.liu@intel.com> wrote:
> 
> > group->type can be VFIO_NO_IOMMU only when vfio_noiommu option is true.
> > And vfio_noiommu option can only be true if CONFIG_VFIO_NOIOMMU is enabled.
> > So checking group->type is enough when testing noiommu.
> >
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  drivers/vfio/group.c | 3 +--
> >  drivers/vfio/vfio.h  | 3 +--
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > index 41a09a2df690..653b62f93474 100644
> > --- a/drivers/vfio/group.c
> > +++ b/drivers/vfio/group.c
> > @@ -133,8 +133,7 @@ static int vfio_group_ioctl_set_container(struct vfio_group
> *group,
> >
> >  	iommufd = iommufd_ctx_from_file(f.file);
> >  	if (!IS_ERR(iommufd)) {
> > -		if (IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> > -		    group->type == VFIO_NO_IOMMU)
> > +		if (group->type == VFIO_NO_IOMMU)
> >  			ret = iommufd_vfio_compat_set_no_iommu(iommufd);
> >  		else
> >  			ret = iommufd_vfio_compat_ioas_create(iommufd);
> > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> > index 5835c74e97ce..1b89e8bc8571 100644
> > --- a/drivers/vfio/vfio.h
> > +++ b/drivers/vfio/vfio.h
> > @@ -108,8 +108,7 @@ void vfio_group_cleanup(void);
> >
> >  static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
> >  {
> > -	return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> > -	       vdev->group->type == VFIO_NO_IOMMU;
> > +	return vdev->group->type == VFIO_NO_IOMMU;
> >  }
> >
> >  #if IS_ENABLED(CONFIG_VFIO_CONTAINER)
> 
> This patch should be dropped.  It's logically correct, but ignores that
> the config option can be determined at compile time and therefore the
> code can be better optimized based on that test.  I think there was a
> specific case where I questioned it, but this drops an otherwise valid
> compiler optimization.  Thanks,

Yes. in v11, you mentioned the compiler optimization and the fact that
vfio_noiommu can only be valid when VFIO_NOIOMMU is enabled. I'm
ok to drop this patch to keep the compiler optimization.

Regards,
Yi Liu
diff mbox series

Patch

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index 41a09a2df690..653b62f93474 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -133,8 +133,7 @@  static int vfio_group_ioctl_set_container(struct vfio_group *group,
 
 	iommufd = iommufd_ctx_from_file(f.file);
 	if (!IS_ERR(iommufd)) {
-		if (IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
-		    group->type == VFIO_NO_IOMMU)
+		if (group->type == VFIO_NO_IOMMU)
 			ret = iommufd_vfio_compat_set_no_iommu(iommufd);
 		else
 			ret = iommufd_vfio_compat_ioas_create(iommufd);
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 5835c74e97ce..1b89e8bc8571 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -108,8 +108,7 @@  void vfio_group_cleanup(void);
 
 static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
 {
-	return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
-	       vdev->group->type == VFIO_NO_IOMMU;
+	return vdev->group->type == VFIO_NO_IOMMU;
 }
 
 #if IS_ENABLED(CONFIG_VFIO_CONTAINER)