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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AC17DC6FD1F for ; Tue, 14 Mar 2023 16:03:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=416A2pnV2QZZRC+nN4iSISKmkaWQbkm0tyr4tYXmqAM=; b=lphJQGb10KG7zx XhM9ro2rKQ7SHhrzf3jPNafik4dT1Y/xD6MLaQYfb8k+M9LxBVVLoUVlZXtV2BvjXgX6rpWWKjf55 giJDfyw5FAQPkXp6AiUnhFcwuai2CvPGqLDexTwhdILdUzHwIma4S45AubXxPQQFGHu6kHdVQ8f5K 8xOKHfEUdNeCRsL81PGoXPiT42op6HTAp2LqaVpv7EwhVeicH81uf4wFCX9mXiOkaJpS0Utq+x6i9 qAljmncs8g5rhByKZ5CU7TtlHw0bYzOI9KP/cczaVnATs+oqH/tYRt6xzWtSbWQO30K7NRhI7wube u0X9Hl6bqgGoHQb7gm1A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pc76h-00Aidc-2z; Tue, 14 Mar 2023 16:02:35 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pc76e-00Aico-1r for linux-arm-kernel@lists.infradead.org; Tue, 14 Mar 2023 16:02:34 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id DBE94B819C5; Tue, 14 Mar 2023 16:02:30 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 631B0C433EF; Tue, 14 Mar 2023 16:02:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678809749; bh=VpHJn4z9r1jAznNA4sbdVvzJN92KMr3BeQTf/ruLWp4=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=DZ0R3uyRp1gp5r1o8Iy5IYxbR/8br2Ig9E0MGjCvsAiAct9860TK3UT3aTSJ+EW6k Y/bCrVpfQBUc4FBjLM2t2NtLBWUTS/3MAez+XdntXa3yk2bo29yX7AIH7hVkId76/H sEPZqtrC8wdbEOV46dmRBNKaS6/6ghTPZqkrzyyp3yBAO1TiGfeAtqajjzPsY4HYzt +LkUzYVOjZ6pkSExvSLKO8Pumn8hYLECvGTP6SBt7X8bF6zDIk9yOSjShwLhX+CwVs jAFHw6ByDiK7dYnsSyIa/16mN4aG5jJSLFCyIIEPo4Xzp+p75s23JYlQcnZG9HQhPu SyG6d0alVkXPQ== Date: Tue, 14 Mar 2023 11:02:27 -0500 From: Bjorn Helgaas To: Ganapatrao Kulkarni Cc: Sathyanarayanan Kuppuswamy , linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, iommu@lists.linux.dev, joro@8bytes.org, bhelgaas@google.com, robin.murphy@arm.com, will@kernel.org, jean-philippe@linaro.org, darren@os.amperecomputing.com, scott@os.amperecomputing.com Subject: Re: [PATCH v2 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF Message-ID: <20230314160227.GA1645738@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <799c6c17-c448-387c-fea1-b5f1c5045819@os.amperecomputing.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230314_090232_913191_871D12D2 X-CRM114-Status: GOOD ( 47.77 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Mar 14, 2023 at 08:06:07PM +0530, Ganapatrao Kulkarni wrote: > On 14-03-2023 06:22 pm, Sathyanarayanan Kuppuswamy wrote: > > On 3/14/23 3:08 AM, Ganapatrao Kulkarni wrote: > > > On 14-03-2023 04:00 am, Sathyanarayanan Kuppuswamy wrote: > > > > On 3/13/23 2:12 PM, Bjorn Helgaas wrote: > > > > > On Mon, Feb 27, 2023 at 08:21:36PM -0800, Ganapatrao Kulkarni wro= te: > > > > > > As per PCI specification (PCI Express Base Specification > > > > > > Revision 6.0, Section 10.5) both PF and VFs of a PCI EP > > > > > > are permitted to be enabled independently for ATS > > > > > > capability, however the STU(Smallest Translation Unit) is > > > > > > shared between PF and VFs. For VFs, it is hardwired to > > > > > > Zero and the associated PF's value applies to VFs. > > > > > > = > > > > > > In the current code, the STU is being configured while > > > > > > enabling the PF ATS. Hence, it is not able to enable ATS > > > > > > for VFs, if it is not enabled on the associated PF > > > > > > already. > > > > > > = > > > > > > Adding a function pci_ats_stu_configure(), which can be > > > > > > called to configure the STU during PF enumeration. Latter > > > > > > enumerations of VFs can successfully enable ATS > > > > > > independently. > > > > > > @@ -46,6 +46,35 @@ bool pci_ats_supported(struct pci_dev *dev) > > > > > > =A0 } > > > > > > =A0 EXPORT_SYMBOL_GPL(pci_ats_supported); > > > > > > =A0 +/** > > > > > > + * pci_ats_stu_configure - Configure STU of a PF. > > > > > > + * @dev: the PCI device > > > > > > + * @ps: the IOMMU page shift > > > > > > + * > > > > > > + * Returns 0 on success, or negative on failure. > > > > > > + */ > > > > > > +int pci_ats_stu_configure(struct pci_dev *dev, int ps) > > > > > > +{ > > > > > > +=A0=A0=A0 u16 ctrl; > > > > > > + > > > > > > +=A0=A0=A0 if (dev->ats_enabled || dev->is_virtfn) > > > > > > +=A0=A0=A0=A0=A0=A0=A0 return 0; > > > > > = > > > > > I might return an error for the VF case on the assumption > > > > > that it's likely an error in the caller.=A0 I guess one could > > > > > argue that it simplifies the caller if it doesn't have to > > > > > check for PF vs VF.=A0 But the fact that STU is shared between > > > > > PF and VFs is an important part of understanding how ATS > > > > > works, so the caller should be aware of the distinction > > > > > anyway. > > > > = > > > > I have already asked this question. But let me repeat it. > > > > = > > > > We don't have any checks for the PF case here. That means you > > > > can re-configure the STU as many times as you want until ATS > > > > is enabled in PF. So, if there are active VFs which uses this > > > > STU, can PF re-configure the STU at will? > > > = > > > IMO, Since STU is shared, programming it multiple times is not expect= ed from callers code do it, however we can add below check to allow to prog= ram STU once from a PF. > > > = > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c > > > index 1611bfa1d5da..f7bb01068e18 100644 > > > --- a/drivers/pci/ats.c > > > +++ b/drivers/pci/ats.c > > > @@ -60,6 +60,10 @@ int pci_ats_stu_configure(struct pci_dev *dev, int= ps) > > > =A0=A0=A0=A0=A0=A0=A0 if (dev->ats_enabled || dev->is_virtfn) > > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return 0; > > > = > > > +=A0=A0=A0=A0=A0=A0 /* Configured already */ > > > +=A0=A0=A0=A0=A0=A0 if (dev->ats_stu) > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return 0; > > = > > Theoretically, you can re-configure STU as long as no one is using > > it. Instead of this check, is there a way to check whether there > > are active VMs which enables ATS? > = > Yes I agree, there is no limitation on how many times you write STU > bits, but practically it is happening while PF is enumerated. > = > The usage of function pci_ats_stu_configure is almost > similar(subset) to pci_enable_ats and only difference is one does > ATS enable + STU program and another does only STU program. What would you think of removing the STU update feature from pci_enable_ats() so it always fails if pci_ats_stu_configure() has not been called, even when called on the PF, e.g., if (ps !=3D pci_physfn(dev)->ats_stu) return -EINVAL; pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, &ctrl); ctrl |=3D PCI_ATS_CTRL_ENABLE; pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl); Would probably also have to set "dev->ats_stu =3D 0" in pci_disable_ats() to allow the possibility of calling pci_ats_stu_configure() again. > IMO, I dont think, there is any need to find how many active VMs > with attached VFs and it is not done for pci_enable_ats as well. Enabling or disabling ATS in a PF or VF has no effect on other functions. But changing STU while a VF has ATS enabled would definitely break any user of that VF, so if it's practical to verify that no VFs have ATS enabled, I think we should. > Also the caller has the requirement to call either > pci_ats_stu_configure or pci_enable_ats while enumerating the PF. > > > > =A0=A0=A0=A0=A0=A0=A0 if (!pci_ats_supported(dev)) > > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > > = > > > > > > + > > > > > > +=A0=A0=A0 if (!pci_ats_supported(dev)) > > > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > > > + > > > > > > +=A0=A0=A0 if (ps < PCI_ATS_MIN_STU) > > > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > > > + > > > > > > +=A0=A0=A0 dev->ats_stu =3D ps; > > > > > > +=A0=A0=A0 pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CTR= L, &ctrl); > > > > > > +=A0=A0=A0 ctrl |=3D PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MI= N_STU); > > > > > > +=A0=A0=A0 pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CT= RL, ctrl); > > > > > > + > > > > > > +=A0=A0=A0 return 0; > > > > > > +} > > > > > > +EXPORT_SYMBOL_GPL(pci_ats_stu_configure); > > > > > > + > > > > > > =A0 /** > > > > > > =A0=A0 * pci_enable_ats - enable the ATS capability > > > > > > =A0=A0 * @dev: the PCI device > > > > > > @@ -68,8 +97,8 @@ int pci_enable_ats(struct pci_dev *dev, int p= s) > > > > > > =A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > > > =A0 =A0=A0=A0=A0=A0 /* > > > > > > -=A0=A0=A0=A0 * Note that enabling ATS on a VF fails unless it'= s already enabled > > > > > > -=A0=A0=A0=A0 * with the same STU on the PF. > > > > > > +=A0=A0=A0=A0 * Note that enabling ATS on a VF fails unless it'= s already > > > > > > +=A0=A0=A0=A0 * configured with the same STU on the PF. > > > > > > =A0=A0=A0=A0=A0=A0 */ > > > > > > =A0=A0=A0=A0=A0 ctrl =3D PCI_ATS_CTRL_ENABLE; > > > > > > =A0=A0=A0=A0=A0 if (dev->is_virtfn) { > > > > > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h > > > > > > index df54cd5b15db..7d62a92aaf23 100644 > > > > > > --- a/include/linux/pci-ats.h > > > > > > +++ b/include/linux/pci-ats.h > > > > > > @@ -8,6 +8,7 @@ > > > > > > =A0 /* Address Translation Service */ > > > > > > =A0 bool pci_ats_supported(struct pci_dev *dev); > > > > > > =A0 int pci_enable_ats(struct pci_dev *dev, int ps); > > > > > > +int pci_ats_stu_configure(struct pci_dev *dev, int ps); > > > > > > =A0 void pci_disable_ats(struct pci_dev *dev); > > > > > > =A0 int pci_ats_queue_depth(struct pci_dev *dev); > > > > > > =A0 int pci_ats_page_aligned(struct pci_dev *dev); > > > > > > @@ -16,6 +17,8 @@ static inline bool pci_ats_supported(struct p= ci_dev *d) > > > > > > =A0 { return false; } > > > > > > =A0 static inline int pci_enable_ats(struct pci_dev *d, int ps) > > > > > > =A0 { return -ENODEV; } > > > > > > +static inline int pci_ats_stu_configure(struct pci_dev *d, int= ps) > > > > > > +{ return -ENODEV; } > > > > > > =A0 static inline void pci_disable_ats(struct pci_dev *d) { } > > > > > > =A0 static inline int pci_ats_queue_depth(struct pci_dev *d) > > > > > > =A0 { return -ENODEV; } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel