Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver

From: Ruidong Tian <tianruidong@linux.alibaba.com>
To: Tyler Baicar <baicar@amperemail.onmicrosoft.com>,
	"ishii.shuuichir@fujitsu.com" <ishii.shuuichir@fujitsu.com>,
	'Tyler Baicar' <baicar@os.amperecomputing.com>,
	"patches@amperecomputing.com" <patches@amperecomputing.com>,
	"abdulhamid@os.amperecomputing.com" 
	<abdulhamid@os.amperecomputing.com>,
	"darren@os.amperecomputing.com" <darren@os.amperecomputing.com>,
	"catalin.marinas@arm.com" <catalin.marinas@arm.com>,
	"will@kernel.org" <will@kernel.org>,
	"maz@kernel.org" <maz@kernel.org>,
	"james.morse@arm.com" <james.morse@arm.com>,
	"alexandru.elisei@arm.com" <alexandru.elisei@arm.com>,
	"suzuki.poulose@arm.com" <suzuki.poulose@arm.com>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"guohanjun@huawei.com" <guohanjun@huawei.com>,
	"sudeep.holla@arm.com" <sudeep.holla@arm.com>,
	"rafael@kernel.org" <rafael@kernel.org>,
	"lenb@kernel.org" <lenb@kernel.org>,
	"tony.luck@intel.com" <tony.luck@intel.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"anshuman.khandual@arm.com" <anshuman.khandual@arm.com>,
	"vincenzo.frascino@arm.com" <vincenzo.frascino@arm.com>,
	"tabba@google.com" <tabba@google.com>,
	"marcan@marcan.st" <marcan@marcan.st>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"jthierry@redhat.com" <jthierry@redhat.com>,
	"masahiroy@kernel.org" <masahiroy@kernel.org>,
	"samitolvanen@google.com" <samitolvanen@google.com>,
	"john.garry@huawei.com" <john.garry@huawei.com>,
	"daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>,
	"gor@linux.ibm.com" <gor@linux.ibm.com>,
	"zhangshaokun@hisilicon.com" <zhangshaokun@hisilicon.com>,
	"tmricht@linux.ibm.com" <tmricht@linux.ibm.com>,
	"dchinner@redhat.com" <dchinner@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
	"Vineeth.Pillai@microsoft.com" <Vineeth.Pillai@microsoft.com>
Cc: baolin.wang@linux.alibaba.com, xueshuai@linux.alibaba.com
Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
Date: Wed, 7 Dec 2022 13:44:35 +0800	[thread overview]
Message-ID: <b365db02-b28c-1b22-2e87-c011cef848e2@linux.alibaba.com> (raw)
In-Reply-To: <7413d707-93a5-3681-e338-adebef198ec5@amperemail.onmicrosoft.com>

Hi, Tyler.

I am very interested in your work about AEST.
When do you plan to update the v2 patch series?

Best regards.

在 2022/5/9 21:37, Tyler Baicar 写道:
> Hi Shuuichirou,
>
> I should be able to get a v2 patch series out by the end of the month.
>
> Thanks,
> Tyler
>
> On 4/20/2022 3:54 AM, ishii.shuuichir@fujitsu.com wrote:
>> Hi, Tyler.
>>
>> When do you plan to post the v2 patch series?
>> Please let me know if you don't mind.
>>
>> Best regards.
>>
>>> -----Original Message-----
>>> From: Tyler Baicar <baicar@amperemail.onmicrosoft.com>
>>> Sent: Friday, December 17, 2021 8:33 AM
>>> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>; 'Tyler 
>>> Baicar'
>>> <baicar@os.amperecomputing.com>; patches@amperecomputing.com;
>>> abdulhamid@os.amperecomputing.com; darren@os.amperecomputing.com;
>>> catalin.marinas@arm.com; will@kernel.org; maz@kernel.org;
>>> james.morse@arm.com; alexandru.elisei@arm.com; suzuki.poulose@arm.com;
>>> lorenzo.pieralisi@arm.com; guohanjun@huawei.com; sudeep.holla@arm.com;
>>> rafael@kernel.org; lenb@kernel.org; tony.luck@intel.com; bp@alien8.de;
>>> mark.rutland@arm.com; anshuman.khandual@arm.com;
>>> vincenzo.frascino@arm.com; tabba@google.com; marcan@marcan.st;
>>> keescook@chromium.org; jthierry@redhat.com; masahiroy@kernel.org;
>>> samitolvanen@google.com; john.garry@huawei.com; 
>>> daniel.lezcano@linaro.org;
>>> gor@linux.ibm.com; zhangshaokun@hisilicon.com; tmricht@linux.ibm.com;
>>> dchinner@redhat.com; tglx@linutronix.de; linux-kernel@vger.kernel.org;
>>> linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
>>> linux-acpi@vger.kernel.org; linux-edac@vger.kernel.org;
>>> Vineeth.Pillai@microsoft.com
>>> Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>>
>>> Hi Shuuichirou,
>>>
>>> Thank you for your feedback!
>>>
>>> On 12/9/2021 3:10 AM, ishii.shuuichir@fujitsu.com wrote:
>>>> Hi, Tyler.
>>>>
>>>> We would like to make a few comments.
>>>>
>>>>> -----Original Message-----
>>>>> From: Tyler Baicar <baicar@os.amperecomputing.com>
>>>>> Sent: Thursday, November 25, 2021 2:07 AM
>>>>> To: patches@amperecomputing.com; abdulhamid@os.amperecomputing.com;
>>>>> darren@os.amperecomputing.com; catalin.marinas@arm.com;
>>>>> will@kernel.org; maz@kernel.org; james.morse@arm.com;
>>>>> alexandru.elisei@arm.com; suzuki.poulose@arm.com;
>>>>> lorenzo.pieralisi@arm.com; guohanjun@huawei.com;
>>>>> sudeep.holla@arm.com; rafael@kernel.org; lenb@kernel.org;
>>>>> tony.luck@intel.com; bp@alien8.de; mark.rutland@arm.com;
>>>>> anshuman.khandual@arm.com; vincenzo.frascino@arm.com;
>>>>> tabba@google.com; marcan@marcan.st; keescook@chromium.org;
>>>>> jthierry@redhat.com; masahiroy@kernel.org; samitolvanen@google.com;
>>>>> john.garry@huawei.com; daniel.lezcano@linaro.org; gor@linux.ibm.com;
>>>>> zhangshaokun@hisilicon.com; tmricht@linux.ibm.com;
>>>>> dchinner@redhat.com; tglx@linutronix.de;
>>>>> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>>> kvmarm@lists.cs.columbia.edu; linux-acpi@vger.kernel.org;
>>>>> linux-edac@vger.kernel.org; Ishii, Shuuichirou/石井
>>>>> 周一郎 <ishii.shuuichir@fujitsu.com>; Vineeth.Pillai@microsoft.com
>>>>> Cc: Tyler Baicar <baicar@os.amperecomputing.com>
>>>>> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>>>>
>>>>> Add support for parsing the ARM Error Source Table and basic handling
>>>>> of errors reported through both memory mapped and system register
>>> interfaces.
>>>>>
>>>>> Assume system register interfaces are only registered with private
>>>>> peripheral interrupts (PPIs); otherwise there is no guarantee the
>>>>> core handling the error is the core which took the error and has the
>>>>> syndrome info in its system registers.
>>>>>
>>>>> Add logging for all detected errors and trigger a kernel panic if
>>>>> there is any uncorrected error present.
>>>>>
>>>>> Signed-off-by: Tyler Baicar <baicar@os.amperecomputing.com>
>>>>> ---
>>>> [...]
>>>>
>>>>> +static int __init aest_init_node(struct acpi_aest_hdr *node) {
>>>>> +    union acpi_aest_processor_data *proc_data;
>>>>> +    union aest_node_spec *node_spec;
>>>>> +    struct aest_node_data *data;
>>>>> +    int ret;
>>>>> +
>>>>> +    data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
>>>>> +    if (!data)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    data->node_type = node->type;
>>>>> +
>>>>> +    node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
>>>>> node->node_specific_offset);
>>>>> +
>>>>> +    switch (node->type) {
>>>>> +    case ACPI_AEST_PROCESSOR_ERROR_NODE:
>>>>> +        memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_processor));
>>>>> +        break;
>>>>> +    case ACPI_AEST_MEMORY_ERROR_NODE:
>>>>> +        memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_memory));
>>>>> +        break;
>>>>> +    case ACPI_AEST_SMMU_ERROR_NODE:
>>>>> +        memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_smmu));
>>>>> +        break;
>>>>> +    case ACPI_AEST_VENDOR_ERROR_NODE:
>>>>> +        memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_vendor));
>>>>> +        break;
>>>>> +    case ACPI_AEST_GIC_ERROR_NODE:
>>>>> +        memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_gic));
>>>>> +        break;
>>>>> +    default:
>>>>> +        kfree(data);
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
>>>>> +        proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data,
>>>>> node_spec,
>>>>> +                     sizeof(acpi_aest_processor));
>>>>> +
>>>>> +        switch (data->data.processor.resource_type) {
>>>>> +        case ACPI_AEST_CACHE_RESOURCE:
>>>>> +            memcpy(&data->proc_data, proc_data,
>>>>> +                   sizeof(struct acpi_aest_processor_cache));
>>>>> +            break;
>>>>> +        case ACPI_AEST_TLB_RESOURCE:
>>>>> +            memcpy(&data->proc_data, proc_data,
>>>>> +                   sizeof(struct acpi_aest_processor_tlb));
>>>>> +            break;
>>>>> +        case ACPI_AEST_GENERIC_RESOURCE:
>>>>> +            memcpy(&data->proc_data, proc_data,
>>>>> +                   sizeof(struct acpi_aest_processor_generic));
>>>>> +            break;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    ret = aest_init_interface(node, data);
>>>>> +    if (ret) {
>>>>> +        kfree(data);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    return aest_init_interrupts(node, data);
>>>> If aest_init_interrupts() failed, is it necessary to release the data
>>>> pointer acquired by kzalloc?
>>> aest_init_interrupts() returns an error if any of the interrupts in 
>>> the interrupt list
>>> fails, but it's possible that some interrupts in the list registered 
>>> successfully. So
>>> we attempt to keep chugging along in that scenario because some 
>>> interrupts may
>>> be enabled and registered with the interface successfully. If any 
>>> interrupt
>>> registration fails, there will be a print notifying that there was a 
>>> failure when
>>> initializing that node.
>>>>> +}
>>>>> +
>>>>> +static void aest_count_ppi(struct acpi_aest_hdr *node)
>>>>> +{
>>>>> +    struct acpi_aest_node_interrupt *interrupt;
>>>>> +    int i;
>>>>> +
>>>>> +    interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
>>>>> +                 node->node_interrupt_offset);
>>>>> +
>>>>> +    for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
>>>>> +        if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
>>>>> +            num_ppi++;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static int aest_starting_cpu(unsigned int cpu)
>>>>> +{
>>>>> +    int i;
>>>>> +
>>>>> +    for (i = 0; i < num_ppi; i++)
>>>>> +        enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int aest_dying_cpu(unsigned int cpu)
>>>>> +{
>>>> Wouldn't it be better to execute disable_percpu_irq(), which is paired
>>>> with enable_percpu_irq(), in aest_dying_cpu()?
>>>
>>> Good point. I will add that in the next version.
>>>
>>> Thanks,
>>>
>>> Tyler
>>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm