Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <79f0cab5-39ad-3d98-3896-6e1ba7b8db21@intel.com>
Date:   Mon, 16 Oct 2023 11:28:15 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [PATCH v4 01/17] iommu: Add hwpt_type with user_data for
 domain_alloc_user op
Content-Language: en-US
To:     Nicolin Chen <nicolinc@nvidia.com>,
        Jason Gunthorpe <jgg@nvidia.com>
CC:     <joro@8bytes.org>, <alex.williamson@redhat.com>,
        <kevin.tian@intel.com>, <robin.murphy@arm.com>,
        <baolu.lu@linux.intel.com>, <cohuck@redhat.com>,
        <eric.auger@redhat.com>, <kvm@vger.kernel.org>,
        <mjrosato@linux.ibm.com>, <chao.p.peng@linux.intel.com>,
        <yi.y.sun@linux.intel.com>, <peterx@redhat.com>,
        <jasowang@redhat.com>, <shameerali.kolothum.thodi@huawei.com>,
        <lulu@redhat.com>, <suravee.suthikulpanit@amd.com>,
        <iommu@lists.linux.dev>, <linux-kernel@vger.kernel.org>,
        <linux-kselftest@vger.kernel.org>, <zhenzhong.duan@intel.com>,
        <joao.m.martins@oracle.com>
References: <20230921075138.124099-1-yi.l.liu@intel.com>
 <20230921075138.124099-2-yi.l.liu@intel.com>
 <20231010165844.GQ3952@nvidia.com>
 <03ba68e9-33ef-35c1-5720-8f559f94f8a1@intel.com>
 <20231012133917.GL3952@nvidia.com>
 <3659d9a7-d9e9-bb73-daf5-41c765e99c8c@intel.com>
 <20231013140456.GR3952@nvidia.com> <ZSmE6unIukqJ3GKu@Asurada-Nvidia>
From:   Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <ZSmE6unIukqJ3GKu@Asurada-Nvidia>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RXRSTWFhTXVrYW53SThHSjBkdnJOd0xJUWNXUmpta0tic0R0ZUZNYUUwbTJ6?=
 =?utf-8?B?MGlOa0FrT0VMTjNaZ3phajhCV2JVR0VSekd5Zm9PNkZlbmpjNW91d1U2SUZn?=
 =?utf-8?B?aVNkN0ZPWGE1MG1Ud0ZkTDRVWjRDNFFJTXFOVFFRNnpKcEU3cHdQbzZCSmE1?=
 =?utf-8?B?bHlYeWoxcE11R2QrMFRKeUNaLzZaMVRUNmdXZHNGdHo0eGIwbjJhYXVXa3Rw?=
 =?utf-8?B?Qml1T2syOVdwRXFWd3NWK242OHJNSFRKWGdZVnJnZkVRZ0hhVHFEUDNuSkhy?=
 =?utf-8?B?VmFxOFNNd2JURWZuUUx0UCtEaSt3eG1RMWpsQ2hQcFZsR3ZpemVDQmRyT1Er?=
 =?utf-8?B?VVpoQW9PK3ZrYXRZOCthSzdWSTNTTUdLdTd2WDNsWisxYlpYQ0FueXJ2eWVk?=
 =?utf-8?B?ZWJKUlV6VW9SVGM2aW9FOWVsQzYrelZMamczWnJndkx0SjNtOEkvK2ZhZTNU?=
 =?utf-8?B?ak9odDdmd2JYOGNaSi91c083YmtEN2l4dVNyamphZUJORGRIMjlYdjdwM1Js?=
 =?utf-8?B?Zk9mdXFBVjh3T0diTTNWSktrMmFQZUxkQ04vQ3puN1VSU0FvRHlpOTFTLzVN?=
 =?utf-8?B?TVNjanhwd1FrbDd4OU84dTh6UWNmZTQ1bDhJTDV5ZVBXYUhDYzUzQW5QVkZz?=
 =?utf-8?B?bEJJajg1b1RFdlo0SXJRYVZWQTIwbVNDQVFrdlZ6bXBOWndBYmNSOVBsdk5k?=
 =?utf-8?B?RURrVE4yaXAwdTBKOXJLR2tOaTFEcVhNbGZ2SEU2V3kyOUl0RVBIMVQzS3o5?=
 =?utf-8?B?MDNLTkk2a1Q5WEhFSEhRd2J4aUR3VUxibVI2RGhSeW13ZG5nVXNKWXFZQVVZ?=
 =?utf-8?B?SmJnVkxQRzlpSlIyZncwZjRsVUJtQ0txVXFwT0ZiVjd5bDBpdHpabHhRVnpr?=
 =?utf-8?B?bjFNWTFCb0M5b1o0NVIxQUxQaDlMTjlTc0VWbnNhMCt4amVsMnVTVGFORHVu?=
 =?utf-8?B?Q3VnQ0FSUTFTMDhQaTVEM1lGS0dHL3IvL3NJYlpHZDZ0ZlVuaFlzQmdkZ3RS?=
 =?utf-8?B?NWlrNkhLdm11cEVMOW1RTHRma0pKZEJUNnVFTUZzejJTN0hGbmtnVGFqanM2?=
 =?utf-8?B?SThOQlFrYTZKY2YzWDVBRFU2ZjRLVjBvYUFvQnVnSW5iVzF3QTVoQnQvOWtF?=
 =?utf-8?B?VjNDdzk4SFAzczBxd3M3ZFBmRE5vTWxwK3NkSkY4dmY5ai9RNks3czBweWVj?=
 =?utf-8?B?T0FoQkY4RDRnZkdpMERYTXpQOFpUYVkwK2E1NG9kZVErVnF2SDZVTnhlYWM5?=
 =?utf-8?B?eVhiRDRZQTNJWFZjaWY0YzFBTXh3M0dZbmw0U1gvZ3NvemVycDdvOWhiV1Nk?=
 =?utf-8?B?Qi9DRmIxK21OV21wL3J5YkI3aThKbEI5WXg1bVN2TUZ1eHBPSWJ3QmRENUtF?=
 =?utf-8?B?YlN2YzJVL2tVczVTMDZKSzNVV1d6Yk1lWndibUxLcTNlR2RQa0FPOXk2bW8y?=
 =?utf-8?B?T014a1FveGROV0NjR1pNTWdDK1Vodk1lazh5WDB2dlVpSENSaW5rV1RQbGN6?=
 =?utf-8?B?aS9GZ0I2dFVBeEVJenl6azN4SU1XTG1Wa25tK1lmeWRhejUvWW1uRU5qQ2Uw?=
 =?utf-8?B?UlJyYXZhQklzVmFKK0dwNGloM3ZDTWg0NXF6TE54cTRRMjRPMEZ4SHNqTE0v?=
 =?utf-8?B?M0VpY2MrTFpUaG5ubVk4ZExOc2kxcTFVUFhBYU5TV3UxdHJUK3R6dU9JN0xr?=
 =?utf-8?B?Q2xvaEhRaVAzQk4veWU0Q3h2eXpvWHlHVGdaa21Fd1RlYVZ6SU1mYmxla2RS?=
 =?utf-8?B?MHppbm5VdlhwOUU3VWdnK3lScjhZcDlra3hZR0RpMU5KZTJablZ2RGdjUWtH?=
 =?utf-8?B?Y3czSU5KSU9aTXQxQ3JMR1ZyblNiNXNQK1N0ak1odFV6cThpS01OVlNDa0hz?=
 =?utf-8?B?K2VkU0xxWGQ4NmlQaW9kQThtQ3dLei9yd1hMeU9Sd3VQbUhBU2pOVG9FNU81?=
 =?utf-8?B?N0ZWTUQ2L3JzNk44YUFEQk9lQW0yNy8xc3drZnQwU3lZZVcvY2xyYmloVEtZ?=
 =?utf-8?B?czFMNVlNaGp6MktGVkZYZVBMU2pmekhaVHBsWnJ3ekhvRnIwUDhDYjVOa3ls?=
 =?utf-8?B?WG9sb0JCRWdzbytMblA5dzNhcnpDUUpweHdLWFI4MFA0cDZQSTVINnhHUVAw?=
 =?utf-8?Q?4XQbeg9UTBxVnCYik5gd29Yt1?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 610981e2-c975-4717-1c0c-08dbcdf79c8c
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Oct 2023 03:25:57.6001
 (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: jP3X435pYg6bQTtiGiO0wEbfIJ8pASd8eIY+4/fG3Lg7taS+FfaGVNBKs3LfyjW0NVwJX/M1Tr+H+DEDhoZGJg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6302
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Sun, 15 Oct 2023 20:26:36 -0700 (PDT)

On 2023/10/14 01:56, Nicolin Chen wrote:
> On Fri, Oct 13, 2023 at 11:04:56AM -0300, Jason Gunthorpe wrote:
>> On Fri, Oct 13, 2023 at 12:33:13PM +0800, Yi Liu wrote:
>>
>>> not really. Below the users of the struct iommu_user_data in my current
>>> iommufd_nesting branch. Only the domain_alloc_user op has type as there
>>> can be multiple vendor specific alloc data types. Basically, I'm ok to
>>> make the change you suggested, just not sure if it is good to add type
>>> as it is only needed by one path.
>>
>> I don't think we should ever have an opaque data blob without a type
>> tag..
> 
> I can add those "missing" data types, and then a driver will be
> responsible for sanitizing the type along with the data_len.
> 
> I notice that the enum iommu_hwpt_data_type in the posted patch
> is confined to the alloc_user uAPI. Perhaps we should share it
> with invalidate too:

invalidation path does not need a type field today as the data
type is vendor specific, vendor driver should know the data type
when calls in.

> 
> /**
>   * enum iommu_hwpt_data_type - IOMMU HWPT Data Type
>   * @IOMMU_HWPT_DATA_NONE: no data
>   * @IOMMU_HWPT_DATA_VTD_S1: Intel VT-d stage-1 page table
>   * @IOMMU_HWPT_DATA_ARM_SMMUV3: ARM SMMUv3 Context Descriptor Table
>   */
> enum iommu_hwpt_data_type {
> 	IOMMU_HWPT_DATA_NONE,
> 	IOMMU_HWPT_DATA_VTD_S1,
> 	IOMMU_HWPT_DATA_ARM_SMMUV3,
> };
> 
> Though inevitably we'd have to define a separate data group for
> things like set_dev_data that is related to idev v.s. hwpt:

yes, the type field is in separate data group per path.

> 
> // IOMMU_DEV_DATA_TYPE sounds like an IOMMU device, other than a
> // passthrough device, so renaming to "_IDEV_" here. And perhaps
> // "set_dev_data" could be "set_idev_data" too? Any better name?
> 
> /**
>   * enum iommu_idev_data_type - Data Type for a Device behind an IOMMU
>   * @IOMMU_IDEV_DATA_NONE: no data
>   * @IOMMU_IDEV_DATA_ARM_SMMUV3: ARM SMMUv3 specific device data
>   */
> enum iommu_idev_data_type {
> 	IOMMU_IDEV_DATA_NONE,
> 	IOMMU_IDEV_DATA_ARM_SMMUV3,
> };
> 
> /**
>   * struct iommu_idev_data_arm_smmuv3 - ARM SMMUv3 specific device data
>   * @sid: The Stream ID that is assigned in the user space
>   *
>   * The SMMUv3 specific user space data for a device that is behind an SMMU HW.
>   * The guest-level user data should be linked to the host-level kernel data,
>   * which will be used by user space cache invalidation commands.
>   */
> struct iommu_idev_data_arm_smmuv3 {
> 	__u32 sid;
> };
> 
> /**
>   * struct iommu_set_idev_data - ioctl(IOMMU_SET_IDEV_DATA)
>   * @size: sizeof(struct iommu_set_idev_data)
>   * @dev_id: The device to set an iommu specific device data
>   * @data_uptr: User pointer of the device user data
>   * @data_len: Length of the device user data
>   *
>   * The device data must be unset using ioctl(IOMMU_UNSET_IDEV_DATA), before
>   * another ioctl(IOMMU_SET_IDEV_DATA) call or before the device itself gets
>   * unbind'd from the iommufd context.
>   */
> struct iommu_set_idev_data {
> 	__u32 size;
> 	__u32 dev_id;
> 	__aligned_u64 data_uptr;
> 	__u32 data_len;
> };
> 
> Thanks
> Nic

-- 
Regards,
Yi Liu