From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     "Liu, Yi L" <yi.l.liu@intel.com>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>
CC:     "cohuck@redhat.com" <cohuck@redhat.com>,
        "eric.auger@redhat.com" <eric.auger@redhat.com>,
        "nicolinc@nvidia.com" <nicolinc@nvidia.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "mjrosato@linux.ibm.com" <mjrosato@linux.ibm.com>,
        "chao.p.peng@linux.intel.com" <chao.p.peng@linux.intel.com>,
        "yi.y.sun@linux.intel.com" <yi.y.sun@linux.intel.com>,
        "peterx@redhat.com" <peterx@redhat.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        "shameerali.kolothum.thodi@huawei.com" 
        <shameerali.kolothum.thodi@huawei.com>,
        "lulu@redhat.com" <lulu@redhat.com>,
        "suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>
Subject: RE: [PATCH v2 13/14] vfio: Add ioctls for device cdev using iommufd
Thread-Topic: [PATCH v2 13/14] vfio: Add ioctls for device cdev using iommufd
Thread-Index: AQHZOgpFuq+WgSot0kSqb57VFjABHq7DKMDw
Date:   Tue, 7 Feb 2023 09:17:29 +0000
Message-ID: <BN9PR11MB5276A5DEB3855788C02A343B8CDB9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20230206090532.95598-1-yi.l.liu@intel.com>
 <20230206090532.95598-14-yi.l.liu@intel.com>
In-Reply-To: <20230206090532.95598-14-yi.l.liu@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SgfzbZ5hmHk4T7FvHIKKCXRuiegDx9zRXLz293TxN8M9+t6v5VRomXue8edL?=
 =?us-ascii?Q?PeTECFAuG2M/VAwXTBDBKlbLWpba3qGJDxmDkJEZcc7MT3y+IBovQAgfNwSf?=
 =?us-ascii?Q?NSmyInUhJH3/T3qo4hBuwyeavBXX6/KG+b4LBSRRwYIkKt0k29hLFinfYyiG?=
 =?us-ascii?Q?xw55D3PABioFqkmhYV/0OlxsDBnROfRGrRb+aXtCarhxkdb4Inh5vQN/K6pn?=
 =?us-ascii?Q?8U75wTRw2jLDBpGFiB0ksldbT8dagwTV2UEF3zmmk4DKNCBJOCtjJrlxqhQB?=
 =?us-ascii?Q?2COIjtP70ucEc/RpPMYsfTEfctBfdFkh59d5iB7XuaePyPC2GKZJ5/4YKXlD?=
 =?us-ascii?Q?4MvVm9pksJUnhAaA1TXPU2sr25Xzl1WS7ICBJu5nB2aPaAwD9sDvN7CvzFzP?=
 =?us-ascii?Q?z3p8z2vFdHDOxp/NR8o64OjMQuun/dkSdoeOjCzq7OfpIi4JTDFWBBU8+0On?=
 =?us-ascii?Q?rp7V7TwiCd9sbWq8pSerzXsBvC6HXiHFqDlBD1ThOXV9DR0wygM+GgnurXKR?=
 =?us-ascii?Q?KpQw4JmMvh1UNBja52oMlJcffshZCa0MmI5FBRu8FDAjHGJfqlWuc+RDwAlM?=
 =?us-ascii?Q?D7hb+HAQZ48/PNMaUNpd1xWq5V1XyNmBxQ+j2WDZ1C6yRTt/LZIIXbsc6XIh?=
 =?us-ascii?Q?AaW4ByQP/sqXbI//f1jaAVODESVfEWUV06TCkXbVG6SGmbDrS8dN0YV7hGTt?=
 =?us-ascii?Q?ZWGgeSBfsq4gfo7TbTCpQVCgwmxWjHg28JVSSJSyipeTUARTQyBCsaZjWmYu?=
 =?us-ascii?Q?2sUtGQ8DVuj9Vnm2OAMoKnq3kGLz/qfnoL30FovQaJK1+4A4Ge6meVoFqEqW?=
 =?us-ascii?Q?Tymo36EZ2cOoDLESVXK6dQ2/IErMJ83st7MOcroHUh/Oi2UC9eZMh1Jzf/HG?=
 =?us-ascii?Q?fb6m9TIRTJ8zM4BK5+oZqhlrR46j32SvnoOxBJUwlcgPRoGUP+bREVI9yt/q?=
 =?us-ascii?Q?DZ+pPg60G9UB0cy7OT4LVSOnv9ru1GIasCpLs0yzCAHCCmA/9Fq5A+60QLgK?=
 =?us-ascii?Q?lFy8AqbMYGyeOjbNqIdH2L7jRM1sOj85C7lqTF5N2s/ZIE7jik7Tu5AJu3gG?=
 =?us-ascii?Q?KkMlNuwGSXFzeaoOJEihiJK8CB63yYnm2pylJA3ZlJifUYldF3yemlUZZtmp?=
 =?us-ascii?Q?L9S6ACC0J4du7CPtoNXgCCw1Eg3iHcEBx1ygQe86wugHCn4CqhEL5dz5i6zM?=
 =?us-ascii?Q?50tSdqoexrLLILBZpYs8N7l/gh+rgPj3nCSuU6mClqOUA8D3Tm0X/AfxcWBg?=
 =?us-ascii?Q?Hm6Gyza+mJ6ieYQNwV9BKRtLfoaKcwLMl28hJ7gC2CZXHGCCHeVTLF6lPIt4?=
 =?us-ascii?Q?6k6BmXnIcBbae5ZbTgDCajat4re5J523rC18x17Y4HCozkUq082cDTlxYydB?=
 =?us-ascii?Q?hDL0DFFvVQxtKjfLCuiWHB05vJpEZzTHjZPH4mBdMCjjV7VdR4zelXf5cVA6?=
 =?us-ascii?Q?FzsXpQQ5ZPj1g2xJGSGhaYJd8hbLAwEK5EJ8zerLT40wPA/RsjJigLQDhmT8?=
 =?us-ascii?Q?+fR0fqM6ut+5eKwDnP+IEvWxPX1K/hPI8sUYyC4MAkkmi2w5eJf+uDVMVhhv?=
 =?us-ascii?Q?l2NgE0iyMzA2rp0hjjCX0vdPk/8pyc+Lg7/PQWeX?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6f19acee-a951-4c1d-d233-08db08ec22c6
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Feb 2023 09:17:29.1660
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: K5Htlcb2JP34ovNoNcs34cE81WlKqkcwzJMWow+EohCkezM1O9yLDF/NDmwEjOmp0Z1GLDMkMYVPYXl1AsjqnA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6874
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, February 6, 2023 5:06 PM
>=20
> +long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
> +				    unsigned long arg)
> +{
> +	struct vfio_device *device =3D df->device;
> +	struct vfio_device_bind_iommufd bind;
> +	struct iommufd_ctx *iommufd =3D NULL;
> +	unsigned long minsz;
> +	struct fd f;
> +	int ret;
> +
> +	minsz =3D offsetofend(struct vfio_device_bind_iommufd, iommufd);

shouldn't the minsz include out_devid?

> +	/*
> +	 * Before the first device open, get the KVM pointer currently
> +	 * associated with the device file (if there is) and obtain a
> +	 * reference. This reference is held until device closed. Save
> +	 * the pointer in the device for use by drivers.
> +	 */
> +	vfio_device_get_kvm_safe(df);

there is no multi-open for cdev so "before the first device open"
doesn't make sense here.

> +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> +			     void __user *arg)
> +{
> +	struct vfio_device *device =3D df->device;
> +	struct vfio_device_attach_iommufd_pt attach;
> +	int ret;
> +
> +	if (copy_from_user(&attach, (void __user *)arg, sizeof(attach)))
> +		return -EFAULT;

lack of a minsz check

> +
> +	if (attach.flags || attach.pt_id =3D=3D IOMMUFD_INVALID_ID)
> +		return -EINVAL;
> +
> +	if (!device->ops->bind_iommufd)
> +		return -ENODEV;
> +
> +	mutex_lock(&device->dev_set->lock);
> +	if (df->noiommu)
> +		return -ENODEV;

-EINVAL

> +
> +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> +			     void __user *arg)
> +{
> +	struct vfio_device *device =3D df->device;
> +	struct vfio_device_detach_iommufd_pt detach;
> +
> +	if (copy_from_user(&detach, (void __user *)arg, sizeof(detach)))
> +		return -EFAULT;

lack of minsz check

> +	mutex_lock(&device->dev_set->lock);
> +	if (df->noiommu)
> +		return -ENODEV;

-EINVAL

> @@ -442,16 +443,41 @@ static int vfio_device_first_open(struct
> vfio_device_file *df,
>  {
>  	struct vfio_device *device =3D df->device;
>  	struct iommufd_ctx *iommufd =3D df->iommufd;
> -	int ret;
> +	int ret =3D 0;
>=20
>  	lockdep_assert_held(&device->dev_set->lock);
>=20
> +	/* df->iommufd and df->noiommu should be exclusive */
> +	if (WARN_ON(iommufd && df->noiommu))

pointless comment

> +		return -EINVAL;
> +
>  	if (!try_module_get(device->dev->driver->owner))
>  		return -ENODEV;
>=20
> +	/*
> +	 * For group path, iommufd pointer is NULL when comes into this

"For group/container path"

> +	 * helper. Its noiommu support is in container.c.
> +	 *
> +	 * For iommufd compat mode, iommufd pointer here is a valid value.
> +	 * Its noiommu support is supposed to be in vfio_iommufd_bind().

remove "supposed to be"

> +	 *
> +	 * For device cdev path, iommufd pointer here is a valid value for
> +	 * normal cases, but it is NULL if it's noiommu. The reason is
> +	 * that userspace uses iommufd=3D=3D-1 to indicate noiommu mode in
> this

"iommufd < 0"

> +	 * path. So caller of this helper will pass in a NULL iommufd

I don't think that is the reason. Instead the caller is required to pass
NULL iommufd pointer to indicate noiommu. Just remove the reason part.

> +	 * pointer. To differentiate it from the group path which also
> +	 * passes NULL iommufd pointer in, df->noiommu is used. For cdev
> +	 * noiommu, df->noiommu would be set to mark noiommu case for
> cdev
> +	 * path.

"To differentiate ..., check df->noiommu which is set only in the cdev path=
"

> +	 *
> +	 * So if df->noiommu is set then this helper just goes ahead to
> +	 * open device. If not, it depends on if iommufd pointer is NULL
> +	 * to handle the group path, iommufd compat mode, normal cases in
> +	 * the cdev path.

this doesn't match the code order. Probably can be removed after earlier
explanation.

> + * @argsz:	 user filled size of this data.
> + * @flags:	 reserved for future extension.
> + * @dev_cookie:	 a per device cookie provided by userspace.
> + * @iommufd:	 iommufd to bind. iommufd < 0 means noiommu.

s/iommufd < 0/a negative value/