Re: [PATCH v2 2/4] acpi/cppc: extract _cpc entry parsing logic

[Jason Andryuk <jason.andryuk@amd.com>]

Adding Rafael and Len, since the patch touches drivers/acpi/.

On 2024-12-05 00:42, Penny Zheng wrote:
> When running as Xen dom0 PVH guest, MADT table is customized
> and may have the "wrong" UID processor number, which is
> inconsistent with the UID in Processor entry in native DSDT.
> 
> As a result, during ACPI boot-up for dom0, linux fails to set
> up proper processor logical id <-> physical id map(acpi_map_cpuid).
> Furthermore, It leads to that some ACPI processor feature data,
> like per-cpu cpc_desc structure, failed to be correctly stored.
> 
> In order to re-parse _CPC entry later for delivering correct data
> in performance hypercall, firstly, we extract parsing logic from
> acpi_cppc_processor_probe() and export it as a new function
> acpi_cppc_processor_parse().
> 
> Also, replace logical processor id with ACPI ID, to show correct print
> info in Xen dom0 PVH guest.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

No further comments by me - leaving untrimmed for Rafael and Len.

Regards,
Jason

> ---
>   drivers/acpi/cppc_acpi.c | 93 +++++++++++++++++++++++-----------------
>   1 file changed, 53 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index c3fc2c05d868..13d6ff84a1e9 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -675,19 +675,11 @@ static int pcc_data_alloc(int pcc_ss_id)
>   static inline void arch_init_invariance_cppc(void) { }
>   #endif
>   
> -/**
> - * acpi_cppc_processor_probe - Search for per CPU _CPC objects.
> - * @pr: Ptr to acpi_processor containing this CPU's logical ID.
> - *
> - *	Return: 0 for success or negative value for err.
> - */
> -int acpi_cppc_processor_probe(struct acpi_processor *pr)
> +static int acpi_cppc_processor_parse(struct acpi_processor *pr, struct cpc_desc *cpc_ptr)
>   {
>   	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
>   	union acpi_object *out_obj, *cpc_obj;
> -	struct cpc_desc *cpc_ptr;
>   	struct cpc_reg *gas_t;
> -	struct device *cpu_dev;
>   	acpi_handle handle = pr->handle;
>   	unsigned int num_ent, i, cpc_rev;
>   	int pcc_subspace_id = -1;
> @@ -706,31 +698,24 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>   	status = acpi_evaluate_object_typed(handle, "_CPC", NULL, &output,
>   			ACPI_TYPE_PACKAGE);
>   	if (ACPI_FAILURE(status)) {
> -		ret = -ENODEV;
> -		goto out_buf_free;
> +		return -ENODEV;
>   	}
>   
>   	out_obj = (union acpi_object *) output.pointer;
>   
> -	cpc_ptr = kzalloc(sizeof(struct cpc_desc), GFP_KERNEL);
> -	if (!cpc_ptr) {
> -		ret = -ENOMEM;
> -		goto out_buf_free;
> -	}
> -
>   	/* First entry is NumEntries. */
>   	cpc_obj = &out_obj->package.elements[0];
>   	if (cpc_obj->type == ACPI_TYPE_INTEGER)	{
>   		num_ent = cpc_obj->integer.value;
>   		if (num_ent <= 1) {
>   			pr_debug("Unexpected _CPC NumEntries value (%d) for CPU:%d\n",
> -				 num_ent, pr->id);
> -			goto out_free;
> +				 num_ent, pr->acpi_id);
> +			goto out_pointer;
>   		}
>   	} else {
>   		pr_debug("Unexpected _CPC NumEntries entry type (%d) for CPU:%d\n",
> -			 cpc_obj->type, pr->id);
> -		goto out_free;
> +			 cpc_obj->type, pr->acpi_id);
> +		goto out_pointer;
>   	}
>   
>   	/* Second entry should be revision. */
> @@ -739,14 +724,14 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>   		cpc_rev = cpc_obj->integer.value;
>   	} else {
>   		pr_debug("Unexpected _CPC Revision entry type (%d) for CPU:%d\n",
> -			 cpc_obj->type, pr->id);
> -		goto out_free;
> +			 cpc_obj->type, pr->acpi_id);
> +		goto out_pointer;
>   	}
>   
>   	if (cpc_rev < CPPC_V2_REV) {
>   		pr_debug("Unsupported _CPC Revision (%d) for CPU:%d\n", cpc_rev,
> -			 pr->id);
> -		goto out_free;
> +			 pr->acpi_id);
> +		goto out_pointer;
>   	}
>   
>   	/*
> @@ -758,8 +743,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>   	    (cpc_rev == CPPC_V3_REV && num_ent != CPPC_V3_NUM_ENT) ||
>   	    (cpc_rev > CPPC_V3_REV && num_ent <= CPPC_V3_NUM_ENT)) {
>   		pr_debug("Unexpected number of _CPC return package entries (%d) for CPU:%d\n",
> -			 num_ent, pr->id);
> -		goto out_free;
> +			 num_ent, pr->acpi_id);
> +		goto out_pointer;
>   	}
>   	if (cpc_rev > CPPC_V3_REV) {
>   		num_ent = CPPC_V3_NUM_ENT;
> @@ -793,7 +778,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>   						goto out_free;
>   				} else if (pcc_subspace_id != gas_t->access_width) {
>   					pr_debug("Mismatched PCC ids in _CPC for CPU:%d\n",
> -						 pr->id);
> +						 pr->acpi_id);
>   					goto out_free;
>   				}
>   			} else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> @@ -848,7 +833,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>   			memcpy(&cpc_ptr->cpc_regs[i-2].cpc_entry.reg, gas_t, sizeof(*gas_t));
>   		} else {
>   			pr_debug("Invalid entry type (%d) in _CPC for CPU:%d\n",
> -				 i, pr->id);
> +				 i, pr->acpi_id);
>   			goto out_free;
>   		}
>   	}
> @@ -864,6 +849,45 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>   		cpc_ptr->cpc_regs[i].cpc_entry.int_value = 0;
>   	}
>   
> +	pr_debug("Parsed _CPC entry for CPU: %d\n", pr->acpi_id);
> +	kfree(output.pointer);
> +	return 0;
> +
> + out_free:
> +	/* Free all the mapped sys mem areas for this CPU */
> +	for (i = 2; i < cpc_ptr->num_entries; i++) {
> +		void __iomem *addr = cpc_ptr->cpc_regs[i-2].sys_mem_vaddr;
> +
> +		if (addr)
> +			iounmap(addr);
> +	}
> + out_pointer:
> +	kfree(output.pointer);
> +	return ret;
> +}
> +
> +/**
> + * acpi_cppc_processor_probe - Search for per CPU _CPC objects.
> + * @pr: Ptr to acpi_processor containing this CPU's logical ID.
> + *
> + *	Return: 0 for success or negative value for err.
> + */
> +int acpi_cppc_processor_probe(struct acpi_processor *pr)
> +{
> +	acpi_handle handle = pr->handle;
> +	struct cpc_desc *cpc_ptr;
> +	struct device *cpu_dev;
> +	int pcc_subspace_id = -1;
> +	int ret = -ENODATA;
> +
> +	cpc_ptr = kzalloc(sizeof(struct cpc_desc), GFP_KERNEL);
> +	if (!cpc_ptr)
> +		return -ENOMEM;
> +
> +	ret = acpi_cppc_processor_parse(pr, cpc_ptr);
> +	if (ret)
> +		goto out_free;
> +	pcc_subspace_id = per_cpu(cpu_pcc_subspace_idx, pr->id);
>   
>   	/* Store CPU Logical ID */
>   	cpc_ptr->cpu_id = pr->id;
> @@ -907,21 +931,10 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>   
>   	arch_init_invariance_cppc();
>   
> -	kfree(output.pointer);
>   	return 0;
>   
>   out_free:
> -	/* Free all the mapped sys mem areas for this CPU */
> -	for (i = 2; i < cpc_ptr->num_entries; i++) {
> -		void __iomem *addr = cpc_ptr->cpc_regs[i-2].sys_mem_vaddr;
> -
> -		if (addr)
> -			iounmap(addr);
> -	}
>   	kfree(cpc_ptr);
> -
> -out_buf_free:
> -	kfree(output.pointer);
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(acpi_cppc_processor_probe);