From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) (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 858CB21B195 for ; Tue, 29 Apr 2025 22:32:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.177 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1745965954; cv=none; b=RKxFMG684yXDOEfMwh4AhmNRnH2xUUsbj7k8N2ikhJJO6YFnA9wCCH6IRSsTB0+CPcD4w95Q9BKhy8t4TNokGtFBCSyFQOhAC6d6CA246fDwbWsBcJ4gT1XhVDtJMTO5dUObdnyj2cekX65IY45nhEA2BQFywCrav0PSrWK9vyU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1745965954; c=relaxed/simple; bh=XyBy/9ECM3Ji4+OBncyGrKzNcU3QNO+8BOFD1+2XlAI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=hy7dmY1ObFRG5dr+J0gs6WWWWEjWfr311i4UKHPhiDRrYnNVXjY3LdSy3sbsspkW4n6HPgmjFgFEnIioDDtWqiLUCHc+QkghC5HAYuRrjIC8mg9LVBOf0jjsYHmd2q19RL8MN7k4ehU/TIgOCzUHQmOZe+QGDIi+u8eiI0sE5XU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=DBIR5vOp; arc=none smtp.client-ip=209.85.214.177 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="DBIR5vOp" Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-2264c9d0295so245875ad.0 for ; Tue, 29 Apr 2025 15:32:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1745965951; x=1746570751; darn=lists.linux.dev; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=AnNdE+HhwllnOl/nQBW5AcYsgv32dfnq2/SzhhfRlf8=; b=DBIR5vOpSKmB31TJdMTIkaC6avquPW638o6xTcc+/nqlVKizhxg2HErHLxQaAK/op2 +Yoc8sfPmRBQPclUg0VLySdUrIbqQSu1NAsy06UrWqLnydxZAaTlpbUFvJibe5Pf4w+I qBe0EAY9s/OmSuwwx2oaAkCWkNJ51ldjdiWvwcaIq9PA5JZt9OvBN2hndIzEmDbqXR0Q t9wm+qpYoetvLiOL/5nGpGXVDBGxcLx2jEp1PRZaiv7DwG1SyRxg/k95hIMRynpuv1tD z2J2WhvSsXA6eXMITPGxa7eaTBuVckr3McPnOFIuymb45180QUMPjd5iMUzJ1bkBYG0i FPxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1745965951; x=1746570751; h=in-reply-to: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=AnNdE+HhwllnOl/nQBW5AcYsgv32dfnq2/SzhhfRlf8=; b=BpXRA4nHSxPIhcMOZmoL5pcdyMQ/TPfTWBWJrqvekoo+mE6DqSPK2qoYi1BrWY1IJb HPInwmYa7BvyUHqyndA+TPcLrnyUSWeots7yaRjjVhxZnEJdKBiVAvZJxPY3Z2guxXJL h21fUyNvm3maHDmNjSzHUw7z04oBhUfnyjjlVjPg8JZVwL96ChtAKpp1kdLXcbbNTHY2 G3Uw/9ZkwwZoHvubSkal8NFWy1DUrWF9EManWHxIrSJMntVw1EPUVDe2k26ZodCJWzBg iWBzluqBPISPTXhjB7RFhbfVBH86ilFfLkfpGWL1vpQe9V8PYY/R4AgfocsBY/Rz4v6H gtdQ== X-Forwarded-Encrypted: i=1; AJvYcCX3I/0OVSBghD3JXsyCjNId1K6F3yjUcIF1MZuX8GnMucfWJfrJ+6mblcUjPX6SmgydkGnuUglP@lists.linux.dev X-Gm-Message-State: AOJu0Yy51NW88XPp/7L1CU8oY+VhfF8EmYe3sD9g1bW6msdN3PaRY2YJ WDiO4Y45KXDkBkQzeXTA+4PvTCojfaQb8l+9YHY4zqHbAsCjY+DksmTH8PzTVg== X-Gm-Gg: ASbGncv+gcBZQSrZauAGT3gkW2upOJc6vdQgBntccLwMHPlPIvkX9H5EW1mE6kBtQMk ZRujGm6xnEIuv+axwq8bLptQk1W0mAFtYKUgB5RAl/XvfIe/0y4DLgj8pZZA+V+AqJdd7NLe3vU 3CjRk7lUCdLWTkLqcqP2EhuRHWj2IBHibo42fyxULTH4mritwoo9P5MdhBpxhXxweb7cSfS14WC 0/1WBKpmqW8ZVVfFedRYt8xHaIv5xV6dRt21G2UROWtkBTVFq1LJK7xCja2rL0vjUN3FsWN+I2n b2VtAhvbQ12BdAJRSP33crlPXLUPmAnOXTrNWbpuIBFMYkAbW1vZGbBtUMQ62Z2F+ezZ6cl4 X-Google-Smtp-Source: AGHT+IEjjGwm4LgyP1atx5l0JHqL9FPxuMAodaX4BR9dLcCBJPPs8xL/z9odFgYaKsV2MBnWB05wdw== X-Received: by 2002:a17:903:1a24:b0:223:4b4f:160c with SMTP id d9443c01a7336-22df5496af5mr477235ad.27.1745965950581; Tue, 29 Apr 2025 15:32:30 -0700 (PDT) Received: from google.com (2.210.143.34.bc.googleusercontent.com. [34.143.210.2]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-7403991f046sm236228b3a.41.2025.04.29.15.32.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Apr 2025 15:32:30 -0700 (PDT) Date: Tue, 29 Apr 2025 22:32:19 +0000 From: Pranjal Shrivastava To: Nicolin Chen Cc: jgg@nvidia.com, kevin.tian@intel.com, corbet@lwn.net, will@kernel.org, bagasdotme@gmail.com, robin.murphy@arm.com, joro@8bytes.org, thierry.reding@gmail.com, vdumpa@nvidia.com, jonathanh@nvidia.com, shuah@kernel.org, jsnitsel@redhat.com, nathan@kernel.org, peterz@infradead.org, yi.l.liu@intel.com, mshavit@google.com, zhangzekun11@huawei.com, iommu@lists.linux.dev, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-tegra@vger.kernel.org, linux-kselftest@vger.kernel.org, patches@lists.linux.dev, mochs@nvidia.com, alok.a.tiwari@oracle.com, vasant.hegde@amd.com Subject: Re: [PATCH v2 20/22] iommu/tegra241-cmdqv: Do not statically map LVCMDQs Message-ID: References: <3981a819a4714b21d11d5c6de561a2d0c6411947.1745646960.git.nicolinc@nvidia.com> Precedence: bulk X-Mailing-List: patches@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3981a819a4714b21d11d5c6de561a2d0c6411947.1745646960.git.nicolinc@nvidia.com> On Fri, Apr 25, 2025 at 10:58:15PM -0700, Nicolin Chen wrote: > To simplify the mappings from global VCMDQs to VINTFs' LVCMDQs, the design > chose to do static allocations and mappings in the global reset function. > > However, with the user-owned VINTF support, it exposes a security concern: > if user space VM only wants one LVCMDQ for a VINTF, statically mapping two > LVCMDQs creates a hidden VCMDQ that user space could DoS attack by writing > ramdon stuff to overwhelm the kernel with unhandleable IRQs. > Nit: I think it's worth mentioning that the current HW only supports 2 LVCMDQs. Since it's not clear from the driver as it calculates this by: regval = readl_relaxed(REG_CMDQV(cmdqv, PARAM)); cmdqv->num_vintfs = 1 << FIELD_GET(CMDQV_NUM_VINTF_LOG2,regval); cmdqv->num_vcmdqs = 1 << FIELD_GET(CMDQV_NUM_VCMDQ_LOG2, regval); cmdqv->num_lvcmdqs_per_vintf = cmdqv->num_vcmdqs / cmdqv->num_vintfs; Or maybe, re-word it to "if user space VM only wants one LVCMDQ for a VINTF, the current driver statically maps num_lvcmdqs_per_vintf which creates hidden vCMDQs [..]" > Thus, to support the user-owned VINTF feature, a LVCMDQ mapping has to be > done dynamically. > > HW allows pre-assigning global VCMDQs in the CMDQ_ALLOC registers, without > finalizing the mappings by keeping CMDQV_CMDQ_ALLOCATED=0. So, add a pair > of map/unmap helper that simply sets/clears that bit. > > Delay the LVCMDQ mappings to tegra241_vintf_hw_init(), and the unmappings > to tegra241_vintf_hw_deinit(). > > However, the dynamic LVCMDQ mapping/unmapping can complicate the timing of > calling tegra241_vcmdq_hw_init/deinit(), which write LVCMDQ address space, > i.e. requiring LVCMDQ to be mapped. Highlight that with a note to the top > of either of them. > > Signed-off-by: Nicolin Chen > --- > .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 37 +++++++++++++++++-- > 1 file changed, 33 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c > index 8d418c131b1b..869c90b660c1 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c > +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c > @@ -351,6 +351,7 @@ tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu, > > /* HW Reset Functions */ > > +/* This function is for LVCMDQ, so @vcmdq must not be unmapped yet */ > static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq) > { > char header[64], *h = lvcmdq_error_header(vcmdq, header, 64); > @@ -379,6 +380,7 @@ static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq) > dev_dbg(vcmdq->cmdqv->dev, "%sdeinited\n", h); > } > > +/* This function is for LVCMDQ, so @vcmdq must be mapped prior */ > static int tegra241_vcmdq_hw_init(struct tegra241_vcmdq *vcmdq) > { > char header[64], *h = lvcmdq_error_header(vcmdq, header, 64); > @@ -404,16 +406,42 @@ static int tegra241_vcmdq_hw_init(struct tegra241_vcmdq *vcmdq) > return 0; > } > > +/* Unmap a global VCMDQ from the pre-assigned LVCMDQ */ > +static void tegra241_vcmdq_unmap_lvcmdq(struct tegra241_vcmdq *vcmdq) > +{ > + u32 regval = readl(REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx))); > + char header[64], *h = lvcmdq_error_header(vcmdq, header, 64); > + > + writel(regval & ~CMDQV_CMDQ_ALLOCATED, > + REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx))); > + dev_dbg(vcmdq->cmdqv->dev, "%sunmapped\n", h); > +} > + > static void tegra241_vintf_hw_deinit(struct tegra241_vintf *vintf) > { > - u16 lidx; > + u16 lidx = vintf->cmdqv->num_lvcmdqs_per_vintf; > > - for (lidx = 0; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++) > - if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) > + /* HW requires to unmap LVCMDQs in descending order */ > + while (lidx--) { > + if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) { > tegra241_vcmdq_hw_deinit(vintf->lvcmdqs[lidx]); > + tegra241_vcmdq_unmap_lvcmdq(vintf->lvcmdqs[lidx]); > + } > + } > vintf_write_config(vintf, 0); > } > > +/* Map a global VCMDQ to the pre-assigned LVCMDQ */ > +static void tegra241_vcmdq_map_lvcmdq(struct tegra241_vcmdq *vcmdq) > +{ > + u32 regval = readl(REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx))); > + char header[64], *h = lvcmdq_error_header(vcmdq, header, 64); > + > + writel(regval | CMDQV_CMDQ_ALLOCATED, > + REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx))); > + dev_dbg(vcmdq->cmdqv->dev, "%smapped\n", h); > +} > + > static int tegra241_vintf_hw_init(struct tegra241_vintf *vintf, bool hyp_own) > { > u32 regval; > @@ -441,8 +469,10 @@ static int tegra241_vintf_hw_init(struct tegra241_vintf *vintf, bool hyp_own) > */ > vintf->hyp_own = !!(VINTF_HYP_OWN & readl(REG_VINTF(vintf, CONFIG))); > > + /* HW requires to map LVCMDQs in ascending order */ > for (lidx = 0; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++) { > if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) { > + tegra241_vcmdq_map_lvcmdq(vintf->lvcmdqs[lidx]); > ret = tegra241_vcmdq_hw_init(vintf->lvcmdqs[lidx]); > if (ret) { > tegra241_vintf_hw_deinit(vintf); > @@ -476,7 +506,6 @@ static int tegra241_cmdqv_hw_reset(struct arm_smmu_device *smmu) > for (lidx = 0; lidx < cmdqv->num_lvcmdqs_per_vintf; lidx++) { > regval = FIELD_PREP(CMDQV_CMDQ_ALLOC_VINTF, idx); > regval |= FIELD_PREP(CMDQV_CMDQ_ALLOC_LVCMDQ, lidx); > - regval |= CMDQV_CMDQ_ALLOCATED; > writel_relaxed(regval, > REG_CMDQV(cmdqv, CMDQ_ALLOC(qidx++))); > } I can't confirm HW behaviour but the changes make sense to me. Acked-by: Pranjal Shrivastava Thanks! > -- > 2.43.0 >