Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0a5ee5c3-5850-92b0-dbe1-dded3ec5d0f8@intel.com>
Date:   Thu, 22 Jun 2023 16:28:49 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Betterbird/102.11.1
Subject: Re: [PATCH v6 10/27] cxl/acpi: Directly bind the CEDT detected CHBCR
 to the Host Bridge's port
To:     Terry Bowman <terry.bowman@amd.com>, <alison.schofield@intel.com>,
        <vishal.l.verma@intel.com>, <ira.weiny@intel.com>,
        <bwidawsk@kernel.org>, <dan.j.williams@intel.com>,
        <Jonathan.Cameron@huawei.com>, <linux-cxl@vger.kernel.org>
CC:     <rrichter@amd.com>, <linux-kernel@vger.kernel.org>,
        <bhelgaas@google.com>
References: <20230622035126.4130151-1-terry.bowman@amd.com>
 <20230622035126.4130151-11-terry.bowman@amd.com>
Content-Language: en-US
From:   Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20230622035126.4130151-11-terry.bowman@amd.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OW9zbDBDMzFmVEhkOHVQMktJZFBzUCtLc3o2SldkZWdsM1NibXBtNnRoQjFF?=
 =?utf-8?B?eEV5THl1Nkc2T1h5a1RsTDRvOWhKVjJzNENBNEQxSW1IcWVnNWNEbFlsSFhs?=
 =?utf-8?B?Q3Zka2h3RlhsemVrbEJVWG96a3hpWmFJZVVCYkhIaWd4U3NMekJFTkRLb21r?=
 =?utf-8?B?bHM4MklCODN4dE5BMzBlWEp2dVBDdmtZSUh0WkpoSHJEWWF1NlBnNy9TUThE?=
 =?utf-8?B?Y2tmc2R5VUw0OFBZaFZLSWhtYWUxWEtScEgrVHpDSFNxeFQvSDVmSXZxUjdz?=
 =?utf-8?B?SWIwNDJMMVVtdHNzVXk5elZYLzBrVE1vSC9TUmUzakMrbDFEb1NpUFhXYjhT?=
 =?utf-8?B?MGRTMDZCYjlYK29na0F3Y2wzV1R3MkE0R0o5VHQ3bkJzLzJBV3RnZ3lqa0w2?=
 =?utf-8?B?Mjg2QmQ4UE5qeDlrbi81cmc4M0l0YzA5dVlQbWdlcVB4bGV5ZzBCUUNSU2xG?=
 =?utf-8?B?VmY3VzNSblpsZi9oLzdKUkh3Sm1aOFg2dVNGM0Y3ZTF1ZnZsWnlFdWsrWTR2?=
 =?utf-8?B?NXlyZnQybzVTdG9nejQ5enpUeldYNWpHNmtLK255ZllITVVhRm80Q2dqM0NB?=
 =?utf-8?B?bUlMQ0U3citwVGZJbjV4MW5FQ0ZwR2RQOFNLMG5uYy9ZZjhqc0ZCL1M5cnlB?=
 =?utf-8?B?Y1g1blNSakpIQUh3REFCbE9vRHRxWEJmTStXemI0dlJESjA1am1yUDhuRmZu?=
 =?utf-8?B?MkpoMzNmdVdOSnRmbmdMQ2JwRFI4cjZnazJGY0MxeDZLb1ROcUJ1ZmdZSVRX?=
 =?utf-8?B?dFpaRFRqNVV1Q0xEUm8rdHZXL1lyUnZnR0tpTXExTWZDRGE2bTJidjFQWlRC?=
 =?utf-8?B?T2x2c0c5SHJ6R0s3cFM4T3NUamw4TmQwT3ZWdU1ZcTJCeEticFRGM0JxWmlj?=
 =?utf-8?B?V0JuQm5Lb2h5ZnI0T2VkTmVDL3hxSlpGM2dvblpvdThMQkx1VTJ6ZkNIYy9E?=
 =?utf-8?B?b1lQZVEwQlYzUzlWS0twWk8rQzQxV0ZrdmdFaS9EZ0NOS0FzQWoyMlpRM1Vj?=
 =?utf-8?B?VmlEZGtQQy96MGhuU0JQcTJidU52UmU1UXRYT1RyY0ZaUm9ia3VTVnVkemFy?=
 =?utf-8?B?am1EY1Fqa2p0bHZRdWRQeFc5NHJjd3JDT2xJMTM4eEQzdGNURDlhUXBadVlP?=
 =?utf-8?B?alVYZ0VNbDAvR3ltSXBBY0Y0cUdMemRwZUl5bENnM1B0MUhDMGhIVzZJUGtm?=
 =?utf-8?B?RUZ5S3ljOGlmZ2UvcUdEY05RdDVFT09JL3pIeFJZUDNuZCtjamQ2MW9taVRS?=
 =?utf-8?B?S0hqS2JNc0haTTRFV3V6dlJQa3c4QlhiTFJ6dTVsU0VDRlVNOGNlV2VsRnhJ?=
 =?utf-8?B?cE5NbDBpU2wxTS82Z0FKM2Q0R25mWE5Tbi9oQzhqVXRjajR3UlVFRGJ4SVIw?=
 =?utf-8?B?RWNmQTBCSWM4eE1NMkcwUDFKbEFYRVVnb0RJdFJmc1NQNEM3bHZZTlFrQ25h?=
 =?utf-8?B?T0V1S2R0UkhvMTJYWm1wUjRoZjYrTXYyUzQ5UVlsVDlWU3oySk9oOUpJS1o1?=
 =?utf-8?B?MkVJYTc1TTVUNlptV2RMRkJjRUlTcnluUitPVUVRUGdwMXFMTmNna2lIZ1RS?=
 =?utf-8?B?clpWcWtjQnZ5Tk9wZXVxcWo1b05lRjF6WVhyak9yZGdtWjFFU0IxR3g0eWZm?=
 =?utf-8?B?TGU0eDZWSjZ0d3lVTGpQZ2M5WmhOSkY5akl2V2k0cTlteFNRNGFwZE1aSm12?=
 =?utf-8?B?c05ZaTRheExhL3VrNzdSd21LSXptbmIvSnE0TnZ6NEF1enUzYy9FcVJ5V2ww?=
 =?utf-8?B?dWRFbVpScjFZcXE4aDBJODFyVkxCWS9acFBWQlJ6akFQb1hIODV0VUFBZy9p?=
 =?utf-8?B?cTZ1Z0Q0QmdDSmh2NDhSN3RsSnBiNnREdHlsZmUrTFVSbWVrVUE3aDA2ZWtF?=
 =?utf-8?B?bVNnZmM2UlBuMVdRYTF3VDBGYWxJRG5jRHNMUTVsRUY3NDdTRUc2MlFzRUtD?=
 =?utf-8?B?NUJUNG5hQ3ZVaXQ1aUszNTdLVjZuNWlIZ1JrMytwV3NOTzdIdnErQ1dka09i?=
 =?utf-8?B?aW5xWEtScG9mMGZacnJoQ28rTmFvNUhrSThOUEtoMmRScWNnR0puZkxLUHFC?=
 =?utf-8?B?UElnQTRkNnNVT1E1QmdNSHF6NzFXT2RTSElSSXV3aDRRK2NPMGV0dVhPaG1T?=
 =?utf-8?B?NHNFV1ZCZXR0UFBEWlpUUy9lNWp3aFBMWXlvaU1xSDUvSjI4MmZmNEdRTkJV?=
 =?utf-8?B?THc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ae94ef31-d839-4de0-f52e-08db73787128
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jun 2023 23:28:53.6235
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RVjFLM9vD/TpaOLuvaELmfd1RS/ZxqjNRRmlwq6fDXwxO2yIWu2QANspZQ7c/oNd9FTJxSraAgrh386jPSdShQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7878
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/21/23 20:51, Terry Bowman wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> During a Host Bridge's downstream port enumeration the CHBS entries in
> the CEDT table are parsed, its Component Register base address
> extracted and then stored in struct cxl_dport. The CHBS may contain
> either the RCRB (RCH mode) or the Host Bridge's Component Registers
> (CHBCR, VH mode). The RCRB further contains the CXL downstream port
> register base address, while in VH mode the CXL Downstream Switch
> Ports are visible in the PCI hierarchy and the DP's component regs are
> disovered using the CXL DVSEC register locator capability. The

s/disovered/discovered/

> Component Registers derived from the CHBS for both modes are different
> and thus also must be treated differently. That is, in RCH mode, the
> component regs base should be bound to the dport, but in VH mode to
> the CXL host bridge's port object.
> 
> The current implementation stores the CHBCR in addition in struct
> cxl_dport and copies it later from there to struct cxl_port. As a
> result, the dport contains the wrong Component Registers base address
> and, e.g. the RAS capability of a CXL Root Port cannot be detected.
> 
> To fix the CHBCR binding, attach it directly to the Host Bridge's
> @cxl_port structure. Do this during port creation of the Host Bridge
> in add_host_bridge_uport(). Factor out CHBS parsing code in
> add_host_bridge_dport() and use it in both functions.
> 
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/cxl/acpi.c | 91 ++++++++++++++++++++++++++++++++--------------
>   1 file changed, 63 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 0c975ee684b0..89ee01323d43 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -335,13 +335,13 @@ struct cxl_chbs_context {
>   	u32 cxl_version;
>   };
>   
> -static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> -			 const unsigned long end)
> +static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
> +			     const unsigned long end)
>   {
>   	struct cxl_chbs_context *ctx = arg;
>   	struct acpi_cedt_chbs *chbs;
>   
> -	if (ctx->base)
> +	if (ctx->base != CXL_RESOURCE_NONE)
>   		return 0;
>   
>   	chbs = (struct acpi_cedt_chbs *) header;
> @@ -350,8 +350,6 @@ static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
>   		return 0;
>   
>   	ctx->cxl_version = chbs->cxl_version;
> -	ctx->base = CXL_RESOURCE_NONE;
> -
>   	if (!chbs->base)
>   		return 0;
>   
> @@ -364,11 +362,35 @@ static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
>   	return 0;
>   }
>   
> +static int cxl_get_chbs(struct device *dev, struct acpi_device *hb,
> +			struct cxl_chbs_context *ctx)
> +{
> +	unsigned long long uid;
> +	int rc;
> +
> +	rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);
> +	if (rc != AE_OK) {
> +		dev_err(dev, "unable to retrieve _UID\n");
> +		return -ENOENT;
> +	}
> +
> +	dev_dbg(dev, "UID found: %lld\n", uid);
> +	*ctx = (struct cxl_chbs_context) {
> +		.dev = dev,
> +		.uid = uid,
> +		.base = CXL_RESOURCE_NONE,
> +		.cxl_version = UINT_MAX,
> +	};
> +
> +	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx);
> +
> +	return 0;
> +}
> +
>   static int add_host_bridge_dport(struct device *match, void *arg)
>   {
>   	acpi_status rc;
>   	struct device *bridge;
> -	unsigned long long uid;
>   	struct cxl_dport *dport;
>   	struct cxl_chbs_context ctx;
>   	struct acpi_pci_root *pci_root;
> @@ -379,41 +401,38 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>   	if (!hb)
>   		return 0;
>   
> -	rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);
> -	if (rc != AE_OK) {
> -		dev_err(match, "unable to retrieve _UID\n");
> -		return -ENODEV;
> -	}
> -
> -	dev_dbg(match, "UID found: %lld\n", uid);
> -
> -	ctx = (struct cxl_chbs_context) {
> -		.dev = match,
> -		.uid = uid,
> -	};
> -	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx);
> +	rc = cxl_get_chbs(match, hb, &ctx);
> +	if (rc)
> +		return rc;
>   
> -	if (!ctx.base) {
> +	if (ctx.cxl_version == UINT_MAX) {
>   		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n",
> -			 uid);
> +			 ctx.uid);
>   		return 0;
>   	}
>   
>   	if (ctx.base == CXL_RESOURCE_NONE) {
>   		dev_warn(match, "CHBS invalid for Host Bridge (UID %lld)\n",
> -			 uid);
> +			 ctx.uid);
>   		return 0;
>   	}
>   
>   	pci_root = acpi_pci_find_root(hb->handle);
>   	bridge = pci_root->bus->bridge;
>   
> +	/*
> +	 * In RCH mode, bind the component regs base to the dport. In
> +	 * VH mode it will be bound to the CXL host bridge's port
> +	 * object later in add_host_bridge_uport().
> +	 */
>   	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
> -		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.base);
> -		dport = devm_cxl_add_rch_dport(root_port, bridge, uid, ctx.base);
> +		dev_dbg(match, "RCRB found for UID %lld: %pa\n", ctx.uid,
> +			&ctx.base);
> +		dport = devm_cxl_add_rch_dport(root_port, bridge, ctx.uid,
> +					       ctx.base);
>   	} else {
> -		dev_dbg(match, "CHBCR found for UID %lld: %pa\n", uid, &ctx.base);
> -		dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.base);
> +		dport = devm_cxl_add_dport(root_port, bridge, ctx.uid,
> +					   CXL_RESOURCE_NONE);
>   	}
>   
>   	if (IS_ERR(dport))
> @@ -435,6 +454,8 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>   	struct cxl_dport *dport;
>   	struct cxl_port *port;
>   	struct device *bridge;
> +	struct cxl_chbs_context ctx;
> +	resource_size_t component_reg_phys;
>   	int rc;
>   
>   	if (!hb)
> @@ -453,12 +474,26 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>   		return 0;
>   	}
>   
> +	rc = cxl_get_chbs(match, hb, &ctx);
> +	if (rc)
> +		return rc;
> +
> +	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
> +		dev_warn(bridge,
> +			 "CXL CHBS version mismatch, skip port registration\n");
> +		return 0;
> +	}
> +
> +	component_reg_phys = ctx.base;
> +	if (component_reg_phys != CXL_RESOURCE_NONE)
> +		dev_dbg(match, "CHBCR found for UID %lld: %pa\n",
> +			ctx.uid, &component_reg_phys);
> +
>   	rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
>   	if (rc)
>   		return rc;
>   
> -	port = devm_cxl_add_port(host, bridge, dport->component_reg_phys,
> -				 dport);
> +	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
>   	if (IS_ERR(port))
>   		return PTR_ERR(port);
>