From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B577F1FDA7C for ; Mon, 27 Jan 2025 10:07:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.180 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737972472; cv=none; b=HwIhe1yzl8f5/RuppCgpr/sgSnVmhyQIMsbCgFUHhrTW66YBnfFmbBhxTW6cDBCTJk2Lb/4O7JNUP9LzxRflQ1LhGkoW7m2OxDu62MwT9V2h+VC/eYCTuM2XxXT4GGbdW2rMzhvmiqrHFsBArAxebqDA/3Mk2UflPnV2MOoWM2M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737972472; c=relaxed/simple; bh=eL5/43sgCUlqrJSROK17HVYRRpMfXDtN+7cw1v/9OX4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Gf4X/LZW0nB8iRy0IsjPvRUzFscgH9eK3jrjtoqICRKWtbp9znA2HcSQZI3hxLqAtotVEePYbjHhGm+4Unv327i3T066lzDEuh/VEnYlAaSyVETqa5JelxA3UGYoz/hVnK0l7n+5QXYmWGUuoq8kUT/Cv79rvWTuK/siyTu/x6c= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=YiDr9AGg; arc=none smtp.client-ip=209.85.214.180 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="YiDr9AGg" Received: by mail-pl1-f180.google.com with SMTP id d9443c01a7336-21628b3fe7dso71504595ad.3 for ; Mon, 27 Jan 2025 02:07:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1737972470; x=1738577270; darn=vger.kernel.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=070oXrw6JzlQMhj9jx9C9mBIZS3mua+LEXrhoFmHLoo=; b=YiDr9AGg5K5plCy7smvTgyvGODhhTdB99pNZ+ixNNGKcEUPoYpffdlOxIV6yzZL4/f 2uf7zR0FV6TeqeSV8Ask9Hq37mlvFGTrcZ06to6Qoqk64t2BLQDiMQ0gogai218IkyAm oRMGtAii+IdMsA8IyRI+LDlGNV8HWXqSYajDL5TK7ZqP2S4ls0fMaGzE2qSa4LQW5MFC gwFnaM5NSKKmFpOXG5NciKxKxZNhDYUp5HZWLKe1tfyLFNCpke7P+6/8XILVVVk+ppng JZafNafmxn62kiWRG1Cl7ICC9YMPGBmFzbfOSZLBLBeiL/xMDWKaQXEyZMUhzyHSfQW0 GcGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737972470; x=1738577270; 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=070oXrw6JzlQMhj9jx9C9mBIZS3mua+LEXrhoFmHLoo=; b=ruRlSfzOa5TkTz1Z1wJuxw9MzMaJ60Dfy2V7+YUsUi7QZSAsokOyXC7oS99zElkTqJ 8cVeS2AgJke1CHsGsX4fjtGjOXyb+TewZrTPISOMrvVMHLvPHxMeNdplbRTSGrlcH+LZ OzLko8PcPIrsjBg93UhcCFiwOHShD9TujZJAH2INiYYoXJ7OId8yMhZ1KtGjwFAFtgru o+Jb5awPo0exG1ifOQaSOr3K7dLCi8A4B4b8jzX0PL1NGhhVEEthME5m3Zn5VT2kdFdb HjfoUTeK/7EEAOeF7qEpZ5PXmbxRQTeQ7f+Bvh7GAuukuOXf/SW7vEekj3O1spjVFfo1 mBNw== X-Forwarded-Encrypted: i=1; AJvYcCVwojiUw8EWe6f+pVua731i6lh+h6In5TyvkmMqvEopSHynUK7opvbJuk+FUNk2gU1CP6Cdgnxe3UKFnHE=@vger.kernel.org X-Gm-Message-State: AOJu0YxRIEw7dngHYvghUJyX7NCsulS8BzNamjpITTk5TQG/xRnKufOL y8/jv2uDIH0LcJe0ziCLgwf/VyL5PYYdlY3X0mXpxUSbdHIBftVwkVhOZ3BdKw== X-Gm-Gg: ASbGncus5d64QXiSD8GwcokEf7/FKfwR6SW3L3tsIA6d0w/Tm6pppjFZp/eQPQj82cD fu+k/L2qWfZsEXsp0fMt1RvtgqDTxGzvZ+FLR9IDCEtijQy7Q+xK4vFM8ArCHXcg9hYcCCjSpAJ 3TUN42cJOLDTCZwDZVq9lw9GW4ILmfQ+0rwqsV+325uE2iHEwIeah5PNA9HJN9bfB9XLhdG5b4Z LQAqRJOXk09XPDXnd9GioigZBNgI/QVgJ3RjI9EZ1w3Q0HOJM2cfWRYhJp+BY8WcL1prxh3infU mqcEr7b2i+iH43I= X-Google-Smtp-Source: AGHT+IHee6gZn25MWqnfAN2HN9vXXC5H6tQFag5Q+tJiCVAy9I4SFkdjChUG8SjSThBL4GQOPibqPA== X-Received: by 2002:a17:902:c950:b0:215:7dbf:f3de with SMTP id d9443c01a7336-21c35558d37mr676086415ad.28.1737972469930; Mon, 27 Jan 2025 02:07:49 -0800 (PST) Received: from thinkpad ([120.60.139.80]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-21da3d9e1besm59681075ad.11.2025.01.27.02.07.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Jan 2025 02:07:49 -0800 (PST) Date: Mon, 27 Jan 2025 15:37:40 +0530 From: Manivannan Sadhasivam To: Daniel Tsai Cc: Jingoo Han , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Rob Herring , Bjorn Helgaas , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] PCI: dwc: Separate MSI out to different controller Message-ID: <20250127100740.fqvg2bflu4fpqbr5@thinkpad> References: <20250115083215.2781310-1-danielsftsai@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20250115083215.2781310-1-danielsftsai@google.com> Subject needs to be improved. Something like, PCI: dwc: Support IRQ affinity by assigning MSIs to supported MSI controller On Wed, Jan 15, 2025 at 08:32:15AM +0000, Daniel Tsai wrote: > From: Tsai Sung-Fu > > Setup the struct irq_affinity at EP side, What do you mean by 'EP side'? > and passing that as 1 of the > function parameter when endpoint calls pci_alloc_irq_vectors_affinity, > this could help to setup non-default irq_affinity for target irq (end up > in irq_desc->irq_common_data.affinity), and we can make use of that to > separate msi vector out to bind to other msi controller. > BY 'other msi controller' you mean the parent interrupt controller that is chained by the DWC MSI controller? > In current design, we have 8 msi controllers, and each of them own up to Current design of what? I believe that you guys at Google want to add support for your secret future SoC, but atleast mention that in the patch descriptions. Otherwise, we don't know whether the patch applies to currently submitted platforms or future ones. > 32 msi vectors, layout as below > > msi_controller0 <- msi_vector0 ~ 31 > msi_controller1 <- msi_vector32 ~ 63 > msi_controller2 <- msi_vector64 ~ 95 > . > . > . > msi_controller7 <- msi_vector224 ~ 255 > > dw_pcie_irq_domain_alloc is allocating msi vector number in a contiguous > fashion, that would end up those allocated msi vectors all handled by > the same msi_controller, which all of them would have irq affinity in > equal. To separate out to different CPU, we need to distribute msi > vectors to different msi_controller, which require to allocate the msi > vector in a stride fashion. > > To do that, the CL make use the cpumask_var_t setup by the endpoint, What is 'CL'? > compare against to see if the affinities are the same, if they are, > bind to msi controller which previously allocated msi vector goes to, if > they are not, assign to new msi controller. > > Signed-off-by: Tsai Sung-Fu > --- > .../pci/controller/dwc/pcie-designware-host.c | 80 +++++++++++++++---- > drivers/pci/controller/dwc/pcie-designware.h | 2 + > 2 files changed, 67 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index d2291c3ceb8be..192d05c473b3b 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -181,25 +181,75 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain, > void *args) > { > struct dw_pcie_rp *pp = domain->host_data; > - unsigned long flags; > - u32 i; > - int bit; > + const struct cpumask *mask; > + unsigned long flags, index, start, size; > + int irq, ctrl, p_irq, *msi_vec_index; > + unsigned int controller_count = (pp->num_vectors / MAX_MSI_IRQS_PER_CTRL); > + > + /* > + * All IRQs on a given controller will use the same parent interrupt, > + * and therefore the same CPU affinity. We try to honor any CPU spreading > + * requests by assigning distinct affinity masks to distinct vectors. > + * So instead of always allocating the msi vectors in a contiguous fashion, > + * the algo here honor whoever comes first can bind the msi controller to > + * its irq affinity mask, or compare its cpumask against > + * currently recorded to decide if binding to this msi controller. > + */ > + > + msi_vec_index = kcalloc(nr_irqs, sizeof(*msi_vec_index), GFP_KERNEL); > + if (!msi_vec_index) > + return -ENOMEM; > > raw_spin_lock_irqsave(&pp->lock, flags); > > - bit = bitmap_find_free_region(pp->msi_irq_in_use, pp->num_vectors, > - order_base_2(nr_irqs)); > + for (irq = 0; irq < nr_irqs; irq++) { > + mask = irq_get_affinity_mask(virq + irq); > + for (ctrl = 0; ctrl < controller_count; ctrl++) { > + start = ctrl * MAX_MSI_IRQS_PER_CTRL; > + size = start + MAX_MSI_IRQS_PER_CTRL; > + if (find_next_bit(pp->msi_irq_in_use, size, start) >= size) { > + cpumask_copy(&pp->msi_ctrl_to_cpu[ctrl], mask); > + break; I don't see how this relates to the affinity setting of the parent interrupt line that the MSI controller is muxed to. Here you are just copying the target MSI affinity mask to the msi_ctrl_to_cpu[] array entry of the controller. And as per the hierarchial IRQ domain implementation, DWC MSI cannot set the IRQ affinity on its own and that's why MSI_FLAG_NO_AFFINITY flag is set in this driver. So I don't see how this patch is relevant. Am I missing something? - Mani > + } > > - raw_spin_unlock_irqrestore(&pp->lock, flags); > + if (cpumask_equal(&pp->msi_ctrl_to_cpu[ctrl], mask) && > + find_next_zero_bit(pp->msi_irq_in_use, size, start) < size) > + break; > + } > > - if (bit < 0) > - return -ENOSPC; > + /* > + * no msi controller matches, we would error return (no space) and > + * clear those previously allocated bit, because all those msi vectors > + * didn't really successfully allocated, so we shouldn't occupied that > + * position in the bitmap in case other endpoint may still make use of > + * those. An extra step when choosing to not allocate in contiguous > + * fashion. > + */ > + if (ctrl == controller_count) { > + for (p_irq = irq - 1; p_irq >= 0; p_irq--) > + bitmap_clear(pp->msi_irq_in_use, msi_vec_index[p_irq], 1); > + raw_spin_unlock_irqrestore(&pp->lock, flags); > + kfree(msi_vec_index); > + return -ENOSPC; > + } > + > + index = bitmap_find_next_zero_area(pp->msi_irq_in_use, > + size, > + start, > + 1, > + 0); > + bitmap_set(pp->msi_irq_in_use, index, 1); > + msi_vec_index[irq] = index; > + } > > - for (i = 0; i < nr_irqs; i++) > - irq_domain_set_info(domain, virq + i, bit + i, > + raw_spin_unlock_irqrestore(&pp->lock, flags); > + > + for (irq = 0; irq < nr_irqs; irq++) > + irq_domain_set_info(domain, virq + irq, msi_vec_index[irq], > pp->msi_irq_chip, > pp, handle_edge_irq, > NULL, NULL); > + kfree(msi_vec_index); > > return 0; > } > @@ -207,15 +257,15 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain, > static void dw_pcie_irq_domain_free(struct irq_domain *domain, > unsigned int virq, unsigned int nr_irqs) > { > - struct irq_data *d = irq_domain_get_irq_data(domain, virq); > + struct irq_data *d; > struct dw_pcie_rp *pp = domain->host_data; > unsigned long flags; > > raw_spin_lock_irqsave(&pp->lock, flags); > - > - bitmap_release_region(pp->msi_irq_in_use, d->hwirq, > - order_base_2(nr_irqs)); > - > + for (int i = 0; i < nr_irqs; i++) { > + d = irq_domain_get_irq_data(domain, virq + i); > + bitmap_clear(pp->msi_irq_in_use, d->hwirq, 1); > + } > raw_spin_unlock_irqrestore(&pp->lock, flags); > } > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 347ab74ac35aa..95629b37a238e 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -373,6 +374,7 @@ struct dw_pcie_rp { > struct irq_chip *msi_irq_chip; > u32 num_vectors; > u32 irq_mask[MAX_MSI_CTRLS]; > + struct cpumask msi_ctrl_to_cpu[MAX_MSI_CTRLS]; > struct pci_host_bridge *bridge; > raw_spinlock_t lock; > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > -- > 2.48.0.rc2.279.g1de40edade-goog > -- மணிவண்ணன் சதாசிவம்