Message ID | 20221118154931.1928298-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 q4csp276946wrr; Fri, 18 Nov 2022 08:04:51 -0800 (PST) X-Google-Smtp-Source: AA0mqf6Yno+1PnXJxXRofTxPpzLt8GloJZqPRelEZFbPHqhGIrDF7RMmI9hpZ5COrfGmeir64bNo X-Received: by 2002:a17:90a:d581:b0:212:b43b:ffe5 with SMTP id v1-20020a17090ad58100b00212b43bffe5mr8596555pju.32.1668787490678; Fri, 18 Nov 2022 08:04:50 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668787490; cv=none; d=google.com; s=arc-20160816; b=yJRSB8TX3O3ThTxZ2DSuCnsdq6W51l5nc6/DpZqKkWo206W7T4teE5MtCWAq5+1SgJ mOEyaVOFmhqhX9g29w4QTO/Y553sKQj+zPDPqbRnfTCZ7p4OeSMEiiTWJ/OxpsxJwNkW XKY8bZ0p7/TU9iZOmtTxlfDaR5C+Tge3xu0YNjw2YnPpDPmyy9aXayO1mwLhVz21r0gZ XWuSVI4Bt8S58Glc5lPZvO9VVAohOyN+rs8fvvLVX8GVLfngay7Bsxr78bhDV8bsET0P 42cwUCLtDnziDOk/sMhMw+U67XyVVMaOAaDZmJpI2iJKtLxsc5aJJSYLLqnC83Eo8+k3 Fi7w== 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=KqQEuKxfkbRxTU5nDAZWsBYliBRDq3obTAXNYs/tvIE=; b=gfYvgiJ3w3WW0uLPdhRu8w+WzGAoK93AHQaHJPZaSdwr2A2cXUS2kpAJtQePh5Lk+j TgOK1c3CG+ueFGSKVVxeDm7RLPgD0SmKEPlWGihdo9NvP/uF35M10R06TEQsRJx7SHTd LjIbWm6iegsnm/+SGOxAqwbYnAzZMvlYkZvSf/b+1FBtmlPNOCbO5PMPsoln5FUncNEP sk8pucoRJTMHpxQTPZJRXJJNnG8b1nxV3zCXutodFBeDD8gTImiR7f1gJFC6qfWXOkIV +LRY2Y4NkFMJ8uBNXoItX+aMICyFBBTnY1ZCYcDxBFOCl+odlJfzLESbjUry5TE1Dxwo mZ+g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@invisiblethingslab.com header.s=fm1 header.b=LHwyay5C; dkim=pass header.i=@messagingengine.com header.s=fm1 header.b=lYaJGjVJ; 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 nv2-20020a17090b1b4200b00212e24a499csi4435412pjb.26.2022.11.18.08.04.23; Fri, 18 Nov 2022 08:04:50 -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=LHwyay5C; dkim=pass header.i=@messagingengine.com header.s=fm1 header.b=lYaJGjVJ; 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 S242126AbiKRPtz (ORCPT <rfc822;kkmonlee@gmail.com> + 99 others); Fri, 18 Nov 2022 10:49:55 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58132 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234455AbiKRPtu (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 18 Nov 2022 10:49:50 -0500 Received: from wout2-smtp.messagingengine.com (wout2-smtp.messagingengine.com [64.147.123.25]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B3F098B100 for <linux-kernel@vger.kernel.org>; Fri, 18 Nov 2022 07:49:49 -0800 (PST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.west.internal (Postfix) with ESMTP id 96280320029B; Fri, 18 Nov 2022 10:49:46 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Fri, 18 Nov 2022 10:49:47 -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= 1668786586; x=1668872986; bh=KqQEuKxfkbRxTU5nDAZWsBYliBRDq3obTAX NYs/tvIE=; b=LHwyay5CtX5MpoR1iLOr8PAfRyE37G7z4XYVAbrRIz/WZCIn+av 5/sb3jQ1GeRJCdrrEV+LYGsuzghXdL2mgSL4HE+w6deMaPZES3L96l7GlPN6Qa/F 3i2X84FaMkaqd8uJRPosVDOsmpl5Pq6scuPW3M8dRp9LujqbpbbSZYJZH6hsFrKa xQzWB9gi7aUbWQppWD4f1t1zp6YFMBUtqFJ1lmJdNEIo0/HSq7O51sB62pJCxHkm oZEq0TDSUN/p/NYXHJ2l2fI7c6jbmRfmqke20ho4CnaDDdrDx8OuPDJ79T8EX2vy I/k4V8i7soF8+CgSZ5QzuI0PHvNCbKf2+yA== 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=1668786586; x=1668872986; bh=KqQEuKxfkbRxT U5nDAZWsBYliBRDq3obTAXNYs/tvIE=; b=lYaJGjVJkv/Wx0CHAk/JR4cjQZOi8 ouePFaQuyM0eTxEQjeiCLnUtNWgYs5+tzUfaWbpZwTJy/D/Z9LlaGQgpgUhqGWzP c+4D5zANcspwHF1gf7yR8XN5UUWe/XlEeQUDkFseLC8XmaWBcHRYui660Xs/Ydy1 IE2JegSQWUOV7ZEuj+0Nf+pk8ItWH24ZVCPgPt1r5cHg/x7QAuMNbMfEq8XToY7d 45S9WBa06KZPxdkokEmhOizI+g3xqr+R7DXTA3kS+3gnLCCR4wOWrM78FBfbcbMY YXtn+wkFDsvgUEiZrTd06LeDj4tQw78rR+OkiV1R/B/yKlKLFQ6H4lHiA== X-ME-Sender: <xms:mal3Y5h2s0SAIu7AYYR2Ljk8Nkk2qVK81MU6DjG8Z10MIzcLDJ38Qw> <xme:mal3Y-AqDOmZq-EummTFHmZUN-j2czr8RkVS8cNCrcpRhCrcpb4cQWVGm-xMDlXdw _CgxYO1JQJN-g> X-ME-Received: <xmr:mal3Y5HZk-bKV_aZEyMIlTTD_S4a71xmStx9L1PvfkdsJZ6epff9MDZ3DWozS2MgoyQkpPe7PjpJ54uLLx53fKTcBOAdTQCqlHUvuqFNFS1uAgUPY_U> X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvgedrhedtgdekvdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvfevufffkffogggtgfesthekredtredtjeenucfhrhhomhepofgrrhgvkhcu ofgrrhgtiiihkhhofihskhhiqdfikphrvggtkhhiuceomhgrrhhmrghrvghksehinhhvih hsihgslhgvthhhihhnghhslhgrsgdrtghomheqnecuggftrfgrthhtvghrnhepleekhfdu leetleelleetteevfeefteffkeetteejheelgfegkeelgeehhfdthedvnecuvehluhhsth gvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgrrhhmrghrvghksehi nhhvihhsihgslhgvthhhihhnghhslhgrsgdrtghomh X-ME-Proxy: <xmx:mal3Y-RU-5Yzmk9KfZNVmM49J_cfXADtNHxJixdzSpRAeymdh_OogA> <xmx:mal3Y2zBu00d8jf_FVavc88jqhftd4AC3rTkakG8MuIk9iHi6w9ufQ> <xmx:mal3Y07_LkErtCrLmbUa5RYlbBhM4iwoZma1vWTbegV9A16dhDc1Vw> <xmx:mql3YwvkzGryKrWF4yHBO2gNhLPlf29KzNX5F4PqVEJte4XkKEcSxw> Feedback-ID: i1568416f:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 18 Nov 2022 10:49:44 -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 v3] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled Date: Fri, 18 Nov 2022 16:49:23 +0100 Message-Id: <20221118154931.1928298-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_H2,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?1749850511751236812?= X-GMAIL-MSGID: =?utf-8?q?1749850511751236812?= |
Series |
[v3] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled
|
|
Commit Message
Marek Marczykowski-Górecki
Nov. 18, 2022, 3:49 p.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.
According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
enabled (in other words: enabling MSI/MSI-X implicitly disables INTx).
Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This
applies to three places:
- checking currently enabled interrupts type,
- transition to MSI/MSI-X - where INTx would be implicitly disabled,
- clearing INTx disable bit - which can be allowed even if MSI/MSI-X is
enabled, as device should consider INTx disabled anyway in that case
Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too")
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
- allow clearing INTx regardless of MSI/MSI-X state, to be consistent
with enabling MSI/MSI-X
Changes in v2:
- restructure the patch to consider not only MASKALL bit, but enabling
MSI/MSI-X generally, without explicitly disabling INTx first
---
drivers/xen/xen-pciback/conf_space.c | 19 +++++++++++------
.../xen/xen-pciback/conf_space_capability.c | 3 ++-
drivers/xen/xen-pciback/conf_space_header.c | 21 +++----------------
3 files changed, 18 insertions(+), 25 deletions(-)
Comments
On Fri, Nov 18, 2022 at 10:50 AM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 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. > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is > enabled (in other words: enabling MSI/MSI-X implicitly disables INTx). > > Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This > applies to three places: > - checking currently enabled interrupts type, > - transition to MSI/MSI-X - where INTx would be implicitly disabled, > - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is > enabled, as device should consider INTx disabled anyway in that case > > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > Changes in v3: > - allow clearing INTx regardless of MSI/MSI-X state, to be consistent > with enabling MSI/MSI-X > Changes in v2: > - restructure the patch to consider not only MASKALL bit, but enabling > MSI/MSI-X generally, without explicitly disabling INTx first > --- I was trying to test your xen-pciback v3 patch, and I am having assignment fail consistently now. It is actually failing to quarantine to domIO in the first place, which matches the failure from the other day (when I more carefully read through the logs). It now consistently fails to quarantine on every boot unlike the other day where it happened once. I added some printks and it 's getting -EBUSY from pdev_msix_assign() which means pci_reset_msix_state() is failing: if ( pci_conf_read16(pdev->sbdf, msix_control_reg(pos)) & PCI_MSIX_FLAGS_MASKALL ) return -EBUSY; # lspci -vv -s 14.3 ... Capabilities: [80] MSI-X: Enable- Count=16 Masked+ Vector table: BAR=0 offset=00002000 PBA: BAR=0 offset=00003000 So it looks like MASKALL is set and prevents assignment. setpci -s 00:14.3 82.W=f cleared that out for me and I could assign the device. My dom0 boots, it runs flask-label-pci for a set of PCI devices (including iwlwifi), then xl pci-assignable-add for all PCI devices which will be passed through, then a little later it boots the associated domains. Dom0 does not have a driver for iwlwifi. I'll have to investigate more to see how MASKALL is getting set. This had not been an issue before your recent patches. Regards, Jason
On Fri, Nov 18, 2022 at 03:46:47PM -0500, Jason Andryuk wrote: > On Fri, Nov 18, 2022 at 10:50 AM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> 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. > > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is > > enabled (in other words: enabling MSI/MSI-X implicitly disables INTx). > > > > Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This > > applies to three places: > > - checking currently enabled interrupts type, > > - transition to MSI/MSI-X - where INTx would be implicitly disabled, > > - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is > > enabled, as device should consider INTx disabled anyway in that case > > > > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > --- > > Changes in v3: > > - allow clearing INTx regardless of MSI/MSI-X state, to be consistent > > with enabling MSI/MSI-X > > Changes in v2: > > - restructure the patch to consider not only MASKALL bit, but enabling > > MSI/MSI-X generally, without explicitly disabling INTx first > > --- > > I was trying to test your xen-pciback v3 patch, and I am having > assignment fail consistently now. It is actually failing to > quarantine to domIO in the first place, which matches the failure from > the other day (when I more carefully read through the logs). It now > consistently fails to quarantine on every boot unlike the other day > where it happened once. Does this include the very first assignment too, or only after domain reboot? If the latter, maybe some cleanup missed clearing MASKALL? FWIW, the patch applied to Qubes (https://github.com/QubesOS/qubes-linux-kernel/pull/680) seems to work fine (the full test run is still in progress, but I see some green marks already). > I added some printks and it 's getting -EBUSY from pdev_msix_assign() > which means pci_reset_msix_state() is failing: > if ( pci_conf_read16(pdev->sbdf, msix_control_reg(pos)) & > PCI_MSIX_FLAGS_MASKALL ) > return -EBUSY; > > # lspci -vv -s 14.3 > ... > Capabilities: [80] MSI-X: Enable- Count=16 Masked+ > Vector table: BAR=0 offset=00002000 > PBA: BAR=0 offset=00003000 > > So it looks like MASKALL is set and prevents assignment. > > setpci -s 00:14.3 82.W=f > cleared that out for me and I could assign the device. > > My dom0 boots, it runs flask-label-pci for a set of PCI devices > (including iwlwifi), then xl pci-assignable-add for all PCI devices > which will be passed through, then a little later it boots the > associated domains. Dom0 does not have a driver for iwlwifi. > > I'll have to investigate more to see how MASKALL is getting set. This > had not been an issue before your recent patches. I guess before the patches nothing set anything in MSI-X capability, because it was hidden... Anyway, to support my cleanup hypothesis, I tried to destroy a PCI-having domain, and it left MSI-X enabled (at least according to the config space). MASKALL was _not_ set, but I haven't checked masking of individual vectors. TBH, I'm not sure what should be responsible for the MSI-X cleanup after guest destroy. Should it be Xen? Qemu? Pciback? Pciback calls PHYSDEVOP_{prepare,release}_msix only when binding/unbinding from the device (so - xl pci-assignable-{add,remove}), so this isn't the right place. Should that be in Xen, in deassign_device() (part of DOMCTL_deassign_device)?
Hi, Marek, On Fri, Nov 18, 2022 at 4:46 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Fri, Nov 18, 2022 at 03:46:47PM -0500, Jason Andryuk wrote: > > On Fri, Nov 18, 2022 at 10:50 AM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> 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. > > > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is > > > enabled (in other words: enabling MSI/MSI-X implicitly disables INTx). > > > > > > Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This > > > applies to three places: > > > - checking currently enabled interrupts type, > > > - transition to MSI/MSI-X - where INTx would be implicitly disabled, > > > - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is > > > enabled, as device should consider INTx disabled anyway in that case > > > > > > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > --- > > > Changes in v3: > > > - allow clearing INTx regardless of MSI/MSI-X state, to be consistent > > > with enabling MSI/MSI-X > > > Changes in v2: > > > - restructure the patch to consider not only MASKALL bit, but enabling > > > MSI/MSI-X generally, without explicitly disabling INTx first > > > --- > > > > I was trying to test your xen-pciback v3 patch, and I am having > > assignment fail consistently now. It is actually failing to > > quarantine to domIO in the first place, which matches the failure from > > the other day (when I more carefully read through the logs). It now > > consistently fails to quarantine on every boot unlike the other day > > where it happened once. > > Does this include the very first assignment too, or only after domain > reboot? If the latter, maybe some cleanup missed clearing MASKALL? It's the quarantine during dom0 boot that fails. Later assignment during VM boot fails. I tried warm reboots and cold boots and it happened both times. I also modified my initrd to halt in there and checked the config space. MASKALL wasn't set at that time. I need to double check - MASKALL may have been unset after dom0 booted in that case. I'll test more to figure when and how MASKALL is getting set. > FWIW, the patch applied to Qubes > (https://github.com/QubesOS/qubes-linux-kernel/pull/680) seems to work > fine (the full test run is still in progress, but I see some green marks > already). Does Qubes quarantine devices explicitly, or are they in dom0 and libvirt/libxl just assigns them when a domain boots? > > I added some printks and it 's getting -EBUSY from pdev_msix_assign() > > which means pci_reset_msix_state() is failing: > > if ( pci_conf_read16(pdev->sbdf, msix_control_reg(pos)) & > > PCI_MSIX_FLAGS_MASKALL ) > > return -EBUSY; > > > > # lspci -vv -s 14.3 > > ... > > Capabilities: [80] MSI-X: Enable- Count=16 Masked+ > > Vector table: BAR=0 offset=00002000 > > PBA: BAR=0 offset=00003000 > > > > So it looks like MASKALL is set and prevents assignment. > > > > setpci -s 00:14.3 82.W=f > > cleared that out for me and I could assign the device. > > > > My dom0 boots, it runs flask-label-pci for a set of PCI devices > > (including iwlwifi), then xl pci-assignable-add for all PCI devices > > which will be passed through, then a little later it boots the > > associated domains. Dom0 does not have a driver for iwlwifi. > > > > I'll have to investigate more to see how MASKALL is getting set. This > > had not been an issue before your recent patches. > > I guess before the patches nothing set anything in MSI-X capability, > because it was hidden... Well, stubdom hasn't even booted when, so it would be the Xen or pciback change to modify MASKALL? > Anyway, to support my cleanup hypothesis, I tried to destroy a > PCI-having domain, and it left MSI-X enabled (at least according to the > config space). MASKALL was _not_ set, but I haven't checked masking of > individual vectors. TBH, I'm not sure what should be responsible for the > MSI-X cleanup after guest destroy. Should it be Xen? Qemu? Pciback? > Pciback calls PHYSDEVOP_{prepare,release}_msix only when > binding/unbinding from the device (so - xl pci-assignable-{add,remove}), > so this isn't the right place. I need to review all this code to give a meaningful response. Would xen-pciback set MASKALL when it binds a device? That happens before xl pci-assignable-add tries to quarantine (assign to to domIO). > Should that be in Xen, in deassign_device() (part of > DOMCTL_deassign_device)? It seems to me that Xen needs to ultimately disable the device. Thanks, Jason
On Sat, Nov 19, 2022 at 09:36:54AM -0500, Jason Andryuk wrote: > Hi, Marek, > > On Fri, Nov 18, 2022 at 4:46 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > On Fri, Nov 18, 2022 at 03:46:47PM -0500, Jason Andryuk wrote: > > > I was trying to test your xen-pciback v3 patch, and I am having > > > assignment fail consistently now. It is actually failing to > > > quarantine to domIO in the first place, which matches the failure from > > > the other day (when I more carefully read through the logs). It now > > > consistently fails to quarantine on every boot unlike the other day > > > where it happened once. > > > > Does this include the very first assignment too, or only after domain > > reboot? If the latter, maybe some cleanup missed clearing MASKALL? > > It's the quarantine during dom0 boot that fails. Later assignment > during VM boot fails. I tried warm reboots and cold boots and it > happened both times. > > I also modified my initrd to halt in there and checked the config > space. MASKALL wasn't set at that time. I need to double check - > MASKALL may have been unset after dom0 booted in that case. > > I'll test more to figure when and how MASKALL is getting set. > > > FWIW, the patch applied to Qubes > > (https://github.com/QubesOS/qubes-linux-kernel/pull/680) seems to work > > fine (the full test run is still in progress, but I see some green marks > > already). > > Does Qubes quarantine devices explicitly, or are they in dom0 and > libvirt/libxl just assigns them when a domain boots? We do quarantine them explicitly, still in initramfs. > > > I added some printks and it 's getting -EBUSY from pdev_msix_assign() > > > which means pci_reset_msix_state() is failing: > > > if ( pci_conf_read16(pdev->sbdf, msix_control_reg(pos)) & > > > PCI_MSIX_FLAGS_MASKALL ) > > > return -EBUSY; > > > > > > # lspci -vv -s 14.3 > > > ... > > > Capabilities: [80] MSI-X: Enable- Count=16 Masked+ > > > Vector table: BAR=0 offset=00002000 > > > PBA: BAR=0 offset=00003000 > > > > > > So it looks like MASKALL is set and prevents assignment. > > > > > > setpci -s 00:14.3 82.W=f > > > cleared that out for me and I could assign the device. > > > > > > My dom0 boots, it runs flask-label-pci for a set of PCI devices > > > (including iwlwifi), then xl pci-assignable-add for all PCI devices > > > which will be passed through, then a little later it boots the > > > associated domains. Dom0 does not have a driver for iwlwifi. > > > > > > I'll have to investigate more to see how MASKALL is getting set. This > > > had not been an issue before your recent patches. > > > > I guess before the patches nothing set anything in MSI-X capability, > > because it was hidden... > > Well, stubdom hasn't even booted when, so it would be the Xen or > pciback change to modify MASKALL? Weird... > > Anyway, to support my cleanup hypothesis, I tried to destroy a > > PCI-having domain, and it left MSI-X enabled (at least according to the > > config space). MASKALL was _not_ set, but I haven't checked masking of > > individual vectors. TBH, I'm not sure what should be responsible for the > > MSI-X cleanup after guest destroy. Should it be Xen? Qemu? Pciback? > > Pciback calls PHYSDEVOP_{prepare,release}_msix only when > > binding/unbinding from the device (so - xl pci-assignable-{add,remove}), > > so this isn't the right place. > > I need to review all this code to give a meaningful response. Would > xen-pciback set MASKALL when it binds a device? That happens before > xl pci-assignable-add tries to quarantine (assign to to domIO). I don't see pciback doing that. And also, my patches shouldn't change behaviour of pciback when binding to a device (so, if it would be doing that, it would happen before my patches too). Maybe that's an interaction with some other patches? > > Should that be in Xen, in deassign_device() (part of > > DOMCTL_deassign_device)? > > It seems to me that Xen needs to ultimately disable the device. That's my intuition too.
On Sat, Nov 19, 2022 at 11:33 AM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Sat, Nov 19, 2022 at 09:36:54AM -0500, Jason Andryuk wrote: > > Hi, Marek, > > > > On Fri, Nov 18, 2022 at 4:46 PM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> wrote: > > > > > > On Fri, Nov 18, 2022 at 03:46:47PM -0500, Jason Andryuk wrote: > > > > I was trying to test your xen-pciback v3 patch, and I am having > > > > assignment fail consistently now. It is actually failing to > > > > quarantine to domIO in the first place, which matches the failure from > > > > the other day (when I more carefully read through the logs). It now > > > > consistently fails to quarantine on every boot unlike the other day > > > > where it happened once. > > > > > > Does this include the very first assignment too, or only after domain > > > reboot? If the latter, maybe some cleanup missed clearing MASKALL? > > > > It's the quarantine during dom0 boot that fails. Later assignment > > during VM boot fails. I tried warm reboots and cold boots and it > > happened both times. > > > > I also modified my initrd to halt in there and checked the config > > space. MASKALL wasn't set at that time. I need to double check - > > MASKALL may have been unset after dom0 booted in that case. > > > > I'll test more to figure when and how MASKALL is getting set. I'm testing with a laptop without a battery. It seems MASKALL remains set when rebooting or when left plugged in. From unplugged, a cold boot doesn't have MASKALL set and the network vm boots. After that, rebooting the laptop leaves MASKALL set on the NIC when the laptop reboots. NIC assignment fails. Shutdown and later boot while left plugged in keeps MASKALL set. NIC assignment fails. I have only tested this scenario for short periods of time, so I don't know if it would eventually clear after a longer time. Regards, Jason
On Mon, Nov 21, 2022 at 10:41:34AM -0500, Jason Andryuk wrote: > On Sat, Nov 19, 2022 at 11:33 AM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > On Sat, Nov 19, 2022 at 09:36:54AM -0500, Jason Andryuk wrote: > > > Hi, Marek, > > > > > > On Fri, Nov 18, 2022 at 4:46 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> wrote: > > > > > > > > On Fri, Nov 18, 2022 at 03:46:47PM -0500, Jason Andryuk wrote: > > > > > I was trying to test your xen-pciback v3 patch, and I am having > > > > > assignment fail consistently now. It is actually failing to > > > > > quarantine to domIO in the first place, which matches the failure from > > > > > the other day (when I more carefully read through the logs). It now > > > > > consistently fails to quarantine on every boot unlike the other day > > > > > where it happened once. > > > > > > > > Does this include the very first assignment too, or only after domain > > > > reboot? If the latter, maybe some cleanup missed clearing MASKALL? > > > > > > It's the quarantine during dom0 boot that fails. Later assignment > > > during VM boot fails. I tried warm reboots and cold boots and it > > > happened both times. > > > > > > I also modified my initrd to halt in there and checked the config > > > space. MASKALL wasn't set at that time. I need to double check - > > > MASKALL may have been unset after dom0 booted in that case. > > > > > > I'll test more to figure when and how MASKALL is getting set. > > I'm testing with a laptop without a battery. It seems MASKALL remains > set when rebooting or when left plugged in. > > From unplugged, a cold boot doesn't have MASKALL set and the network vm boots. > > After that, rebooting the laptop leaves MASKALL set on the NIC when > the laptop reboots. NIC assignment fails. > > Shutdown and later boot while left plugged in keeps MASKALL set. NIC > assignment fails. I have only tested this scenario for short periods > of time, so I don't know if it would eventually clear after a longer > time. That's interesting, seems like firmware is not resetting the device properly. Maybe related to enabled wake on lan? Anyway, resetting the device at domain create/destroy is AFAIR normally done by pciback (at the instruction by the toolstack). Should it maybe be done when assigning to pciback initially too? Or maybe in this specific case, device reset doesn't properly clear MASKALL, so pciback should clear it explicitly (after ensuring the MSI-X enable is cleared too)?
On Mon, Nov 21, 2022 at 05:16:37PM +0100, Marek Marczykowski-Górecki wrote: > On Mon, Nov 21, 2022 at 10:41:34AM -0500, Jason Andryuk wrote: > > On Sat, Nov 19, 2022 at 11:33 AM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> wrote: > > > > > > On Sat, Nov 19, 2022 at 09:36:54AM -0500, Jason Andryuk wrote: > > > > Hi, Marek, > > > > > > > > On Fri, Nov 18, 2022 at 4:46 PM Marek Marczykowski-Górecki > > > > <marmarek@invisiblethingslab.com> wrote: > > > > > > > > > > On Fri, Nov 18, 2022 at 03:46:47PM -0500, Jason Andryuk wrote: > > > > > > I was trying to test your xen-pciback v3 patch, and I am having > > > > > > assignment fail consistently now. It is actually failing to > > > > > > quarantine to domIO in the first place, which matches the failure from > > > > > > the other day (when I more carefully read through the logs). It now > > > > > > consistently fails to quarantine on every boot unlike the other day > > > > > > where it happened once. > > > > > > > > > > Does this include the very first assignment too, or only after domain > > > > > reboot? If the latter, maybe some cleanup missed clearing MASKALL? > > > > > > > > It's the quarantine during dom0 boot that fails. Later assignment > > > > during VM boot fails. I tried warm reboots and cold boots and it > > > > happened both times. > > > > > > > > I also modified my initrd to halt in there and checked the config > > > > space. MASKALL wasn't set at that time. I need to double check - > > > > MASKALL may have been unset after dom0 booted in that case. > > > > > > > > I'll test more to figure when and how MASKALL is getting set. > > > > I'm testing with a laptop without a battery. It seems MASKALL remains > > set when rebooting or when left plugged in. > > > > From unplugged, a cold boot doesn't have MASKALL set and the network vm boots. > > > > After that, rebooting the laptop leaves MASKALL set on the NIC when > > the laptop reboots. NIC assignment fails. > > > > Shutdown and later boot while left plugged in keeps MASKALL set. NIC > > assignment fails. I have only tested this scenario for short periods > > of time, so I don't know if it would eventually clear after a longer > > time. > > That's interesting, seems like firmware is not resetting the device > properly. Maybe related to enabled wake on lan? > > Anyway, resetting the device at domain create/destroy is AFAIR normally > done by pciback (at the instruction by the toolstack). Should it maybe > be done when assigning to pciback initially too? Or maybe in this > specific case, device reset doesn't properly clear MASKALL, so pciback > should clear it explicitly (after ensuring the MSI-X enable is cleared > too)? Can you check if `echo 1 > /sys/bus/pci/devices/$SBDF/reset` clears MASKALL on this device? I'm tempted to add libxl__device_pci_reset() as part of libxl__device_pci_assignable_add().
On Mon, Nov 28, 2022 at 8:44 AM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Mon, Nov 21, 2022 at 05:16:37PM +0100, Marek Marczykowski-Górecki wrote: > > On Mon, Nov 21, 2022 at 10:41:34AM -0500, Jason Andryuk wrote: > > > On Sat, Nov 19, 2022 at 11:33 AM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> wrote: > > > > > > > > On Sat, Nov 19, 2022 at 09:36:54AM -0500, Jason Andryuk wrote: > > > > > Hi, Marek, > > > > > > > > > > On Fri, Nov 18, 2022 at 4:46 PM Marek Marczykowski-Górecki > > > > > <marmarek@invisiblethingslab.com> wrote: > > > > > > > > > > > > On Fri, Nov 18, 2022 at 03:46:47PM -0500, Jason Andryuk wrote: > > > > > > > I was trying to test your xen-pciback v3 patch, and I am having > > > > > > > assignment fail consistently now. It is actually failing to > > > > > > > quarantine to domIO in the first place, which matches the failure from > > > > > > > the other day (when I more carefully read through the logs). It now > > > > > > > consistently fails to quarantine on every boot unlike the other day > > > > > > > where it happened once. > > > > > > > > > > > > Does this include the very first assignment too, or only after domain > > > > > > reboot? If the latter, maybe some cleanup missed clearing MASKALL? > > > > > > > > > > It's the quarantine during dom0 boot that fails. Later assignment > > > > > during VM boot fails. I tried warm reboots and cold boots and it > > > > > happened both times. > > > > > > > > > > I also modified my initrd to halt in there and checked the config > > > > > space. MASKALL wasn't set at that time. I need to double check - > > > > > MASKALL may have been unset after dom0 booted in that case. > > > > > > > > > > I'll test more to figure when and how MASKALL is getting set. > > > > > > I'm testing with a laptop without a battery. It seems MASKALL remains > > > set when rebooting or when left plugged in. > > > > > > From unplugged, a cold boot doesn't have MASKALL set and the network vm boots. > > > > > > After that, rebooting the laptop leaves MASKALL set on the NIC when > > > the laptop reboots. NIC assignment fails. > > > > > > Shutdown and later boot while left plugged in keeps MASKALL set. NIC > > > assignment fails. I have only tested this scenario for short periods > > > of time, so I don't know if it would eventually clear after a longer > > > time. > > > > That's interesting, seems like firmware is not resetting the device > > properly. Maybe related to enabled wake on lan? > > > > Anyway, resetting the device at domain create/destroy is AFAIR normally > > done by pciback (at the instruction by the toolstack). Should it maybe > > be done when assigning to pciback initially too? Or maybe in this > > specific case, device reset doesn't properly clear MASKALL, so pciback > > should clear it explicitly (after ensuring the MSI-X enable is cleared > > too)? > > Can you check if `echo 1 > /sys/bus/pci/devices/$SBDF/reset` clears > MASKALL on this device? `echo 1 > ..../reset` did not clear MASKALL. After shutting down the domain with the iwlwifi card, lspci from dom0 shows: MSI-X: Enable+ Count=16 Masked+ Hmm, Xen logged: (XEN) cannot disable IRQ 137: masking MSI-X on 0000:00:14.3 Oh, looking back, I see that was logged during my earlier testing of this patch set, but I missed it. It seems like Xen set Enable and Masked itself in __pci_disable_msix() since memory decoding is not enabled. I'm still investigating, but I wanted to give an update. It seems like Xen should clear MASKALL when booting. Something like clearing MASKALL in pdev_msi_init() when !ENABLE & MASKALL. However, I have seen the system boot with both Enable and Maskall set on the iwlwifi nic. Is it risky to just unilaterally clear both of those when enumerating PCI devices? It doesn't seem appropriate to leave them set without a driver controlling them. -Jason
On Fri, Nov 18, 2022 at 04:49:23PM +0100, 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. > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is > enabled (in other words: enabling MSI/MSI-X implicitly disables INTx). > > Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This > applies to three places: > - checking currently enabled interrupts type, > - transition to MSI/MSI-X - where INTx would be implicitly disabled, > - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is > enabled, as device should consider INTx disabled anyway in that case > > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Ping? The issue pointed out by Jason was fixed in Xen: https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=913751d7af6e78d65c1e2adf4887193c827f0c5e > --- > Changes in v3: > - allow clearing INTx regardless of MSI/MSI-X state, to be consistent > with enabling MSI/MSI-X > Changes in v2: > - restructure the patch to consider not only MASKALL bit, but enabling > MSI/MSI-X generally, without explicitly disabling INTx first > --- > drivers/xen/xen-pciback/conf_space.c | 19 +++++++++++------ > .../xen/xen-pciback/conf_space_capability.c | 3 ++- > drivers/xen/xen-pciback/conf_space_header.c | 21 +++---------------- > 3 files changed, 18 insertions(+), 25 deletions(-) > > diff --git a/drivers/xen/xen-pciback/conf_space.c b/drivers/xen/xen-pciback/conf_space.c > index 059de92aea7d..d47eee6c5143 100644 > --- a/drivers/xen/xen-pciback/conf_space.c > +++ b/drivers/xen/xen-pciback/conf_space.c > @@ -288,12 +288,6 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) > u16 val; > int ret = 0; > > - err = pci_read_config_word(dev, PCI_COMMAND, &val); > - if (err) > - return err; > - if (!(val & PCI_COMMAND_INTX_DISABLE)) > - ret |= INTERRUPT_TYPE_INTX; > - > /* > * Do not trust dev->msi(x)_enabled here, as enabling could be done > * bypassing the pci_*msi* functions, by the qemu. > @@ -316,6 +310,19 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) > if (val & PCI_MSIX_FLAGS_ENABLE) > ret |= INTERRUPT_TYPE_MSIX; > } > + > + /* > + * PCIe spec says device cannot use INTx if MSI/MSI-X is enabled, > + * so check for INTx only when both are disabled. > + */ > + if (!ret) { > + err = pci_read_config_word(dev, PCI_COMMAND, &val); > + if (err) > + return err; > + if (!(val & PCI_COMMAND_INTX_DISABLE)) > + ret |= INTERRUPT_TYPE_INTX; > + } > + > 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..eb4c1af44f5c 100644 > --- a/drivers/xen/xen-pciback/conf_space_capability.c > +++ b/drivers/xen/xen-pciback/conf_space_capability.c > @@ -236,10 +236,11 @@ static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value, > return PCIBIOS_SET_FAILED; > > if (new_value & field_config->enable_bit) { > - /* don't allow enabling together with other interrupt types */ > + /* don't allow enabling together with other interrupt type */ > int int_type = xen_pcibk_get_interrupt_type(dev); > > if (int_type == INTERRUPT_TYPE_NONE || > + int_type == INTERRUPT_TYPE_INTX || > int_type == field_config->int_type) > goto write; > return PCIBIOS_SET_FAILED; > diff --git a/drivers/xen/xen-pciback/conf_space_header.c b/drivers/xen/xen-pciback/conf_space_header.c > index 981435103af1..fc0332645966 100644 > --- a/drivers/xen/xen-pciback/conf_space_header.c > +++ b/drivers/xen/xen-pciback/conf_space_header.c > @@ -104,24 +104,9 @@ static int command_write(struct pci_dev *dev, int offset, u16 value, void *data) > pci_clear_mwi(dev); > } > > - if (dev_data && dev_data->allow_interrupt_control) { > - if ((cmd->val ^ value) & PCI_COMMAND_INTX_DISABLE) { > - if (value & PCI_COMMAND_INTX_DISABLE) { > - pci_intx(dev, 0); > - } else { > - /* Do not allow enabling INTx together with MSI or MSI-X. */ > - switch (xen_pcibk_get_interrupt_type(dev)) { > - case INTERRUPT_TYPE_NONE: > - pci_intx(dev, 1); > - break; > - case INTERRUPT_TYPE_INTX: > - break; > - default: > - return PCIBIOS_SET_FAILED; > - } > - } > - } > - } > + if (dev_data && dev_data->allow_interrupt_control && > + ((cmd->val ^ value) & PCI_COMMAND_INTX_DISABLE)) > + pci_intx(dev, !(value & PCI_COMMAND_INTX_DISABLE)); > > cmd->val = value; > > -- > 2.37.3 >
On 18.11.22 16:49, 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. > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is > enabled (in other words: enabling MSI/MSI-X implicitly disables INTx). > > Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This > applies to three places: > - checking currently enabled interrupts type, > - transition to MSI/MSI-X - where INTx would be implicitly disabled, > - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is > enabled, as device should consider INTx disabled anyway in that case > > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Acked-by: Juergen Gross <jgross@suse.com> Juergen
On Mon, Oct 16, 2023 at 11:05:10AM +0200, Roger Pau Monné wrote: > On Fri, Nov 18, 2022 at 04:49:23PM +0100, 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. > > According to the PCIe spec, device cannot use INTx when MSI/MSI-X is > > enabled (in other words: enabling MSI/MSI-X implicitly disables INTx). > > > > Change the logic to consider INTx disabled if MSI/MSI-X is enabled. This > > applies to three places: > > - checking currently enabled interrupts type, > > - transition to MSI/MSI-X - where INTx would be implicitly disabled, > > - clearing INTx disable bit - which can be allowed even if MSI/MSI-X is > > enabled, as device should consider INTx disabled anyway in that case > > Is this last point strictly needed? From the description above it > seems Linux only cares about enabling MSI(-X) without the disable INTx > bit set. I'm not sure, but it seems logical to have it symmetric. > > > > > Fixes: 5e29500eba2a ("xen-pciback: Allow setting PCI_MSIX_FLAGS_MASKALL too") > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > --- > > Changes in v3: > > - allow clearing INTx regardless of MSI/MSI-X state, to be consistent > > with enabling MSI/MSI-X > > Changes in v2: > > - restructure the patch to consider not only MASKALL bit, but enabling > > MSI/MSI-X generally, without explicitly disabling INTx first > > --- > > drivers/xen/xen-pciback/conf_space.c | 19 +++++++++++------ > > .../xen/xen-pciback/conf_space_capability.c | 3 ++- > > drivers/xen/xen-pciback/conf_space_header.c | 21 +++---------------- > > 3 files changed, 18 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/xen/xen-pciback/conf_space.c b/drivers/xen/xen-pciback/conf_space.c > > index 059de92aea7d..d47eee6c5143 100644 > > --- a/drivers/xen/xen-pciback/conf_space.c > > +++ b/drivers/xen/xen-pciback/conf_space.c > > @@ -288,12 +288,6 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) > > u16 val; > > int ret = 0; > > > > - err = pci_read_config_word(dev, PCI_COMMAND, &val); > > - if (err) > > - return err; > > - if (!(val & PCI_COMMAND_INTX_DISABLE)) > > - ret |= INTERRUPT_TYPE_INTX; > > - > > /* > > * Do not trust dev->msi(x)_enabled here, as enabling could be done > > * bypassing the pci_*msi* functions, by the qemu. > > @@ -316,6 +310,19 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) > > if (val & PCI_MSIX_FLAGS_ENABLE) > > ret |= INTERRUPT_TYPE_MSIX; > > } > > Since we are explicitly hiding INTx now, should we also do something > about MSI(X) being both enabled at the same time? The spec states: > > "System configuration software sets one of these bits to enable either > MSI or MSI-X, but never both simultaneously. Behavior is undefined if > both MSI and MSI-X are enabled simultaneously." > > So finding both MSI and MSI-X enabled likely means something has gone > very wrong? Likely to be done in a separate change, just realized > while looking at the patch context. Pciback try to prevent such situation (that's exactly the point of checking the current interrupt type). But if you get into such situation somehow anyway (likely bypassing pciback), then pciback will still allow to disable one of them, so you can fix the situation (the enforcement of "only one type at the time" is done setting the enable bit, but you can still clear it). If both MSI and MSI-X are enabled xen_pcibk_get_interrupt_type() will return both bits set. > > + > > + /* > > + * PCIe spec says device cannot use INTx if MSI/MSI-X is enabled, > > + * so check for INTx only when both are disabled. > > + */ > > + if (!ret) { > > + err = pci_read_config_word(dev, PCI_COMMAND, &val); > > + if (err) > > + return err; > > + if (!(val & PCI_COMMAND_INTX_DISABLE)) > > + ret |= INTERRUPT_TYPE_INTX; > > + } > > + > > 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..eb4c1af44f5c 100644 > > --- a/drivers/xen/xen-pciback/conf_space_capability.c > > +++ b/drivers/xen/xen-pciback/conf_space_capability.c > > @@ -236,10 +236,11 @@ static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value, > > return PCIBIOS_SET_FAILED; > > > > if (new_value & field_config->enable_bit) { > > - /* don't allow enabling together with other interrupt types */ > > + /* don't allow enabling together with other interrupt type */ > > This comment needs to be adjusted to note that we allow enabling while > INTx is not disabled in the command register, in order to please > Linuxes MSI(-X) startup sequence. Ok. > FWIW, another option would be to simply disable INTX here once MSI(-X) > is attempted to be enabled, won't that avoid having to modify > xen_pcibk_get_interrupt_type()? I would rather avoid implicit changes to other bits, it may lead to hard to debug corner cases (in this case, for example, if domU decides to disable MSI-X later on, it would be left with INTx disabled too, so no interrupts at all).
diff --git a/drivers/xen/xen-pciback/conf_space.c b/drivers/xen/xen-pciback/conf_space.c index 059de92aea7d..d47eee6c5143 100644 --- a/drivers/xen/xen-pciback/conf_space.c +++ b/drivers/xen/xen-pciback/conf_space.c @@ -288,12 +288,6 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) u16 val; int ret = 0; - err = pci_read_config_word(dev, PCI_COMMAND, &val); - if (err) - return err; - if (!(val & PCI_COMMAND_INTX_DISABLE)) - ret |= INTERRUPT_TYPE_INTX; - /* * Do not trust dev->msi(x)_enabled here, as enabling could be done * bypassing the pci_*msi* functions, by the qemu. @@ -316,6 +310,19 @@ int xen_pcibk_get_interrupt_type(struct pci_dev *dev) if (val & PCI_MSIX_FLAGS_ENABLE) ret |= INTERRUPT_TYPE_MSIX; } + + /* + * PCIe spec says device cannot use INTx if MSI/MSI-X is enabled, + * so check for INTx only when both are disabled. + */ + if (!ret) { + err = pci_read_config_word(dev, PCI_COMMAND, &val); + if (err) + return err; + if (!(val & PCI_COMMAND_INTX_DISABLE)) + ret |= INTERRUPT_TYPE_INTX; + } + 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..eb4c1af44f5c 100644 --- a/drivers/xen/xen-pciback/conf_space_capability.c +++ b/drivers/xen/xen-pciback/conf_space_capability.c @@ -236,10 +236,11 @@ static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value, return PCIBIOS_SET_FAILED; if (new_value & field_config->enable_bit) { - /* don't allow enabling together with other interrupt types */ + /* don't allow enabling together with other interrupt type */ int int_type = xen_pcibk_get_interrupt_type(dev); if (int_type == INTERRUPT_TYPE_NONE || + int_type == INTERRUPT_TYPE_INTX || int_type == field_config->int_type) goto write; return PCIBIOS_SET_FAILED; diff --git a/drivers/xen/xen-pciback/conf_space_header.c b/drivers/xen/xen-pciback/conf_space_header.c index 981435103af1..fc0332645966 100644 --- a/drivers/xen/xen-pciback/conf_space_header.c +++ b/drivers/xen/xen-pciback/conf_space_header.c @@ -104,24 +104,9 @@ static int command_write(struct pci_dev *dev, int offset, u16 value, void *data) pci_clear_mwi(dev); } - if (dev_data && dev_data->allow_interrupt_control) { - if ((cmd->val ^ value) & PCI_COMMAND_INTX_DISABLE) { - if (value & PCI_COMMAND_INTX_DISABLE) { - pci_intx(dev, 0); - } else { - /* Do not allow enabling INTx together with MSI or MSI-X. */ - switch (xen_pcibk_get_interrupt_type(dev)) { - case INTERRUPT_TYPE_NONE: - pci_intx(dev, 1); - break; - case INTERRUPT_TYPE_INTX: - break; - default: - return PCIBIOS_SET_FAILED; - } - } - } - } + if (dev_data && dev_data->allow_interrupt_control && + ((cmd->val ^ value) & PCI_COMMAND_INTX_DISABLE)) + pci_intx(dev, !(value & PCI_COMMAND_INTX_DISABLE)); cmd->val = value;