From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from 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 39D8AE7716A for ; Tue, 17 Dec 2024 10:14:18 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.858720.1270957 (Exim 4.92) (envelope-from ) id 1tNUaX-0001ew-Hw; Tue, 17 Dec 2024 10:14:01 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 858720.1270957; Tue, 17 Dec 2024 10:14:01 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1tNUaX-0001ep-Dg; Tue, 17 Dec 2024 10:14:01 +0000 Received: by outflank-mailman (input) for mailman id 858720; Tue, 17 Dec 2024 10:14:00 +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 1tNUaW-0001ej-SK for xen-devel@lists.xenproject.org; Tue, 17 Dec 2024 10:14:00 +0000 Received: from mail-wr1-x434.google.com (mail-wr1-x434.google.com [2a00:1450:4864:20::434]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id a1484a17-bc5f-11ef-99a3-01e77a169b0f; Tue, 17 Dec 2024 11:13:58 +0100 (CET) Received: by mail-wr1-x434.google.com with SMTP id ffacd0b85a97d-385d7f19f20so2554426f8f.1 for ; Tue, 17 Dec 2024 02:13:58 -0800 (PST) Received: from localhost ([84.78.159.3]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-388c7fac5bbsm11066562f8f.0.2024.12.17.02.13.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Dec 2024 02:13:57 -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 Sender: "Xen-devel" X-Inumbo-ID: a1484a17-bc5f-11ef-99a3-01e77a169b0f DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=citrix.com; s=google; t=1734430438; x=1735035238; darn=lists.xenproject.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=u8arajGDs6ktkoavY4rzlsf1c1cP5MJcU3EzxUXBa+U=; b=PUSuRzTq6G3vsPEVBOfsYLRKgH74gG90TUVYm/t7TI9TUt/UUW7MPlsTFuRh1oRLxQ x4d8eHBVNRhkuyK4wvg1Ss7dFTXJ4a9Q3bttErsfJc4OpQGH+uLEy4Xa15WEgzZN7Bsx 3gc/jCbfrvZsG8gloTZNK3SavIZCSv/uFpnl8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734430438; x=1735035238; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=u8arajGDs6ktkoavY4rzlsf1c1cP5MJcU3EzxUXBa+U=; b=miiswObtsaA8/U8vx7MYVQEALWPfFGysbla0WcojrjY+XZLgVu12gsbyISKtJMKI+9 xOKTFOVOA6cIynp/TDwKT78XS1jREI7KvXxMGZk81PDd7pbldt1YVGF0JhuKb38n12bP 4UNK5v4C3nq5q/OFg7rLnoUB2Wl8wjeQsVL21dxVlUomU6exLe1EOVDA8bk9y2Yzwu3O JHsvb40RoqE1zpuwhdewxmaVhCHDKckibEHtSO8u2jenvrlKhbO58JFQO2sfE2DGyhl+ MBBTLWHF69zk+LIAcPB9RgmHs887Vsj2+8byMya8Jl/O3WoJiK0k7rfs6uFSXMcpDsLd IV2Q== X-Forwarded-Encrypted: i=1; AJvYcCUHpYdJ13qgsjjDiZxEZX6EfxkzCgSJcFysP88kBAwIt4qohUYjlR8o00nl3wV4Y8Rbg+2+acYYldY=@lists.xenproject.org X-Gm-Message-State: AOJu0YyY6dlIQN8aCY7Ap0vjLmDpxEfU6blG17rLMipcF3R77NHwdZnR Bcv8EkfvB7vWRB6mTjpYzVp7z2lF1B8Bi+gTrB+rUBrX5H5QDtNRNd3JSXZOUSo= X-Gm-Gg: ASbGnctVBhduiykTkXPvq5OIXLgAPufzvflfx3dhZEj0dU18v39PODvt8Z8f/icLtRh 5s0C8tt+Iy7H5gTlVVbvvMIP5PzVDPn0HnuNN99Z+fsiqQkdU7eDHvjn4434yzyj3uoMwz4Ja8i 9v3Ux73lqsPM55M11e2XW0j2dHQIzlC7JfVwDAtEaaj+/22LRhhBisq/EfucFv5gaoFUxAHRPTw SIElMT+bFgV59fzOodZB/QayYryBSA8t76iVnLBmv5E4mDeIhsZFes28994Aw== X-Google-Smtp-Source: AGHT+IFZOYiH3Lp+d7c3IwqfVvrJzv9LfhQkadnYh+yiXsg63+Fi3WdnIKMzN5v9yy0Mix+L9bzXEw== X-Received: by 2002:a05:6000:4026:b0:385:e879:45cc with SMTP id ffacd0b85a97d-388da3941d2mr2416167f8f.19.1734430438094; Tue, 17 Dec 2024 02:13:58 -0800 (PST) Date: Tue, 17 Dec 2024 11:13:56 +0100 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Jan Beulich Cc: Andrew Cooper , Sergii Dmytruk , xen-devel@lists.xenproject.org Subject: Re: [PATCH] x86/io-apic: prevent early exit from i8259 loop detection Message-ID: References: <20241217090045.6251-1-roger.pau@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Tue, Dec 17, 2024 at 10:40:31AM +0100, Jan Beulich wrote: > On 17.12.2024 10:00, Roger Pau Monne wrote: > > Avoid exiting early from the loop when a pin that could be connected to the > > i8259 is found, as such early exit would leave the EOI handler translation > > array only partially allocated and/or initialized. > > > > Otherwise on systems with multiple IO-APICs and an unmasked ExtINT pin on > > any IO-APIC that's no the last one the following NULL pointer dereference > > triggers: > > > > (XEN) Enabling APIC mode. Using 2 I/O APICs > > (XEN) ----[ Xen-4.20-unstable x86_64 debug=y Not tainted ]---- > > (XEN) CPU: 0 > > (XEN) RIP: e008:[] __ioapic_write_entry+0x83/0x95 > > [...] > > (XEN) Xen call trace: > > (XEN) [] R __ioapic_write_entry+0x83/0x95 > > (XEN) [] F amd_iommu_ioapic_update_ire+0x1ea/0x273 > > (XEN) [] F iommu_update_ire_from_apic+0xa/0xc > > (XEN) [] F __ioapic_write_entry+0x93/0x95 > > (XEN) [] F arch/x86/io_apic.c#clear_IO_APIC_pin+0x7c/0x10e > > (XEN) [] F arch/x86/io_apic.c#clear_IO_APIC+0x2d/0x61 > > (XEN) [] F enable_IO_APIC+0x2e3/0x34f > > (XEN) [] F smp_prepare_cpus+0x254/0x27a > > (XEN) [] F __start_xen+0x1ce1/0x23ae > > (XEN) [] F __high_start+0x8e/0x90 > > (XEN) > > (XEN) Pagetable walk from 0000000000000000: > > (XEN) L4[0x000] = 000000007dbfd063 ffffffffffffffff > > (XEN) L3[0x000] = 000000007dbfa063 ffffffffffffffff > > (XEN) L2[0x000] = 000000007dbcc063 ffffffffffffffff > > (XEN) L1[0x000] = 0000000000000000 ffffffffffffffff > > (XEN) > > (XEN) **************************************** > > (XEN) Panic on CPU 0: > > (XEN) FATAL PAGE FAULT > > (XEN) [error_code=0002] > > (XEN) Faulting linear address: 0000000000000000 > > (XEN) **************************************** > > (XEN) > > (XEN) Reboot in five seconds... > > > > Reported-by: Sergii Dmytruk > > Fixes: 86001b3970fe ('x86/io-apic: fix directed EOI when using AMD-Vi interrupt remapping') > > Signed-off-by: Roger Pau Monné > > Hmm, considering the earlier change was backported, I'm even inclined to > delay 4.18.4 a little, for taking this one there as well. > > > --- a/xen/arch/x86/io_apic.c > > +++ b/xen/arch/x86/io_apic.c > > @@ -1389,14 +1389,15 @@ void __init enable_IO_APIC(void) > > /* If the interrupt line is enabled and in ExtInt mode > > * I have found the pin where the i8259 is connected. > > */ > > - if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT)) { > > + if ( ioapic_i8259.apic == -1 && entry.mask == 0 && > > + entry.delivery_mode == dest_ExtINT ) > > + { > > + ASSERT(ioapic_i8259.pin == -1); > > I'm not sure of the value of this assertion. It is provable that ... > > > ioapic_i8259.apic = apic; > > ioapic_i8259.pin = pin; > > ... both fields are updated together (and not earlier on), and hence once > we've been here neither field will still be -1. No strong opinion, as all asserts I've placed it here in case the logic to set apic or pin would change. > Looking around I further notice that it's generally ioapic_i8259.pin that > we check against -1, so I wonder whether - just for consistency - the if() > condition wouldn't better too use that one. As with the above, no strong opinion really, it's true that most checks tend to use ioapic_i8259.pin != -1 instead of the apic field, so I can adjust to that. > Preferably with these adjustments: > Reviewed-by: Jan Beulich Thanks, Roger.