| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 30 Jun 2022 12:36:40 +0800
From: Jianmin Lv <lvjianmin@...ngson.cn>
To: Marc Zyngier <maz@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
Hanjun Guo <guohanjun@...wei.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Jiaxun Yang <jiaxun.yang@...goat.com>,
Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH V13 06/13] irqchip/loongson-pch-pic: Add ACPI init support
On 2022/6/29 下午7:20, Marc Zyngier wrote:
> On Mon, 27 Jun 2022 12:39:50 +0100,
> Jianmin Lv <lvjianmin@...ngson.cn> wrote:
>>
>> From: Huacai Chen <chenhuacai@...ngson.cn>
>>
>> We are preparing to add new Loongson (based on LoongArch, not compatible
>> with old MIPS-based Loongson) support. LoongArch use ACPI other than DT
>> as its boot protocol, so add ACPI init support.
>
> Drop this paragraph.
>
Ok, I'll drop it for the patch and other patches of this series.
>>
>> PCH-PIC/PCH-MSI stands for "Interrupt Controller" that described in
>> Section 5 of "Loongson 7A1000 Bridge User Manual". For more information
>> please refer Documentation/loongarch/irq-chip-model.rst.
>>
>> For systems with two chipsets, there are two related pch irqdomains,
>> each of which has the same node id as its parent irqdomain. So we
>> defined a structure to mantain the relation of node and it's parent
>> irqdomain.
>>
>> struct acpi_vector_group {
>> int node;
>> struct irq_domain *parent;
>> };
>>
>> The parent irqdomain will be set for node id in parent irqdomain driver
>> before pch irqdomain is created.
>>
>> Co-developed-by: Jianmin Lv <lvjianmin@...ngson.cn>
>> Signed-off-by: Jianmin Lv <lvjianmin@...ngson.cn>
>> Signed-off-by: Huacai Chen <chenhuacai@...ngson.cn>
>> ---
>> arch/loongarch/include/asm/irq.h | 11 +-
>> arch/loongarch/kernel/irq.c | 2 +-
>> drivers/irqchip/irq-loongson-pch-pic.c | 200 ++++++++++++++++++++++++++++-----
>> 3 files changed, 179 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h
>> index 48c0ce4..a444dc6 100644
>> --- a/arch/loongarch/include/asm/irq.h
>> +++ b/arch/loongarch/include/asm/irq.h
>> @@ -48,6 +48,12 @@ static inline bool on_irq_stack(int cpu, unsigned long sp)
>> #define MAX_IO_PICS 2
>> #define NR_IRQS (64 + (256 * MAX_IO_PICS))
>>
>> +struct acpi_vector_group {
>> + int node;
>> + struct irq_domain *parent;
>> +};
>> +extern struct acpi_vector_group pch_group[MAX_IO_PICS];
>> +
>> #define CORES_PER_EIO_NODE 4
>>
>> #define LOONGSON_CPU_UART0_VEC 10 /* CPU UART0 */
>> @@ -108,8 +114,7 @@ int pch_lpc_acpi_init(struct irq_domain *parent,
>> struct acpi_madt_lpc_pic *acpi_pchlpc);
>> struct irq_domain *pch_msi_acpi_init(struct irq_domain *parent,
>> struct acpi_madt_msi_pic *acpi_pchmsi);
>> -struct irq_domain *pch_pic_acpi_init(struct irq_domain *parent,
>> - struct acpi_madt_bio_pic *acpi_pchpic);
>> +int find_pch_pic(u32 gsi);
>>
>> extern struct acpi_madt_lio_pic *acpi_liointc;
>> extern struct acpi_madt_eio_pic *acpi_eiointc[MAX_IO_PICS];
>> @@ -123,7 +128,7 @@ struct irq_domain *pch_pic_acpi_init(struct irq_domain *parent,
>> extern struct irq_domain *liointc_domain;
>> extern struct fwnode_handle *pch_lpc_handle;
>> extern struct irq_domain *pch_msi_domain[MAX_IO_PICS];
>> -extern struct irq_domain *pch_pic_domain[MAX_IO_PICS];
>> +extern struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
>>
>> extern irqreturn_t loongson3_ipi_interrupt(int irq, void *dev);
>>
>> diff --git a/arch/loongarch/kernel/irq.c b/arch/loongarch/kernel/irq.c
>> index 07d6059..f2115be 100644
>> --- a/arch/loongarch/kernel/irq.c
>> +++ b/arch/loongarch/kernel/irq.c
>> @@ -28,7 +28,7 @@
>> struct irq_domain *cpu_domain;
>> struct irq_domain *liointc_domain;
>> struct irq_domain *pch_msi_domain[MAX_IO_PICS];
>> -struct irq_domain *pch_pic_domain[MAX_IO_PICS];
>> +struct acpi_vector_group pch_group[MAX_IO_PICS];
>>
>> /*
>> * 'what should we do if we get a hw irq event on an illegal vector'.
>> diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
>> index a4eb8a2..170a5b9 100644
>> --- a/drivers/irqchip/irq-loongson-pch-pic.c
>> +++ b/drivers/irqchip/irq-loongson-pch-pic.c
>> @@ -33,13 +33,40 @@
>> #define PIC_REG_IDX(irq_id) ((irq_id) / PIC_COUNT_PER_REG)
>> #define PIC_REG_BIT(irq_id) ((irq_id) % PIC_COUNT_PER_REG)
>>
>> +static int nr_pics;
>> +
>> struct pch_pic {
>> void __iomem *base;
>> struct irq_domain *pic_domain;
>> + struct fwnode_handle *domain_handle;
>> u32 ht_vec_base;
>> raw_spinlock_t pic_lock;
>> + u32 gsi_end;
>> + u32 gsi_base;
>> };
>>
>> +static struct pch_pic *pch_pic_priv[2];
>
> Why 2? Is that related to MAX_IO_PICS being 2?
>
Yes, thanks, I should use MAX_IO_PICS for it.
>> +struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
>> +
>> +int find_pch_pic(u32 gsi)
>> +{
>> + int i;
>> +
>> + /* Find the PCH_PIC that manages this GSI. */
>> + for (i = 0; i < MAX_IO_PICS; i++) {
>> + struct pch_pic *priv = pch_pic_priv[i];
>> +
>> + if (!priv)
>> + return -1;
>> +
>> + if (gsi >= priv->gsi_base && gsi <= priv->gsi_end)
>> + return i;
>> + }
>> +
>> + pr_err("ERROR: Unable to locate PCH_PIC for GSI %d\n", gsi);
>> + return -1;
>> +}
>> +
>> static void pch_pic_bitset(struct pch_pic *priv, int offset, int bit)
>> {
>> u32 reg;
>> @@ -139,6 +166,24 @@ static void pch_pic_ack_irq(struct irq_data *d)
>> .irq_set_type = pch_pic_set_type,
>> };
>>
>> +static int pch_pic_domain_translate(struct irq_domain *d,
>> + struct irq_fwspec *fwspec,
>> + unsigned long *hwirq,
>> + unsigned int *type)
>> +{
>> + struct pch_pic *priv = d->host_data;
>> + struct device_node *of_node = to_of_node(fwspec->fwnode);
>> +
>> + if (fwspec->param_count < 1)
>> + return -EINVAL;
>> + if (of_node)
>> + *hwirq += priv->ht_vec_base;
>> + else
>> + *hwirq = fwspec->param[0] - priv->gsi_base;
>> + *type = IRQ_TYPE_NONE;
>
> Have you tested this on MIPS HW? This looks like a regression on the
> OF path, as it used irq_domain_translate_twocell().
>
No, I don't test on MIPS for this version patch, I want to use 'of_node'
to distinguish MIPS and LoongArch, but it seems that there is still
other problem, I'll fix it as following in next version and test it on MIPS.
if (of_node) {
*hwirq = fwspec->param[0] + priv->ht_vec_base;
*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
} else {
*hwirq = fwspec->param[0] - priv->gsi_base;
*type = IRQ_TYPE_NONE;
}
>> +
>> + return 0;
>> +}
>> static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
>> unsigned int nr_irqs, void *arg)
>> {
>> @@ -149,13 +194,13 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
>> struct irq_fwspec parent_fwspec;
>> struct pch_pic *priv = domain->host_data;
>>
>> - err = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
>> + err = pch_pic_domain_translate(domain, fwspec, &hwirq, &type);
>> if (err)
>> return err;
>>
>> parent_fwspec.fwnode = domain->parent->fwnode;
>> parent_fwspec.param_count = 1;
>> - parent_fwspec.param[0] = hwirq + priv->ht_vec_base;
>> + parent_fwspec.param[0] = hwirq;
>>
>> err = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
>> if (err)
>> @@ -170,7 +215,7 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
>> }
>>
>> static const struct irq_domain_ops pch_pic_domain_ops = {
>> - .translate = irq_domain_translate_twocell,
>> + .translate = pch_pic_domain_translate,
>> .alloc = pch_pic_alloc,
>> .free = irq_domain_free_irqs_parent,
>> };
>> @@ -180,7 +225,7 @@ static void pch_pic_reset(struct pch_pic *priv)
>> int i;
>>
>> for (i = 0; i < PIC_COUNT; i++) {
>> - /* Write vectored ID */
>> + /* Write vector ID */
>> writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(i));
>> /* Hardcode route to HT0 Lo */
>> writeb(1, priv->base + PCH_INT_ROUTE(i));
>> @@ -198,50 +243,41 @@ static void pch_pic_reset(struct pch_pic *priv)
>> }
>> }
>>
>> -static int pch_pic_of_init(struct device_node *node,
>> - struct device_node *parent)
>> +static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
>> + struct irq_domain *parent_domain, struct fwnode_handle *domain_handle,
>> + u32 gsi_base)
>> {
>> + int vec_count;
>> struct pch_pic *priv;
>> - struct irq_domain *parent_domain;
>> - int err;
>>
>> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> if (!priv)
>> return -ENOMEM;
>>
>> raw_spin_lock_init(&priv->pic_lock);
>> - priv->base = of_iomap(node, 0);
>> - if (!priv->base) {
>> - err = -ENOMEM;
>> + priv->base = ioremap(addr, size);
>> + if (!priv->base)
>> goto free_priv;
>> - }
>>
>> - parent_domain = irq_find_host(parent);
>> - if (!parent_domain) {
>> - pr_err("Failed to find the parent domain\n");
>> - err = -ENXIO;
>> - goto iounmap_base;
>> - }
>> + priv->domain_handle = domain_handle;
>>
>> - if (of_property_read_u32(node, "loongson,pic-base-vec",
>> - &priv->ht_vec_base)) {
>> - pr_err("Failed to determine pic-base-vec\n");
>> - err = -EINVAL;
>> - goto iounmap_base;
>> - }
>> + priv->ht_vec_base = vec_base;
>
> Why isn't this pre-filled on the OF path as it isn't relevant to ACPI?
>
No, ht_vec_base is required for both of OF and ACPI.
pch_pic_init is called from pch_pic_of_init for OF path and from
pch_pic_init_irqdomain for ACPI path, so the 'vec_base' is passed into
pch_pic_init to pre-fill 'ht_vec_base' from both path.
>> + vec_count = ((readq(priv->base) >> 48) & 0xff) + 1;
>
> Is that valid on the old HW?
>
Yes.
>> + priv->gsi_base = gsi_base;
>
> Why isn't this pre-filled on the ACPI path as it isn't relevant to OF?
>
Yes, it's only for ACPI.
>> + priv->gsi_end = gsi_base + vec_count - 1;
>
> I don't think this needs to be ACPI specific. Just track the vector
> count, which applies to both ACPI and DT.
>
Agree, thanks for your suggestion, tracking vector count is enough for both.
>>
>> priv->pic_domain = irq_domain_create_hierarchy(parent_domain, 0,
>> - PIC_COUNT,
>> - of_node_to_fwnode(node),
>> - &pch_pic_domain_ops,
>> - priv);
>> + vec_count, priv->domain_handle,
>> + &pch_pic_domain_ops, priv);
>> +
>> if (!priv->pic_domain) {
>> pr_err("Failed to create IRQ domain\n");
>> - err = -ENOMEM;
>> goto iounmap_base;
>> }
>>
>> pch_pic_reset(priv);
>> + pch_pic_handle[nr_pics] = domain_handle;
>> + pch_pic_priv[nr_pics++] = priv;
>>
>> return 0;
>>
>> @@ -250,7 +286,111 @@ static int pch_pic_of_init(struct device_node *node,
>> free_priv:
>> kfree(priv);
>>
>> - return err;
>> + return -EINVAL;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +
>> +static int pch_pic_of_init(struct device_node *node,
>> + struct device_node *parent)
>> +{
>> + int err, vec_base;
>> + struct resource res;
>> + struct irq_domain *parent_domain;
>> +
>> + if (of_address_to_resource(node, 0, &res))
>> + return -EINVAL;
>> +
>> + parent_domain = irq_find_host(parent);
>> + if (!parent_domain) {
>> + pr_err("Failed to find the parent domain\n");
>> + return -ENXIO;
>> + }
>> +
>> + if (of_property_read_u32(node, "loongson,pic-base-vec", &vec_base)) {
>> + pr_err("Failed to determine pic-base-vec\n");
>> + return -EINVAL;
>> + }
>> +
>> + err = pch_pic_init(res.start, resource_size(&res), vec_base,
>> + parent_domain, of_node_to_fwnode(node), 0);
>> + if (err < 0)
>> + return err;
>> +
>> + return 0;
>> }
>>
>> IRQCHIP_DECLARE(pch_pic, "loongson,pch-pic-1.0", pch_pic_of_init);
>> +
>> +#endif
>> +
>> +#ifdef CONFIG_ACPI
>> +static int __init
>> +lpcintc_parse_madt(union acpi_subtable_headers *header,
>> + const unsigned long end)
>> +{
>> + struct acpi_madt_lpc_pic *lpcintc_entry = (struct acpi_madt_lpc_pic *)header;
>> +
>> + return pch_lpc_acpi_init(pch_pic_priv[0]->pic_domain, lpcintc_entry);
>> +}
>> +
>> +static int __init acpi_cascade_irqdomain_init(void)
>> +{
>> + acpi_table_parse_madt(ACPI_MADT_TYPE_LPC_PIC,
>> + lpcintc_parse_madt, 0);
>> + return 0;
>> +}
>
> Missing blank line between functions
>
Ok, thanks for your correction.
>> +int __init pch_pic_init_irqdomain(struct irq_domain *parent,
>> + struct acpi_madt_bio_pic *acpi_pchpic)
>> +{
>> + int ret, vec_base;
>> + struct fwnode_handle *domain_handle;
>> +
>> + if (!acpi_pchpic)
>> + return -EINVAL;
>> +
>> + vec_base = acpi_pchpic->gsi_base - GSI_MIN_PCH_IRQ;
>> +
>> + domain_handle = irq_domain_alloc_fwnode((phys_addr_t *)acpi_pchpic);
>> + if (!domain_handle) {
>> + pr_err("Unable to allocate domain handle\n");
>> + return -ENOMEM;
>> + }
>> +
>> + ret = pch_pic_init(acpi_pchpic->address, acpi_pchpic->size,
>> + vec_base, parent, domain_handle, acpi_pchpic->gsi_base);
>> + if (ret == 0 && acpi_pchpic->id == 0)
>> + acpi_cascade_irqdomain_init();
>> +
>> + return ret;
>> +}
>> +
>> +struct irq_domain *acpi_get_pch_parent(int node)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < MAX_IO_PICS; i++) {
>> + if (node == pch_group[i].node)
>> + return pch_group[i].parent;
>> + }
>> + return NULL;
>> +}
>> +
>> +static int __init pchintc_parse_madt(union acpi_subtable_headers *header,
>> + const unsigned long end)
>> +{
>> + struct acpi_madt_bio_pic *pch_pic_entry = (struct acpi_madt_bio_pic *)header;
>> +
>> + return pch_pic_init_irqdomain(acpi_get_pch_parent((pch_pic_entry->address >> 44) & 0xf),
>> + pch_pic_entry);
>> +}
>> +
>> +static int __init pch_pic_acpi_init(void)
>> +{
>> + acpi_table_parse_madt(ACPI_MADT_TYPE_BIO_PIC,
>
> Where is this defined? It only appears in the documentation, and
> nowhere else...
>
It's defined in the patch to ACPICA(which has been merged, please to see
https://github.com/acpica/acpica/commit/1dc530059a3e6202e941e6a9478cf30f092bfb47).
the patch will be synchronized to linux kernel by maintainer of ACPICA.
>> + pchintc_parse_madt, 0);
>> +
>> + return 0;
>> +}
>> +early_initcall(pch_pic_acpi_init);
>
> Why can't you use IRQCHIP_ACPI_DECLARE here? This is terribly fragile,
> and will eventually break. I really don't want to rely on this.
>
In early time, the change here is implemented using
IRQCHIP_ACPI_DECLARE, but we found that calling order(during
irqchip_init) of the entry declared using IRQCHIP_ACPI_DECLARE is
depended on the compiling order(driver order in Makefile) of the driver.
For removing the dependency to the compiling order, the new way here is
used(I looked into ARM, it seems that GIC driver uses
IRQCHIP_ACPI_DECLARE, and ITS driver uses early_initcall too.).
> M.
>
Powered by blists - more mailing lists