Message ID | 20221117114122.1588338-1-marmarek@invisiblethingslab.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp352137wrr; Thu, 17 Nov 2022 03:44:11 -0800 (PST) X-Google-Smtp-Source: AA0mqf4m2te/8NXFU6NFFlzYn4BsvMVTz8h0tK3VIQAN0sExl9uu8Ov5sVU9eskG6+p4ebuTuMHz X-Received: by 2002:a17:906:f02:b0:799:a731:b44d with SMTP id z2-20020a1709060f0200b00799a731b44dmr1822891eji.405.1668685451062; Thu, 17 Nov 2022 03:44:11 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668685451; cv=none; d=google.com; s=arc-20160816; b=dil04Aws4oNW029XysHSE2yg1JRxxmr+i6m0QRGNHcW7O8qp2fEjPbUI4nSk6l6ILr 5ukPfFNQKyjfpnmB5zZFPC4Y1fSOPL+Vn7Zv78jrp3zmF2w4s5LoZByeaiCv49Gr03vV wGU1OAVZ4HHEo6xX762ztXsqwZ/Jz0v4mvVV4lZRvFRqy4MWOvPGMnOD1SeYujitWcsO YEj4am7GEiD5eZLTYoy4wHNjMeM6+dOjq4qpSontrFRhxNiCG3/qvBlkpq7+eIRQ3l3w xVRJHc0jjyCrwkR/8itFdogLl0ngYDelCADso93cEpwYl65QJ2mLwzkdVCl54FE+xSyf BdlQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:feedback-id:dkim-signature :dkim-signature; bh=5Hp8B5ibzq9PE++sS2FmCLXLWnvRx0WKWkGDICvery8=; b=xVavug/wujmOEkpnn6KbE+/7ZEf8t7I/hRvGj086oio8FHP/FEMDK1s8jmHJ4rn0pM 23KTD2lPnK8QtE4g0d2froVLodyaWyso5Ui2Xhol4IhyaE6osZRRrqlxntrbH1uaQ/ve qSCd+UQw5G5IMq8g4CwOcIQBp0QSSbthJI6vx5fSJUqGO6tzdiLlSKmpppAlYMcY7Uph H+sQrn+VxBWUPj4SgmJv8g16kQoiB9vEEo4g4ZfOopizjOS/aK30/kjsx3madf7jaJBD U4sgXpgtIYr92wKnQ8ui3E6jyn81Ywzoj/rYG6mNQW/UguUh+tmCdtptNatHB0Rcla+6 p2pA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@invisiblethingslab.com header.s=fm1 header.b=XLW2Y5Ll; dkim=pass header.i=@messagingengine.com header.s=fm1 header.b=t86OowYV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id br13-20020a170906d14d00b0078d450cbb02si373800ejb.452.2022.11.17.03.43.47; Thu, 17 Nov 2022 03:44:11 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@invisiblethingslab.com header.s=fm1 header.b=XLW2Y5Ll; dkim=pass header.i=@messagingengine.com header.s=fm1 header.b=t86OowYV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239843AbiKQLmF (ORCPT <rfc822;a1648639935@gmail.com> + 99 others); Thu, 17 Nov 2022 06:42:05 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48586 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239506AbiKQLlr (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 17 Nov 2022 06:41:47 -0500 Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D6EAB4B981 for <linux-kernel@vger.kernel.org>; Thu, 17 Nov 2022 03:41:45 -0800 (PST) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id A48665C018F; Thu, 17 Nov 2022 06:41:42 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Thu, 17 Nov 2022 06:41:42 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= invisiblethingslab.com; h=cc:cc:content-transfer-encoding :content-type:date:date:from:from:in-reply-to:message-id :mime-version:reply-to:sender:subject:subject:to:to; s=fm1; t= 1668685302; x=1668771702; bh=5Hp8B5ibzq9PE++sS2FmCLXLWnvRx0WKWkG DICvery8=; b=XLW2Y5LlGasq5/yW0AGCXIhwRQR01dr8Gg1ST7fCFUblQ/xp/Gs jGNz+xLMRbE4DtNzzfAGroV8+fgobWBJcjz8GNR4XpiaRtNuVjfHFta1iYUCwTtX N5m1fyR+LAhTZwh4llPsC3tFMR1xHYZ6Zk4IWxyB/6lNS0nRdSdKDJvpZBI2cq2R g6WbNhn+HCHP4x38xNOy3h3iQHYhjlRL72TBFjYgj/+m485nRdeShySBHIa2Qmjx GHhUFup7JtwPergDM38yApBR/QLtikhd8drBPuZ/GqILARp2HZ4Bxr3+t8Q60FRO HiEIKSE/uy/Gaa95atWIs7ltOID2r3xuiGg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:message-id:mime-version:reply-to:sender:subject :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; t=1668685302; x=1668771702; bh=5Hp8B5ibzq9PE ++sS2FmCLXLWnvRx0WKWkGDICvery8=; b=t86OowYVgnKExhpiOft5nF5yLOuXL Ky7yp6hRfOzqzZ69nls2SqCmFZflynB5w3puHMW4ZO7c7h2HcoeuIjf/wO+f+c07 hAzwacA3IKTp92kFEjbBSD74oXHop3J2PRFxK6vaz5ZJIhISi9tN8knGiauvKOPz RIs0EfWtvMBcxcdIQY7DY3A0g9DHLC2jwKxvOF8gMxbnVsmZwGpHQ8ZB0k/dJe8/ +w+3wrb2Jaa7Dg+uWICnNe0Bc8zVyE2GFBWauAkI0u1b6pvOifyvOpf0kOJ+ruLw pFNxgzUOZDQvOJT8fOGLIS6eh8ilqNwD2QjF0k4vk1Khjykc4KVHQjDKg== X-ME-Sender: <xms:9h12Y67c1Bp-yr1wFD_ab-4uxftuJjg-e_E7Ywzriv9K9VTs1pC34Q> <xme:9h12Yz5mREd-IBtTCDO6X79QQi1UTqQHuMA-nGlhW8e7IsD2KxPMaM7GN5GNaVlUD 0sXh3YblbvIVA> X-ME-Received: <xmr:9h12Y5eUg8hrNiuaFHe3THCZTR3WzBI3lsX4XT90D8s6mERQDoCwCDOoAYRorRXIf53kKMxDEaazMUoPYYXJnm1ZbihXnVecT5WU_oXCnm9JQG__0ic> X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvgedrgeekgddvlecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvfevufffkffogggtgfesthekredtredtjeenucfhrhhomhepofgrrhgvkhcu ofgrrhgtiiihkhhofihskhhiqdfikphrvggtkhhiuceomhgrrhhmrghrvghksehinhhvih hsihgslhgvthhhihhnghhslhgrsgdrtghomheqnecuggftrfgrthhtvghrnhepleekhfdu leetleelleetteevfeefteffkeetteejheelgfegkeelgeehhfdthedvnecuvehluhhsth gvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgrrhhmrghrvghksehi nhhvihhsihgslhgvthhhihhnghhslhgrsgdrtghomh X-ME-Proxy: <xmx:9h12Y3JanckpXKof0E2oF7b_jbBV7FV8WbRlxnidAAWpU0ULVflyVQ> <xmx:9h12Y-KynbBK7QdiJ-0yTIETDLxyyYdm918F7SpCwa3esIjOa6gDgA> <xmx:9h12Y4zSGkfReIjmlZdoiCIlmE_WkWQeE5S2nvV4srstBmOSnWvO3A> <xmx:9h12YzGQ9Kpb6lK_Ba0OZ87dJ2dTb2n-IDavDoph4z7PraX5hFulJQ> Feedback-ID: i1568416f:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 17 Nov 2022 06:41:41 -0500 (EST) From: =?utf-8?q?Marek_Marczykowski-G=C3=B3recki?= <marmarek@invisiblethingslab.com> To: linux-kernel@vger.kernel.org Cc: =?utf-8?q?Marek_Marczykowski-G=C3=B3recki?= <marmarek@invisiblethingslab.com>, Juergen Gross <jgross@suse.com>, Stefano Stabellini <sstabellini@kernel.org>, Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>, Jan Beulich <jbeulich@suse.com>, xen-devel@lists.xenproject.org (moderated list:XEN HYPERVISOR INTERFACE) Subject: [PATCH] xen-pciback: Consider MSI-X enabled only when MASKALL bit is cleared Date: Thu, 17 Nov 2022 12:41:17 +0100 Message-Id: <20221117114122.1588338-1-marmarek@invisiblethingslab.com> X-Mailer: git-send-email 2.37.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749743515340367423?= X-GMAIL-MSGID: =?utf-8?q?1749743515340367423?= |
Series |
xen-pciback: Consider MSI-X enabled only when MASKALL bit is cleared
|
|
Commit Message
Marek Marczykowski-Górecki
Nov. 17, 2022, 11:41 a.m. UTC
Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until
the table is filled. Then it disables INTx just before clearing MASKALL
bit. Currently this approach is rejected by xen-pciback.
Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long
as PCI_MSIX_FLAGS_MASKALL is set too.
Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too")
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
drivers/xen/xen-pciback/conf_space.c | 2 +-
drivers/xen/xen-pciback/conf_space_capability.c | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
Comments
On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: > Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until > the table is filled. Then it disables INTx just before clearing MASKALL > bit. Currently this approach is rejected by xen-pciback. > Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long > as PCI_MSIX_FLAGS_MASKALL is set too. The use of MSI-X interrupts is conditional on only the MSI-X Enable bit. Setting MSI-X Enable effectively overrides the Interrupt Disable bit in the Command register. PCIe 6.0.1 section 7.7.2.2. "MSI-X Enable ... is prohibited from using INTx interrupts (if implemented)." And there is similar wording for MSI Enable. I think you need to shuffle the checks for MSI/MSI-X in xen_pcibk_get_interrupt_type() so they are _before_ the check of the Interrupt Disable bit in the Command register. David
On Thu, Nov 17, 2022 at 12:54:51PM +0000, David Vrabel wrote: > On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: > > Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until > > the table is filled. Then it disables INTx just before clearing MASKALL > > bit. Currently this approach is rejected by xen-pciback. > > Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long > > as PCI_MSIX_FLAGS_MASKALL is set too. > > The use of MSI-X interrupts is conditional on only the MSI-X Enable bit. > Setting MSI-X Enable effectively overrides the Interrupt Disable bit in the > Command register. That means the second chunk of the patch may even drop the '(new_value & PCI_MSIX_FLAGS_MASKALL)' part, right? > PCIe 6.0.1 section 7.7.2.2. "MSI-X Enable ... is prohibited from using INTx > interrupts (if implemented)." And there is similar wording for MSI Enable. And this would mean the 'field_config->int_type == INTERRUPT_TYPE_MSIX' part isn't necessary either. Jan in another thread pointed out that disabling INTx explicitly is still a useful workaround for a flawed hardware. But if that isn't mandated by the spec, maybe it doesn't need to be enforced by pciback either? > I think you need to shuffle the checks for MSI/MSI-X in > xen_pcibk_get_interrupt_type() so they are _before_ the check of the > Interrupt Disable bit in the Command register. Note the xen_pcibk_get_interrupt_type() returns a bitmask of enabled types. It seems it should consider INTx only if both MSI and MSI-X are disabled. I'll make the adjustment. But this alone, won't cover enabling part, as INTx is the only one active at the time.
On 17.11.2022 14:13, Marek Marczykowski-Górecki wrote: > On Thu, Nov 17, 2022 at 12:54:51PM +0000, David Vrabel wrote: >> On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: >>> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until >>> the table is filled. Then it disables INTx just before clearing MASKALL >>> bit. Currently this approach is rejected by xen-pciback. >>> Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long >>> as PCI_MSIX_FLAGS_MASKALL is set too. >> >> The use of MSI-X interrupts is conditional on only the MSI-X Enable bit. >> Setting MSI-X Enable effectively overrides the Interrupt Disable bit in the >> Command register. > > That means the second chunk of the patch may even drop the '(new_value & > PCI_MSIX_FLAGS_MASKALL)' part, right? > >> PCIe 6.0.1 section 7.7.2.2. "MSI-X Enable ... is prohibited from using INTx >> interrupts (if implemented)." And there is similar wording for MSI Enable. > > And this would mean the 'field_config->int_type == INTERRUPT_TYPE_MSIX' > part isn't necessary either. > > Jan in another thread pointed out that disabling INTx explicitly is > still a useful workaround for a flawed hardware. But if that isn't > mandated by the spec, maybe it doesn't need to be enforced by pciback > either? Well, allowing a device to go into a mode exhibiting undefined behavior is what we ought to prevent when it comes to a DomU doing so vs overall host safety. Jan
On 17.11.2022 13:28, Andrew Cooper wrote: > On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: >> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until >> the table is filled. Then it disables INTx just before clearing MASKALL >> bit. Currently this approach is rejected by xen-pciback. >> Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long >> as PCI_MSIX_FLAGS_MASKALL is set too. >> >> Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") >> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > The PCI spec states that devices are not permitted to use INTx when MSI > or MSI-X is enabled. The mask status has no legitimate bearing on irq type. > > INTx_DISABLE exists as a bodge to mean "INTx not permitted even when > neither MSI nor MSI-X are enabled", and exists because in some case, > transiently disabling MSI is the only safe way to update the descriptor. > > > I can believe that this change fixes a an issue, but the logic surely > cannot be correct overall. Question then is - what can we do without altering the sequence of steps Linux (and likely other OSes) take? Imo Marek's proposal is the least bad option, because everything else would be more intrusive or wouldn't take effect for existing released kernel versions running in guests. Jan
On 17.11.2022 12:41, Marek Marczykowski-Górecki wrote: > --- a/drivers/xen/xen-pciback/conf_space.c > +++ b/drivers/xen/xen-pciback/conf_space.c > @@ -313,7 +313,7 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) > &val); > if (err) > return err; > - if (val & PCI_MSIX_FLAGS_ENABLE) > + if (val & PCI_MSIX_FLAGS_ENABLE && !(val & PCI_MSIX_FLAGS_MASKALL)) > ret |= INTERRUPT_TYPE_MSIX; > } > return ret ?: INTERRUPT_TYPE_NONE; Considering Andrew's reply, maybe it wasn't a good suggestion to change the code here. If, however, you/we decide to keep the change, then please add another pair of parentheses around the operands of the left hand &. If the change was to be dropped again, I think ... > --- a/drivers/xen/xen-pciback/conf_space_capability.c > +++ b/drivers/xen/xen-pciback/conf_space_capability.c > @@ -242,6 +242,10 @@ static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value, > if (int_type == INTERRUPT_TYPE_NONE || > int_type == field_config->int_type) > goto write; > + if (int_type == INTERRUPT_TYPE_INTX && ... this would need extending to also cover the INTX|MSIX case (i.e. if a second such write made it here when the enable+maskall bits are already set). Jan > + field_config->int_type == INTERRUPT_TYPE_MSIX && > + (new_value & PCI_MSIX_FLAGS_MASKALL)) > + goto write; > return PCIBIOS_SET_FAILED; > } >
On Thu, Nov 17, 2022 at 02:33:16PM +0100, Jan Beulich wrote: > On 17.11.2022 14:13, Marek Marczykowski-Górecki wrote: > > On Thu, Nov 17, 2022 at 12:54:51PM +0000, David Vrabel wrote: > >> On 17/11/2022 11:41, Marek Marczykowski-Górecki wrote: > >>> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until > >>> the table is filled. Then it disables INTx just before clearing MASKALL > >>> bit. Currently this approach is rejected by xen-pciback. > >>> Allow setting PCI_MSIX_FLAGS_ENABLE while INTx is still enabled as long > >>> as PCI_MSIX_FLAGS_MASKALL is set too. > >> > >> The use of MSI-X interrupts is conditional on only the MSI-X Enable bit. > >> Setting MSI-X Enable effectively overrides the Interrupt Disable bit in the > >> Command register. > > > > That means the second chunk of the patch may even drop the '(new_value & > > PCI_MSIX_FLAGS_MASKALL)' part, right? > > > >> PCIe 6.0.1 section 7.7.2.2. "MSI-X Enable ... is prohibited from using INTx > >> interrupts (if implemented)." And there is similar wording for MSI Enable. > > > > And this would mean the 'field_config->int_type == INTERRUPT_TYPE_MSIX' > > part isn't necessary either. > > > > Jan in another thread pointed out that disabling INTx explicitly is > > still a useful workaround for a flawed hardware. But if that isn't > > mandated by the spec, maybe it doesn't need to be enforced by pciback > > either? > > Well, allowing a device to go into a mode exhibiting undefined behavior > is what we ought to prevent when it comes to a DomU doing so vs overall > host safety. If the spec prohibits using INTx if MSI/MSI-X is enabled (regardless of PCI_COMMAND_INTX_DISABLE bit), then well-behaving device should be fine (we aren't hitting undefined behavior). As for buggy device, it wouldn't be much different from a device ignoring PCI_COMMAND_INTX_DISABLE completely, no (besides the latter being probably much less probable bug)? If the above is assumption is correct, it seems such device may not function correctly without extra workarounds (which are in the driver interest to apply), but should not affect overall host safety (as in: beyond the guest having that device assigned). I think pciback should only enforce what's necessary to prevent one guest hurting others (or the hypervisor), but it doesn't need to prevent guest hurting itself.
diff --git a/drivers/xen/xen-pciback/conf_space.c b/drivers/xen/xen-pciback/conf_space.c index 059de92aea7d..e8923bffc175 100644 --- a/drivers/xen/xen-pciback/conf_space.c +++ b/drivers/xen/xen-pciback/conf_space.c @@ -313,7 +313,7 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) &val); if (err) return err; - if (val & PCI_MSIX_FLAGS_ENABLE) + if (val & PCI_MSIX_FLAGS_ENABLE && !(val & PCI_MSIX_FLAGS_MASKALL)) ret |= INTERRUPT_TYPE_MSIX; } return ret ?: INTERRUPT_TYPE_NONE; diff --git a/drivers/xen/xen-pciback/conf_space_capability.c b/drivers/xen/xen-pciback/conf_space_capability.c index 097316a74126..5c851f916ebc 100644 --- a/drivers/xen/xen-pciback/conf_space_capability.c +++ b/drivers/xen/xen-pciback/conf_space_capability.c @@ -242,6 +242,10 @@ static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value, if (int_type == INTERRUPT_TYPE_NONE || int_type == field_config->int_type) goto write; + if (int_type == INTERRUPT_TYPE_INTX && + field_config->int_type == INTERRUPT_TYPE_MSIX && + (new_value & PCI_MSIX_FLAGS_MASKALL)) + goto write; return PCIBIOS_SET_FAILED; }