lkml.org 
[lkml]   [2024]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs
    On Fri, 26 Apr 2024 14:51:21 +0100,
    Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
    >
    > From: James Morse <james.morse@arm.com>
    >
    > To support virtual CPU hotplug, ACPI has added an 'online capable' bit
    > to the MADT GICC entries. This indicates a disabled CPU entry may not
    > be possible to online via PSCI until firmware has set enabled bit in
    > _STA.
    >
    > This means that a "usable" GIC is one that is marked as either enabled,

    nit: "GIC" usually designs the whole HW infrastructure (distributor,
    redistributors, and ITSs). My understanding is that you are only
    referring to the redistributors.

    > or online capable. Therefore, change acpi_gicc_is_usable() to check both
    > bits. However, we need to change the test in gic_acpi_match_gicc() back
    > to testing just the enabled bit so the count of enabled distributors is
    > correct.
    >
    > What about the redistributor in the GICC entry? ACPI doesn't want to say.
    > Assume the worst: When a redistributor is described in the GICC entry,
    > but the entry is marked as disabled at boot, assume the redistributor
    > is inaccessible.
    >
    > The GICv3 driver doesn't support late online of redistributors, so this
    > means the corresponding CPU can't be brought online either.
    > Rather than modifying cpu masks that may already have been used,
    > register a new cpuhp callback to fail this case. This must run earlier
    > than the main gic_starting_cpu() so that this case can be rejected
    > before the section of cpuhp that runs on the CPU that is coming up as
    > that is not allowed to fail. This solution keeps the handling of this
    > broken firmware corner case local to the GIC driver. As precise ordering
    > of this callback doesn't need to be controlled as long as it is
    > in that initial prepare phase, use CPUHP_BP_PREPARE_DYN.
    >
    > Systems that want CPU hotplug in a VM can ensure their redistributors
    > are always-on, and describe them that way with a GICR entry in the MADT.
    >
    > Suggested-by: Marc Zyngier <maz@kernel.org>
    > Signed-off-by: James Morse <james.morse@arm.com>
    > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
    > Tested-by: Miguel Luis <miguel.luis@oracle.com>
    > Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
    > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
    >
    > ---
    > Thanks to Marc for review and suggestions!
    > v8: Change the handling of broken rdists to fail cpuhp rather than
    > modifying the cpu_present and cpu_possible masks.
    > Updated commit text to reflect that.
    > Added a sb tag for Marc given this is more or less what he put
    > in his review comment.
    > ---
    > drivers/irqchip/irq-gic-v3.c | 38 ++++++++++++++++++++++++++++++++++--
    > include/linux/acpi.h | 3 ++-
    > 2 files changed, 38 insertions(+), 3 deletions(-)
    >
    > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
    > index 10af15f93d4d..b4685991953e 100644
    > --- a/drivers/irqchip/irq-gic-v3.c
    > +++ b/drivers/irqchip/irq-gic-v3.c
    > @@ -44,6 +44,8 @@
    >
    > #define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
    >
    > +static struct cpumask broken_rdists __read_mostly;
    > +
    > struct redist_region {
    > void __iomem *redist_base;
    > phys_addr_t phys_base;
    > @@ -1293,6 +1295,18 @@ static void gic_cpu_init(void)
    > #define MPIDR_TO_SGI_RS(mpidr) (MPIDR_RS(mpidr) << ICC_SGI1R_RS_SHIFT)
    > #define MPIDR_TO_SGI_CLUSTER_ID(mpidr) ((mpidr) & ~0xFUL)
    >
    > +/*
    > + * gic_starting_cpu() is called after the last point where cpuhp is allowed
    > + * to fail. So pre check for problems earlier.
    > + */
    > +static int gic_check_rdist(unsigned int cpu)
    > +{
    > + if (cpumask_test_cpu(cpu, &broken_rdists))
    > + return -EINVAL;
    > +
    > + return 0;
    > +}
    > +
    > static int gic_starting_cpu(unsigned int cpu)
    > {
    > gic_cpu_init();
    > @@ -1384,6 +1398,10 @@ static void __init gic_smp_init(void)
    > };
    > int base_sgi;
    >
    > + cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
    > + "irqchip/arm/gicv3:checkrdist",
    > + gic_check_rdist, NULL);
    > +
    > cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
    > "irqchip/arm/gicv3:starting",
    > gic_starting_cpu, NULL);
    > @@ -2363,11 +2381,24 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
    > (struct acpi_madt_generic_interrupt *)header;
    > u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
    > u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
    > + int cpu = get_cpu_for_acpi_id(gicc->uid);
    > void __iomem *redist_base;
    >
    > if (!acpi_gicc_is_usable(gicc))
    > return 0;
    >
    > + /*
    > + * Capable but disabled CPUs can be brought online later. What about
    > + * the redistributor? ACPI doesn't want to say!
    > + * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
    > + * Otherwise, prevent such CPUs from being brought online.
    > + */
    > + if (!(gicc->flags & ACPI_MADT_ENABLED)) {

    Now this makes the above acpi_gicc_is_usable() very odd. It checks for
    MADT_ENABLED *or* GICC_ONLINE_CAPABLE. But we definitely don't want to
    deal with the lack of MADT_ENABLED.

    So why don't we explicitly check for individual flags and get rid of
    acpi_gicc_is_usable(), as its new definition doesn't tell you anything
    useful?

    > + pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
    > + cpumask_set_cpu(cpu, &broken_rdists);

    Given that get_cpu_for_acpi_id() can return -EINVAL, you'd want to
    check that. Also, I'd like to drop the _once on the warning.
    Indicating all the broken CPUs is useful information, and only happens
    once per boot.

    > + return 0;
    > + }
    > +
    > redist_base = ioremap(gicc->gicr_base_address, size);
    > if (!redist_base)
    > return -ENOMEM;
    > @@ -2413,9 +2444,12 @@ static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
    >
    > /*
    > * If GICC is enabled and has valid gicr base address, then it means
    > - * GICR base is presented via GICC
    > + * GICR base is presented via GICC. The redistributor is only known to
    > + * be accessible if the GICC is marked as enabled. If this bit is not
    > + * set, we'd need to add the redistributor at runtime, which isn't
    > + * supported.
    > */
    > - if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address)
    > + if (gicc->flags & ACPI_MADT_ENABLED && gicc->gicr_base_address)
    > acpi_data.enabled_rdists++;
    >
    > return 0;
    > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
    > index 9844a3f9c4e5..fcfb7bb6789e 100644
    > --- a/include/linux/acpi.h
    > +++ b/include/linux/acpi.h
    > @@ -239,7 +239,8 @@ void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
    >
    > static inline bool acpi_gicc_is_usable(struct acpi_madt_generic_interrupt *gicc)
    > {
    > - return gicc->flags & ACPI_MADT_ENABLED;
    > + return gicc->flags & (ACPI_MADT_ENABLED |
    > + ACPI_MADT_GICC_ONLINE_CAPABLE);
    > }
    >
    > /* the following numa functions are architecture-dependent */

    Thanks,

    M.

    --
    Without deviation from the norm, progress is not possible.

    \
     
     \ /
      Last update: 2024-05-27 18:05    [W:4.454 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site