From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 428B6C433DB for ; Mon, 22 Feb 2021 12:55:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E64AE64E41 for ; Mon, 22 Feb 2021 12:55:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231157AbhBVMzE (ORCPT ); Mon, 22 Feb 2021 07:55:04 -0500 Received: from szxga05-in.huawei.com ([45.249.212.191]:12563 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230258AbhBVMVK (ORCPT ); Mon, 22 Feb 2021 07:21:10 -0500 Received: from DGGEMS403-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4Dkh6W2gbCzMcZG; Mon, 22 Feb 2021 20:18:27 +0800 (CST) Received: from [10.174.184.42] (10.174.184.42) by DGGEMS403-HUB.china.huawei.com (10.3.19.203) with Microsoft SMTP Server id 14.3.498.0; Mon, 22 Feb 2021 20:20:24 +0800 Subject: Re: [PATCH v11 01/13] vfio: VFIO_IOMMU_SET_PASID_TABLE To: Auger Eric , , , , , , , , , , References: <20201116110030.32335-1-eric.auger@redhat.com> <20201116110030.32335-2-eric.auger@redhat.com> <84a111da-1969-1701-9a6d-cae8d7c285c6@huawei.com> CC: , , , , , From: Keqian Zhu Message-ID: Date: Mon, 22 Feb 2021 20:20:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.184.42] X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Eric, On 2021/2/22 18:53, Auger Eric wrote: > Hi Keqian, > > On 2/2/21 1:34 PM, Keqian Zhu wrote: >> Hi Eric, >> >> On 2020/11/16 19:00, Eric Auger wrote: >>> From: "Liu, Yi L" >>> >>> This patch adds an VFIO_IOMMU_SET_PASID_TABLE ioctl >>> which aims to pass the virtual iommu guest configuration >>> to the host. This latter takes the form of the so-called >>> PASID table. >>> >>> Signed-off-by: Jacob Pan >>> Signed-off-by: Liu, Yi L >>> Signed-off-by: Eric Auger >>> >>> --- >>> v11 -> v12: >>> - use iommu_uapi_set_pasid_table >>> - check SET and UNSET are not set simultaneously (Zenghui) >>> >>> v8 -> v9: >>> - Merge VFIO_IOMMU_ATTACH/DETACH_PASID_TABLE into a single >>> VFIO_IOMMU_SET_PASID_TABLE ioctl. >>> >>> v6 -> v7: >>> - add a comment related to VFIO_IOMMU_DETACH_PASID_TABLE >>> >>> v3 -> v4: >>> - restore ATTACH/DETACH >>> - add unwind on failure >>> >>> v2 -> v3: >>> - s/BIND_PASID_TABLE/SET_PASID_TABLE >>> >>> v1 -> v2: >>> - s/BIND_GUEST_STAGE/BIND_PASID_TABLE >>> - remove the struct device arg >>> --- >>> drivers/vfio/vfio_iommu_type1.c | 65 +++++++++++++++++++++++++++++++++ >>> include/uapi/linux/vfio.h | 19 ++++++++++ >>> 2 files changed, 84 insertions(+) >>> >>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >>> index 67e827638995..87ddd9e882dc 100644 >>> --- a/drivers/vfio/vfio_iommu_type1.c >>> +++ b/drivers/vfio/vfio_iommu_type1.c >>> @@ -2587,6 +2587,41 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu, >>> return ret; >>> } >>> >>> +static void >>> +vfio_detach_pasid_table(struct vfio_iommu *iommu) >>> +{ >>> + struct vfio_domain *d; >>> + >>> + mutex_lock(&iommu->lock); >>> + list_for_each_entry(d, &iommu->domain_list, next) >>> + iommu_detach_pasid_table(d->domain); >>> + >>> + mutex_unlock(&iommu->lock); >>> +} >>> + >>> +static int >>> +vfio_attach_pasid_table(struct vfio_iommu *iommu, unsigned long arg) >>> +{ >>> + struct vfio_domain *d; >>> + int ret = 0; >>> + >>> + mutex_lock(&iommu->lock); >>> + >>> + list_for_each_entry(d, &iommu->domain_list, next) { >>> + ret = iommu_uapi_attach_pasid_table(d->domain, (void __user *)arg); >> This design is not very clear to me. This assumes all iommu_domains share the same pasid table. >> >> As I understand, it's reasonable when there is only one group in the domain, and only one domain in the vfio_iommu. >> If more than one group in the vfio_iommu, the guest may put them into different guest iommu_domain, then they have different pasid table. >> >> Is this the use scenario? > > the vfio_iommu is attached to a container. all the groups within a > container share the same set of page tables (linux > Documentation/driver-api/vfio.rst). So to me if you want to use > different pasid tables, the groups need to be attached to different > containers. Does that make sense to you? OK, so this is what I understand about the design. A little question is that when we perform attach_pasid_table on a container, maybe we ought to do a sanity check to make sure that only one group is in this container, instead of iterating all domain? To be frank, my main concern is that if we put each group into different container under nested mode, then we give up the possibility that they can share stage2 page tables, which saves host memory and reduces the time of preparing environment for VM. To me, I'd like to understand the "container shares page table" to be: 1) share stage2 page table under nested mode. 2) share stage1 page table under non-nested mode. As when we perform "map" on a container: 1) under nested mode, we setup stage2 mapping. 2) under non-nested mode, we setup stage1 mapping. Indeed, to realize stage2 mapping sharing, we should do much more work to refactor SMMU_DOMAIN... Hope you can consider this. :) Thanks, Keqian > > Thanks > > Eric >> >> Thanks, >> Keqian >> >>> + if (ret) >>> + goto unwind; >>> + } >>> + goto unlock; >>> +unwind: >>> + list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) { >>> + iommu_detach_pasid_table(d->domain); >>> + } >>> +unlock: >>> + mutex_unlock(&iommu->lock); >>> + return ret; >>> +} >>> + >>> static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu, >>> struct vfio_info_cap *caps) >>> { >>> @@ -2747,6 +2782,34 @@ static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu, >>> -EFAULT : 0; >>> } >>> >>> +static int vfio_iommu_type1_set_pasid_table(struct vfio_iommu *iommu, >>> + unsigned long arg) >>> +{ >>> + struct vfio_iommu_type1_set_pasid_table spt; >>> + unsigned long minsz; >>> + int ret = -EINVAL; >>> + >>> + minsz = offsetofend(struct vfio_iommu_type1_set_pasid_table, flags); >>> + >>> + if (copy_from_user(&spt, (void __user *)arg, minsz)) >>> + return -EFAULT; >>> + >>> + if (spt.argsz < minsz) >>> + return -EINVAL; >>> + >>> + if (spt.flags & VFIO_PASID_TABLE_FLAG_SET && >>> + spt.flags & VFIO_PASID_TABLE_FLAG_UNSET) >>> + return -EINVAL; >>> + >>> + if (spt.flags & VFIO_PASID_TABLE_FLAG_SET) >>> + ret = vfio_attach_pasid_table(iommu, arg + minsz); >>> + else if (spt.flags & VFIO_PASID_TABLE_FLAG_UNSET) { >>> + vfio_detach_pasid_table(iommu); >>> + ret = 0; >>> + } >>> + return ret; >>> +} >>> + >>> static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu, >>> unsigned long arg) >>> { >>> @@ -2867,6 +2930,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data, >>> return vfio_iommu_type1_unmap_dma(iommu, arg); >>> case VFIO_IOMMU_DIRTY_PAGES: >>> return vfio_iommu_type1_dirty_pages(iommu, arg); >>> + case VFIO_IOMMU_SET_PASID_TABLE: >>> + return vfio_iommu_type1_set_pasid_table(iommu, arg); >>> default: >>> return -ENOTTY; >>> } >>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >>> index 2f313a238a8f..78ce3ce6c331 100644 >>> --- a/include/uapi/linux/vfio.h >>> +++ b/include/uapi/linux/vfio.h >>> @@ -14,6 +14,7 @@ >>> >>> #include >>> #include >>> +#include >>> >>> #define VFIO_API_VERSION 0 >>> >>> @@ -1180,6 +1181,24 @@ struct vfio_iommu_type1_dirty_bitmap_get { >>> >>> #define VFIO_IOMMU_DIRTY_PAGES _IO(VFIO_TYPE, VFIO_BASE + 17) >>> >>> +/* >>> + * VFIO_IOMMU_SET_PASID_TABLE - _IOWR(VFIO_TYPE, VFIO_BASE + 22, >>> + * struct vfio_iommu_type1_set_pasid_table) >>> + * >>> + * The SET operation passes a PASID table to the host while the >>> + * UNSET operation detaches the one currently programmed. Setting >>> + * a table while another is already programmed replaces the old table. >>> + */ >>> +struct vfio_iommu_type1_set_pasid_table { >>> + __u32 argsz; >>> + __u32 flags; >>> +#define VFIO_PASID_TABLE_FLAG_SET (1 << 0) >>> +#define VFIO_PASID_TABLE_FLAG_UNSET (1 << 1) >>> + struct iommu_pasid_table_config config; /* used on SET */ >>> +}; >>> + >>> +#define VFIO_IOMMU_SET_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) >>> + >>> /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */ >>> >>> /* >>> >> > > . > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3D484C433E0 for ; Mon, 22 Feb 2021 12:20:38 +0000 (UTC) Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A681464E44 for ; Mon, 22 Feb 2021 12:20:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A681464E44 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=iommu-bounces@lists.linux-foundation.org Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 31C30839DC; Mon, 22 Feb 2021 12:20:37 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id t6bDda7wBPRZ; Mon, 22 Feb 2021 12:20:35 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp1.osuosl.org (Postfix) with ESMTP id 2D80283906; Mon, 22 Feb 2021 12:20:35 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 0228BC000B; Mon, 22 Feb 2021 12:20:35 +0000 (UTC) Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id D4A0CC0001 for ; Mon, 22 Feb 2021 12:20:33 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id C1DC585F2F for ; Mon, 22 Feb 2021 12:20:33 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id juQxs3K-2R7E for ; Mon, 22 Feb 2021 12:20:32 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 81F1885F15 for ; Mon, 22 Feb 2021 12:20:31 +0000 (UTC) Received: from DGGEMS403-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4Dkh6W2gbCzMcZG; Mon, 22 Feb 2021 20:18:27 +0800 (CST) Received: from [10.174.184.42] (10.174.184.42) by DGGEMS403-HUB.china.huawei.com (10.3.19.203) with Microsoft SMTP Server id 14.3.498.0; Mon, 22 Feb 2021 20:20:24 +0800 Subject: Re: [PATCH v11 01/13] vfio: VFIO_IOMMU_SET_PASID_TABLE To: Auger Eric , , , , , , , , , , References: <20201116110030.32335-1-eric.auger@redhat.com> <20201116110030.32335-2-eric.auger@redhat.com> <84a111da-1969-1701-9a6d-cae8d7c285c6@huawei.com> From: Keqian Zhu Message-ID: Date: Mon, 22 Feb 2021 20:20:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.174.184.42] X-CFilter-Loop: Reflected Cc: jean-philippe@linaro.org, vivek.gautam@arm.com, zhangfei.gao@linaro.org X-BeenThere: iommu@lists.linux-foundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Development issues for Linux IOMMU support List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: iommu-bounces@lists.linux-foundation.org Sender: "iommu" Hi Eric, On 2021/2/22 18:53, Auger Eric wrote: > Hi Keqian, > > On 2/2/21 1:34 PM, Keqian Zhu wrote: >> Hi Eric, >> >> On 2020/11/16 19:00, Eric Auger wrote: >>> From: "Liu, Yi L" >>> >>> This patch adds an VFIO_IOMMU_SET_PASID_TABLE ioctl >>> which aims to pass the virtual iommu guest configuration >>> to the host. This latter takes the form of the so-called >>> PASID table. >>> >>> Signed-off-by: Jacob Pan >>> Signed-off-by: Liu, Yi L >>> Signed-off-by: Eric Auger >>> >>> --- >>> v11 -> v12: >>> - use iommu_uapi_set_pasid_table >>> - check SET and UNSET are not set simultaneously (Zenghui) >>> >>> v8 -> v9: >>> - Merge VFIO_IOMMU_ATTACH/DETACH_PASID_TABLE into a single >>> VFIO_IOMMU_SET_PASID_TABLE ioctl. >>> >>> v6 -> v7: >>> - add a comment related to VFIO_IOMMU_DETACH_PASID_TABLE >>> >>> v3 -> v4: >>> - restore ATTACH/DETACH >>> - add unwind on failure >>> >>> v2 -> v3: >>> - s/BIND_PASID_TABLE/SET_PASID_TABLE >>> >>> v1 -> v2: >>> - s/BIND_GUEST_STAGE/BIND_PASID_TABLE >>> - remove the struct device arg >>> --- >>> drivers/vfio/vfio_iommu_type1.c | 65 +++++++++++++++++++++++++++++++++ >>> include/uapi/linux/vfio.h | 19 ++++++++++ >>> 2 files changed, 84 insertions(+) >>> >>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >>> index 67e827638995..87ddd9e882dc 100644 >>> --- a/drivers/vfio/vfio_iommu_type1.c >>> +++ b/drivers/vfio/vfio_iommu_type1.c >>> @@ -2587,6 +2587,41 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu, >>> return ret; >>> } >>> >>> +static void >>> +vfio_detach_pasid_table(struct vfio_iommu *iommu) >>> +{ >>> + struct vfio_domain *d; >>> + >>> + mutex_lock(&iommu->lock); >>> + list_for_each_entry(d, &iommu->domain_list, next) >>> + iommu_detach_pasid_table(d->domain); >>> + >>> + mutex_unlock(&iommu->lock); >>> +} >>> + >>> +static int >>> +vfio_attach_pasid_table(struct vfio_iommu *iommu, unsigned long arg) >>> +{ >>> + struct vfio_domain *d; >>> + int ret = 0; >>> + >>> + mutex_lock(&iommu->lock); >>> + >>> + list_for_each_entry(d, &iommu->domain_list, next) { >>> + ret = iommu_uapi_attach_pasid_table(d->domain, (void __user *)arg); >> This design is not very clear to me. This assumes all iommu_domains share the same pasid table. >> >> As I understand, it's reasonable when there is only one group in the domain, and only one domain in the vfio_iommu. >> If more than one group in the vfio_iommu, the guest may put them into different guest iommu_domain, then they have different pasid table. >> >> Is this the use scenario? > > the vfio_iommu is attached to a container. all the groups within a > container share the same set of page tables (linux > Documentation/driver-api/vfio.rst). So to me if you want to use > different pasid tables, the groups need to be attached to different > containers. Does that make sense to you? OK, so this is what I understand about the design. A little question is that when we perform attach_pasid_table on a container, maybe we ought to do a sanity check to make sure that only one group is in this container, instead of iterating all domain? To be frank, my main concern is that if we put each group into different container under nested mode, then we give up the possibility that they can share stage2 page tables, which saves host memory and reduces the time of preparing environment for VM. To me, I'd like to understand the "container shares page table" to be: 1) share stage2 page table under nested mode. 2) share stage1 page table under non-nested mode. As when we perform "map" on a container: 1) under nested mode, we setup stage2 mapping. 2) under non-nested mode, we setup stage1 mapping. Indeed, to realize stage2 mapping sharing, we should do much more work to refactor SMMU_DOMAIN... Hope you can consider this. :) Thanks, Keqian > > Thanks > > Eric >> >> Thanks, >> Keqian >> >>> + if (ret) >>> + goto unwind; >>> + } >>> + goto unlock; >>> +unwind: >>> + list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) { >>> + iommu_detach_pasid_table(d->domain); >>> + } >>> +unlock: >>> + mutex_unlock(&iommu->lock); >>> + return ret; >>> +} >>> + >>> static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu, >>> struct vfio_info_cap *caps) >>> { >>> @@ -2747,6 +2782,34 @@ static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu, >>> -EFAULT : 0; >>> } >>> >>> +static int vfio_iommu_type1_set_pasid_table(struct vfio_iommu *iommu, >>> + unsigned long arg) >>> +{ >>> + struct vfio_iommu_type1_set_pasid_table spt; >>> + unsigned long minsz; >>> + int ret = -EINVAL; >>> + >>> + minsz = offsetofend(struct vfio_iommu_type1_set_pasid_table, flags); >>> + >>> + if (copy_from_user(&spt, (void __user *)arg, minsz)) >>> + return -EFAULT; >>> + >>> + if (spt.argsz < minsz) >>> + return -EINVAL; >>> + >>> + if (spt.flags & VFIO_PASID_TABLE_FLAG_SET && >>> + spt.flags & VFIO_PASID_TABLE_FLAG_UNSET) >>> + return -EINVAL; >>> + >>> + if (spt.flags & VFIO_PASID_TABLE_FLAG_SET) >>> + ret = vfio_attach_pasid_table(iommu, arg + minsz); >>> + else if (spt.flags & VFIO_PASID_TABLE_FLAG_UNSET) { >>> + vfio_detach_pasid_table(iommu); >>> + ret = 0; >>> + } >>> + return ret; >>> +} >>> + >>> static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu, >>> unsigned long arg) >>> { >>> @@ -2867,6 +2930,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data, >>> return vfio_iommu_type1_unmap_dma(iommu, arg); >>> case VFIO_IOMMU_DIRTY_PAGES: >>> return vfio_iommu_type1_dirty_pages(iommu, arg); >>> + case VFIO_IOMMU_SET_PASID_TABLE: >>> + return vfio_iommu_type1_set_pasid_table(iommu, arg); >>> default: >>> return -ENOTTY; >>> } >>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >>> index 2f313a238a8f..78ce3ce6c331 100644 >>> --- a/include/uapi/linux/vfio.h >>> +++ b/include/uapi/linux/vfio.h >>> @@ -14,6 +14,7 @@ >>> >>> #include >>> #include >>> +#include >>> >>> #define VFIO_API_VERSION 0 >>> >>> @@ -1180,6 +1181,24 @@ struct vfio_iommu_type1_dirty_bitmap_get { >>> >>> #define VFIO_IOMMU_DIRTY_PAGES _IO(VFIO_TYPE, VFIO_BASE + 17) >>> >>> +/* >>> + * VFIO_IOMMU_SET_PASID_TABLE - _IOWR(VFIO_TYPE, VFIO_BASE + 22, >>> + * struct vfio_iommu_type1_set_pasid_table) >>> + * >>> + * The SET operation passes a PASID table to the host while the >>> + * UNSET operation detaches the one currently programmed. Setting >>> + * a table while another is already programmed replaces the old table. >>> + */ >>> +struct vfio_iommu_type1_set_pasid_table { >>> + __u32 argsz; >>> + __u32 flags; >>> +#define VFIO_PASID_TABLE_FLAG_SET (1 << 0) >>> +#define VFIO_PASID_TABLE_FLAG_UNSET (1 << 1) >>> + struct iommu_pasid_table_config config; /* used on SET */ >>> +}; >>> + >>> +#define VFIO_IOMMU_SET_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) >>> + >>> /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */ >>> >>> /* >>> >> > > . > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1734C433DB for ; Mon, 22 Feb 2021 12:20:37 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id E447264E32 for ; Mon, 22 Feb 2021 12:20:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E447264E32 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 4734A4A522; Mon, 22 Feb 2021 07:20:36 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id aKYpf93cxf9b; Mon, 22 Feb 2021 07:20:34 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id EE7B14B0A0; Mon, 22 Feb 2021 07:20:34 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 6BE3740667 for ; Mon, 22 Feb 2021 07:20:33 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id BnAMWW2xuxdf for ; Mon, 22 Feb 2021 07:20:30 -0500 (EST) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 651DD4005D for ; Mon, 22 Feb 2021 07:20:30 -0500 (EST) Received: from DGGEMS403-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4Dkh6W2gbCzMcZG; Mon, 22 Feb 2021 20:18:27 +0800 (CST) Received: from [10.174.184.42] (10.174.184.42) by DGGEMS403-HUB.china.huawei.com (10.3.19.203) with Microsoft SMTP Server id 14.3.498.0; Mon, 22 Feb 2021 20:20:24 +0800 Subject: Re: [PATCH v11 01/13] vfio: VFIO_IOMMU_SET_PASID_TABLE To: Auger Eric , , , , , , , , , , References: <20201116110030.32335-1-eric.auger@redhat.com> <20201116110030.32335-2-eric.auger@redhat.com> <84a111da-1969-1701-9a6d-cae8d7c285c6@huawei.com> From: Keqian Zhu Message-ID: Date: Mon, 22 Feb 2021 20:20:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.174.184.42] X-CFilter-Loop: Reflected Cc: jean-philippe@linaro.org, jacob.jun.pan@linux.intel.com, nicoleotsuka@gmail.com, vivek.gautam@arm.com, yi.l.liu@intel.com, zhangfei.gao@linaro.org X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu Hi Eric, On 2021/2/22 18:53, Auger Eric wrote: > Hi Keqian, > > On 2/2/21 1:34 PM, Keqian Zhu wrote: >> Hi Eric, >> >> On 2020/11/16 19:00, Eric Auger wrote: >>> From: "Liu, Yi L" >>> >>> This patch adds an VFIO_IOMMU_SET_PASID_TABLE ioctl >>> which aims to pass the virtual iommu guest configuration >>> to the host. This latter takes the form of the so-called >>> PASID table. >>> >>> Signed-off-by: Jacob Pan >>> Signed-off-by: Liu, Yi L >>> Signed-off-by: Eric Auger >>> >>> --- >>> v11 -> v12: >>> - use iommu_uapi_set_pasid_table >>> - check SET and UNSET are not set simultaneously (Zenghui) >>> >>> v8 -> v9: >>> - Merge VFIO_IOMMU_ATTACH/DETACH_PASID_TABLE into a single >>> VFIO_IOMMU_SET_PASID_TABLE ioctl. >>> >>> v6 -> v7: >>> - add a comment related to VFIO_IOMMU_DETACH_PASID_TABLE >>> >>> v3 -> v4: >>> - restore ATTACH/DETACH >>> - add unwind on failure >>> >>> v2 -> v3: >>> - s/BIND_PASID_TABLE/SET_PASID_TABLE >>> >>> v1 -> v2: >>> - s/BIND_GUEST_STAGE/BIND_PASID_TABLE >>> - remove the struct device arg >>> --- >>> drivers/vfio/vfio_iommu_type1.c | 65 +++++++++++++++++++++++++++++++++ >>> include/uapi/linux/vfio.h | 19 ++++++++++ >>> 2 files changed, 84 insertions(+) >>> >>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >>> index 67e827638995..87ddd9e882dc 100644 >>> --- a/drivers/vfio/vfio_iommu_type1.c >>> +++ b/drivers/vfio/vfio_iommu_type1.c >>> @@ -2587,6 +2587,41 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu, >>> return ret; >>> } >>> >>> +static void >>> +vfio_detach_pasid_table(struct vfio_iommu *iommu) >>> +{ >>> + struct vfio_domain *d; >>> + >>> + mutex_lock(&iommu->lock); >>> + list_for_each_entry(d, &iommu->domain_list, next) >>> + iommu_detach_pasid_table(d->domain); >>> + >>> + mutex_unlock(&iommu->lock); >>> +} >>> + >>> +static int >>> +vfio_attach_pasid_table(struct vfio_iommu *iommu, unsigned long arg) >>> +{ >>> + struct vfio_domain *d; >>> + int ret = 0; >>> + >>> + mutex_lock(&iommu->lock); >>> + >>> + list_for_each_entry(d, &iommu->domain_list, next) { >>> + ret = iommu_uapi_attach_pasid_table(d->domain, (void __user *)arg); >> This design is not very clear to me. This assumes all iommu_domains share the same pasid table. >> >> As I understand, it's reasonable when there is only one group in the domain, and only one domain in the vfio_iommu. >> If more than one group in the vfio_iommu, the guest may put them into different guest iommu_domain, then they have different pasid table. >> >> Is this the use scenario? > > the vfio_iommu is attached to a container. all the groups within a > container share the same set of page tables (linux > Documentation/driver-api/vfio.rst). So to me if you want to use > different pasid tables, the groups need to be attached to different > containers. Does that make sense to you? OK, so this is what I understand about the design. A little question is that when we perform attach_pasid_table on a container, maybe we ought to do a sanity check to make sure that only one group is in this container, instead of iterating all domain? To be frank, my main concern is that if we put each group into different container under nested mode, then we give up the possibility that they can share stage2 page tables, which saves host memory and reduces the time of preparing environment for VM. To me, I'd like to understand the "container shares page table" to be: 1) share stage2 page table under nested mode. 2) share stage1 page table under non-nested mode. As when we perform "map" on a container: 1) under nested mode, we setup stage2 mapping. 2) under non-nested mode, we setup stage1 mapping. Indeed, to realize stage2 mapping sharing, we should do much more work to refactor SMMU_DOMAIN... Hope you can consider this. :) Thanks, Keqian > > Thanks > > Eric >> >> Thanks, >> Keqian >> >>> + if (ret) >>> + goto unwind; >>> + } >>> + goto unlock; >>> +unwind: >>> + list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) { >>> + iommu_detach_pasid_table(d->domain); >>> + } >>> +unlock: >>> + mutex_unlock(&iommu->lock); >>> + return ret; >>> +} >>> + >>> static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu, >>> struct vfio_info_cap *caps) >>> { >>> @@ -2747,6 +2782,34 @@ static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu, >>> -EFAULT : 0; >>> } >>> >>> +static int vfio_iommu_type1_set_pasid_table(struct vfio_iommu *iommu, >>> + unsigned long arg) >>> +{ >>> + struct vfio_iommu_type1_set_pasid_table spt; >>> + unsigned long minsz; >>> + int ret = -EINVAL; >>> + >>> + minsz = offsetofend(struct vfio_iommu_type1_set_pasid_table, flags); >>> + >>> + if (copy_from_user(&spt, (void __user *)arg, minsz)) >>> + return -EFAULT; >>> + >>> + if (spt.argsz < minsz) >>> + return -EINVAL; >>> + >>> + if (spt.flags & VFIO_PASID_TABLE_FLAG_SET && >>> + spt.flags & VFIO_PASID_TABLE_FLAG_UNSET) >>> + return -EINVAL; >>> + >>> + if (spt.flags & VFIO_PASID_TABLE_FLAG_SET) >>> + ret = vfio_attach_pasid_table(iommu, arg + minsz); >>> + else if (spt.flags & VFIO_PASID_TABLE_FLAG_UNSET) { >>> + vfio_detach_pasid_table(iommu); >>> + ret = 0; >>> + } >>> + return ret; >>> +} >>> + >>> static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu, >>> unsigned long arg) >>> { >>> @@ -2867,6 +2930,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data, >>> return vfio_iommu_type1_unmap_dma(iommu, arg); >>> case VFIO_IOMMU_DIRTY_PAGES: >>> return vfio_iommu_type1_dirty_pages(iommu, arg); >>> + case VFIO_IOMMU_SET_PASID_TABLE: >>> + return vfio_iommu_type1_set_pasid_table(iommu, arg); >>> default: >>> return -ENOTTY; >>> } >>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >>> index 2f313a238a8f..78ce3ce6c331 100644 >>> --- a/include/uapi/linux/vfio.h >>> +++ b/include/uapi/linux/vfio.h >>> @@ -14,6 +14,7 @@ >>> >>> #include >>> #include >>> +#include >>> >>> #define VFIO_API_VERSION 0 >>> >>> @@ -1180,6 +1181,24 @@ struct vfio_iommu_type1_dirty_bitmap_get { >>> >>> #define VFIO_IOMMU_DIRTY_PAGES _IO(VFIO_TYPE, VFIO_BASE + 17) >>> >>> +/* >>> + * VFIO_IOMMU_SET_PASID_TABLE - _IOWR(VFIO_TYPE, VFIO_BASE + 22, >>> + * struct vfio_iommu_type1_set_pasid_table) >>> + * >>> + * The SET operation passes a PASID table to the host while the >>> + * UNSET operation detaches the one currently programmed. Setting >>> + * a table while another is already programmed replaces the old table. >>> + */ >>> +struct vfio_iommu_type1_set_pasid_table { >>> + __u32 argsz; >>> + __u32 flags; >>> +#define VFIO_PASID_TABLE_FLAG_SET (1 << 0) >>> +#define VFIO_PASID_TABLE_FLAG_UNSET (1 << 1) >>> + struct iommu_pasid_table_config config; /* used on SET */ >>> +}; >>> + >>> +#define VFIO_IOMMU_SET_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) >>> + >>> /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */ >>> >>> /* >>> >> > > . > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm