From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f175.google.com (mail-pl1-f175.google.com [209.85.214.175]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7242037CD59 for ; Mon, 23 Mar 2026 21:34:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.175 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774301678; cv=none; b=YtG4LlsX3jXxAz8bbYF2XgLetMpxOD+jrhgxN75hN+igZfA+/1+wqDWpFLpu7Kd4PU+3EotMx4oioOvySsk2QaAZ9nS+BqwrYQ1k70GmUweDjuPJxzjVD184tRn4fVy6P2q7qbP8c8onnPgM8rYzGsF3ZAdW11dW1pbhG1NfnJU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774301678; c=relaxed/simple; bh=RsieRZ+VCWLZIXMh2Ozv0KLI7UIr8x0rFUR6Sii/tzc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=G58szz2Rj30KodyjmnOXCQh8FGfwcPwP5Rkpo2FSq2PYFlb29K6gJqwjo8L1ejBLwhsaWgfWil5GXmeB6Upj+ZE2C25tiNpXbJCmcmgXiuGZT9e/vqJ9qnn/2uwvQqO4mTx2nUAWkAB3CgIKR2iQMzavhdH9o4JyV2uRFJyN+KI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=b0GK61sr; arc=none smtp.client-ip=209.85.214.175 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="b0GK61sr" Received: by mail-pl1-f175.google.com with SMTP id d9443c01a7336-2b052ec7176so11615ad.1 for ; Mon, 23 Mar 2026 14:34:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1774301677; x=1774906477; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=FVYWkdGYSoCJ0Jra5vEPuFKgbrIGxBQ8Jn3YEa+aygo=; b=b0GK61srwT3fMcJGI0DXQX9/hUnn5KJHftHeD+ATuFkIXV3lSfEEjP2qhOQGPqgNkC lDsSk6VHn4zhKcXIFJCl9uo1UqE7HdeiM1Bx2IzNHDt7EQ1B/zki90FmNJT3V4BQT8+s HFU88UbgdgqDE4zQ13rj5b7+I5TutXeFfVnz4IwJVaTS7e3H5UNCLNVNv3DNjjttVop4 lr+MQ5hOB9kxfjJz7933XlPBU31zn9EdGrQiCPizXStv7rvRY43rkGFTQqJDnkHHzrWa XBQu5uTC+N4OZi6r7NunXgBIucSjtBAe89bVKAehFGoFXp7PnvClP3+0k7H6v8MxCWgU vTmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774301677; x=1774906477; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=FVYWkdGYSoCJ0Jra5vEPuFKgbrIGxBQ8Jn3YEa+aygo=; b=RMfAJzQHBioydf9B+1ACu2WPWHKzIUyDAdOMuInX7rJ4HlJk0e5t424/pkHkullH6f cZpp35Te+pjp3byiOJZNt2VAsHhHxQt7DdPn9qjb5LFnLyd+W8HA8Z4a08sMOdcyHGt0 hQxWjX2R6Ylnr7YmfZ4ee2J7j7pTWAfoUbbZ+D9A1vCrdC/5Us7Hki6xsyoHzSVc1JlH ZgxORqVCOpC1e8njSGSjqVlJ0yqyCoe0KKmi+ikM9zi9HH5DvU2CCarhdqQV7HL5eFsR 6n6F/5rHgzEolhzR8Ghu4EgBhw4rX8Ag8jtMGPTxclh3jsGtGLUWrlAkUO5wMWy2TqeI w0Sw== X-Forwarded-Encrypted: i=1; AJvYcCWcmzfZfpSfNmW3gtgc0BmgKXprjzPPv+PeA/6DcMcALt/eGIXwp/Ku5iCewl0CNZuEFyWuN5/lQ3WsMRI=@vger.kernel.org X-Gm-Message-State: AOJu0Yx0A80ojoSY9qIUl5IpG8eAB7P7Zr8OgatS18NDgCyE/OD2K/7a vIn0NjiiF2aSRmRoe5RGnB8Z7SaDLOS1DMRUOzh54MR9AeS6LPOXX5bV4hx6mjBLLw== X-Gm-Gg: ATEYQzynyrbsLBNqQSvErTdmK4AO6Sa1K+ddV4wvTxKER2A/PfJtomijRMA66JRlZsS 7IGv1B5ornDDA9YzFRbXyrfukSBAuJ29aA9GGQOQg8UYvaJZ7s6pUH5zG60SJ58RE3LsVDsQ0BY 7zPC0/LkL/DZTBp5MAD4hgnrnvcM3jjw1krNGXJeR7eermwzav/ieHLuqVkDPNos4AV0K0rHC5p 43sbVgFDue8qeyKhE8kwh/kMVM3AkiivyeX0sVRiccPGV3forER7oFFF+4eohy+r7G9jTypmIQ0 ugDp+f+6SoQBRdD/yj8ew+OXiRPimxOarfr5HAVQ8lJT7HuPa8WmfxIm97vh9DqReS4kbzafvDR /BD1OQFFgGlBnIRcZUuYQ23NS3G5aDfE3mA8X9lO4r9Lc6hp9VdB/AVCB4zh2J8vUfRHwm35T0a faBa60jhZltjvOqUzPr9TvhSCQ5aX+Y64iZotmPYWqZjTthOx1xV9a15TW0pBExjQLr5mAo17d X-Received: by 2002:a17:902:f712:b0:2ad:ab68:f2c3 with SMTP id d9443c01a7336-2b0a6737893mr333575ad.13.1774301676102; Mon, 23 Mar 2026 14:34:36 -0700 (PDT) Received: from google.com (168.136.83.34.bc.googleusercontent.com. [34.83.136.168]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-82c38565096sm5283490b3a.51.2026.03.23.14.34.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Mar 2026 14:34:35 -0700 (PDT) Date: Mon, 23 Mar 2026 21:34:31 +0000 From: Samiullah Khawaja To: Vipin Sharma Cc: David Woodhouse , Lu Baolu , Joerg Roedel , Will Deacon , Jason Gunthorpe , YiFei Zhu , Robin Murphy , Kevin Tian , Alex Williamson , Shuah Khan , iommu@lists.linux.dev, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Saeed Mahameed , Adithya Jayachandran , Parav Pandit , Leon Romanovsky , William Tu , Pratyush Yadav , Pasha Tatashin , David Matlack , Andrew Morton , Chris Li , Pranjal Shrivastava Subject: Re: [PATCH 11/14] iommufd-lu: Persist iommu hardware pagetables for live update Message-ID: References: <20260203220948.2176157-1-skhawaja@google.com> <20260203220948.2176157-12-skhawaja@google.com> <20260320230654.GB659132.vipinsh@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20260320230654.GB659132.vipinsh@google.com> On Mon, Mar 23, 2026 at 01:28:13PM -0700, Vipin Sharma wrote: >On Tue, Feb 03, 2026 at 10:09:45PM +0000, Samiullah Khawaja wrote: >> From: YiFei Zhu >> >> The caller is expected to mark each HWPT to be preserved with an ioctl >> call, with a token that will be used in restore. At preserve time, each >> HWPT's domain is then called with iommu_domain_preserve to preserve the > >Nit: Add () after iommu_domain_preserve, it makes easier to know this is >a function. Will do. > >> diff --git a/drivers/iommu/iommufd/liveupdate.c b/drivers/iommu/iommufd/liveupdate.c >> +static int iommufd_save_hwpts(struct iommufd_ctx *ictx, >> + struct iommufd_lu *iommufd_lu, >> + struct liveupdate_session *session) >> +{ >> + struct iommufd_hwpt_paging *hwpt, **hwpts = NULL; > >We should rename hwpts to something to denote that it is a list. Agreed. Will do. > >> + struct iommu_domain_ser *domain_ser; >> + struct iommufd_hwpt_lu *hwpt_lu; >> + struct iommufd_object *obj; >> + unsigned int nr_hwpts = 0; >> + unsigned long index; >> + unsigned int i; >> + int rc = 0; >> + >> + if (iommufd_lu) { >> + hwpts = kcalloc(iommufd_lu->nr_hwpts, sizeof(*hwpts), >> + GFP_KERNEL); >> + if (!hwpts) >> + return -ENOMEM; >> + } >> + >> + xa_lock(&ictx->objects); >> + xa_for_each(&ictx->objects, index, obj) { >> + if (obj->type != IOMMUFD_OBJ_HWPT_PAGING) >> + continue; >> + >> + hwpt = container_of(obj, struct iommufd_hwpt_paging, common.obj); >> + if (!hwpt->lu_preserve) >> + continue; >> + >> + if (hwpt->ioas) { >> + /* >> + * Obtain exclusive access to the IOAS and IOPT while we >> + * set immutability >> + */ >> + mutex_lock(&hwpt->ioas->mutex); >> + down_write(&hwpt->ioas->iopt.domains_rwsem); >> + down_write(&hwpt->ioas->iopt.iova_rwsem); >> + >> + hwpt->ioas->iopt.lu_map_immutable = true; > >It feels odd that this is not cleaned up in this function as a part of >its error handling. Now it becomes caller resposibility to handle clean >up for the side effect created by this function. The immutablity is set early during preservation and it might be required to unset it in the caller also when error occurs. I have to rework this to not acquire a mutex lock in a spin lock, so I think this part can also be reworked by doing it in following steps, - Alloc memory for preservation. - Mark immutable. - Preserve using IOMMU core APIs. > >IMO, this function should clean up lu_map_immutable instread of callers >to make sure they call cleanups. You can also try exploring XA_MARK_* >APIs if that can help in cleaning up easily. Agreed. > >> +int iommufd_liveupdate_unregister_lufs(void) >> +{ >> + WARN_ON(iommu_liveupdate_unregister_flb(&iommufd_lu_handler)); >> + >> + return liveupdate_unregister_file_handler(&iommufd_lu_handler); > >This seems little insconsistent, iommu_liveupdate_unregister_flb() has >WARN_ON and liveupdate_unregister_file_handler() does not. The error from unregister_file_handler() is propagated to the caller, but the error from unregister_flb() is not propagated as need to call the unregister_file_handler(). > >Also, refer https://lore.kernel.org/all/20260226160353.6f3371bc@shazbot.org/ Yes. This will need updates according to the new changes in LUO where the unregister functions has void return type. > >> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c >> @@ -775,11 +775,21 @@ static int __init iommufd_init(void) >> if (ret) >> goto err_misc; >> } >> + >> + if (IS_ENABLED(CONFIG_IOMMU_LIVEUPDATE)) { >> + ret = iommufd_liveupdate_register_lufs(); >> + if (ret) >> + goto err_vfio_misc; >> + } >> + > >Do we need IS_ENABLED here? iommufd_private.h is providing definition >for both values of CONFIG_IOMMU_LIVEUPDATE. > >Nit: What is lufs? Should we just rename to iommufd_liveupdate_register()? Will update both. > >> ret = iommufd_test_init(); >> if (ret) >> - goto err_vfio_misc; >> + goto err_lufs; >> return 0; >> >> +err_lufs: >> + if (IS_ENABLED(CONFIG_IOMMU_LIVEUPDATE)) >> + iommufd_liveupdate_unregister_lufs(); >> err_vfio_misc: >> if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER)) >> misc_deregister(&vfio_misc_dev); >> @@ -791,6 +801,8 @@ static int __init iommufd_init(void) >> static void __exit iommufd_exit(void) >> { >> iommufd_test_exit(); >> + if (IS_ENABLED(CONFIG_IOMMU_LIVEUPDATE)) >> + iommufd_liveupdate_unregister_lufs(); > >Same as above. Will update. > >> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c >> @@ -1427,6 +1429,12 @@ struct iopt_pages *iopt_alloc_file_pages(struct file *file, >> pages->file = get_file(file); >> pages->start = start - start_byte; >> pages->type = IOPT_ADDRESS_FILE; >> + >> + pages->seals = 0; > >This is already 0. Will update. > >> + seals = memfd_get_seals(file); >> + if (seals > 0) >> + pages->seals = seals; >> + >> return pages; >> } >> >> --- /dev/null >> +++ b/include/linux/kho/abi/iommufd.h >> + >> +struct iommufd_lu { >> + unsigned int nr_hwpts; > >Should this be u32 or u64? > >> + struct iommufd_hwpt_lu hwpts[]; >> +}; >> + > >Should this also be __packed? Will do both u32 and packed. > >> +#endif /* _LINUX_KHO_ABI_IOMMUFD_H */ >> -- >> 2.53.0.rc2.204.g2597b5adb4-goog >>