| 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: Tue, 12 Mar 2019 21:59:09 +0800 (CST)
From: "Liu Xiang" <liuxiang_1999@....com>
To: "Marc Zyngier" <marc.zyngier@....com>
Cc: "Liu Xiang" <liu.xiang6@....com.cn>, tglx@...utronix.de,
jason@...edaemon.net, linux-kernel@...r.kernel.org
Subject: Re:Re: [PATCH] irqchip/gic: fix passing wrong start irq number to
irq_alloc_descs() for secondary GICs
Hi, Marc
Thanks for your reply！
At 2019-03-11 23:55:11, "Marc Zyngier" <marc.zyngier@....com> wrote:
>On 11/03/2019 14:52, Liu Xiang wrote:
>> For secondary GICs, the start irq number should skip over SGIs
>> and PPIs. Its value should be 32. So we should pass hwirq_base to
>> irq_alloc_descs() rather than a constant number 16.
>>
>> Signed-off-by: Liu Xiang <liu.xiang6@....com.cn>
>> ---
>> drivers/irqchip/irq-gic.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index ba2a37a..351f576 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -1157,7 +1157,7 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
>>
>> gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>>
>> - irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
>> + irq_base = irq_alloc_descs(irq_start, hwirq_base, gic_irqs,
>> numa_node_id());
>> if (irq_base < 0) {
>> WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
>>
>
>I suggest you look at __irq_alloc_descs(), and understand what the
>various parameters mean. What you're doing here has absolutely no
>effect.
>
>The right thing to do would be to get rid of this altogether, except
>that we have exactly *one* platform in the tree that is still non-DT
>(some unmaintained Cavium piece of junk). But we can still simplify it,
>as this guy doesn't have a secondary GIC (it is braindead enough).
>
>What I'm suggesting instead is:
>
>From b41fdc4a7bf9045e4871c5b15905ea732ffd044f Mon Sep 17 00:00:00 2001
>From: Marc Zyngier <marc.zyngier@....com>
>Date: Mon, 11 Mar 2019 15:38:10 +0000
>Subject: [PATCH] irqchip/gic: Drop support for secondary GIC in non-DT systems
>
>We do not have any in-tree platform with this pathological setup,
>and only a single system (Cavium's cns3xxx) isn't DT aware.
>
>Let's drop the secondary GIC support for now, until we remove
>the above horror altogether.
>
>Signed-off-by: Marc Zyngier <marc.zyngier@....com>
>---
> arch/arm/mach-cns3xxx/core.c | 2 +-
> drivers/irqchip/irq-gic.c | 45 ++++++++++++---------------------
> include/linux/irqchip/arm-gic.h | 3 +--
> 3 files changed, 18 insertions(+), 32 deletions(-)
>
>diff --git a/arch/arm/mach-cns3xxx/core.c b/arch/arm/mach-cns3xxx/core.c
>index 7d5a44a06648..f676592d8402 100644
>--- a/arch/arm/mach-cns3xxx/core.c
>+++ b/arch/arm/mach-cns3xxx/core.c
>@@ -90,7 +90,7 @@ void __init cns3xxx_map_io(void)
> /* used by entry-macro.S */
> void __init cns3xxx_init_irq(void)
> {
>- gic_init(0, 29, IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT),
>+ gic_init(IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT),
> IOMEM(CNS3XXX_TC11MP_GIC_CPU_BASE_VIRT));
> }
>
>diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>index ba2a37a27a54..fd3110c171ba 100644
>--- a/drivers/irqchip/irq-gic.c
>+++ b/drivers/irqchip/irq-gic.c
>@@ -1089,11 +1089,10 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev,
> #endif
> }
>
>-static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
>+static int gic_init_bases(struct gic_chip_data *gic,
> struct fwnode_handle *handle)
> {
>- irq_hw_number_t hwirq_base;
>- int gic_irqs, irq_base, ret;
>+ int gic_irqs, ret;
>
> if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && gic->percpu_offset) {
> /* Frankein-GIC without banked registers... */
>@@ -1145,28 +1144,21 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
> } else { /* Legacy support */
> /*
> * For primary GICs, skip over SGIs.
>- * For secondary GICs, skip over PPIs, too.
>+ * No secondary GIC support whatsoever.
> */
>- if (gic == &gic_data[0] && (irq_start & 31) > 0) {
>- hwirq_base = 16;
>- if (irq_start != -1)
>- irq_start = (irq_start & ~31) + 16;
>- } else {
>- hwirq_base = 32;
>- }
>+ int irq_base;
>
>- gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>+ gic_irqs -= 16; /* calculate # of irqs to allocate */
>
>- irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
>+ irq_base = irq_alloc_descs(16, 16, gic_irqs,
> numa_node_id());
> if (irq_base < 0) {
>- WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
>- irq_start);
>- irq_base = irq_start;
>+ WARN(1, "Cannot allocate irq_descs @ IRQ16, assuming pre-allocated\n");
>+ irq_base = 16;
> }
>
> gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base,
>- hwirq_base, &gic_irq_domain_ops, gic);
>+ 16, &gic_irq_domain_ops, gic);
> }
>
> if (WARN_ON(!gic->domain)) {
>@@ -1195,7 +1187,6 @@ static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
> }
>
> static int __init __gic_init_bases(struct gic_chip_data *gic,
>- int irq_start,
> struct fwnode_handle *handle)
> {
> char *name;
>@@ -1231,32 +1222,28 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
> gic_init_chip(gic, NULL, name, false);
> }
>
>- ret = gic_init_bases(gic, irq_start, handle);
>+ ret = gic_init_bases(gic, handle);
> if (ret)
> kfree(name);
>
> return ret;
> }
>
>-void __init gic_init(unsigned int gic_nr, int irq_start,
>- void __iomem *dist_base, void __iomem *cpu_base)
>+void __init gic_init(void __iomem *dist_base, void __iomem *cpu_base)
> {
> struct gic_chip_data *gic;
>
>- if (WARN_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR))
>- return;
>-
> /*
> * Non-DT/ACPI systems won't run a hypervisor, so let's not
> * bother with these...
> */
> static_branch_disable(&supports_deactivate_key);
>
>- gic = &gic_data[gic_nr];
>+ gic = &gic_data[0];
> gic->raw_dist_base = dist_base;
> gic->raw_cpu_base = cpu_base;
>
>- __gic_init_bases(gic, irq_start, NULL);
>+ __gic_init_bases(gic, NULL);
> }
>
> static void gic_teardown(struct gic_chip_data *gic)
>@@ -1399,7 +1386,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq)
> if (ret)
> return ret;
>
>- ret = gic_init_bases(*gic, -1, &dev->of_node->fwnode);
>+ ret = gic_init_bases(*gic, &dev->of_node->fwnode);
> if (ret) {
> gic_teardown(*gic);
> return ret;
>@@ -1459,7 +1446,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
> if (gic_cnt == 0 && !gic_check_eoimode(node, &gic->raw_cpu_base))
> static_branch_disable(&supports_deactivate_key);
>
>- ret = __gic_init_bases(gic, -1, &node->fwnode);
>+ ret = __gic_init_bases(gic, &node->fwnode);
> if (ret) {
> gic_teardown(gic);
> return ret;
>@@ -1650,7 +1637,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
> return -ENOMEM;
> }
>
>- ret = __gic_init_bases(gic, -1, domain_handle);
>+ ret = __gic_init_bases(gic, domain_handle);
> if (ret) {
> pr_err("Failed to initialise GIC\n");
> irq_domain_free_fwnode(domain_handle);
>diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
>index 626179077bb0..0f049b384ccd 100644
>--- a/include/linux/irqchip/arm-gic.h
>+++ b/include/linux/irqchip/arm-gic.h
>@@ -158,8 +158,7 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq);
> * Legacy platforms not converted to DT yet must use this to init
> * their GIC
> */
>-void gic_init(unsigned int nr, int start,
>- void __iomem *dist , void __iomem *cpu);
>+void gic_init(void __iomem *dist , void __iomem *cpu);
>
> int gicv2m_init(struct fwnode_handle *parent_handle,
> struct irq_domain *parent);
>--
>2.20.1
>
>Thanks,
>
> M.
>--
>Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists