Message ID | 20240216184153.2714504-2-oliver.upton@linux.dev |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-69182-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:c619:b0:108:e6aa:91d0 with SMTP id hn25csp712792dyb; Fri, 16 Feb 2024 10:43:26 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCWoCLaqIdSK6zToLZ2Y8vype1/6BfF3lYZ+OJFx9hYMQzcbeuLUsiI4oArNjHZc4bISerUh7kXIzTCeQzq52ltCjCvEGg== X-Google-Smtp-Source: AGHT+IHOZEh0DpLpIQGU39LlF8F3Ypj884eCrUN+2Km8sSFJp52Ds/0aU1yizQ43cz1qrM4wGSGU X-Received: by 2002:a17:902:d2c2:b0:1db:2d82:e803 with SMTP id n2-20020a170902d2c200b001db2d82e803mr6633943plc.20.1708109006504; Fri, 16 Feb 2024 10:43:26 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708109006; cv=pass; d=google.com; s=arc-20160816; b=Vmw0H/A5KG9fKI28HihQHPw3t6Pxz/Q7DyH349OsHwb4U/F+MMFtsHKtDyzIwhUlyt nXbemDZbjbPHrWyRkXpmo27oB+yYLvep3lwwya0UVCnfxsrDbe/0m+LYu2CyWHTgFDHd aaffY812ZhFFpiYvoU6f/qJhGut57wtm8o04yGh8wUgtwtxX/rWBriOhxP+mJLgDw6kj G3iYCeTcduclEsTNKObNpMJ8S0UWcFe1qzzdF+cVnhIUJ4eJjeIhGU5NnM+7l48PriyQ oQEdIqyPlENbl7E52/XLFbl6ljXrWGGVy4U6FGvTnU46r8+QEAtydcee53/7bdWDGaRK O8ag== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=qQ663bMVQErfiC1nnRDiwhyxZXGKefti52HAGa4Tyg4=; fh=OGIcGdyoke6ohfCedcQQ3UJr3JDRxZcQDHVG7x10Yg0=; b=ZP+o3yRXZdeisEfpex7IBixA/lAcv2SQ6PCje2dwQ4DgzaDx9Qm7cbEgqW73sQVrge fss0RKqkpvcOajLucsdzYp+i4zLxTe1hIQWeaAnkG47GGkw54qzs8iFQGnCGLCj+MDtq S5DDJLNsdsl5v1eTx9jjKBVa8EUBbLpiOAxWwUqAjbv8oc8uL63yHLDipj1XPQ/+Bg0P AZnWQ1jJHcPc0ty2WB7dRuwxUcP77PXo+xZkTHFJvDjTFDzKrCcfH+yl16r16V7s/X8D crHPdp71dbFgovD4fwdDbZkLi2llJj0eXUoDKaLCQeqdVmiQFpaMRFFKhyutEq4oSVUq /DqA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b="H/JVGd4/"; arc=pass (i=1 spf=pass spfdomain=linux.dev dkim=pass dkdomain=linux.dev dmarc=pass fromdomain=linux.dev); spf=pass (google.com: domain of linux-kernel+bounces-69182-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-69182-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id z14-20020a170902ccce00b001dbb88392bbsi241636ple.449.2024.02.16.10.43.26 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 16 Feb 2024 10:43:26 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-69182-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b="H/JVGd4/"; arc=pass (i=1 spf=pass spfdomain=linux.dev dkim=pass dkdomain=linux.dev dmarc=pass fromdomain=linux.dev); spf=pass (google.com: domain of linux-kernel+bounces-69182-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-69182-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 3FA442839A1 for <ouuuleilei@gmail.com>; Fri, 16 Feb 2024 18:43:26 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 72E3713959C; Fri, 16 Feb 2024 18:42:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="H/JVGd4/" Received: from out-187.mta0.migadu.com (out-187.mta0.migadu.com [91.218.175.187]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CDCA2135A74 for <linux-kernel@vger.kernel.org>; Fri, 16 Feb 2024 18:42:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.187 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708108932; cv=none; b=kkoVcSF7glreMpLtdyR3kegV2K2fuPEs47pBXGy9cERzP9Jdlged4m7rSTDphHuFsqr0rFA8dPxBxq3UiLsT0LEfJe7VF27DYOzdO69ZjyEoN4d57L77Eve3cy6E2uFx8rRJ0j7kGCYsMBhHnUA+NwP83OZU2bXLy2BxDGqEDVg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708108932; c=relaxed/simple; bh=dH0k6IN0MHlaNID2kd6SHBELB8QrefZu66kvwU5n4cs=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=mU3Z4rr3pdjEMEeztTDDU9BYgI0FUlc9QpTWe8hMx+tGIX9sfEQRbjFF3xV0kx4oPxIO5Xl/F3UllHo3FZPvzZJllKjs4QYuY4cxDsVv+zMRB4Wlub3DzfOpCa0dR9imyvPuOqAjcDu2mN+8Vz00IhmHINFRLegIga2JbhxCOLk= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=H/JVGd4/; arc=none smtp.client-ip=91.218.175.187 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1708108927; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qQ663bMVQErfiC1nnRDiwhyxZXGKefti52HAGa4Tyg4=; b=H/JVGd4/7Qq9RFnGveFyggh0J3Hiz/SYlGE4oLCu0pRw3RL+NYbIT9UL+2eGUevSFydFmQ PChqNQZt1fy48ANP9BQdYkMZZY2UaaMHjgyxXAX09BXBszi6N6AdOjz0n4yFyxssgBGdGf +wOUy0mYdWR1ebkABIDqrOxY2J1j9AU= From: Oliver Upton <oliver.upton@linux.dev> To: kvmarm@lists.linux.dev Cc: kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Zenghui Yu <yuzenghui@huawei.com>, linux-kernel@vger.kernel.org, Oliver Upton <oliver.upton@linux.dev> Subject: [PATCH v3 01/10] KVM: arm64: vgic: Store LPIs in an xarray Date: Fri, 16 Feb 2024 18:41:44 +0000 Message-ID: <20240216184153.2714504-2-oliver.upton@linux.dev> In-Reply-To: <20240216184153.2714504-1-oliver.upton@linux.dev> References: <20240216184153.2714504-1-oliver.upton@linux.dev> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791082109580281315 X-GMAIL-MSGID: 1791082109580281315 |
Series |
KVM: arm64: Avoid serializing LPI get() / put()
|
|
Commit Message
Oliver Upton
Feb. 16, 2024, 6:41 p.m. UTC
Using a linked-list for LPIs is less than ideal as it of course requires
iterative searches to find a particular entry. An xarray is a better
data structure for this use case, as it provides faster searches and can
still handle a potentially sparse range of INTID allocations.
Start by storing LPIs in an xarray, punting usage of the xarray to a
subsequent change.
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
arch/arm64/kvm/vgic/vgic-init.c | 3 +++
arch/arm64/kvm/vgic/vgic-its.c | 16 ++++++++++++++++
arch/arm64/kvm/vgic/vgic.c | 1 +
include/kvm/arm_vgic.h | 2 ++
4 files changed, 22 insertions(+)
Comments
On 2024/2/17 02:41, Oliver Upton wrote: > Using a linked-list for LPIs is less than ideal as it of course requires > iterative searches to find a particular entry. An xarray is a better > data structure for this use case, as it provides faster searches and can > still handle a potentially sparse range of INTID allocations. > > Start by storing LPIs in an xarray, punting usage of the xarray to a > subsequent change. > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> [..] > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c > index db2a95762b1b..c126014f8395 100644 > --- a/arch/arm64/kvm/vgic/vgic.c > +++ b/arch/arm64/kvm/vgic/vgic.c > @@ -131,6 +131,7 @@ void __vgic_put_lpi_locked(struct kvm *kvm, struct vgic_irq *irq) > return; > > list_del(&irq->lpi_list); > + xa_erase(&dist->lpi_xa, irq->intid); We can get here *after* grabbing the vgic_cpu->ap_list_lock (e.g., vgic_flush_pending_lpis()/vgic_put_irq()). And as according to vGIC's "Locking order", we should disable interrupts before taking the xa_lock in xa_erase() and we would otherwise see bad things like deadlock.. It's not a problem before patch #10, where we drop the lpi_list_lock and start taking the xa_lock with interrupts enabled. Consider switching to use xa_erase_irq() instead? > dist->lpi_list_count--; > > kfree(irq); Thanks, Zenghui
Hi Zenghui, On Wed, Feb 21, 2024 at 12:30:24AM +0800, Zenghui Yu wrote: > On 2024/2/17 02:41, Oliver Upton wrote: > > Using a linked-list for LPIs is less than ideal as it of course requires > > iterative searches to find a particular entry. An xarray is a better > > data structure for this use case, as it provides faster searches and can > > still handle a potentially sparse range of INTID allocations. > > > > Start by storing LPIs in an xarray, punting usage of the xarray to a > > subsequent change. > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > [..] > > > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c > > index db2a95762b1b..c126014f8395 100644 > > --- a/arch/arm64/kvm/vgic/vgic.c > > +++ b/arch/arm64/kvm/vgic/vgic.c > > @@ -131,6 +131,7 @@ void __vgic_put_lpi_locked(struct kvm *kvm, struct vgic_irq *irq) > > return; > > list_del(&irq->lpi_list); > > + xa_erase(&dist->lpi_xa, irq->intid); > > We can get here *after* grabbing the vgic_cpu->ap_list_lock (e.g., > vgic_flush_pending_lpis()/vgic_put_irq()). And as according to vGIC's > "Locking order", we should disable interrupts before taking the xa_lock > in xa_erase() and we would otherwise see bad things like deadlock.. Nice catch! Yeah, the general intention was to disable interrupts outside of the xa_lock, however: > It's not a problem before patch #10, where we drop the lpi_list_lock and > start taking the xa_lock with interrupts enabled. Consider switching to > use xa_erase_irq() instead? I don't think this change is safe until #10, as the implied xa_unlock_irq() would re-enable interrupts before the lpi_list_lock is dropped. Or do I have wires crossed?
On Tue, 20 Feb 2024 16:30:24 +0000, Zenghui Yu <zenghui.yu@linux.dev> wrote: > > On 2024/2/17 02:41, Oliver Upton wrote: > > Using a linked-list for LPIs is less than ideal as it of course requires > > iterative searches to find a particular entry. An xarray is a better > > data structure for this use case, as it provides faster searches and can > > still handle a potentially sparse range of INTID allocations. > > > > Start by storing LPIs in an xarray, punting usage of the xarray to a > > subsequent change. > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > [..] > > > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c > > index db2a95762b1b..c126014f8395 100644 > > --- a/arch/arm64/kvm/vgic/vgic.c > > +++ b/arch/arm64/kvm/vgic/vgic.c > > @@ -131,6 +131,7 @@ void __vgic_put_lpi_locked(struct kvm *kvm, struct vgic_irq *irq) > > return; > > list_del(&irq->lpi_list); > > + xa_erase(&dist->lpi_xa, irq->intid); > > We can get here *after* grabbing the vgic_cpu->ap_list_lock (e.g., > vgic_flush_pending_lpis()/vgic_put_irq()). And as according to vGIC's > "Locking order", we should disable interrupts before taking the xa_lock > in xa_erase() and we would otherwise see bad things like deadlock.. > > It's not a problem before patch #10, where we drop the lpi_list_lock and > start taking the xa_lock with interrupts enabled. Consider switching to > use xa_erase_irq() instead? But does it actually work? xa_erase_irq() uses spin_lock_irq(), followed by spin_unlock_irq(). So if we were already in interrupt context, we would end-up reenabling interrupts. At least, this should be the irqsave version. The question is whether we manipulate LPIs (in the get/put sense) on the back of an interrupt handler (like we do for the timer). It isn't obvious to me that it is the case, but I haven't spent much time staring at this code recently. Thanks, M.
On Tue, Feb 20, 2024 at 05:24:50PM +0000, Marc Zyngier wrote: > On Tue, 20 Feb 2024 16:30:24 +0000, > Zenghui Yu <zenghui.yu@linux.dev> wrote: > > > > On 2024/2/17 02:41, Oliver Upton wrote: > > > Using a linked-list for LPIs is less than ideal as it of course requires > > > iterative searches to find a particular entry. An xarray is a better > > > data structure for this use case, as it provides faster searches and can > > > still handle a potentially sparse range of INTID allocations. > > > > > > Start by storing LPIs in an xarray, punting usage of the xarray to a > > > subsequent change. > > > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > > > [..] > > > > > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c > > > index db2a95762b1b..c126014f8395 100644 > > > --- a/arch/arm64/kvm/vgic/vgic.c > > > +++ b/arch/arm64/kvm/vgic/vgic.c > > > @@ -131,6 +131,7 @@ void __vgic_put_lpi_locked(struct kvm *kvm, struct vgic_irq *irq) > > > return; > > > list_del(&irq->lpi_list); > > > + xa_erase(&dist->lpi_xa, irq->intid); > > > > We can get here *after* grabbing the vgic_cpu->ap_list_lock (e.g., > > vgic_flush_pending_lpis()/vgic_put_irq()). And as according to vGIC's > > "Locking order", we should disable interrupts before taking the xa_lock > > in xa_erase() and we would otherwise see bad things like deadlock.. > > > > It's not a problem before patch #10, where we drop the lpi_list_lock and > > start taking the xa_lock with interrupts enabled. Consider switching to > > use xa_erase_irq() instead? > > But does it actually work? xa_erase_irq() uses spin_lock_irq(), > followed by spin_unlock_irq(). So if we were already in interrupt > context, we would end-up reenabling interrupts. At least, this should > be the irqsave version. This is what I was planning to do, although I may kick it out to patch 10 to avoid churn. > The question is whether we manipulate LPIs (in the get/put sense) on > the back of an interrupt handler (like we do for the timer). It isn't > obvious to me that it is the case, but I haven't spent much time > staring at this code recently. I think we can get into here both from contexts w/ interrupts disabled or enabled. irqfd_wakeup() expects to be called w/ interrupts disabled. All the more reason to use irqsave() / irqrestore() flavors of all of this, and a reminder to go check all callsites that implicitly take the xa_lock.
On Tue, 20 Feb 2024 17:43:03 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > On Tue, Feb 20, 2024 at 05:24:50PM +0000, Marc Zyngier wrote: > > On Tue, 20 Feb 2024 16:30:24 +0000, > > Zenghui Yu <zenghui.yu@linux.dev> wrote: > > > > > > On 2024/2/17 02:41, Oliver Upton wrote: > > > > Using a linked-list for LPIs is less than ideal as it of course requires > > > > iterative searches to find a particular entry. An xarray is a better > > > > data structure for this use case, as it provides faster searches and can > > > > still handle a potentially sparse range of INTID allocations. > > > > > > > > Start by storing LPIs in an xarray, punting usage of the xarray to a > > > > subsequent change. > > > > > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > > > > > [..] > > > > > > > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c > > > > index db2a95762b1b..c126014f8395 100644 > > > > --- a/arch/arm64/kvm/vgic/vgic.c > > > > +++ b/arch/arm64/kvm/vgic/vgic.c > > > > @@ -131,6 +131,7 @@ void __vgic_put_lpi_locked(struct kvm *kvm, struct vgic_irq *irq) > > > > return; > > > > list_del(&irq->lpi_list); > > > > + xa_erase(&dist->lpi_xa, irq->intid); > > > > > > We can get here *after* grabbing the vgic_cpu->ap_list_lock (e.g., > > > vgic_flush_pending_lpis()/vgic_put_irq()). And as according to vGIC's > > > "Locking order", we should disable interrupts before taking the xa_lock > > > in xa_erase() and we would otherwise see bad things like deadlock.. > > > > > > It's not a problem before patch #10, where we drop the lpi_list_lock and > > > start taking the xa_lock with interrupts enabled. Consider switching to > > > use xa_erase_irq() instead? > > > > But does it actually work? xa_erase_irq() uses spin_lock_irq(), > > followed by spin_unlock_irq(). So if we were already in interrupt > > context, we would end-up reenabling interrupts. At least, this should > > be the irqsave version. > > This is what I was planning to do, although I may kick it out to patch > 10 to avoid churn. > > > The question is whether we manipulate LPIs (in the get/put sense) on > > the back of an interrupt handler (like we do for the timer). It isn't > > obvious to me that it is the case, but I haven't spent much time > > staring at this code recently. > > I think we can get into here both from contexts w/ interrupts disabled > or enabled. irqfd_wakeup() expects to be called w/ interrupts disabled. > > All the more reason to use irqsave() / irqrestore() flavors of all of > this, and a reminder to go check all callsites that implicitly take the > xa_lock. Sounds good. Maybe you can also update the locking order "documentation" to include the xa_lock? I expect that it will ultimately replace lpi_list_lock. Thanks, M.
On Tue, Feb 20, 2024 at 05:53:55PM +0000, Marc Zyngier wrote: > On Tue, 20 Feb 2024 17:43:03 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > I think we can get into here both from contexts w/ interrupts disabled > > or enabled. irqfd_wakeup() expects to be called w/ interrupts disabled. > > > > All the more reason to use irqsave() / irqrestore() flavors of all of > > this, and a reminder to go check all callsites that implicitly take the > > xa_lock. > > Sounds good. Maybe you can also update the locking order > "documentation" to include the xa_lock? I expect that it will > ultimately replace lpi_list_lock. Yep, I got to the point of deleting the lpi_list_lock on the full series, which is where I update the documentation. I really didn't want people to know I'm adding yet another layer of locking in the interim... Anyways, I think there's sufficient feedback to justify a respin. I'll make sure the documentation is updated w/ the xa_lock for the stuff I'm trying to land in 6.9.
On 2024/2/21 1:15, Oliver Upton wrote: > Hi Zenghui, > > On Wed, Feb 21, 2024 at 12:30:24AM +0800, Zenghui Yu wrote: >> On 2024/2/17 02:41, Oliver Upton wrote: >>> Using a linked-list for LPIs is less than ideal as it of course requires >>> iterative searches to find a particular entry. An xarray is a better >>> data structure for this use case, as it provides faster searches and can >>> still handle a potentially sparse range of INTID allocations. >>> >>> Start by storing LPIs in an xarray, punting usage of the xarray to a >>> subsequent change. >>> >>> Signed-off-by: Oliver Upton <oliver.upton@linux.dev> >> >> [..] >> >>> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c >>> index db2a95762b1b..c126014f8395 100644 >>> --- a/arch/arm64/kvm/vgic/vgic.c >>> +++ b/arch/arm64/kvm/vgic/vgic.c >>> @@ -131,6 +131,7 @@ void __vgic_put_lpi_locked(struct kvm *kvm, struct vgic_irq *irq) >>> return; >>> list_del(&irq->lpi_list); >>> + xa_erase(&dist->lpi_xa, irq->intid); >> >> We can get here *after* grabbing the vgic_cpu->ap_list_lock (e.g., >> vgic_flush_pending_lpis()/vgic_put_irq()). And as according to vGIC's >> "Locking order", we should disable interrupts before taking the xa_lock >> in xa_erase() and we would otherwise see bad things like deadlock.. > > Nice catch! > > Yeah, the general intention was to disable interrupts outside of the > xa_lock, however: > >> It's not a problem before patch #10, where we drop the lpi_list_lock and >> start taking the xa_lock with interrupts enabled. Consider switching to >> use xa_erase_irq() instead? > > I don't think this change is safe until #10, as the implied xa_unlock_irq() > would re-enable interrupts before the lpi_list_lock is dropped. Or do I > have wires crossed? No, you're right. My intention was to fix it in patch #10. And as you've both pointed out, using xa_erase_irq() can hardly be the correct fix. My mistake :-( . Thanks, Zenghui
On Wed, Feb 21, 2024 at 01:11:02PM +0800, Zenghui Yu wrote: > No, you're right. My intention was to fix it in patch #10. And as > you've both pointed out, using xa_erase_irq() can hardly be the correct > fix. My mistake :-( . Still -- thank you for pointing out the very obvious bug at the end of the series :)
diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c index e949e1d0fd9f..411719053107 100644 --- a/arch/arm64/kvm/vgic/vgic-init.c +++ b/arch/arm64/kvm/vgic/vgic-init.c @@ -56,6 +56,7 @@ void kvm_vgic_early_init(struct kvm *kvm) INIT_LIST_HEAD(&dist->lpi_list_head); INIT_LIST_HEAD(&dist->lpi_translation_cache); raw_spin_lock_init(&dist->lpi_list_lock); + xa_init_flags(&dist->lpi_xa, XA_FLAGS_LOCK_IRQ); } /* CREATION */ @@ -366,6 +367,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm) if (vgic_supports_direct_msis(kvm)) vgic_v4_teardown(kvm); + + xa_destroy(&dist->lpi_xa); } static void __kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu) diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index e2764d0ffa9f..fb2d3c356984 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -52,6 +52,12 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid, if (!irq) return ERR_PTR(-ENOMEM); + ret = xa_reserve_irq(&dist->lpi_xa, intid, GFP_KERNEL_ACCOUNT); + if (ret) { + kfree(irq); + return ERR_PTR(ret); + } + INIT_LIST_HEAD(&irq->lpi_list); INIT_LIST_HEAD(&irq->ap_list); raw_spin_lock_init(&irq->irq_lock); @@ -86,12 +92,22 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid, goto out_unlock; } + ret = xa_err(xa_store(&dist->lpi_xa, intid, irq, 0)); + if (ret) { + xa_release(&dist->lpi_xa, intid); + kfree(irq); + goto out_unlock; + } + list_add_tail(&irq->lpi_list, &dist->lpi_list_head); dist->lpi_list_count++; out_unlock: raw_spin_unlock_irqrestore(&dist->lpi_list_lock, flags); + if (ret) + return ERR_PTR(ret); + /* * We "cache" the configuration table entries in our struct vgic_irq's. * However we only have those structs for mapped IRQs, so we read in diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c index db2a95762b1b..c126014f8395 100644 --- a/arch/arm64/kvm/vgic/vgic.c +++ b/arch/arm64/kvm/vgic/vgic.c @@ -131,6 +131,7 @@ void __vgic_put_lpi_locked(struct kvm *kvm, struct vgic_irq *irq) return; list_del(&irq->lpi_list); + xa_erase(&dist->lpi_xa, irq->intid); dist->lpi_list_count--; kfree(irq); diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 8cc38e836f54..795b35656b54 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -13,6 +13,7 @@ #include <linux/spinlock.h> #include <linux/static_key.h> #include <linux/types.h> +#include <linux/xarray.h> #include <kvm/iodev.h> #include <linux/list.h> #include <linux/jump_label.h> @@ -275,6 +276,7 @@ struct vgic_dist { /* Protects the lpi_list and the count value below. */ raw_spinlock_t lpi_list_lock; + struct xarray lpi_xa; struct list_head lpi_list_head; int lpi_list_count;