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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 5A59EC3DA61 for ; Sun, 21 Jul 2024 08:51:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=blMJ5CUEcmYvSUk3ZYMTYy2ptp/2K3A7omMls3BzyVA=; b=ELukHt9T6P2xbApPgDTiJltbVs hDlVlkUtr7l43kCT2NmPs1yTlGSM4yIPhPrtU1UpfV934llsYd3pd6H0I8d1HlOrTeFOBOZLgAUWc 6KtgMJQ4tQIBe3j+LjHjqa2FEK1QEB2Sdb0dsYvUVJas6WbGD4M/kL2xt9jrGm266gAI0MpkDjQSo T9LZZBKgLW6ZOu6NhmT5WL+HPDPWVxAdpYMdHVNKcTzgjk21Kd8wFpJrWwgh9Y/7/7VKROdvuzMGH GGDfcuUyYKZ1b3MqcJQGqD6PF1384M4aNfde36hiHd+Egu050kbxsSXxtsH8pmoIV67PBmGNRuk26 f4U2ctZw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sVSHU-00000006VDI-0ahW; Sun, 21 Jul 2024 08:51:00 +0000 Received: from mail-pj1-x102d.google.com ([2607:f8b0:4864:20::102d]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sVSH7-00000006V9I-2V6x for linux-arm-kernel@lists.infradead.org; Sun, 21 Jul 2024 08:50:39 +0000 Received: by mail-pj1-x102d.google.com with SMTP id 98e67ed59e1d1-2cd48ad7f0dso231150a91.0 for ; Sun, 21 Jul 2024 01:50:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1721551836; x=1722156636; darn=lists.infradead.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=blMJ5CUEcmYvSUk3ZYMTYy2ptp/2K3A7omMls3BzyVA=; b=qeSvEFTiz7yrI9rkyKUzdsp67VsApuKMU6W3ZSiSKLRvmv5BZvfXkHG9Uumeu26P6Z maJaJVOGLLvc0o079KaesJK5ZpaGdUjGRfWd5joFyUpJQgERFvF4frn294Xd/keQwgqQ W5QHcW2qn/effWv/nAEHjXfCTCuF0YvIT617xJ4Os/smX8aGBmHGVhQ4O9mhob2/c/S3 9qtD+JGWaOV13mcOa8czJYicoq88/VbThWhPLg9jpmsLEqFyTPRcDOsRTlWWC+MAijAx IHLF63DWqxTwegQCBADjY9kcYwjKmdC1EDQlhijq4sDD/yIeZpU+G6I9SWcwmMVHJxFZ QndQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721551836; x=1722156636; 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=blMJ5CUEcmYvSUk3ZYMTYy2ptp/2K3A7omMls3BzyVA=; b=XKJTSen4GhkKtAQ6EGlVrPXbEB48+D4KIPi7cx4rfp3SvQdgxaJIFs4gDBRngjDxja YMPvZDTPcBrFHgCThFzHO72FHnPEwOeKf+RPp7ShHriQZBzGaTJnzgyDOEN0mkfc4ra4 lvygjOFv91MfRwsnLqZ8WmoQw3AuKgXa4Hs5BnVZZxm1LmPeuq/ys6TIQdJbYoSZrjTd Mv7K59o2eLptz/AnObw26vDuEVT28EGJWhviltAW7GYGrWdfjAW8f/Hp2D1CfFgxCyrP NF6tkSSacp/fi3/yGiHUb1sp/PeaB4ppeO6b3UdDSfJbvAFHyGGT5WUNSHrmVN+PfMy2 6zgg== X-Forwarded-Encrypted: i=1; AJvYcCUCT4F4FqTsRz2gG0Z2popEQ/jreztT3LmXuM8RMA0wLBLQO8oRIocR0tqx22qMCmGLGBGY1ij2zdctoSkMJ2vHFZKLDSRyoki8BHEAXqS/C7K6f8M= X-Gm-Message-State: AOJu0Yyabe3vteWWo0tVmpBnyxuge9X9gOGEFcNhxXWMlCpQPWiOtUfA M60M345L5WoKZltMw7O4L1EOct+5RqJ2/99hjDShPKhdBnWNljdhjkh8mc07jg== X-Google-Smtp-Source: AGHT+IEEDmd00hA3jTB0a6TdvwUfKHPTBRovNKbfh5jFjeaEsgNkvsM6tDX15cW9DGrBUz0GPn7P3g== X-Received: by 2002:a17:90a:7307:b0:2c9:81fd:4c27 with SMTP id 98e67ed59e1d1-2cd27415a88mr2731703a91.14.1721551836504; Sun, 21 Jul 2024 01:50:36 -0700 (PDT) Received: from thinkpad ([120.56.206.118]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2ccf7b5c385sm4776539a91.18.2024.07.21.01.50.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 21 Jul 2024 01:50:36 -0700 (PDT) Date: Sun, 21 Jul 2024 14:20:32 +0530 From: Manivannan Sadhasivam To: Marc Zyngier Cc: tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: MSIs not freed in GICv3 ITS driver Message-ID: <20240721085032.GL1908@thinkpad> References: <20240708153933.GC5745@thinkpad> <865xtf4woi.wl-maz@kernel.org> <20240709173708.GA44420@thinkpad> <877cdupdvu.wl-maz@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <877cdupdvu.wl-maz@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240721_015037_681617_507AAD83 X-CRM114-Status: GOOD ( 53.04 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jul 09, 2024 at 08:24:37PM +0100, Marc Zyngier wrote: > On Tue, 09 Jul 2024 18:37:08 +0100, > Manivannan Sadhasivam wrote: > > > > On Mon, Jul 08, 2024 at 06:31:57PM +0100, Marc Zyngier wrote: > > > Mani, > > > > > > On Mon, 08 Jul 2024 16:39:33 +0100, > > > Manivannan Sadhasivam wrote: > > > > > > > > Hi Marc, Thomas, > > > > > > > > I'm seeing a weird behavior with GICv3 ITS driver while allocating MSIs from > > > > PCIe devices. When the PCIe driver (I'm using virtio_pci_common.c) tries to > > > > allocate non power of 2 MSIs (like 3), then the GICv3 MSI driver always rounds > > > > the MSI count to power of 2 to find the order. In this case, the order becomes 2 > > > > in its_alloc_device_irq(). > > > > > > That's because we can only allocate EventIDs as a number of ID > > > bits. So you can't have *1* MSI, nor 3. You can have 2, 4, 8, or > > > 2^24. This is a power-of-two architecture. > > > > > > > Ah okay. > > > > > > So 4 entries are allocated by bitmap_find_free_region(). > > > > > > Assuming you're calling about its_alloc_device_irq(), it looks like a > > > bug. Or rather, some laziness on my part. The thing is, this bitmap is > > > only dealing with sub-allocation in the pool that has been given to > > > the endpoint. So the power-of-two crap doesn't really matter unless > > > you are dealing with Multi-MSI, which has actual alignment > > > requirements. > > > > > > > Okay. > > > > > > > > > > But since the PCIe driver has only requested 3 MSIs, its_irq_domain_alloc() > > > > will only allocate 3 MSIs, leaving one bitmap entry unused. > > > > > > > > And when the driver frees the MSIs using pci_free_irq_vectors(), only 3 > > > > allocated MSIs were freed and their bitmap entries were also released. But the > > > > entry for the additional bitmap was never released. Due to this, > > > > its_free_device() was also never called, resulting in the ITS device not getting > > > > freed. > > > > > > > > So when the PCIe driver tries to request the MSIs again (PCIe device being > > > > removed and inserted back), because the ITS device was not freed previously, > > > > MSIs were again requested for the same ITS device. And due to the stale bitmap > > > > entry, the ITS driver refuses to allocate 4 MSIs as only 3 bitmap entries were > > > > available. This forces the PCIe driver to reduce the MSI count, which is sub > > > > optimal. > > > > > > > > This behavior might be applicable to other irqchip drivers handling MSI as well. > > > > I want to know if this behavior is already known with MSI and irqchip drivers? > > > > > > > > For fixing this issue, the PCIe drivers could always request MSIs of power of 2, > > > > and use a dummy MSI handler for the extra number of MSIs allocated. This could > > > > also be done in the generic MSI driver itself to avoid changes in the PCIe > > > > drivers. But I wouldn't say it is the best possible fix. > > > > > > No, that's terrible. This is just papering over a design mistake, and > > > I refuse to go down that road. > > > > > > > Agree. But what about other MSI drivers? And because of the MSI design, they > > also round the requested MSI count to power of 2, leading to unused vectors and > > those also wouldn't get freed. > > This has absolutely nothing to do with the "design" of MSIs. It has > everything to do with not special-casing Multi-MSI. > > > I think this power of 2 limitation should be > > imposed at the API level or in the MSI driver instead of silently keeping unused > > vectors in irqchip drivers. > > You really have the wrong end of the stick. The MSi API has *zero* > control over the payload allocation. How could it? The whole point of > having an MSI driver is to insulate the core code from such stuff. > Right, but because of the way most of the MSI drivers (not all?) use bitmap to allocate MSIs, this issue is also present in all of them. So I think the fix is not just applicable for the gic-its-v3 driver alone. > > > > > > > > > > Is there any other way to address this issue? Or am I missing something > > > > completely? > > > > > > Well, since each endpoint handled by an ITS has its allocation tracked > > > by a bitmap, it makes more sense to precisely track the allocation. > > > > > > Here's a quick hack that managed to survive a VM boot. It may even > > > work. The only problem with it is that it probably breaks a Multi-MSi > > > device sitting behind a non-transparent bridge that would get its MSIs > > > allocated after another device. In this case, we wouldn't honor the > > > required alignment and things would break. > > > > > > So take this as a proof of concept. If that works, I'll think of how > > > to deal with this crap in a more suitable way... > > > > > > > This works fine. Now the ITS driver allocates requested number of MSIs, thanks! > > Well, as I said, this breaks tons of other things so I'm not going to > merge this any time soon. Certainly not before Thomas gets his MSI > rework upstream. And then I need to work out how to deal with > Multi-MSI in the correct way. > > So don't hold your breath. > Sure thing. Thanks for getting it around quickly. I'll wait for a proper fix. - Mani -- மணிவண்ணன் சதாசிவம்