2023-11-03 20:16:39

by Steve Wahl

[permalink] [raw]
Subject: Re: [PATCH 1/3] x86/apic: Drop apic::delivery_mode

On Thu, Nov 02, 2023 at 12:26:19PM +0000, Andrew Cooper wrote:
> This field is set to APIC_DELIVERY_MODE_FIXED in all cases, and is read
> exactly once. Fold the constant in uv_program_mmr() and drop the field.
>
> Searching for the origin of the stale HyperV comment reveals commit
> a31e58e129f7 ("x86/apic: Switch all APICs to Fixed delivery mode") which
> notes:
>
> As a consequence of this change, the apic::irq_delivery_mode field is
> now pointless, but this needs to be cleaned up in a separate patch.
>
> 6 years is long enough for this technical debt to have survived.
>
> Signed-off-by: Andrew Cooper <[email protected]>

Reveiewed-by: Steve Wahl <[email protected]>

> ---
> arch/x86/include/asm/apic.h | 2 --
> arch/x86/kernel/apic/apic_flat_64.c | 2 --
> arch/x86/kernel/apic/apic_noop.c | 1 -
> arch/x86/kernel/apic/apic_numachip.c | 2 --
> arch/x86/kernel/apic/bigsmp_32.c | 1 -
> arch/x86/kernel/apic/probe_32.c | 1 -
> arch/x86/kernel/apic/x2apic_cluster.c | 1 -
> arch/x86/kernel/apic/x2apic_phys.c | 1 -
> arch/x86/kernel/apic/x2apic_uv_x.c | 1 -
> arch/x86/platform/uv/uv_irq.c | 2 +-
> drivers/pci/controller/pci-hyperv.c | 7 -------
> 11 files changed, 1 insertion(+), 20 deletions(-)
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 5af4ec1a0f71..841afbd7bfe7 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -272,8 +272,6 @@ struct apic {
> void (*send_IPI_all)(int vector);
> void (*send_IPI_self)(int vector);
>
> - enum apic_delivery_modes delivery_mode;
> -
> u32 disable_esr : 1,
> dest_mode_logical : 1,
> x2apic_set_max_apicid : 1;
> diff --git a/arch/x86/kernel/apic/apic_flat_64.c b/arch/x86/kernel/apic/apic_flat_64.c
> index 032a84e2c3cc..e526b226910b 100644
> --- a/arch/x86/kernel/apic/apic_flat_64.c
> +++ b/arch/x86/kernel/apic/apic_flat_64.c
> @@ -82,7 +82,6 @@ static struct apic apic_flat __ro_after_init = {
> .acpi_madt_oem_check = flat_acpi_madt_oem_check,
> .apic_id_registered = default_apic_id_registered,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = true,
>
> .disable_esr = 0,
> @@ -153,7 +152,6 @@ static struct apic apic_physflat __ro_after_init = {
> .acpi_madt_oem_check = physflat_acpi_madt_oem_check,
> .apic_id_registered = default_apic_id_registered,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = false,
>
> .disable_esr = 0,
> diff --git a/arch/x86/kernel/apic/apic_noop.c b/arch/x86/kernel/apic/apic_noop.c
> index 966d7cf10b95..70e7dfc3cc84 100644
> --- a/arch/x86/kernel/apic/apic_noop.c
> +++ b/arch/x86/kernel/apic/apic_noop.c
> @@ -45,7 +45,6 @@ static void noop_apic_write(u32 reg, u32 val)
> struct apic apic_noop __ro_after_init = {
> .name = "noop",
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = true,
>
> .disable_esr = 0,
> diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
> index 63f3d7be9dc7..8f5a42ad1f9f 100644
> --- a/arch/x86/kernel/apic/apic_numachip.c
> +++ b/arch/x86/kernel/apic/apic_numachip.c
> @@ -222,7 +222,6 @@ static const struct apic apic_numachip1 __refconst = {
> .probe = numachip1_probe,
> .acpi_madt_oem_check = numachip1_acpi_madt_oem_check,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = false,
>
> .disable_esr = 0,
> @@ -259,7 +258,6 @@ static const struct apic apic_numachip2 __refconst = {
> .probe = numachip2_probe,
> .acpi_madt_oem_check = numachip2_acpi_madt_oem_check,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = false,
>
> .disable_esr = 0,
> diff --git a/arch/x86/kernel/apic/bigsmp_32.c b/arch/x86/kernel/apic/bigsmp_32.c
> index 0e5535add4b5..863c3002a574 100644
> --- a/arch/x86/kernel/apic/bigsmp_32.c
> +++ b/arch/x86/kernel/apic/bigsmp_32.c
> @@ -80,7 +80,6 @@ static struct apic apic_bigsmp __ro_after_init = {
> .name = "bigsmp",
> .probe = probe_bigsmp,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = false,
>
> .disable_esr = 1,
> diff --git a/arch/x86/kernel/apic/probe_32.c b/arch/x86/kernel/apic/probe_32.c
> index 9a06df6cdd68..f851ccf1e14f 100644
> --- a/arch/x86/kernel/apic/probe_32.c
> +++ b/arch/x86/kernel/apic/probe_32.c
> @@ -35,7 +35,6 @@ static struct apic apic_default __ro_after_init = {
> .probe = probe_default,
> .apic_id_registered = default_apic_id_registered,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = true,
>
> .disable_esr = 0,
> diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
> index affbff65e497..7d15f6c3b718 100644
> --- a/arch/x86/kernel/apic/x2apic_cluster.c
> +++ b/arch/x86/kernel/apic/x2apic_cluster.c
> @@ -227,7 +227,6 @@ static struct apic apic_x2apic_cluster __ro_after_init = {
> .probe = x2apic_cluster_probe,
> .acpi_madt_oem_check = x2apic_acpi_madt_oem_check,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = true,
>
> .disable_esr = 0,
> diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
> index 788cdb4ee394..8bb740e22b7d 100644
> --- a/arch/x86/kernel/apic/x2apic_phys.c
> +++ b/arch/x86/kernel/apic/x2apic_phys.c
> @@ -145,7 +145,6 @@ static struct apic apic_x2apic_phys __ro_after_init = {
> .probe = x2apic_phys_probe,
> .acpi_madt_oem_check = x2apic_acpi_madt_oem_check,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = false,
>
> .disable_esr = 0,
> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> index 7d304ef1a7f5..ae4f0c1a7b43 100644
> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -805,7 +805,6 @@ static struct apic apic_x2apic_uv_x __ro_after_init = {
> .probe = uv_probe,
> .acpi_madt_oem_check = uv_acpi_madt_oem_check,
>
> - .delivery_mode = APIC_DELIVERY_MODE_FIXED,
> .dest_mode_logical = false,
>
> .disable_esr = 0,
> diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
> index 4221259a5870..a379501b7a69 100644
> --- a/arch/x86/platform/uv/uv_irq.c
> +++ b/arch/x86/platform/uv/uv_irq.c
> @@ -35,7 +35,7 @@ static void uv_program_mmr(struct irq_cfg *cfg, struct uv_irq_2_mmr_pnode *info)
> mmr_value = 0;
> entry = (struct uv_IO_APIC_route_entry *)&mmr_value;
> entry->vector = cfg->vector;
> - entry->delivery_mode = apic->delivery_mode;
> + entry->delivery_mode = APIC_DELIVERY_MODE_FIXED;
> entry->dest_mode = apic->dest_mode_logical;
> entry->polarity = 0;
> entry->trigger = 0;
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index bed3cefdaf19..f5d2ef8572e7 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -650,13 +650,6 @@ static void hv_arch_irq_unmask(struct irq_data *data)
> PCI_FUNC(pdev->devfn);
> params->int_target.vector = hv_msi_get_int_vector(data);
>
> - /*
> - * Honoring apic->delivery_mode set to APIC_DELIVERY_MODE_FIXED by
> - * setting the HV_DEVICE_INTERRUPT_TARGET_MULTICAST flag results in a
> - * spurious interrupt storm. Not doing so does not seem to have a
> - * negative effect (yet?).
> - */
> -
> if (hbus->protocol_version >= PCI_PROTOCOL_VERSION_1_2) {
> /*
> * PCI_PROTOCOL_VERSION_1_2 supports the VP_SET version of the
>
> --
> 2.30.2
>

--
Steve Wahl, Hewlett Packard Enterprise