| Message ID | 1573031953-12894-8-git-send-email-andrii.anisov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Build XEN with ARM Compiler 6.6.3 | expand |
On Wed, 6 Nov 2019, Andrii Anisov wrote: > From: Andrii Anisov <andrii_anisov@epam.com> > > ARM Compiler 6.6 has a proven bug: static data symbols, moved to an init > section, becomes global. Thus these symbols clash with ones defined in > gic-v2.c. The straight forward way to resolve the issue is to add the GIC > version suffix, at least for one of the conflicting side. > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> The patch is acceptable but this seems a very serious compiler bug. This, together with the other bug described in the previous patch, makes me think the ARMCC is not quite ready for showtime. Do you know if there are any later version of the compiler that don't have these problems? I would hate to introduce these workarounds, especially the one in patch #6, if they won't be required anymore with the next ARMCC version. > --- > xen/arch/arm/gic-v3.c | 68 +++++++++++++++++++++++++-------------------------- > 1 file changed, 34 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 0f6cbf6..f57597a 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -1328,14 +1328,14 @@ static const hw_irq_controller gicv3_guest_irq_type = { > .set_affinity = gicv3_irq_set_affinity, > }; > > -static paddr_t __initdata dbase = INVALID_PADDR; > -static paddr_t __initdata vbase = INVALID_PADDR, vsize = 0; > -static paddr_t __initdata cbase = INVALID_PADDR, csize = 0; > +static paddr_t __initdata dbase_v3 = INVALID_PADDR; > +static paddr_t __initdata vbase_v3 = INVALID_PADDR, vsize_v3 = 0; > +static paddr_t __initdata cbase_v3 = INVALID_PADDR, csize_v3 = 0; > > /* If the GICv3 supports GICv2, initialize it */ > static void __init gicv3_init_v2(void) > { > - if ( cbase == INVALID_PADDR || vbase == INVALID_PADDR ) > + if ( cbase_v3 == INVALID_PADDR || vbase_v3 == INVALID_PADDR ) > return; > > /* > @@ -1343,26 +1343,26 @@ static void __init gicv3_init_v2(void) > * So only support GICv2 on GICv3 when the virtual CPU interface is > * at least GUEST_GICC_SIZE. > */ > - if ( vsize < GUEST_GICC_SIZE ) > + if ( vsize_v3 < GUEST_GICC_SIZE ) > { > printk(XENLOG_WARNING > "GICv3: WARNING: Not enabling support for GICv2 compat mode.\n" > "Size of GICV (%#"PRIpaddr") must at least be %#llx.\n", > - vsize, GUEST_GICC_SIZE); > + vsize_v3, GUEST_GICC_SIZE); > return; > } > > printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n", > - cbase, vbase); > + cbase_v3, vbase_v3); > > - vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0); > + vgic_v2_setup_hw(dbase_v3, cbase_v3, csize_v3, vbase_v3, 0); > } > > static void __init gicv3_ioremap_distributor(paddr_t dist_paddr) > { > if ( dist_paddr & ~PAGE_MASK ) > panic("GICv3: Found unaligned distributor address %"PRIpaddr"\n", > - dbase); > + dbase_v3); > > gicv3.map_dbase = ioremap_nocache(dist_paddr, SZ_64K); > if ( !gicv3.map_dbase ) > @@ -1375,11 +1375,11 @@ static void __init gicv3_dt_init(void) > int res, i; > const struct dt_device_node *node = gicv3_info.node; > > - res = dt_device_get_address(node, 0, &dbase, NULL); > + res = dt_device_get_address(node, 0, &dbase_v3, NULL); > if ( res ) > panic("GICv3: Cannot find a valid distributor address\n"); > > - gicv3_ioremap_distributor(dbase); > + gicv3_ioremap_distributor(dbase_v3); > > if ( !dt_property_read_u32(node, "#redistributor-regions", > &gicv3.rdist_count) ) > @@ -1416,10 +1416,10 @@ static void __init gicv3_dt_init(void) > * provided. > */ > res = dt_device_get_address(node, 1 + gicv3.rdist_count, > - &cbase, &csize); > + &cbase_v3, &csize_v3); > if ( !res ) > dt_device_get_address(node, 1 + gicv3.rdist_count + 2, > - &vbase, &vsize); > + &vbase_v3, &vsize_v3); > } > > static int gicv3_iomem_deny_access(const struct domain *d) > @@ -1427,7 +1427,7 @@ static int gicv3_iomem_deny_access(const struct domain *d) > int rc, i; > unsigned long mfn, nr; > > - mfn = dbase >> PAGE_SHIFT; > + mfn = dbase_v3 >> PAGE_SHIFT; > nr = PFN_UP(SZ_64K); > rc = iomem_deny_access(d, mfn, mfn + nr); > if ( rc ) > @@ -1446,19 +1446,19 @@ static int gicv3_iomem_deny_access(const struct domain *d) > return rc; > } > > - if ( cbase != INVALID_PADDR ) > + if ( cbase_v3 != INVALID_PADDR ) > { > - mfn = cbase >> PAGE_SHIFT; > - nr = PFN_UP(csize); > + mfn = cbase_v3 >> PAGE_SHIFT; > + nr = PFN_UP(csize_v3); > rc = iomem_deny_access(d, mfn, mfn + nr); > if ( rc ) > return rc; > } > > - if ( vbase != INVALID_PADDR ) > + if ( vbase_v3 != INVALID_PADDR ) > { > - mfn = vbase >> PAGE_SHIFT; > - nr = PFN_UP(csize); > + mfn = vbase_v3 >> PAGE_SHIFT; > + nr = PFN_UP(csize_v3); > return iomem_deny_access(d, mfn, mfn + nr); > } > > @@ -1564,8 +1564,8 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, > /* Read from APIC table and fill up the GIC variables */ > if ( !cpu_base_assigned ) > { > - cbase = processor->base_address; > - vbase = processor->gicv_base_address; > + cbase_v3 = processor->base_address; > + vbase_v3 = processor->gicv_base_address; > gicv3_info.maintenance_irq = processor->vgic_interrupt; > > if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE ) > @@ -1577,8 +1577,8 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, > } > else > { > - if ( cbase != processor->base_address > - || vbase != processor->gicv_base_address > + if ( cbase_v3 != processor->base_address > + || vbase_v3 != processor->gicv_base_address > || gicv3_info.maintenance_irq != processor->vgic_interrupt ) > { > printk("GICv3: GICC entries are not same in MADT table\n"); > @@ -1599,7 +1599,7 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, > if ( BAD_MADT_ENTRY(dist, end) ) > return -EINVAL; > > - dbase = dist->base_address; > + dbase_v3 = dist->base_address; > > return 0; > } > @@ -1674,7 +1674,7 @@ static void __init gicv3_acpi_init(void) > if ( count <= 0 ) > panic("GICv3: No valid GICD entries exists\n"); > > - gicv3_ioremap_distributor(dbase); > + gicv3_ioremap_distributor(dbase_v3); > > /* Get number of redistributor */ > count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, > @@ -1722,15 +1722,15 @@ static void __init gicv3_acpi_init(void) > * Also set the size of the GICC and GICV when there base address > * is not invalid as those values are not present in ACPI. > */ > - if ( !cbase ) > - cbase = INVALID_PADDR; > + if ( !cbase_v3 ) > + cbase_v3 = INVALID_PADDR; > else > - csize = SZ_8K; > + csize_v3 = SZ_8K; > > - if ( !vbase ) > - vbase = INVALID_PADDR; > + if ( !vbase_v3 ) > + vbase_v3 = INVALID_PADDR; > else > - vsize = GUEST_GICC_SIZE; > + vsize_v3 = GUEST_GICC_SIZE; > > } > #else > @@ -1789,7 +1789,7 @@ static int __init gicv3_init(void) > " gic_maintenance_irq=%u\n" > " gic_rdist_stride=%#x\n" > " gic_rdist_regions=%d\n", > - dbase, gicv3_info.maintenance_irq, > + dbase_v3, gicv3_info.maintenance_irq, > gicv3.rdist_stride, gicv3.rdist_count); > printk(" redistributor regions:\n"); > for ( i = 0; i < gicv3.rdist_count; i++ ) > @@ -1803,7 +1803,7 @@ static int __init gicv3_init(void) > reg = readl_relaxed(GICD + GICD_TYPER); > intid_bits = GICD_TYPE_ID_BITS(reg); > > - vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions, intid_bits); > + vgic_v3_setup_hw(dbase_v3, gicv3.rdist_count, gicv3.rdist_regions, intid_bits); > gicv3_init_v2(); > > spin_lock_init(&gicv3.lock); > -- > 2.7.4 >
On Tue, 12 Nov 2019, 05:59 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Wed, 6 Nov 2019, Andrii Anisov wrote: > > From: Andrii Anisov <andrii_anisov@epam.com> > > > > ARM Compiler 6.6 has a proven bug: static data symbols, moved to an init > > section, becomes global. Thus these symbols clash with ones defined in > > gic-v2.c. The straight forward way to resolve the issue is to add the GIC > > version suffix, at least for one of the conflicting side. > > > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > > The patch is acceptable but this seems a very serious compiler bug. > I am a bit worried this is not going to prevent introducing any similar bug in the future. I think, we have a way to enforce uniq symbols (see CONFIG_UNIQUE_SYMBOLS). Would it work for you here? This, together with the other bug described in the previous patch, makes > me think the ARMCC is not quite ready for showtime. Do you know if there > are any later version of the compiler that don't have these problems? > Related to this as this been reported to Arm? Cheers,
Hello Stefano, On 11.11.19 22:59, Stefano Stabellini wrote: > this seems a very serious compiler bug. Yep. > This, together with the other bug described in the previous patch, makes > me think the ARMCC is not quite ready for showtime. Yet, this particular ARM Compiler version is safety certified and LTS. > Do you know if there > are any later version of the compiler that don't have these problems? I don't know, ARM did not say something special about it. As I know, the reason to take this compiler version was that it is the "latest and greatest" safety certified > I would hate to introduce these workarounds I hated finding and publishing these workarounds, but here we are. The main question here is if XEN needs a tag "Support safety certified compiler" by the cost of accepting such workarounds. Then discuss how to reduce their stench.
On Thu, 14 Nov 2019, Andrii Anisov wrote: > Hello Stefano, > > On 11.11.19 22:59, Stefano Stabellini wrote: > > this seems a very serious compiler bug. > > Yep. > > > This, together with the other bug described in the previous patch, makes > > me think the ARMCC is not quite ready for showtime. > > Yet, this particular ARM Compiler version is safety certified and LTS. > > > Do you know if there > > are any later version of the compiler that don't have these problems? > > I don't know, ARM did not say something special about it. As I know, the > reason to take this compiler version was that it is the "latest and greatest" > safety certified > > > I would hate to introduce these workarounds > > I hated finding and publishing these workarounds, but here we are. > > The main question here is if XEN needs a tag "Support safety certified > compiler" by the cost of accepting such workarounds. > Then discuss how to reduce their stench. Before we get to that point, maybe we can raise the issue with Arm using our combined channels. I'll raise it internally at Xilinx, and we could also discuss it during one of the next FuSa calls (list in CC).
Hello Stefano, I suppose, in the discussion with ARM, it might be useful to come with existing support case numbers. Here they are: Case ID. sort descending<https://support.developer.arm.com/my-cases/?_ga=2.64479850.476004835.1574326833-426220411.1564048157#> Status. sort descending<https://support.developer.arm.com/my-cases/?_ga=2.64479850.476004835.1574326833-426220411.1564048157#> Product. sort descending<https://support.developer.arm.com/my-cases/?_ga=2.64479850.476004835.1574326833-426220411.1564048157#> Summary. sort descending<https://support.developer.arm.com/my-cases/?_ga=2.64479850.476004835.1574326833-426220411.1564048157#> Created On. sort descending<https://support.developer.arm.com/my-cases/?_ga=2.64479850.476004835.1574326833-426220411.1564048157#> Updated on . sort descending<https://support.developer.arm.com/my-cases/?_ga=2.64479850.476004835.1574326833-426220411.1564048157#> Actions CAS-138402-Y0Y9C3<https://support.developer.arm.com/case-details/?id=9c2bd9a0-8af1-e911-b862-28187887f93a> Defect/Enhancement ARM COMPILER 6 C style shift operators are missed among supported scatter file expressions Oct-18-2019 Nov-19-2019 CAS-137352-T7F4V1<https://support.developer.arm.com/case-details/?id=bacb8de4-92ea-e911-b862-28187887f93a> More Information Needed DS-5 ARM COMPILER-6 ULTIMATE FL ARM Linker defined symbols are not counted as referred from a steering file. Oct-09-2019 Nov-12-2019 CAS-138292-L5S0V0<https://support.developer.arm.com/case-details/?id=c69f223a-ebf0-e911-b862-28187887f93a> More Information Needed ARM COMPILER 6 Static data symbols, moved to init section, becomes global. Oct-17-2019 Oct-24-2019 CAS-137357-Z7W3B8<https://support.developer.arm.com/case-details/?id=cd97ea01-97ea-e911-b862-28187887f93a> Defect/Enhancement DS-5 ARM COMPILER-6 ULTIMATE FL ARM Compiler 6 compiles data only C file with SoftVFP attribute. Oct-09-2019 Oct-18-2019 CAS-137359-V7G6W6<https://support.developer.arm.com/case-details/?id=c921919d-97ea-e911-b862-28187887f93a> Closed DS-5 ARM COMPILER-6 ULTIMATE FL How to rename sections using ARM Compiler 6 tools? Oct-09-2019 Oct-11-2019 ANDRII ANISOV Lead Systems Engineer Office: +380 44 390 5457<tel:+380%2044%20390%205457> x 66766<tel:66766> Cell: +380 50 573 8852<tel:+380%2050%20573%208852> Email: andrii_anisov@epam.com<mailto:andrii_anisov@epam.com> Kyiv, Ukraine (GMT+3) epam.com<http://www.epam.com> CONFIDENTIALITY CAUTION AND DISCLAIMER This message is intended only for the use of the individual(s) or entity(ies) to which it is addressed and contains information that is legally privileged and confidential. If you are not the intended recipient, or the person responsible for delivering the message to the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. All unintended recipients are obliged to delete this message and destroy any printed copies.
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 0f6cbf6..f57597a 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1328,14 +1328,14 @@ static const hw_irq_controller gicv3_guest_irq_type = { .set_affinity = gicv3_irq_set_affinity, }; -static paddr_t __initdata dbase = INVALID_PADDR; -static paddr_t __initdata vbase = INVALID_PADDR, vsize = 0; -static paddr_t __initdata cbase = INVALID_PADDR, csize = 0; +static paddr_t __initdata dbase_v3 = INVALID_PADDR; +static paddr_t __initdata vbase_v3 = INVALID_PADDR, vsize_v3 = 0; +static paddr_t __initdata cbase_v3 = INVALID_PADDR, csize_v3 = 0; /* If the GICv3 supports GICv2, initialize it */ static void __init gicv3_init_v2(void) { - if ( cbase == INVALID_PADDR || vbase == INVALID_PADDR ) + if ( cbase_v3 == INVALID_PADDR || vbase_v3 == INVALID_PADDR ) return; /* @@ -1343,26 +1343,26 @@ static void __init gicv3_init_v2(void) * So only support GICv2 on GICv3 when the virtual CPU interface is * at least GUEST_GICC_SIZE. */ - if ( vsize < GUEST_GICC_SIZE ) + if ( vsize_v3 < GUEST_GICC_SIZE ) { printk(XENLOG_WARNING "GICv3: WARNING: Not enabling support for GICv2 compat mode.\n" "Size of GICV (%#"PRIpaddr") must at least be %#llx.\n", - vsize, GUEST_GICC_SIZE); + vsize_v3, GUEST_GICC_SIZE); return; } printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n", - cbase, vbase); + cbase_v3, vbase_v3); - vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0); + vgic_v2_setup_hw(dbase_v3, cbase_v3, csize_v3, vbase_v3, 0); } static void __init gicv3_ioremap_distributor(paddr_t dist_paddr) { if ( dist_paddr & ~PAGE_MASK ) panic("GICv3: Found unaligned distributor address %"PRIpaddr"\n", - dbase); + dbase_v3); gicv3.map_dbase = ioremap_nocache(dist_paddr, SZ_64K); if ( !gicv3.map_dbase ) @@ -1375,11 +1375,11 @@ static void __init gicv3_dt_init(void) int res, i; const struct dt_device_node *node = gicv3_info.node; - res = dt_device_get_address(node, 0, &dbase, NULL); + res = dt_device_get_address(node, 0, &dbase_v3, NULL); if ( res ) panic("GICv3: Cannot find a valid distributor address\n"); - gicv3_ioremap_distributor(dbase); + gicv3_ioremap_distributor(dbase_v3); if ( !dt_property_read_u32(node, "#redistributor-regions", &gicv3.rdist_count) ) @@ -1416,10 +1416,10 @@ static void __init gicv3_dt_init(void) * provided. */ res = dt_device_get_address(node, 1 + gicv3.rdist_count, - &cbase, &csize); + &cbase_v3, &csize_v3); if ( !res ) dt_device_get_address(node, 1 + gicv3.rdist_count + 2, - &vbase, &vsize); + &vbase_v3, &vsize_v3); } static int gicv3_iomem_deny_access(const struct domain *d) @@ -1427,7 +1427,7 @@ static int gicv3_iomem_deny_access(const struct domain *d) int rc, i; unsigned long mfn, nr; - mfn = dbase >> PAGE_SHIFT; + mfn = dbase_v3 >> PAGE_SHIFT; nr = PFN_UP(SZ_64K); rc = iomem_deny_access(d, mfn, mfn + nr); if ( rc ) @@ -1446,19 +1446,19 @@ static int gicv3_iomem_deny_access(const struct domain *d) return rc; } - if ( cbase != INVALID_PADDR ) + if ( cbase_v3 != INVALID_PADDR ) { - mfn = cbase >> PAGE_SHIFT; - nr = PFN_UP(csize); + mfn = cbase_v3 >> PAGE_SHIFT; + nr = PFN_UP(csize_v3); rc = iomem_deny_access(d, mfn, mfn + nr); if ( rc ) return rc; } - if ( vbase != INVALID_PADDR ) + if ( vbase_v3 != INVALID_PADDR ) { - mfn = vbase >> PAGE_SHIFT; - nr = PFN_UP(csize); + mfn = vbase_v3 >> PAGE_SHIFT; + nr = PFN_UP(csize_v3); return iomem_deny_access(d, mfn, mfn + nr); } @@ -1564,8 +1564,8 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, /* Read from APIC table and fill up the GIC variables */ if ( !cpu_base_assigned ) { - cbase = processor->base_address; - vbase = processor->gicv_base_address; + cbase_v3 = processor->base_address; + vbase_v3 = processor->gicv_base_address; gicv3_info.maintenance_irq = processor->vgic_interrupt; if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE ) @@ -1577,8 +1577,8 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, } else { - if ( cbase != processor->base_address - || vbase != processor->gicv_base_address + if ( cbase_v3 != processor->base_address + || vbase_v3 != processor->gicv_base_address || gicv3_info.maintenance_irq != processor->vgic_interrupt ) { printk("GICv3: GICC entries are not same in MADT table\n"); @@ -1599,7 +1599,7 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, if ( BAD_MADT_ENTRY(dist, end) ) return -EINVAL; - dbase = dist->base_address; + dbase_v3 = dist->base_address; return 0; } @@ -1674,7 +1674,7 @@ static void __init gicv3_acpi_init(void) if ( count <= 0 ) panic("GICv3: No valid GICD entries exists\n"); - gicv3_ioremap_distributor(dbase); + gicv3_ioremap_distributor(dbase_v3); /* Get number of redistributor */ count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, @@ -1722,15 +1722,15 @@ static void __init gicv3_acpi_init(void) * Also set the size of the GICC and GICV when there base address * is not invalid as those values are not present in ACPI. */ - if ( !cbase ) - cbase = INVALID_PADDR; + if ( !cbase_v3 ) + cbase_v3 = INVALID_PADDR; else - csize = SZ_8K; + csize_v3 = SZ_8K; - if ( !vbase ) - vbase = INVALID_PADDR; + if ( !vbase_v3 ) + vbase_v3 = INVALID_PADDR; else - vsize = GUEST_GICC_SIZE; + vsize_v3 = GUEST_GICC_SIZE; } #else @@ -1789,7 +1789,7 @@ static int __init gicv3_init(void) " gic_maintenance_irq=%u\n" " gic_rdist_stride=%#x\n" " gic_rdist_regions=%d\n", - dbase, gicv3_info.maintenance_irq, + dbase_v3, gicv3_info.maintenance_irq, gicv3.rdist_stride, gicv3.rdist_count); printk(" redistributor regions:\n"); for ( i = 0; i < gicv3.rdist_count; i++ ) @@ -1803,7 +1803,7 @@ static int __init gicv3_init(void) reg = readl_relaxed(GICD + GICD_TYPER); intid_bits = GICD_TYPE_ID_BITS(reg); - vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions, intid_bits); + vgic_v3_setup_hw(dbase_v3, gicv3.rdist_count, gicv3.rdist_regions, intid_bits); gicv3_init_v2(); spin_lock_init(&gicv3.lock);