[PATCH 8/9] x86/apic: match destination id with destination mode

[PATCH 8/9] x86/apic: match destination id with destination mode

[Pan, Jacob jun]

>From 791a44040dde6670f90a729cf91ed302d84b875a Mon Sep 17 00:00:00 2001
From: Jacob Pan <jacob.jun.pan@intel.com>
Date: Tue, 12 May 2009 10:51:08 -0700
Subject: [PATCH] x86/apic: match destination id with destination mode

current code assigns logical destination IDs regardless of destination modes, this is not a problem since we only use logical delivery so far. but it could be a problem with platforms only supports physical mode

Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
---
 arch/x86/kernel/apic/io_apic.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 4d0216f..c84dc3d 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1431,7 +1431,10 @@ int setup_ioapic_entry(int apic_id, int irq,
 	} else {
 		entry->delivery_mode = apic->irq_delivery_mode;
 		entry->dest_mode = apic->irq_dest_mode;
-		entry->dest = destination;
+		if (apic->irq_dest_mode)
+			entry->dest = destination;
+		else	/* physical ID of BSP */
+			entry->dest = boot_cpu_id;
 		entry->vector = vector;
 	}
 
@@ -1576,7 +1579,10 @@ static void __init setup_timer_IRQ0_pin(unsigned int apic_id, unsigned int pin,
 	 */
 	entry.dest_mode = apic->irq_dest_mode;
 	entry.mask = 0;			/* don't mask IRQ for edge */
-	entry.dest = apic->cpu_mask_to_apicid(apic->target_cpus());
+	if (!apic->irq_dest_mode)
+		entry.dest = boot_cpu_id;	/* physical apic id */
+	else
+		entry.dest = apic->cpu_mask_to_apicid(apic->target_cpus());
 	entry.delivery_mode = apic->irq_delivery_mode;
 	entry.polarity = 0;
 	entry.trigger = 0;
@@ -2343,7 +2349,8 @@ set_ioapic_affinity_irq_desc(struct irq_desc *desc, const struct cpumask *mask)
 	dest = set_desc_affinity(desc, mask);
 	if (dest != BAD_APICID) {
 		/* Only the high 8 bits are valid. */
-		dest = SET_APIC_LOGICAL_ID(dest);
+		if (apic->irq_dest_mode)
+			dest = SET_APIC_LOGICAL_ID(dest);
 		__target_IO_APIC_irq(irq, dest, cfg);
 		ret = 0;
 	}
@@ -3273,9 +3280,11 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, struct msi_ms
 	err = assign_irq_vector(irq, cfg, apic->target_cpus());
 	if (err)
 		return err;
-
-	dest = apic->cpu_mask_to_apicid_and(cfg->domain, apic->target_cpus());
-
+	if (apic->irq_dest_mode)
+		dest = apic->cpu_mask_to_apicid_and(cfg->domain,
+						apic->target_cpus());
+	else
+		dest = boot_cpu_id;
 	if (irq_remapped(irq)) {
 		struct irte irte;
 		int ir_index;
@@ -3339,8 +3348,10 @@ static int set_msi_irq_affinity(unsigned int irq, const struct cpumask *mask)
 	struct irq_cfg *cfg;
 	struct msi_msg msg;
 	unsigned int dest;
-
-	dest = set_desc_affinity(desc, mask);
+	if (apic->irq_dest_mode)
+		dest = set_desc_affinity(desc, mask);
+	else
+		dest = boot_cpu_id;
 	if (dest == BAD_APICID)
 		return -1;
 
-- 
1.5.6.5

Re: [PATCH 8/9] x86/apic: match destination id with destination mode

[Ingo Molnar]

* Pan, Jacob jun <jacob.jun.pan@intel.com> wrote:

> >From 791a44040dde6670f90a729cf91ed302d84b875a Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@intel.com>
> Date: Tue, 12 May 2009 10:51:08 -0700
> Subject: [PATCH] x86/apic: match destination id with destination mode
> 
> current code assigns logical destination IDs regardless of destination modes, this is not a problem since we only use logical delivery so far. but it could be a problem with platforms only supports physical mode

Please fix your commit msg line-warps to be at 70 cols or so.

> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
> ---
>  arch/x86/kernel/apic/io_apic.c |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 4d0216f..c84dc3d 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1431,7 +1431,10 @@ int setup_ioapic_entry(int apic_id, int irq,
>  	} else {
>  		entry->delivery_mode = apic->irq_delivery_mode;
>  		entry->dest_mode = apic->irq_dest_mode;
> -		entry->dest = destination;
> +		if (apic->irq_dest_mode)
> +			entry->dest = destination;
> +		else	/* physical ID of BSP */
> +			entry->dest = boot_cpu_id;
>  		entry->vector = vector;
>  	}
>  
> @@ -1576,7 +1579,10 @@ static void __init setup_timer_IRQ0_pin(unsigned int apic_id, unsigned int pin,
>  	 */
>  	entry.dest_mode = apic->irq_dest_mode;
>  	entry.mask = 0;			/* don't mask IRQ for edge */
> -	entry.dest = apic->cpu_mask_to_apicid(apic->target_cpus());
> +	if (!apic->irq_dest_mode)
> +		entry.dest = boot_cpu_id;	/* physical apic id */
> +	else
> +		entry.dest = apic->cpu_mask_to_apicid(apic->target_cpus());
>  	entry.delivery_mode = apic->irq_delivery_mode;
>  	entry.polarity = 0;
>  	entry.trigger = 0;
> @@ -2343,7 +2349,8 @@ set_ioapic_affinity_irq_desc(struct irq_desc *desc, const struct cpumask *mask)
>  	dest = set_desc_affinity(desc, mask);
>  	if (dest != BAD_APICID) {
>  		/* Only the high 8 bits are valid. */
> -		dest = SET_APIC_LOGICAL_ID(dest);
> +		if (apic->irq_dest_mode)
> +			dest = SET_APIC_LOGICAL_ID(dest);
>  		__target_IO_APIC_irq(irq, dest, cfg);
>  		ret = 0;
>  	}
> @@ -3273,9 +3280,11 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, struct msi_ms
>  	err = assign_irq_vector(irq, cfg, apic->target_cpus());
>  	if (err)
>  		return err;
> -
> -	dest = apic->cpu_mask_to_apicid_and(cfg->domain, apic->target_cpus());
> -
> +	if (apic->irq_dest_mode)
> +		dest = apic->cpu_mask_to_apicid_and(cfg->domain,
> +						apic->target_cpus());
> +	else
> +		dest = boot_cpu_id;
>  	if (irq_remapped(irq)) {
>  		struct irte irte;
>  		int ir_index;
> @@ -3339,8 +3348,10 @@ static int set_msi_irq_affinity(unsigned int irq, const struct cpumask *mask)
>  	struct irq_cfg *cfg;
>  	struct msi_msg msg;
>  	unsigned int dest;
> -
> -	dest = set_desc_affinity(desc, mask);
> +	if (apic->irq_dest_mode)
> +		dest = set_desc_affinity(desc, mask);
> +	else
> +		dest = boot_cpu_id;
>  	if (dest == BAD_APICID)
>  		return -1;

The question here is whether this should layer on top of Jeremy's 
IO-APIC driverization patches. I think it should.

	Ingo

Re: [PATCH 8/9] x86/apic: match destination id with destination mode

[Eric W. Biederman]

Ingo Molnar <mingo@elte.hu> writes:

> The question here is whether this should layer on top of Jeremy's 
> IO-APIC driverization patches. I think it should.

The patch is a bad hack that is totally misdocumented.  A bit
like the Xen apic changes in that respect.

I haven't seen Jeremy's IO-APIC driverization patches.  

I am stumped why we need any driverization in this area.  x86_64 and has
had for years a mechanism that is perfectly fine for abstracting this.
i386 also has had something similar and last I looked we just about
had that code merged.

Xen doesn't have ioapics so it doesn't need us faking writes to
ioapics.  Xen either needs to parse the ioapic tables itself
or Xen needs a proper interface to be given the table information.

I this patch can be replaced by a 2 line change to the apic mode
logic to force us into physflat mode on moorestown.

Eric

RE: [PATCH 8/9] x86/apic: match destination id with destination mode

[Pan, Jacob jun]

>Ingo Molnar <mingo@elte.hu> writes:
>
>> The question here is whether this should layer on top of Jeremy's
>> IO-APIC driverization patches. I think it should.
>
>The patch is a bad hack that is totally misdocumented.  A bit
>like the Xen apic changes in that respect.
>
>I haven't seen Jeremy's IO-APIC driverization patches.
>
>I am stumped why we need any driverization in this area.  x86_64 and has
>had for years a mechanism that is perfectly fine for abstracting this.
>i386 also has had something similar and last I looked we just about
>had that code merged.
>
>Xen doesn't have ioapics so it doesn't need us faking writes to
>ioapics.  Xen either needs to parse the ioapic tables itself
>or Xen needs a proper interface to be given the table information.
>
>I this patch can be replaced by a 2 line change to the apic mode
>logic to force us into physflat mode on moorestown.
>
>Eric
>

[[JPAN]] For Moorestown production silicon, we will use apic_default which uses
 logical dest mode. This patch is not required.
But, I think it is wrong to assign destination ID without looking at the mode 
bit. If we have a new apic_xxxx with phy dest mode, we would have logical APIC 
ID assigned to physical mode.

Re: [PATCH 8/9] x86/apic: match destination id with destination mode

[Eric W. Biederman]

"Pan, Jacob jun" <jacob.jun.pan@intel.com> writes:

> [[JPAN]] For Moorestown production silicon, we will use apic_default which uses
>  logical dest mode. This patch is not required.
> But, I think it is wrong to assign destination ID without looking at the mode 
> bit. If we have a new apic_xxxx with phy dest mode, we would have logical APIC 
> ID assigned to physical mode.

Both phys dest and logical dest use the same bits in the apic.
The code that assigns the destination knows which mode we are operating in.

Even not supporting logical mode is likely ok.  The key thing is doing the
work in the auto detect logic and not doing something weird in the assignments.

Eric

Re: [PATCH 8/9] x86/apic: match destination id with destination mode

[Ingo Molnar]

* Eric W. Biederman <ebiederm@xmission.com> wrote:

> Ingo Molnar <mingo@elte.hu> writes:
> 
> > The question here is whether this should layer on top of 
> > Jeremy's IO-APIC driverization patches. I think it should.
> 
> The patch is a bad hack that is totally misdocumented.  A bit like 
> the Xen apic changes in that respect.
> 
> I haven't seen Jeremy's IO-APIC driverization patches.
> 
> I am stumped why we need any driverization in this area.  x86_64 
> and has had for years a mechanism that is perfectly fine for 
> abstracting this. i386 also has had something similar and last I 
> looked we just about had that code merged.

We have the local apic abstracted out into struct apic on both 
32-bit and 64-bit, but not the IO-APIC methods.

> Xen doesn't have ioapics so it doesn't need us faking writes to 
> ioapics.  Xen either needs to parse the ioapic tables itself or 
> Xen needs a proper interface to be given the table information.
> 
> I this patch can be replaced by a 2 line change to the apic mode 
> logic to force us into physflat mode on moorestown.

Yes, probably. I havent looked deeply into all the merits yet, there 
were so many other structural problems with these patches.

	Ingo

Re: [PATCH 8/9] x86/apic: match destination id with destination mode

[Eric W. Biederman]

"Pan, Jacob jun" <jacob.jun.pan@intel.com> writes:

>>From 791a44040dde6670f90a729cf91ed302d84b875a Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@intel.com>
> Date: Tue, 12 May 2009 10:51:08 -0700
> Subject: [PATCH] x86/apic: match destination id with destination mode
>
> current code assigns logical destination IDs regardless of
> destination modes, this is not a problem since we only use logical
> delivery so far. but it could be a problem with platforms only
> supports physical mode  

The description of this patch is nonsense.

Linux runs on several systems that only support physical ids.
The have > 8 processors.

Eric

Re: [PATCH 8/9] x86/apic: match destination id with destination mode

[Yinghai Lu]

On Thu, Jun 25, 2009 at 5:15 PM, Pan, Jacob jun<jacob.jun.pan@intel.com> wrote:
> From 791a44040dde6670f90a729cf91ed302d84b875a Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@intel.com>
> Date: Tue, 12 May 2009 10:51:08 -0700
> Subject: [PATCH] x86/apic: match destination id with destination mode
>
> current code assigns logical destination IDs regardless of destination modes, this is not a problem since we only use logical delivery so far. but it could be a problem with platforms only supports physical mode
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@intel.com>
> ---
>  arch/x86/kernel/apic/io_apic.c |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 4d0216f..c84dc3d 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1431,7 +1431,10 @@ int setup_ioapic_entry(int apic_id, int irq,
>        } else {
>                entry->delivery_mode = apic->irq_delivery_mode;
>                entry->dest_mode = apic->irq_dest_mode;
> -               entry->dest = destination;
> +               if (apic->irq_dest_mode)
> +                       entry->dest = destination;
> +               else    /* physical ID of BSP */
> +                       entry->dest = boot_cpu_id;
>                entry->vector = vector;
>        }
>
> @@ -1576,7 +1579,10 @@ static void __init setup_timer_IRQ0_pin(unsigned int apic_id, unsigned int pin,
>         */
>        entry.dest_mode = apic->irq_dest_mode;
>        entry.mask = 0;                 /* don't mask IRQ for edge */
> -       entry.dest = apic->cpu_mask_to_apicid(apic->target_cpus());
> +       if (!apic->irq_dest_mode)
> +               entry.dest = boot_cpu_id;       /* physical apic id */
> +       else
> +               entry.dest = apic->cpu_mask_to_apicid(apic->target_cpus());
>        entry.delivery_mode = apic->irq_delivery_mode;
>        entry.polarity = 0;
>        entry.trigger = 0;
> @@ -2343,7 +2349,8 @@ set_ioapic_affinity_irq_desc(struct irq_desc *desc, const struct cpumask *mask)
>        dest = set_desc_affinity(desc, mask);
>        if (dest != BAD_APICID) {
>                /* Only the high 8 bits are valid. */
> -               dest = SET_APIC_LOGICAL_ID(dest);
> +               if (apic->irq_dest_mode)
> +                       dest = SET_APIC_LOGICAL_ID(dest);
>                __target_IO_APIC_irq(irq, dest, cfg);
>                ret = 0;
>        }
> @@ -3273,9 +3280,11 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, struct msi_ms
>        err = assign_irq_vector(irq, cfg, apic->target_cpus());
>        if (err)
>                return err;
> -
> -       dest = apic->cpu_mask_to_apicid_and(cfg->domain, apic->target_cpus());
> -
> +       if (apic->irq_dest_mode)
> +               dest = apic->cpu_mask_to_apicid_and(cfg->domain,
> +                                               apic->target_cpus());
> +       else
> +               dest = boot_cpu_id;
>        if (irq_remapped(irq)) {
>                struct irte irte;
>                int ir_index;
> @@ -3339,8 +3348,10 @@ static int set_msi_irq_affinity(unsigned int irq, const struct cpumask *mask)
>        struct irq_cfg *cfg;
>        struct msi_msg msg;
>        unsigned int dest;
> -
> -       dest = set_desc_affinity(desc, mask);
> +       if (apic->irq_dest_mode)
> +               dest = set_desc_affinity(desc, mask);
> +       else
> +               dest = boot_cpu_id;
>        if (dest == BAD_APICID)
>                return -1;
>

for physical flat mode

apic->cpu_mask_to_apicid() is

static unsigned int physflat_cpu_mask_to_apicid(const struct cpumask *cpumask)
{
        int cpu;

        /*
         * We're using fixed IRQ delivery, can only return one phys APIC ID.
         * May as well be the first.
         */
        cpu = cpumask_first(cpumask);
        if ((unsigned)cpu < nr_cpu_ids)
                return per_cpu(x86_cpu_to_apicid, cpu);
        else
                return BAD_APICID;
}


so this patch will put all ioapic routing to boot_cpu_id what is that?
how can you assume that is is apic id.?
that is NOT apic id.

the patch is totally wrong.

Nacked-by: Yinghai Lu <yinghai@kernel.org>


YH

RE: [PATCH 8/9] x86/apic: match destination id with destination mode

[Pan, Jacob jun]

>so this patch will put all ioapic routing to boot_cpu_id what is that?
>how can you assume that is is apic id.?
>that is NOT apic id.
[[JPAN]] I don't know a better way to assign physical APIC IDs. Any suggestions?
At least giving boot_cpu_id would prevent system to crash( when you have logical 
ID assigned as physical).