From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=103.168.172.147 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793836511; b=TCAwibUdRbIO1tkqD70h0sRLwafMrdOBWjVY1Myd8zXZkjs+z6Xdtksjg5IjO6bVl5eW 5PGM5AaFWK6EHK5AXv9F+VHE9u57oV3MI6CDYI3OwMMcvuOaNBN2MVXcUxANyozJ8il2b k8ScQVna7r8rWxjt70+Y5hq3zdcI+toBNu2KOJjqEKKh9Uuy+aFu6z4/fxJayXiSqMghJ Mj8En9crjwlSO7eHw7SqUNFz4m/o2Gp2Or2PHCEYJ+1m2KUkbDqoWsMLXkhQJrXVTEo88 v+lM9oP6eeeeECrVi57hLLsA8sdO/VLL9MF/mmfnINtkRjVDuQPZpm9AKKlx6Zq9qew== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793836511; h=DMARC-Filter:Received:Received:DKIM-Signature:DKIM-Signature: X-DKIM2-Info:DKIM2-Signature:X-DKIM2-Info:Message-Instance: X-ME-Sender:X-ME-Received:X-ME-Proxy-Cause:X-ME-Proxy:Feedback-ID: Received:Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: X-Mailer:MIME-Version:Content-Type:Content-Transfer-Encoding; bh=Pdqghi/Hsjfp283ilYdqjHRoyabbxkVpexYmZMhDhdY=; b=cL7zxhzBd17xKltpZXeBiFmcSKO6+hfLatdIBadhCQrRvOzWTJdXBXzKFDM/fNbdaAT6 dzOVmbIu6cLgQR6ZIInWbSkL1WvqGpqCdymG0KPKO7m2aVzw9XUWTLoBlXYc9cFOJ7fRz wiyoSWwS57Y6w0uleRH7CE9h41ieSQYTzcpo/04vCJPl922ozQTjJWGXSXsZhLZg9u/OT F8lndioiTB20Ap8l/zfMjmpp1dJ3rBdE0p0jzGK8fT06CneXMIGybcwIjm109HLlSL5Ye CregtnDTZ3kYHarHiQE+SIMIu9VLNmF5DtaF/yQ6Zw17itEN1Awg5w12JAfYPPZa2Hg== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass header.from=shazbot.org; spf=pass smtp.mailfrom=shazbot.org; dkim=pass header.d=shazbot.org header.i=@shazbot.org header.a=rsa-sha256 header.s=fm2 header.b=zNTmUdCu; dkim=pass header.d=messagingengine.com header.i=@messagingengine.com header.a=rsa-sha256 header.s=fm3 header.b=Ufnaj8A1; arc=none smtp.remote-ip=103.168.172.147 Received: from fout-a4-smtp.messagingengine.com (fout-a4-smtp.messagingengine.com [103.168.172.147]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5D2D2606D835 for ; Wed, 4 Nov 2026 23:55:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org 5D2D2606D835 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=shazbot.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=shazbot.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=shazbot.org header.i=@shazbot.org header.a=rsa-sha256 header.s=fm2 header.b=zNTmUdCu; dkim=pass (2048-bit key, unprotected) header.d=messagingengine.com header.i=@messagingengine.com header.a=rsa-sha256 header.s=fm3 header.b=Ufnaj8A1 Received: from phl-compute-04.internal (phl-compute-04.internal [10.202.2.44]) by mailfout.phl.internal (Postfix) with ESMTP id C7C1DEC0187 for ; Wed, 4 Nov 2026 18:55:10 -0500 (EST) Received: from phl-frontend-03 ([10.202.2.162]) by phl-compute-04.internal (MEProxy); Wed, 04 Nov 2026 18:55:10 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=shazbot.org; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm2; t=1793836510; x=1793922910; bh=Pdqghi/Hsjfp283ilYdqjHRoyabbxkVpexYmZMhDhdY=; b= zNTmUdCuAzk5AH1ke3y7LGl+N1DTeLpQMr/u8xf87XC5JgWRH2jrv6vPyS02EzSG TjOh6VqG7cClyiYcelrMEBFPQ3tKh7o8S/YwRt7nArclLQYDQhLpCn+F9H+xItsG xbuSC+gCcu3v6yy087hbnzbs+qCy5mCcg9D4Z97d6WYh1IZEh07vowqZw2nbi8uZ HVSNqwqz5w93SYpaxODXk4gRTX/H+Bitr+jWxLKHhNSI6BRsbBJs/tXyATRyo+xK 335XGkA8u2hhhFjy0ooMc/M8aRn1Hf7xQgC1yzjRC+q7cO/lDxBGbHgHMOVDL8P+ feiOG2I1Am+zY6yINYsHkg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t=1793836510; x= 1793922910; bh=Pdqghi/Hsjfp283ilYdqjHRoyabbxkVpexYmZMhDhdY=; b=U fnaj8A1hxajSrvbUY8A2oX+ObXUMht6Ai+C3HJ0N03hwNHbZHsB5FuFbOdgbR+p+ Vthhe3er1YtgFB+nU+mRbaU8VFnShOo7MJhX/5HT5USmSPcTPL+GasKDxxjPOQXg rA/xDrsvOHOWdDENwQBPnwq3+HoERwQnU8SXpIJXa9vD+6eOXVPgfApTrNnmgROZ nESjDsaGIDBf8lhAU3HS+4rx8CHgNZqu42NfVtxhqd5FaZHPuqRi1USHt43P/EbT Zzl5mdXhOqLOn8Yi0GJONzhBWGtj3+At9IUflbkzjnsouC5ThhrAkr04g940zNw0 HM0/6OuyW8lXY/USvOVRQ== X-DKIM2-Info: draft=ietf-dkim-dkim2-spec-08; repo=github.com/dkim2wg/interop; date=2026-10-28; sw=lmtpprox; action=sign d=shazbot.org a=rsa-sha256; DKIM2-Signature: i=1; m=1; t=1793836510; d=shazbot.org; mf=PGFsZXhAc2hhemJvdC5vcmc+; rt=PGxpbnV4LWRvY0B2Z2VyLmtlcm5lbC5vcmc+; s=fm2:rsa-sha256:P5dj/LvxtK9JfI/HaNmHwfSDhLWY9hloJYQ8/3P0LLL9G51 5MOXYzm3DBW9oope4nho5DPVN85i8inFHGJjgUgbXyJEQxGlngJQu/X/RinFrsDy IfSGvtxcjUNeyGvXU8RpY9V/qcPATwkInXyCIAYJ4wti3vyHSDUqiQli6WwPDuy0 203d5abxn/xiqkuHy81q47zw2Q3ZXjRcgjHx+KGPS2dSXLHhbg+MTmYbNco36XF6 P3kEixbHVc1SFjqE78tTnS0Lv6eQ4jtO5IqUSPv/cBL2WChU/GCuDq6yuaQZhycD Altm1muONNimfkrPBz/J3lRzrcRaGFjHRixLOCA==; X-DKIM2-Info: draft=ietf-dkim-dkim2-spec-08; repo=github.com/dkim2wg/interop; date=2026-10-28; sw=lmtpprox; action=mi-m=1; hc=12; hn=cc,content-transfer-encoding,content-type,date,feedback-id, from,in-reply-to,message-id,mime-version,references,subject,to; Message-Instance: m=1; h=sha256:VdutSf7WooPCXQoXuer3NdVF7PRKt9A3AUJb+IYX6+A=:aI+WDWM7VeqR9QslZ0VneK6Aaji8eaR6RRm+IM960l0=; X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: dmFkZTFU/kbVOsWxOTPcCpe6x61hsj1WNmmGso+RiYOPL9PXBn0IITeBo+AW6SFW+G4ekC /hzYoF162vC7PC2Mtt2USNZ/eCRzPfnSvLH4GpnZYykCQlv/g/2dfaxUihNQ2PdVnmznVt eyzTFpFT8ROSV9PpSF+vkXBd8GjUZhM6/FJB6rHUdJL+IEmDIn9qqg8D9AaoFAEgqB3zaI LQuuKEKLXGp7sIkYnu8IYt8f4dndp/+v360R0jn1bNCYEbWntuZR9TDLjZCVdVdTAj6SKy IHj9Y41QEBh5Iv1k5SCIJDAY7TVRfrnmdQbz1lqnEzzeOazcj9kVYNjTrGiFbntAEPcYeX 6duqXQYi5Z8CokUmEWQafV1IgET6f9oJp+QFwdgPjBBgCgRoJ4AV/DRQRgalnKr+BsByEz /oKjQp5wiHN1ScigJptBQqxSxfjsV1jDxVD1bhdL/AJW6pOhCSAJguUUe0nkjjedWRl0wJ 8Xpkd6qDRdwuTERhd+l0OX8pCNESHKdyubF6uKNGkPA9BrqlL/XFzTubjohZfboFoN8fHk 79s3SnJfsAdhqlx+phocTufsnxRzhPs1GZXC6Iqepa0k1rXC7Otgax+JErgsPM2zEVXZa6 blG+jDSbqiAzg3aaVPRZ4c/Nf+e8obJX4YznqSqvg/W2LcPK8JxetNG8viUA X-ME-Proxy: Feedback-ID: i03f14258:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 4 Nov 2026 18:55:08 -0500 (EST) Date: Wed, 4 Nov 2026 16:52:47 -0700 From: Alex Williamson To: Cc: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , alex@shazbot.org Subject: Re: [PATCH v6 15/31] vfio/pci: Route every reset point through a try-reset helper Message-ID: <20261104165247.2597f574@shazbot.org> In-Reply-To: <20261016161839.2765592-16-mhonap@nvidia.com> References: <20261016161839.2765592-1-mhonap@nvidia.com> <20261016161839.2765592-16-mhonap@nvidia.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-pc-linux-gnu) X-Mailing-List: linux-doc@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Fri, 16 Oct 2026 21:48:22 +0530 wrote: > From: Manish Honap > > A provider such as vfio-cxl must reset its device itself: a CXL Type-2 > function must not take an FLR, and its reset changes what the provider > lets the fault path map. vfio-pci-core resets a function at five points: > enable, close, VFIO_DEVICE_RESET, a virtualized PCIe or AF FLR, and the > guest's CXL Reset doorbell in the virtualized CXL DVSEC. What substantiates the claim that a type-2 device must not take an FLR? Here we're promoting all resets to a CXL reset, but a CXL reset and an FLR have different scopes. FLR is exposed to the user through config space. A user might reasonably expect an FLR scoped reset through the PCIe/AF capabilities. The CXL spec defines FLR vs CXL reset, so this actually makes the device non CXL compliant through vfio-pci. This is a massive policy imposition. What makes this necessary? > Add a mandatory reset op to the provider ops, and refuse to register ops > without it. Add vfio_pci_try_reset_function(), which keeps the trylock > of pci_try_reset_function(): it returns -EAGAIN when the device lock is > contended, and otherwise runs the provider's reset with the device lock > and memory_lock held. The close path keeps its bridge and device trylock > and calls a locked variant. vfio_pci_try_reset_function() saves the PCI > state first, as pci_try_reset_function() does, for the provider's reset > to restore. The locked variant does not save: on close the reset must > restore the open-time state that vfio_pci_core_disable() has just > loaded. A failed save is returned and the reset is not attempted. > > The enable path still fails only on -EAGAIN. A provider checks at bind > that it can reset the device, so reset_works is set for a provider > device whatever the result of the first reset. > > The CXL Reset doorbell now runs the reset and reports its result in > Status2: Reset Error on failure, Reset Done on success. > > Suggested-by: Alex Williamson > Assisted-by: LLM > Signed-off-by: Manish Honap > --- > drivers/vfio/pci/vfio_pci_config.c | 19 ++++++-- > drivers/vfio/pci/vfio_pci_core.c | 71 ++++++++++++++++++++++++++++-- > drivers/vfio/pci/vfio_pci_priv.h | 1 + > include/linux/vfio_pci_core.h | 7 +++ > 4 files changed, 90 insertions(+), 8 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > index 1fc463540211..6fbece95cb9f 100644 > --- a/drivers/vfio/pci/vfio_pci_config.c > +++ b/drivers/vfio/pci/vfio_pci_config.c > @@ -910,7 +910,7 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos, > if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) { > vfio_pci_zap_and_down_write_memory_lock(vdev); > vfio_pci_dma_buf_move(vdev, true); > - pci_try_reset_function(vdev->pdev); > + vfio_pci_try_reset_function(vdev); > if (__vfio_pci_memory_enabled(vdev)) > vfio_pci_dma_buf_move(vdev, false); > up_write(&vdev->memory_lock); > @@ -995,7 +995,7 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos, > if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) { > vfio_pci_zap_and_down_write_memory_lock(vdev); > vfio_pci_dma_buf_move(vdev, true); > - pci_try_reset_function(vdev->pdev); > + vfio_pci_try_reset_function(vdev); > if (__vfio_pci_memory_enabled(vdev)) > vfio_pci_dma_buf_move(vdev, false); > up_write(&vdev->memory_lock); > @@ -1786,9 +1786,20 @@ static int vfio_cxl_dvsec_write(struct vfio_pci_core_device *vdev, int pos, > if (!(ctrl2 & PCI_DVSEC_CXL_DISABLE_CACHING)) > status2 &= ~PCI_DVSEC_CXL_CACHE_INV; > if (ctrl2 & PCI_DVSEC_CXL_INIT_CXL_RST) { > + int ret; > + > ctrl2 &= ~PCI_DVSEC_CXL_INIT_CXL_RST; > - status2 &= ~PCI_DVSEC_CXL_RST_ERR; > - status2 |= PCI_DVSEC_CXL_RST_DONE; > + > + /* Run the real reset here and report its result in Status2. */ > + vfio_pci_zap_and_down_write_memory_lock(vdev); > + vfio_pci_dma_buf_move(vdev, true); > + ret = vfio_pci_try_reset_function(vdev); > + if (__vfio_pci_memory_enabled(vdev)) > + vfio_pci_dma_buf_move(vdev, false); > + up_write(&vdev->memory_lock); > + > + status2 &= ~(PCI_DVSEC_CXL_RST_DONE | PCI_DVSEC_CXL_RST_ERR); > + status2 |= ret ? PCI_DVSEC_CXL_RST_ERR : PCI_DVSEC_CXL_RST_DONE; > } > > *pctrl2 = cpu_to_le16(ctrl2); > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index 1e0ac28b7564..6676c561465b 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -605,6 +605,62 @@ static void vfio_pci_core_unmap_bars(struct vfio_pci_core_device *vdev) > } > } > > +/** > + * vfio_pci_try_reset_function() - reset the function without blocking > + * @vdev: vfio PCI device, with memory_lock held for write > + * > + * Reset through the PCI core, or let the provider reset a device it serves. > + * The provider resets the device with its PCI device lock held, so take that > + * lock here, without waiting, as pci_try_reset_function() does. > + * > + * Return: 0 on success, -EAGAIN when the device lock is contended, or the > + * reset error. For a provider device a failed pci_save_state() is also > + * returned, and the reset is not attempted: unlike pci_try_reset_function(), > + * the provider's reset needs a valid saved state to restore. > + */ > +int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev) > +{ > + struct pci_dev *pdev = vdev->pdev; > + int ret; > + > + lockdep_assert_held_write(&vdev->memory_lock); > + > + if (!vdev->cxl_ops) > + return pci_try_reset_function(pdev); > + > + if (!pci_dev_trylock(pdev)) > + return -EAGAIN; > + > + /* > + * The provider restores PCI state from the saved copy, so save the > + * current state first, as pci_try_reset_function() does. The locked > + * variant on the close path must not: it resets with the state that > + * vfio_pci_core_disable() has just loaded. > + */ > + ret = pci_save_state(pdev); > + if (!ret) > + ret = vdev->cxl_ops->reset(vdev); > + pci_dev_unlock(pdev); > + > + return ret; > +} > + > +/* > + * As vfio_pci_try_reset_function(), for a caller that holds the device lock. > + * Only the close path calls this. It takes memory_lock with the device lock > + * held, so user config access is blocked meanwhile: the order a sysfs reset > + * uses. That cannot deadlock against a PCI_COMMAND write holding memory_lock > + * because no user remains at close. Please tell the AI not to add any comments, these are difficult to parse and often unnecessary. If there are specific locking requirements for this function, they should be annotated. If this function cannot be annotated, and it not general, even within a static context, maybe it shouldn't be a separate function. > + */ > +static int vfio_pci_reset_function_locked(struct vfio_pci_core_device *vdev) > +{ > + if (!vdev->cxl_ops) > + return __pci_reset_function_locked(vdev->pdev); > + > + guard(rwsem_write)(&vdev->memory_lock); > + return vdev->cxl_ops->reset(vdev); > +} > + > static void vfio_pci_core_free_dev_regions(struct vfio_pci_core_device *vdev) > { > for (int i = 0; i < vdev->num_regions; i++) > @@ -636,11 +692,14 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev) > goto out_power; > > /* If reset fails because of the device lock, fail this path entirely */ > - ret = pci_try_reset_function(pdev); > + down_write(&vdev->memory_lock); > + ret = vfio_pci_try_reset_function(vdev); > + up_write(&vdev->memory_lock); > if (ret == -EAGAIN) > goto out_disable_device; > > - vdev->reset_works = !ret; > + /* A provider checks at bind that it can reset the device. */ > + vdev->reset_works = vdev->cxl_ops || !ret; I suppose, but why is ret not still a valid indicator? > pci_save_state(pdev); > vdev->pci_saved_state = pci_store_saved_state(pdev); > if (!vdev->pci_saved_state) > @@ -846,7 +905,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev) > if (bridge && !pci_dev_trylock(bridge)) > goto out_restore_state; > if (pci_dev_trylock(pdev)) { > - if (!__pci_reset_function_locked(pdev)) > + if (!vfio_pci_reset_function_locked(vdev)) > vdev->needs_reset = false; > pci_dev_unlock(pdev); > } > @@ -1541,7 +1600,7 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev, > vfio_pci_set_power_state(vdev, PCI_D0); > > vfio_pci_dma_buf_move(vdev, true); > - ret = pci_try_reset_function(vdev->pdev); > + ret = vfio_pci_try_reset_function(vdev); > if (__vfio_pci_memory_enabled(vdev)) > vfio_pci_dma_buf_move(vdev, false); > up_write(&vdev->memory_lock); > @@ -2397,6 +2456,10 @@ int vfio_pci_core_register_cxl_ops(const struct vfio_cxl_ops *ops) > { > guard(rwsem_write)(&vfio_pci_cxl_ops_rwsem); > > + /* vfio-pci-core never resets a provider's device by itself. */ What a bizarre comment. Relevance? > + if (!ops->reset) > + return -EINVAL; > + > if (vfio_pci_cxl_ops) > return -EBUSY; > > diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h > index 09a68c99b43c..dbede6e24fa9 100644 > --- a/drivers/vfio/pci/vfio_pci_priv.h > +++ b/drivers/vfio/pci/vfio_pci_priv.h > @@ -80,6 +80,7 @@ int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev, > pci_power_t state); > > void vfio_pci_zap_and_down_write_memory_lock(struct vfio_pci_core_device *vdev); > +int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev); > u16 vfio_pci_memory_lock_and_enable(struct vfio_pci_core_device *vdev); > void vfio_pci_memory_unlock_and_restore(struct vfio_pci_core_device *vdev, > u16 cmd); > diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h > index 991a3fcd84eb..cb9ba92b696a 100644 > --- a/include/linux/vfio_pci_core.h > +++ b/include/linux/vfio_pci_core.h > @@ -87,6 +87,13 @@ struct vfio_cxl_ops { > */ > void (*reset_prepare)(struct vfio_pci_core_device *vdev); > int (*reset_done)(struct vfio_pci_core_device *vdev); > + /* > + * Reset the function in place of the PCI core; mandatory. Called with > + * the PCI device lock and memory_lock (for write) held. The caller > + * provides the saved PCI state the reset restores: the try-reset path > + * saves it first, the close path keeps the open-time copy. > + */ Reads like more AI generated comments. These things like to talk, please curb them. Look at vfio_device_ops for an example how to briefly and concisely document callback ops like a human. Thanks, Alex PS - I'm still working on the remainder of the series. > + int (*reset)(struct vfio_pci_core_device *vdev); > /* Pinned per bound CXL device so vfio-cxl cannot unload under usage */ > struct module *owner; > };