From patchwork Thu Nov 10 16:59:33 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Vrabel X-Patchwork-Id: 13039023 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 lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (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 C70D5C4321E for ; Thu, 10 Nov 2022 17:00:17 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.442031.696083 (Exim 4.92) (envelope-from ) id 1otAuI-0003AC-TX; Thu, 10 Nov 2022 17:00:02 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 442031.696083; Thu, 10 Nov 2022 17:00:02 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1otAuI-00038X-Ni; Thu, 10 Nov 2022 17:00:02 +0000 Received: by outflank-mailman (input) for mailman id 442031; Thu, 10 Nov 2022 17:00:01 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1otAuH-0002uk-Au for xen-devel@lists.xenproject.org; Thu, 10 Nov 2022 17:00:01 +0000 Received: from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com [2a00:1450:4864:20::32b]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id 1cbb9d02-6119-11ed-91b5-6bf2151ebd3b; Thu, 10 Nov 2022 18:00:00 +0100 (CET) Received: by mail-wm1-x32b.google.com with SMTP id fn7-20020a05600c688700b003b4fb113b86so1587687wmb.0 for ; Thu, 10 Nov 2022 09:00:00 -0800 (PST) Received: from pear.davidvrabel.org.uk (pear.davidvrabel.org.uk. [82.70.146.41]) by smtp.googlemail.com with ESMTPSA id j5-20020a05600c1c0500b003cfbbd54178sm9403666wms.2.2022.11.10.08.59.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Nov 2022 08:59:59 -0800 (PST) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list X-Inumbo-ID: 1cbb9d02-6119-11ed-91b5-6bf2151ebd3b DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:sender:from:to:cc:subject:date :message-id:reply-to; bh=XT451c2UjX6nYO0HqV5+AhJZLhiaFB3XbFUJQqv3uCY=; b=CULL7YC3PMCsFzcTYGiZxMBXj6yB7FCvh+dUXW6kjrLQStq+pPz28vAty3KSawfQcD Au1t6o+Ie3GEoRXBo2+DRT0f+j/RIsu2YyKYG+hWBH4aVCCFe/OaOeTHhsyPyTozzXGG 0S+GVk7SomYkoOdo2fEAaW8hGiLvnSMi+DLmuhptKn1ecNB4b2DTknM1XqFuT2Qgrp13 Pgj5t70odUKK7UzUX7MVApSHfyXik31o+kf9p2zc1NEH9wLdw7FWgNBSNGOPNx4PWGTc pVDMC4/cRDKHNBYBbmvdH7GvgUsSyulOxcEdnoDAFtSfwJWmivUmjGhhdaGhJfC1TRUU Obaw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:sender:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=XT451c2UjX6nYO0HqV5+AhJZLhiaFB3XbFUJQqv3uCY=; b=MgZZwub8mWWeSI+cvMkTAORrr3f9GbZ0MjoNsV1nu/mDrs3SEDVkUZfesOTII5+YTz ugAR71WWg4OpXSSWBc2eQVY9g5o0OZQEBbzCcyl3Uuf8dnRK7QwsetaSHMVF4u6bbFty VMC3Sl1i093nelr//cO+KB6huWtwsigKXDt7qsqrI2bWJV59aQztdEsarWaPpDwoQ8TX hIlEylPxt7EBDeCohAroIt0RQRyyAS9kkqphhJIid8Gqc431pbdghglqui8jk/lWkM// 8ZC/9c0YmS3ODC00MKYUI69c9anxsH59XmR3cT5u3Is0zQRXRbspoaPlhNO4gSXfSTqF psiA== X-Gm-Message-State: ACrzQf34s55O7IFlpF3GdU+Q5b3yzGLzrY7oRL6/M1Gpy7pvqDw67wDf TauLQuZlpF+EVjtYiMnre3h+v6KNYC41EA== X-Google-Smtp-Source: AMsMyM7GfvspitH2uSrPLzAx3bEUHQuXm3d3qm+8bJzz0Yle0Sh2GhBYOAU4hJ45yWvKo9t9l+nHLQ== X-Received: by 2002:a05:600c:1e26:b0:3cf:5238:13fc with SMTP id ay38-20020a05600c1e2600b003cf523813fcmr51763388wmb.151.1668099599882; Thu, 10 Nov 2022 08:59:59 -0800 (PST) Sender: David Vrabel From: David Vrabel X-Google-Original-From: David Vrabel To: xen-devel@lists.xenproject.org Cc: Jan Beulich , Andrew Cooper , =?utf-8?q?Roger_Pau_Monn=C3=A9?= , Wei Liu , David Vrabel Subject: [PATCH 1/3] x86/msi: consistently handle BAR mapping failures in MSI-X setup Date: Thu, 10 Nov 2022 16:59:33 +0000 Message-Id: <20221110165935.106376-2-dvrabel@amazon.co.uk> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20221110165935.106376-1-dvrabel@amazon.co.uk> References: <20221110165935.106376-1-dvrabel@amazon.co.uk> MIME-Version: 1.0 When setting up an MSI-X vector in msix_capability_init() the error handling after a BAR mapping failure is different depending on whether the first page fails or a subsequent page. There's no reason to break working vectors so consistently use the later error handling behaviour. The zap_on_error flag was added as part of XSA-337, beb54596cfda (x86/MSI-X: restrict reading of table/PBA bases from BARs), but appears to be unrelated to XSA-337 and is not useful because: 1. table.first and pba.first are not used unless msix->used_vectors > 0. 2. Force disabling MSI-X in this error path is not necessary as the per-vector mask is still still set. Signed-off-by: David Vrabel CR: https://code.amazon.com/reviews/CR-79020908 --- xen/arch/x86/msi.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index d0bf63df1d..8bde6b9be1 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -776,7 +776,7 @@ static int msix_capability_init(struct pci_dev *dev, u8 bus = dev->bus; u8 slot = PCI_SLOT(dev->devfn); u8 func = PCI_FUNC(dev->devfn); - bool maskall = msix->host_maskall, zap_on_error = false; + bool maskall = msix->host_maskall; unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX); @@ -875,8 +875,6 @@ static int msix_capability_init(struct pci_dev *dev, BITS_TO_LONGS(msix->nr_entries) - 1); WARN_ON(rangeset_overlaps_range(mmio_ro_ranges, msix->pba.first, msix->pba.last)); - - zap_on_error = true; } else if ( !msix->table.first ) { @@ -897,14 +895,6 @@ static int msix_capability_init(struct pci_dev *dev, if ( idx < 0 ) { - if ( zap_on_error ) - { - msix->table.first = 0; - msix->pba.first = 0; - - control &= ~PCI_MSIX_FLAGS_ENABLE; - } - pci_conf_write16(dev->sbdf, msix_control_reg(pos), control); xfree(entry); return idx; From patchwork Thu Nov 10 16:59:34 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Vrabel X-Patchwork-Id: 13039021 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 lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (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 F3999C43217 for ; Thu, 10 Nov 2022 17:00:16 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.442032.696101 (Exim 4.92) (envelope-from ) id 1otAuK-0003h9-9a; Thu, 10 Nov 2022 17:00:04 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 442032.696101; Thu, 10 Nov 2022 17:00:04 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1otAuK-0003g9-06; Thu, 10 Nov 2022 17:00:04 +0000 Received: by outflank-mailman (input) for mailman id 442032; Thu, 10 Nov 2022 17:00:03 +0000 Received: from se1-gles-flk1-in.inumbo.com ([94.247.172.50] helo=se1-gles-flk1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1otAuI-0002uf-Oz for xen-devel@lists.xenproject.org; Thu, 10 Nov 2022 17:00:02 +0000 Received: from mail-wm1-x32e.google.com (mail-wm1-x32e.google.com [2a00:1450:4864:20::32e]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id 1d976744-6119-11ed-8fd2-01056ac49cbb; Thu, 10 Nov 2022 18:00:01 +0100 (CET) Received: by mail-wm1-x32e.google.com with SMTP id v124-20020a1cac82000000b003cf7a4ea2caso3881881wme.5 for ; Thu, 10 Nov 2022 09:00:01 -0800 (PST) Received: from pear.davidvrabel.org.uk (pear.davidvrabel.org.uk. [82.70.146.41]) by smtp.googlemail.com with ESMTPSA id j5-20020a05600c1c0500b003cfbbd54178sm9403666wms.2.2022.11.10.08.59.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Nov 2022 09:00:00 -0800 (PST) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list X-Inumbo-ID: 1d976744-6119-11ed-8fd2-01056ac49cbb DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:sender:from:to:cc:subject:date :message-id:reply-to; bh=3MJeW3qL/c7Nl4JPpfIgJfXaM8AY5MEnPk3q1bmBjO0=; b=AQvCwpnwVjp3w+MErojUhbmijOfiwpaBMHtyR/Ux9TYtGaRbrG3eIEXpvlhlN5cdqs 2siEbbuirbd8VG84r6buUjggBxObJ2jots3hO3qEZhSOSNzXgfMHZkelr4bNxpCQVa+I o+z/fiklDX0anJMA+QjU8u/W2LG+6FUw7fxs7kyAyWDe9hBuWvHbTn7SepiLt2AUT9k/ EUXBNEnVyjIO0wDdf7hxtJrDcU80JyjOI3zJzh18meU4d78HQZ0UxoCJW4Ktdr03gB5a /btoLShA3+6q8ualNTEw4skoAlb8cSbaLYF/7g9ldcjYIFsy0YmoqaNE4s/3DeWmiIvf 8ziw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:sender:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=3MJeW3qL/c7Nl4JPpfIgJfXaM8AY5MEnPk3q1bmBjO0=; b=3N4RQczP+4nnZ5lEzHxAbIsuvgbkkSqjEjD+510Fiw1b0oOo5WnkTCVUkS5OOTj4vl DvOczIiuCSsLrNWj5tqBuHD7iQGSJYzqHWZqIexrqEM2RkMG5/CaZ7YPC5WEgONXoVGm QS57b6TYSL1xx/joE443Q+ctH4LWFGSiu/yT4eGmxzb5DqmSTAiTGztxCXJKWEPX+naC KC9bBj0jeXMZoQXfp/TRkT/h6l2tED5g2r8+mLYpvXNOtvnPS4Usg4Qh6l1QAK62PI9c /wEiTyYJfaiIWhyIX3+o55H+MiTyeEd8MwTmdmMMLFqisNpGd/0ed85HSxPNJkiN79rv +foA== X-Gm-Message-State: ACrzQf05ceyyawdpb2b1pPpAlFK+lmHP+ZZg23zL5MGbD6my9ke7YJLq A7DJo4OPatDM/QliUYdRMHsHwOWqpR2MmQ== X-Google-Smtp-Source: AMsMyM40n8czUkrqnA7n+hbZVa0+8Z+b/+T5uaerrS29tarrjqZX00xmpUVP3PYRoSHVqyKEW9glFw== X-Received: by 2002:a7b:c3d8:0:b0:3cf:9b7b:b96c with SMTP id t24-20020a7bc3d8000000b003cf9b7bb96cmr21909005wmj.113.1668099601471; Thu, 10 Nov 2022 09:00:01 -0800 (PST) Sender: David Vrabel From: David Vrabel X-Google-Original-From: David Vrabel To: xen-devel@lists.xenproject.org Cc: Jan Beulich , Andrew Cooper , =?utf-8?q?Roger_Pau_Monn=C3=A9?= , Wei Liu , David Vrabel Subject: [PATCH 2/3] x86/msi: remove return value from msi_set_mask_bit() Date: Thu, 10 Nov 2022 16:59:34 +0000 Message-Id: <20221110165935.106376-3-dvrabel@amazon.co.uk> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20221110165935.106376-1-dvrabel@amazon.co.uk> References: <20221110165935.106376-1-dvrabel@amazon.co.uk> MIME-Version: 1.0 The return value was only used for WARN()s or BUG()s so it has no functional purpose. Simplify the code by removing it. The meaning of the return value and the purpose of the various WARNs() and BUGs() is rather unclear. The only failure path (where an MSI-X vector needs to be masked but the MSI-X table is not accessible) has a useful warning message. Signed-off-by: David Vrabel CR: https://code.amazon.com/reviews/CR-79020927 Signed-off-by: David Vrabel --- xen/arch/x86/msi.c | 34 +++++++++------------------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index 8bde6b9be1..6c675d11d1 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -314,7 +314,7 @@ int msi_maskable_irq(const struct msi_desc *entry) || entry->msi_attrib.maskbit; } -static bool msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) +static void msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) { struct msi_desc *entry = desc->msi_desc; struct pci_dev *pdev; @@ -361,11 +361,6 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) if ( likely(control & PCI_MSIX_FLAGS_ENABLE) ) break; - - entry->msi_attrib.host_masked = host; - entry->msi_attrib.guest_masked = guest; - - flag = true; } else if ( flag && !(control & PCI_MSIX_FLAGS_MASKALL) ) { @@ -385,14 +380,10 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) control |= PCI_MSIX_FLAGS_MASKALL; pci_conf_write16(pdev->sbdf, msix_control_reg(entry->msi_attrib.pos), control); - return flag; - default: - return 0; + break; } entry->msi_attrib.host_masked = host; entry->msi_attrib.guest_masked = guest; - - return 1; } static int msi_get_mask_bit(const struct msi_desc *entry) @@ -418,16 +409,12 @@ static int msi_get_mask_bit(const struct msi_desc *entry) void cf_check mask_msi_irq(struct irq_desc *desc) { - if ( unlikely(!msi_set_mask_bit(desc, 1, - desc->msi_desc->msi_attrib.guest_masked)) ) - BUG_ON(!(desc->status & IRQ_DISABLED)); + msi_set_mask_bit(desc, 1, desc->msi_desc->msi_attrib.guest_masked); } void cf_check unmask_msi_irq(struct irq_desc *desc) { - if ( unlikely(!msi_set_mask_bit(desc, 0, - desc->msi_desc->msi_attrib.guest_masked)) ) - WARN(); + msi_set_mask_bit(desc, 0, desc->msi_desc->msi_attrib.guest_masked); } void guest_mask_msi_irq(struct irq_desc *desc, bool mask) @@ -437,15 +424,13 @@ void guest_mask_msi_irq(struct irq_desc *desc, bool mask) static unsigned int cf_check startup_msi_irq(struct irq_desc *desc) { - if ( unlikely(!msi_set_mask_bit(desc, 0, !!(desc->status & IRQ_GUEST))) ) - WARN(); + msi_set_mask_bit(desc, 0, !!(desc->status & IRQ_GUEST)); return 0; } static void cf_check shutdown_msi_irq(struct irq_desc *desc) { - if ( unlikely(!msi_set_mask_bit(desc, 1, 1)) ) - BUG_ON(!(desc->status & IRQ_DISABLED)); + msi_set_mask_bit(desc, 1, 1); } void cf_check ack_nonmaskable_msi_irq(struct irq_desc *desc) @@ -1358,10 +1343,9 @@ int pci_restore_msi_state(struct pci_dev *pdev) for ( i = 0; ; ) { - if ( unlikely(!msi_set_mask_bit(desc, - entry[i].msi_attrib.host_masked, - entry[i].msi_attrib.guest_masked)) ) - BUG(); + msi_set_mask_bit(desc, + entry[i].msi_attrib.host_masked, + entry[i].msi_attrib.guest_masked); if ( !--nr ) break; From patchwork Thu Nov 10 16:59:35 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Vrabel X-Patchwork-Id: 13039024 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 lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (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 D9D9BC43219 for ; Thu, 10 Nov 2022 17:00:17 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.442033.696111 (Exim 4.92) (envelope-from ) id 1otAuM-0004GR-Lu; Thu, 10 Nov 2022 17:00:06 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 442033.696111; Thu, 10 Nov 2022 17:00:06 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1otAuM-0004Fr-I9; Thu, 10 Nov 2022 17:00:06 +0000 Received: by outflank-mailman (input) for mailman id 442033; Thu, 10 Nov 2022 17:00:05 +0000 Received: from se1-gles-flk1-in.inumbo.com ([94.247.172.50] helo=se1-gles-flk1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1otAuK-0002uf-TZ for xen-devel@lists.xenproject.org; Thu, 10 Nov 2022 17:00:05 +0000 Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [2a00:1450:4864:20::332]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id 1ea114a4-6119-11ed-8fd2-01056ac49cbb; Thu, 10 Nov 2022 18:00:03 +0100 (CET) Received: by mail-wm1-x332.google.com with SMTP id h133-20020a1c218b000000b003cf4d389c41so3893540wmh.3 for ; Thu, 10 Nov 2022 09:00:03 -0800 (PST) Received: from pear.davidvrabel.org.uk (pear.davidvrabel.org.uk. [82.70.146.41]) by smtp.googlemail.com with ESMTPSA id j5-20020a05600c1c0500b003cfbbd54178sm9403666wms.2.2022.11.10.09.00.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Nov 2022 09:00:02 -0800 (PST) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list X-Inumbo-ID: 1ea114a4-6119-11ed-8fd2-01056ac49cbb DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:sender:from:to:cc:subject:date :message-id:reply-to; bh=dhv0GF+4ldSl9zCEdd3fJE2z3jaTvmUFTFv68dRAO/4=; b=IRHDhysvNWJWMsqBjN/T1DE6hOaiVDieZCMNS0K82fU3npGmvQfhh2jR+bmXZy3lLW L4yCcXuLxxl8R1YGF58dI5Y50XUPGnXhdoMYyCNMJo0dfBaY4wmHMIrKjkuNcDggEbhg zTdgHcgGJZgmKsVpRgwO2O0JFKbeuPjzAE8cidqiA/qOpODgqhWumRrjCiBBiI5Kvlub bJg4CCaPWtv/Ewer0VfcGMMxB1LBvtdlhSCkigtRaFtuxYpIe+5/gsNjmwHmR7NmW9pR 0Jq64EdHo7lU0Dc2JmX58nJTbDH8Tkwrtx8iZoGZP6u2nC955WZ9twM64aeS4svzyrrv aiwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:sender:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=dhv0GF+4ldSl9zCEdd3fJE2z3jaTvmUFTFv68dRAO/4=; b=lEt8OCFO/S9DrHQ+0v2JR/Wrx+9Egm/R/ed68PwhC39jlmdENq/qWH+Pi1cLRm/ipu qNvhTOv8B2KtCR1JQyMEx7PbN/cHqgrOWKHzbvuGgknA8oHL/33NsvTuqI8ounwVi2Ib +60ltZYedXmk+wM6uNL/fFBFSrxAZXuQsXUUnwypIRKxcN8l0vEdfGq6RSdzMSlRgpmC O1/smy7+l76Gohom8fuvwsI1rknMLQUQmKZ8igok5ox6uZGTGJQCWJKOjYFVLekdVJ7l /qBNQR3Xu81WZ5F+k6ye060kciXT6CNlXudAXzsEr2IQHW+p0Ra7QhkS9m/UWd0hqV4D diOg== X-Gm-Message-State: ACrzQf2Dda4MINtYxJCZumi3hoqyhhLmqSwbErNOZ8HD6AY61slwYwrY gq5RK2cDY/4O8X2ze6UUg6+cqfcJhZPMGg== X-Google-Smtp-Source: AMsMyM5uvX6gpkkiFwXXfo8XmYbzXmlGMvesWd1fEUHxGEBga/cz37onbiuekcmb+5yuh8J7lsAsvA== X-Received: by 2002:a7b:c5d8:0:b0:3cf:9cd9:a889 with SMTP id n24-20020a7bc5d8000000b003cf9cd9a889mr20999323wmk.26.1668099603118; Thu, 10 Nov 2022 09:00:03 -0800 (PST) Sender: David Vrabel From: David Vrabel X-Google-Original-From: David Vrabel To: xen-devel@lists.xenproject.org Cc: Jan Beulich , Andrew Cooper , =?utf-8?q?Roger_Pau_Monn=C3=A9?= , Wei Liu , David Vrabel Subject: [PATCH 3/3] x86/pci: Fix racy accesses to MSI-X Control register Date: Thu, 10 Nov 2022 16:59:35 +0000 Message-Id: <20221110165935.106376-4-dvrabel@amazon.co.uk> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20221110165935.106376-1-dvrabel@amazon.co.uk> References: <20221110165935.106376-1-dvrabel@amazon.co.uk> MIME-Version: 1.0 Concurrent access the the MSI-X control register are not serialized with a suitable lock. For example, in msix_capability_init() access use the pcidevs_lock() but some calls to msi_set_mask_bit() use the interrupt descriptor lock. This can lead to MSI-X being incorrectly disabled and subsequent failures due to msix_memory_decoded() calls that check for MSI-X being enabled. This was seen with some non-compliant hardware that gated MSI-X messages on the per-vector mask bit only (i.e., the MSI-X Enable bit and Function Mask bits in the MSI-X Control register were ignored). An interrupt (with a pending move) for vector 0 would occur while vector 1 was being initialized in msix_capability_init(). Updates the the Control register would race and the vector 1 initialization would intermittently fail with -ENXIO. Typically a race between initializing a vector and another vector being moved doesn't occur because: 1. Racy Control accesses only occur when MSI-X is (guest) disabled 2 Hardware should only raise interrupts when MSI-X is enabled and unmasked. 3. Xen always sets Function Mask when temporarily enabling MSI-X. But there may be other race conditions depending on hardware and guest driver behaviour (e.g., disabling MSI-X when a IRQ has a pending move on another PCPU). Fix this by: 1. Tracking the host and guest enable state in a similar way to the host and guest maskall state. Note that since multiple CPUs can be updating different vectors concurrently, a counter is needed for the host enable state. 2. Add a new lock for serialize the Control read/modify/write sequence. 3. Wrap the above in two helper functions (msix_update_lock(), and msix_update_unlock()), which bracket any MSI-X register updates that require MSI-X to be (temporarily) enabled. Signed-off-by: David Vrabel SIM: https://t.corp.amazon.com/P63914633 CR: https://code.amazon.com/reviews/CR-79020945 --- xen/arch/x86/include/asm/msi.h | 3 + xen/arch/x86/msi.c | 215 +++++++++++++++++---------------- xen/drivers/passthrough/msi.c | 1 + 3 files changed, 114 insertions(+), 105 deletions(-) diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h index fe670895ee..aa36e44f4e 100644 --- a/xen/arch/x86/include/asm/msi.h +++ b/xen/arch/x86/include/asm/msi.h @@ -237,7 +237,10 @@ struct arch_msix { int table_refcnt[MAX_MSIX_TABLE_PAGES]; int table_idx[MAX_MSIX_TABLE_PAGES]; spinlock_t table_lock; + spinlock_t control_lock; bool host_maskall, guest_maskall; + uint16_t host_enable; + bool guest_enable; domid_t warned; }; diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index 6c675d11d1..8e394da07a 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -147,6 +147,57 @@ static bool msix_memory_decoded(const struct pci_dev *dev, unsigned int pos) return memory_decoded(dev); } + +/* + * Ensure MSI-X interrupts are masked during setup. Some devices require + * MSI-X to be enabled before we can touch the MSI-X registers. We need + * to mask all the vectors to prevent interrupts coming in before they're + * fully set up. + */ +static uint16_t msix_update_lock(struct pci_dev *dev, unsigned int pos) +{ + uint16_t control, new_control; + unsigned long flags; + + spin_lock_irqsave(&dev->msix->control_lock, flags); + + dev->msix->host_enable++; + + control = pci_conf_read16(dev->sbdf, msix_control_reg(pos)); + if ( !(control & PCI_MSIX_FLAGS_ENABLE) ) + { + new_control = control | PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL; + pci_conf_write16(dev->sbdf, msix_control_reg(pos), new_control); + } + else + dev->msix->guest_enable = true; + + spin_unlock_irqrestore(&dev->msix->control_lock, flags); + + return control; +} + +static void msix_update_unlock(struct pci_dev *dev, unsigned int pos, uint16_t control) +{ + uint16_t new_control; + unsigned long flags; + + spin_lock_irqsave(&dev->msix->control_lock, flags); + + dev->msix->host_enable--; + + new_control = control & ~(PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL); + + if ( dev->msix->host_enable || dev->msix->guest_enable ) + new_control |= PCI_MSIX_FLAGS_ENABLE; + if ( dev->msix->host_maskall || dev->msix->guest_maskall || dev->msix->host_enable ) + new_control |= PCI_MSIX_FLAGS_MASKALL; + if ( new_control != control ) + pci_conf_write16(dev->sbdf, msix_control_reg(pos), control); + + spin_unlock_irqrestore(&dev->msix->control_lock, flags); +} + /* * MSI message composition */ @@ -288,7 +339,7 @@ static void msi_set_enable(struct pci_dev *dev, int enable) __msi_set_enable(seg, bus, slot, func, pos, enable); } -static void msix_set_enable(struct pci_dev *dev, int enable) +static void msix_force_disable(struct pci_dev *dev) { int pos; u16 control, seg = dev->seg; @@ -299,11 +350,16 @@ static void msix_set_enable(struct pci_dev *dev, int enable) pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX); if ( pos ) { + spin_lock_irq(&dev->msix->control_lock); + + dev->msix->host_enable = false; + dev->msix->guest_enable = false; + control = pci_conf_read16(dev->sbdf, msix_control_reg(pos)); control &= ~PCI_MSIX_FLAGS_ENABLE; - if ( enable ) - control |= PCI_MSIX_FLAGS_ENABLE; pci_conf_write16(dev->sbdf, msix_control_reg(pos), control); + + spin_unlock_irq(&dev->msix->control_lock); } } @@ -318,9 +374,10 @@ static void msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) { struct msi_desc *entry = desc->msi_desc; struct pci_dev *pdev; - u16 seg, control; + u16 seg; u8 bus, slot, func; - bool flag = host || guest, maskall; + bool flag = host || guest; + uint16_t control; ASSERT(spin_is_locked(&desc->lock)); BUG_ON(!entry || !entry->dev); @@ -343,30 +400,18 @@ static void msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) } break; case PCI_CAP_ID_MSIX: - maskall = pdev->msix->host_maskall; - control = pci_conf_read16(pdev->sbdf, - msix_control_reg(entry->msi_attrib.pos)); - if ( unlikely(!(control & PCI_MSIX_FLAGS_ENABLE)) ) - { - pdev->msix->host_maskall = 1; - pci_conf_write16(pdev->sbdf, - msix_control_reg(entry->msi_attrib.pos), - control | (PCI_MSIX_FLAGS_ENABLE | - PCI_MSIX_FLAGS_MASKALL)); - } + control = msix_update_lock(pdev, entry->msi_attrib.pos); + if ( likely(memory_decoded(pdev)) ) { writel(flag, entry->mask_base + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); readl(entry->mask_base + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); - - if ( likely(control & PCI_MSIX_FLAGS_ENABLE) ) - break; } - else if ( flag && !(control & PCI_MSIX_FLAGS_MASKALL) ) + else if ( !pdev->msix->host_maskall && !pdev->msix->guest_maskall ) { domid_t domid = pdev->domain->domain_id; - maskall = true; + pdev->msix->host_maskall = true; if ( pdev->msix->warned != domid ) { pdev->msix->warned = domid; @@ -375,11 +420,8 @@ static void msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) desc->irq, domid, &pdev->sbdf); } } - pdev->msix->host_maskall = maskall; - if ( maskall || pdev->msix->guest_maskall ) - control |= PCI_MSIX_FLAGS_MASKALL; - pci_conf_write16(pdev->sbdf, - msix_control_reg(entry->msi_attrib.pos), control); + + msix_update_unlock(pdev, entry->msi_attrib.pos, control); break; } entry->msi_attrib.host_masked = host; @@ -494,26 +536,19 @@ static struct msi_desc *alloc_msi_entry(unsigned int nr) int setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc) { - const struct pci_dev *pdev = msidesc->dev; - unsigned int cpos = msix_control_reg(msidesc->msi_attrib.pos); - u16 control = ~0; + struct pci_dev *pdev = msidesc->dev; + uint16_t control = 0; int rc; if ( msidesc->msi_attrib.type == PCI_CAP_ID_MSIX ) - { - control = pci_conf_read16(pdev->sbdf, cpos); - if ( !(control & PCI_MSIX_FLAGS_ENABLE) ) - pci_conf_write16(pdev->sbdf, cpos, - control | (PCI_MSIX_FLAGS_ENABLE | - PCI_MSIX_FLAGS_MASKALL)); - } + control = msix_update_lock(pdev, msidesc->msi_attrib.pos); rc = __setup_msi_irq(desc, msidesc, msi_maskable_irq(msidesc) ? &pci_msi_maskable : &pci_msi_nonmaskable); - if ( !(control & PCI_MSIX_FLAGS_ENABLE) ) - pci_conf_write16(pdev->sbdf, cpos, control); + if ( control ) + msix_update_unlock(pdev, msidesc->msi_attrib.pos, control); return rc; } @@ -754,14 +789,14 @@ static int msix_capability_init(struct pci_dev *dev, { struct arch_msix *msix = dev->msix; struct msi_desc *entry = NULL; - u16 control; u64 table_paddr; u32 table_offset; u16 seg = dev->seg; u8 bus = dev->bus; u8 slot = PCI_SLOT(dev->devfn); u8 func = PCI_FUNC(dev->devfn); - bool maskall = msix->host_maskall; + uint16_t control; + int ret = 0; unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX); @@ -770,37 +805,22 @@ static int msix_capability_init(struct pci_dev *dev, ASSERT(pcidevs_locked()); - control = pci_conf_read16(dev->sbdf, msix_control_reg(pos)); - /* - * Ensure MSI-X interrupts are masked during setup. Some devices require - * MSI-X to be enabled before we can touch the MSI-X registers. We need - * to mask all the vectors to prevent interrupts coming in before they're - * fully set up. - */ - msix->host_maskall = 1; - pci_conf_write16(dev->sbdf, msix_control_reg(pos), - control | (PCI_MSIX_FLAGS_ENABLE | - PCI_MSIX_FLAGS_MASKALL)); - - if ( unlikely(!memory_decoded(dev)) ) - { - pci_conf_write16(dev->sbdf, msix_control_reg(pos), - control & ~PCI_MSIX_FLAGS_ENABLE); - return -ENXIO; - } - if ( desc ) { entry = alloc_msi_entry(1); if ( !entry ) - { - pci_conf_write16(dev->sbdf, msix_control_reg(pos), - control & ~PCI_MSIX_FLAGS_ENABLE); return -ENOMEM; - } ASSERT(msi); } + control = msix_update_lock(dev, pos); + + if ( unlikely(!memory_decoded(dev)) ) + { + ret = -ENXIO; + goto out; + } + /* Locate MSI-X table region */ table_offset = pci_conf_read32(dev->sbdf, msix_table_offset_reg(pos)); if ( !msix->used_entries && @@ -834,10 +854,8 @@ static int msix_capability_init(struct pci_dev *dev, { if ( !msi || !msi->table_base ) { - pci_conf_write16(dev->sbdf, msix_control_reg(pos), - control & ~PCI_MSIX_FLAGS_ENABLE); - xfree(entry); - return -ENXIO; + ret = -ENXIO; + goto out; } table_paddr = msi->table_base; } @@ -863,9 +881,8 @@ static int msix_capability_init(struct pci_dev *dev, } else if ( !msix->table.first ) { - pci_conf_write16(dev->sbdf, msix_control_reg(pos), control); - xfree(entry); - return -ENODATA; + ret = -ENODATA; + goto out; } else table_paddr = (msix->table.first << PAGE_SHIFT) + @@ -880,9 +897,8 @@ static int msix_capability_init(struct pci_dev *dev, if ( idx < 0 ) { - pci_conf_write16(dev->sbdf, msix_control_reg(pos), control); - xfree(entry); - return idx; + ret = idx; + goto out; } base = fix_to_virt(idx) + (entry_paddr & (PAGE_SIZE - 1)); @@ -906,12 +922,6 @@ static int msix_capability_init(struct pci_dev *dev, if ( !msix->used_entries ) { - maskall = false; - if ( !msix->guest_maskall ) - control &= ~PCI_MSIX_FLAGS_MASKALL; - else - control |= PCI_MSIX_FLAGS_MASKALL; - if ( rangeset_add_range(mmio_ro_ranges, msix->table.first, msix->table.last) ) WARN(); @@ -940,23 +950,13 @@ static int msix_capability_init(struct pci_dev *dev, WARN_ON(msix->table.first != (table_paddr >> PAGE_SHIFT)); ++msix->used_entries; - /* Restore MSI-X enabled bits */ - if ( !hardware_domain ) - { - /* - * ..., except for internal requests (before Dom0 starts), in which - * case we rather need to behave "normally", i.e. not follow the split - * brain model where Dom0 actually enables MSI (and disables INTx). - */ - pci_intx(dev, false); - control |= PCI_MSIX_FLAGS_ENABLE; - control &= ~PCI_MSIX_FLAGS_MASKALL; - maskall = 0; - } - msix->host_maskall = maskall; - pci_conf_write16(dev->sbdf, msix_control_reg(pos), control); + out: + if ( ret < 0 ) + xfree(entry); - return 0; + msix_update_unlock(dev, pos, control); + + return ret; } /** @@ -1180,7 +1180,7 @@ void pci_cleanup_msi(struct pci_dev *pdev) { /* Disable MSI and/or MSI-X */ msi_set_enable(pdev, 0); - msix_set_enable(pdev, 0); + msix_force_disable(pdev); msi_free_irqs(pdev); } @@ -1229,11 +1229,20 @@ int pci_msi_conf_write_intercept(struct pci_dev *pdev, unsigned int reg, if ( reg != msix_control_reg(pos) || size != 2 ) return -EACCES; + spin_lock_irq(&pdev->msix->control_lock); + + pdev->msix->guest_enable = !!(*data & PCI_MSIX_FLAGS_ENABLE); + if ( pdev->msix->host_enable ) + *data |= PCI_MSIX_FLAGS_ENABLE; pdev->msix->guest_maskall = !!(*data & PCI_MSIX_FLAGS_MASKALL); if ( pdev->msix->host_maskall ) *data |= PCI_MSIX_FLAGS_MASKALL; - return 1; + pci_conf_write16(pdev->sbdf, reg, *data); + + spin_unlock_irq(&pdev->msix->control_lock); + + return -EPERM; /* Already done the write. */ } } @@ -1324,15 +1333,12 @@ int pci_restore_msi_state(struct pci_dev *pdev) } else if ( !type && entry->msi_attrib.type == PCI_CAP_ID_MSIX ) { - control = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); - pci_conf_write16(pdev->sbdf, msix_control_reg(pos), - control | (PCI_MSIX_FLAGS_ENABLE | - PCI_MSIX_FLAGS_MASKALL)); + control = msix_update_lock(pdev, pos); + if ( unlikely(!memory_decoded(pdev)) ) { + msix_update_unlock(pdev, pos, control); spin_unlock_irqrestore(&desc->lock, flags); - pci_conf_write16(pdev->sbdf, msix_control_reg(pos), - control & ~PCI_MSIX_FLAGS_ENABLE); return -ENXIO; } } @@ -1372,8 +1378,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) } if ( type == PCI_CAP_ID_MSIX ) - pci_conf_write16(pdev->sbdf, msix_control_reg(pos), - control | PCI_MSIX_FLAGS_ENABLE); + msix_update_unlock(pdev, pos, control); return 0; } diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c index ce1a450f6f..436c78b7aa 100644 --- a/xen/drivers/passthrough/msi.c +++ b/xen/drivers/passthrough/msi.c @@ -44,6 +44,7 @@ int pdev_msi_init(struct pci_dev *pdev) return -ENOMEM; spin_lock_init(&msix->table_lock); + spin_lock_init(&msix->control_lock); ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos)); msix->nr_entries = msix_table_size(ctrl);