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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B269C46467 for ; Wed, 4 Jan 2023 20:45:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235325AbjADUpd (ORCPT ); Wed, 4 Jan 2023 15:45:33 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46622 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229464AbjADUpb (ORCPT ); Wed, 4 Jan 2023 15:45:31 -0500 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 03B301BE81 for ; Wed, 4 Jan 2023 12:45:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1672865130; x=1704401130; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=JPhN81ERRIjZzS1z/TE/lkSjEk449sf/QV8soOROeBY=; b=JDZnTD1cs4kYEm7mHEK6q3JoyxB/XN6dQmNeTjzcBdtCjuiRGNpI0Cs8 11eYd39awu38yMyjykfV5c6qauXjQfHhFvgi34+tEi2ToVECc5R93ITp+ /uCk10OYPBqhVpRG74KT2CFv3Q1sMtcL5uAWwgJmVtro5MvHSaS0uWbbz XCt0KjSkb9c5zSfN0EeojVijLcQ4jwtVftx1JtinMbZS9DeUmWUeI5VMn NbhwkepuXduaBeRSQZSY89cpM4ufI+05go3KvbRA4Aau1cCafXNdQ0Zii aTTY65BqvwS6dQKg5U9DfEzWWCeEZ5pRFRUEXUkCupHH4DMNmQNidabVz A==; X-IronPort-AV: E=McAfee;i="6500,9779,10580"; a="301729319" X-IronPort-AV: E=Sophos;i="5.96,301,1665471600"; d="scan'208";a="301729319" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Jan 2023 12:45:29 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10580"; a="687656569" X-IronPort-AV: E=Sophos;i="5.96,301,1665471600"; d="scan'208";a="687656569" Received: from aschofie-mobl2.amr.corp.intel.com (HELO aschofie-mobl2) ([10.209.42.169]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Jan 2023 12:45:28 -0800 Date: Wed, 4 Jan 2023 12:45:27 -0800 From: Alison Schofield To: Jonathan Cameron Cc: Dan Williams , Ira Weiny , Vishal Verma , Ben Widawsky , Dave Jiang , linux-cxl@vger.kernel.org Subject: Re: [PATCH 1/4] cxl/region: Add a DPA to HPA translation helper Message-ID: References: <31c38d6711cc3a000a5307b8ebf3b6e88675e17f.1669153711.git.alison.schofield@intel.com> <20221130162736.0000333f@Huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221130162736.0000333f@Huawei.com> Precedence: bulk List-ID: X-Mailing-List: linux-cxl@vger.kernel.org On Wed, Nov 30, 2022 at 04:27:36PM +0000, Jonathan Cameron wrote: > On Tue, 22 Nov 2022 15:07:48 -0800 > alison.schofield@intel.com wrote: > > > From: Alison Schofield > > > > CXL devices may report errors and events using their DPA (device > > physical address). When a CXL device contributes capacity to a > > CXL region, the device's physical addresses are mapped to HPA's. > > (host physical addresses) > > > > Provide a helper to calculate the HPA when given a DPA, a region, > > and the devices position in the region interleave. > > device's > > > > > Verify that the HPA is within the expected ranges that this device > > contributes to the region interleave set. > > > > The initial use case is translating the DPAs that CXL devices > > report in media error records. > > > > Signed-off-by: Alison Schofield > > I'm not following the maths for the 3/6/12 way interleave cases. > Yeah, they were off. I reworked this to strictly follow the spec. > > --- > > drivers/cxl/core/core.h | 3 ++ > > drivers/cxl/core/region.c | 80 +++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 83 insertions(+) > > > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > > index 1d8f87be283f..72b58e53c394 100644 > > --- a/drivers/cxl/core/core.h > > +++ b/drivers/cxl/core/core.h > > @@ -67,6 +67,9 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port, > > return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev); > > } > > > > +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr, > > + struct cxl_endpoint_decoder *cxled); > > + > > int cxl_memdev_init(void); > > void cxl_memdev_exit(void); > > void cxl_mbox_init(void); > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index f9ae5ad284ff..c847517e766c 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -1865,6 +1865,86 @@ static void cxlr_pmem_unregister(void *dev) > > device_unregister(dev); > > } > > > > +static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos) > > +{ > > + struct cxl_region_params *p = &cxlr->params; > > + int gran = p->interleave_granularity; > > + int ways = p->interleave_ways; > > + u64 stride; > > + > > + /* Is the hpa within this region at all */ > > + if (hpa < p->res->start || hpa > p->res->end) { > > + dev_dbg(&cxlr->dev, > > + "Addr trans fail: hpa 0x%llx not in region\n", hpa); > > + return false; > > + } > > + /* Is the hpa in an expected stride for its pos(-ition) */ > > + stride = p->res->start + pos * gran; > > + do { > > + if (hpa >= stride && hpa <= stride + gran - 1) > < stride + gran seems simpler. > Got it. > > + return true; > > + > > + stride = stride + ways * gran; > > + } while (stride < p->res->end); > > That's going to take a 'while' with a large region. Can we do something quicker > along the lines of... > > Something like (untested) > u64 offset = hpa - p->res_start; > > /* Offset within the right 'stride' */ > offset = offset % (gran * ways); > > if (offset >= pos * gran && offset < (pos + 1) * gran) > return true; > > .. I did adopt this calc you offered up here. I kept this range check function in the new patch, so you'll see it again. > > + > > + dev_dbg(&cxlr->dev, > > + "Addr trans fail: hpa 0x%llx not in any stride\n", hpa); > > + > > + return false; > > +} > > + > > +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr, > > + struct cxl_endpoint_decoder *cxled) > > +{ > > + struct cxl_region_params *p = &cxlr->params; > > + u64 dpa_offset, hpa_offset, hpa; > > + int rc, pos = cxled->pos; > > + u16 eig; > > + u8 eiw; > > + > > + rc = ways_to_cxl(p->interleave_ways, &eiw); > I lost track a bit, but I think we ended up with einw? Yes. Above and below are updated w latest names: ways_to_eiw() and granularity_to_eig() > > + if (rc) > > + return rc; > > + rc = granularity_to_cxl(p->interleave_granularity, &eig); > > + if (rc) > > + return rc; > > + > > + /* > > + * Reverse the HPA->DPA decode logic defined > > + * in the CXL Spec 3.0 Section 8.2.4.19.13 > > + * Implementation Note: Device Decode Logic > Short line wrap. Probably want to go nearer 80 chars > > > + * > > + * The device position in the region interleave > > + * set was removed in the HPA->DPA translation. > > + * Put it back to reconstruct the HPA. > > + */ > > + > > + /* Remove the dpa base */ > > + dpa_offset = dpa - cxl_dpa_resource_start(cxled); > > + > > + /* Restore the position */ > > + if (eiw <= 8) { > > This looks to be the power of 2 version, so should that be eiw < 8? > This calc section got a rewrite w 2 cases eiw < 8 and 'all others'. > > + hpa_offset = (dpa_offset & GENMASK_ULL(51, eig + 8)) << eiw; > > + hpa_offset |= GENMASK_ULL(eig + 8 + eiw, eig + 8) > > + & (pos << (eig + 8)); > > Why is the masking needed here? I think pos should always fit in the iw bits. Not needed. > > > + } > > + if (eiw == 9) > > else if would show clearly that only one of these ifs is true. > > > + hpa_offset |= BIT(eig + eiw) & (pos & 0x01); > > I don't follow this logic. hpa_offset hasn't been set to anything before > this point + the right hand side of the above will always be 0 (I think...) > > > > + if (eiw == 10) > else if > > + hpa_offset |= GENMASK_ULL(eig + eiw, eig + 8) & (pos & 0x03); Rework has 2 cases (eiw < 8), else all others. I hope you can pick up the review in the new patch. Thanks Jonathan! Alison > > + > > + /* The lower bits remain unchanged */ > > + hpa_offset |= dpa_offset & GENMASK_ULL(eig + 7, 0); > > + > > + /* Apply the hpa_offset to region base address */ > > + hpa = hpa_offset + p->res->start; > > + > > + if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos)) > > + return 0; > > + > > + return hpa; > > +} > > + > > /** > > * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge > > * @cxlr: parent CXL region for this pmem region bridge device >