linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
@ 2013-07-12  1:22 Youquan Song
  2013-07-23  9:17 ` Ingo Molnar
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Youquan Song @ 2013-07-12  1:22 UTC (permalink / raw)
  To: linux-kernel, hpa, yinghai, tglx, mingo; +Cc: Youquan Song, Youquan Song

x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt routed
from IOAPIC or delivered in MSI mode will keep 8 bits destination APICID. 
In order to support x2APIC, the VT-d interrupt remapping is introduced to
translate the destination APICID to 32 bits in x2APIC mode and keep the device
compatible in this way.

x2APIC support both logical and physical mode in destination mode.  
In logical destination mode, the 32 bits Logical APICID has 2 sub-fields: 
 16 bits cluster ID and 16 bits logical ID within the cluster and it is 
required VT-d interrupt remapping in x2APIC cluster mode.
In physical destination mode, the 8 bits physical id is compatible with 32 
bits physical id when CPU number < 256. 
When interrupt remapping initialization fail on platform with CPU number < 256, 
current kernel only enables x2APIC physical mode in virutalization environment,
while we also can enable x2APIC physcial mode in native kernel this situation,
and the device interrupt will use 8 bits destination APICID in physical mode
and be compatible with x2APIC physical when < 256 CPUs.
 
So we can benefit from x2APIC vs xAPIC MMIO:
 - x2APIC MSR read/write is faster than xAPIC mmio
 - x2APIC only ICR write to deliver interrupt without polling ICR deliver 
   status bit and xAPIC need poll to read ICR deliver status bit.
 - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.
  
Signed-off-by: Youquan Song <youquan.song@intel.com>
---
 arch/x86/kernel/apic/apic.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 904611b..51a065a 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1603,11 +1603,8 @@ void __init enable_IR_x2apic(void)
 		goto skip_x2apic;
 
 	if (ret < 0) {
-		/* IR is required if there is APIC ID > 255 even when running
-		 * under KVM
-		 */
-		if (max_physical_apicid > 255 ||
-		    !hypervisor_x2apic_available()) {
+		/* IR is required if there is APIC ID > 255 */
+		if (max_physical_apicid > 255) {
 			if (x2apic_preenabled)
 				disable_x2apic();
 			goto skip_x2apic;
-- 
1.7.7.4


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-12  1:22 [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Youquan Song
@ 2013-07-23  9:17 ` Ingo Molnar
  2013-07-24 14:04   ` Youquan Song
  2013-07-24  3:55 ` [tip:x86/apic] x86/apic: Enable x2APIC physical mode on native hardware too, when there are fewer than 256 CPUs tip-bot for Youquan Song
  2013-07-24  4:24 ` [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Yinghai Lu
  2 siblings, 1 reply; 23+ messages in thread
From: Ingo Molnar @ 2013-07-23  9:17 UTC (permalink / raw)
  To: Youquan Song; +Cc: linux-kernel, hpa, yinghai, tglx, Youquan Song


* Youquan Song <youquan.song@intel.com> wrote:

> x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt 
> routed from IOAPIC or delivered in MSI mode will keep 8 bits destination 
> APICID. In order to support x2APIC, the VT-d interrupt remapping is 
> introduced to translate the destination APICID to 32 bits in x2APIC mode 
> and keep the device compatible in this way.
> 
> x2APIC support both logical and physical mode in destination mode.  In 
> logical destination mode, the 32 bits Logical APICID has 2 sub-fields:
>  16 bits cluster ID and 16 bits logical ID within the cluster and it is 
> required VT-d interrupt remapping in x2APIC cluster mode. In physical 
> destination mode, the 8 bits physical id is compatible with 32 bits 
> physical id when CPU number < 256. When interrupt remapping 
> initialization fail on platform with CPU number < 256, current kernel 
> only enables x2APIC physical mode in virutalization environment, while 
> we also can enable x2APIC physcial mode in native kernel this situation, 
> and the device interrupt will use 8 bits destination APICID in physical 
> mode and be compatible with x2APIC physical when < 256 CPUs.
>  
> So we can benefit from x2APIC vs xAPIC MMIO:
>  - x2APIC MSR read/write is faster than xAPIC mmio
>  - x2APIC only ICR write to deliver interrupt without polling ICR deliver 
>    status bit and xAPIC need poll to read ICR deliver status bit.
>  - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.

That looks interesting. How many systems are affected by this change in 
practice? Have you tested it on affected hardware?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [tip:x86/apic] x86/apic: Enable x2APIC physical mode on native hardware too, when there are fewer than 256 CPUs
  2013-07-12  1:22 [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Youquan Song
  2013-07-23  9:17 ` Ingo Molnar
@ 2013-07-24  3:55 ` tip-bot for Youquan Song
  2013-07-24  4:24 ` [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Yinghai Lu
  2 siblings, 0 replies; 23+ messages in thread
From: tip-bot for Youquan Song @ 2013-07-24  3:55 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, tglx, youquan.song, youquan.song

Commit-ID:  3d1acb49d22fbbae96524040e9e2d4cbbb3adbef
Gitweb:     http://git.kernel.org/tip/3d1acb49d22fbbae96524040e9e2d4cbbb3adbef
Author:     Youquan Song <youquan.song@intel.com>
AuthorDate: Thu, 11 Jul 2013 21:22:39 -0400
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 23 Jul 2013 11:15:42 +0200

x86/apic: Enable x2APIC physical mode on native hardware too, when there are fewer than 256 CPUs

x2APIC extends APICID from 8 bits to 32 bits, but the device
interrupt routed from IOAPIC or delivered in MSI mode will keep
8 bits destination APICID.  In order to support x2APIC, the VT-d
interrupt remapping is introduced to translate the destination
APICID to 32 bits in x2APIC mode and keep the device compatible
in this way.

x2APIC support both logical and physical mode in destination
mode.

In logical destination mode, the 32 bits Logical APICID
has 2 sub-fields: 16 bits cluster ID and 16 bits logical ID within
the cluster and it is required VT-d interrupt remapping in x2APIC
cluster mode.

In physical destination mode, the 8 bits physical id is
compatible with 32  bits physical id when CPU number < 256.

When interrupt remapping initialization fails on platforms with
CPU number < 256, the current kernel only enables x2APIC physical
mode in virtualization environment, while we could also can enable
x2APIC physcial mode in native kernel this situation.

In this case the device interrupt will use 8 bits destination
APICID in physical mode and be compatible with x2APIC physical
when < 256 CPUs.

So we can benefit from x2APIC vs xAPIC MMIO:

 - x2APIC MSR read/write is faster than xAPIC mmio

 - x2APIC only ICR write to deliver interrupt without polling ICR deliver
   status bit and xAPIC need poll to read ICR deliver status bit.

 - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.

Signed-off-by: Youquan Song <youquan.song@intel.com>
Cc: Youquan Song <youquan.song@linux.intel.com>
Cc: hpa@linux.intel.com
Cc: yinghai@kernel.org
Link: http://lkml.kernel.org/r/1373592159-459-1-git-send-email-youquan.song@intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/apic/apic.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index eca89c5..d9dd5a6 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1622,11 +1622,8 @@ void __init enable_IR_x2apic(void)
 		goto skip_x2apic;
 
 	if (ret < 0) {
-		/* IR is required if there is APIC ID > 255 even when running
-		 * under KVM
-		 */
-		if (max_physical_apicid > 255 ||
-		    !hypervisor_x2apic_available()) {
+		/* IR is required if there is APIC ID > 255 */
+		if (max_physical_apicid > 255) {
 			if (x2apic_preenabled)
 				disable_x2apic();
 			goto skip_x2apic;

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-12  1:22 [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Youquan Song
  2013-07-23  9:17 ` Ingo Molnar
  2013-07-24  3:55 ` [tip:x86/apic] x86/apic: Enable x2APIC physical mode on native hardware too, when there are fewer than 256 CPUs tip-bot for Youquan Song
@ 2013-07-24  4:24 ` Yinghai Lu
  2013-07-24  6:22   ` Gleb Natapov
  2013-07-24 14:45   ` Konrad Rzeszutek Wilk
  2 siblings, 2 replies; 23+ messages in thread
From: Yinghai Lu @ 2013-07-24  4:24 UTC (permalink / raw)
  To: Youquan Song, Gleb Natapov, Sheng Yang, Konrad Rzeszutek Wilk
  Cc: Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner,
	Ingo Molnar, Youquan Song

On Thu, Jul 11, 2013 at 6:22 PM, Youquan Song <youquan.song@intel.com> wrote:
> x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt routed
> from IOAPIC or delivered in MSI mode will keep 8 bits destination APICID.
> In order to support x2APIC, the VT-d interrupt remapping is introduced to
> translate the destination APICID to 32 bits in x2APIC mode and keep the device
> compatible in this way.
>
> x2APIC support both logical and physical mode in destination mode.
> In logical destination mode, the 32 bits Logical APICID has 2 sub-fields:
>  16 bits cluster ID and 16 bits logical ID within the cluster and it is
> required VT-d interrupt remapping in x2APIC cluster mode.
> In physical destination mode, the 8 bits physical id is compatible with 32
> bits physical id when CPU number < 256.
> When interrupt remapping initialization fail on platform with CPU number < 256,
> current kernel only enables x2APIC physical mode in virutalization environment,
> while we also can enable x2APIC physcial mode in native kernel this situation,
> and the device interrupt will use 8 bits destination APICID in physical mode
> and be compatible with x2APIC physical when < 256 CPUs.
>
> So we can benefit from x2APIC vs xAPIC MMIO:
>  - x2APIC MSR read/write is faster than xAPIC mmio
>  - x2APIC only ICR write to deliver interrupt without polling ICR deliver
>    status bit and xAPIC need poll to read ICR deliver status bit.
>  - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.
>
> Signed-off-by: Youquan Song <youquan.song@intel.com>
> ---
>  arch/x86/kernel/apic/apic.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 904611b..51a065a 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1603,11 +1603,8 @@ void __init enable_IR_x2apic(void)
>                 goto skip_x2apic;
>
>         if (ret < 0) {
> -               /* IR is required if there is APIC ID > 255 even when running
> -                * under KVM
> -                */
> -               if (max_physical_apicid > 255 ||
> -                   !hypervisor_x2apic_available()) {
> +               /* IR is required if there is APIC ID > 255 */
> +               if (max_physical_apicid > 255) {
>                         if (x2apic_preenabled)
>                                 disable_x2apic();
>                         goto skip_x2apic;

Those are kvm and xen related.

Add more Cc.

Yinghai

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-24  4:24 ` [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Yinghai Lu
@ 2013-07-24  6:22   ` Gleb Natapov
  2013-07-25 14:05     ` Yinghai Lu
  2013-07-24 14:45   ` Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 23+ messages in thread
From: Gleb Natapov @ 2013-07-24  6:22 UTC (permalink / raw)
  To: Yinghai Lu
  Cc: Youquan Song, Sheng Yang, Konrad Rzeszutek Wilk,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner,
	Ingo Molnar, Youquan Song

On Tue, Jul 23, 2013 at 09:24:45PM -0700, Yinghai Lu wrote:
> On Thu, Jul 11, 2013 at 6:22 PM, Youquan Song <youquan.song@intel.com> wrote:
> > x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt routed
> > from IOAPIC or delivered in MSI mode will keep 8 bits destination APICID.
> > In order to support x2APIC, the VT-d interrupt remapping is introduced to
> > translate the destination APICID to 32 bits in x2APIC mode and keep the device
> > compatible in this way.
> >
> > x2APIC support both logical and physical mode in destination mode.
> > In logical destination mode, the 32 bits Logical APICID has 2 sub-fields:
> >  16 bits cluster ID and 16 bits logical ID within the cluster and it is
> > required VT-d interrupt remapping in x2APIC cluster mode.
> > In physical destination mode, the 8 bits physical id is compatible with 32
> > bits physical id when CPU number < 256.
> > When interrupt remapping initialization fail on platform with CPU number < 256,
> > current kernel only enables x2APIC physical mode in virutalization environment,
> > while we also can enable x2APIC physcial mode in native kernel this situation,
> > and the device interrupt will use 8 bits destination APICID in physical mode
> > and be compatible with x2APIC physical when < 256 CPUs.
> >
> > So we can benefit from x2APIC vs xAPIC MMIO:
> >  - x2APIC MSR read/write is faster than xAPIC mmio
> >  - x2APIC only ICR write to deliver interrupt without polling ICR deliver
> >    status bit and xAPIC need poll to read ICR deliver status bit.
> >  - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.
> >
> > Signed-off-by: Youquan Song <youquan.song@intel.com>
> > ---
> >  arch/x86/kernel/apic/apic.c |    7 ++-----
> >  1 files changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > index 904611b..51a065a 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -1603,11 +1603,8 @@ void __init enable_IR_x2apic(void)
> >                 goto skip_x2apic;
> >
> >         if (ret < 0) {
> > -               /* IR is required if there is APIC ID > 255 even when running
> > -                * under KVM
> > -                */
> > -               if (max_physical_apicid > 255 ||
> > -                   !hypervisor_x2apic_available()) {
> > +               /* IR is required if there is APIC ID > 255 */
> > +               if (max_physical_apicid > 255) {
> >                         if (x2apic_preenabled)
> >                                 disable_x2apic();
> >                         goto skip_x2apic;
> 
> Those are kvm and xen related.
>
This change does not affect kvm or xen since they already enable x2apic
without IR. But back then when I added the apicid > 255 check I had to
add the hypervisor check too because I was told by Intel that x2apic
without IR is not architectural. Seeing that this patch comes from Intel
engineer it is possible that things changed since then.

--
			Gleb.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-23  9:17 ` Ingo Molnar
@ 2013-07-24 14:04   ` Youquan Song
  2013-07-25 22:01     ` Ingo Molnar
  0 siblings, 1 reply; 23+ messages in thread
From: Youquan Song @ 2013-07-24 14:04 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Youquan Song, linux-kernel, hpa, yinghai, tglx, Youquan Song

On Tue, Jul 23, 2013 at 11:17:29AM +0200, Ingo Molnar wrote:
> 
> * Youquan Song <youquan.song@intel.com> wrote:
> 
> > x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt 
> > routed from IOAPIC or delivered in MSI mode will keep 8 bits destination 
> > APICID. In order to support x2APIC, the VT-d interrupt remapping is 
> > introduced to translate the destination APICID to 32 bits in x2APIC mode 
> > and keep the device compatible in this way.
> > 
> > x2APIC support both logical and physical mode in destination mode.  In 
> > logical destination mode, the 32 bits Logical APICID has 2 sub-fields:
> >  16 bits cluster ID and 16 bits logical ID within the cluster and it is 
> > required VT-d interrupt remapping in x2APIC cluster mode. In physical 
> > destination mode, the 8 bits physical id is compatible with 32 bits 
> > physical id when CPU number < 256. When interrupt remapping 
> > initialization fail on platform with CPU number < 256, current kernel 
> > only enables x2APIC physical mode in virutalization environment, while 
> > we also can enable x2APIC physcial mode in native kernel this situation, 
> > and the device interrupt will use 8 bits destination APICID in physical 
> > mode and be compatible with x2APIC physical when < 256 CPUs.
> >  
> > So we can benefit from x2APIC vs xAPIC MMIO:
> >  - x2APIC MSR read/write is faster than xAPIC mmio
> >  - x2APIC only ICR write to deliver interrupt without polling ICR deliver 
> >    status bit and xAPIC need poll to read ICR deliver status bit.
> >  - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.
> 
> That looks interesting. How many systems are affected by this change in 
> practice? Have you tested it on affected hardware?

Thanks Ingo!
The machines will be affected: CPU support x2APIC and CPU number < 256,
chipset does not support VT-d2 or VT-d is disabled in BIOS. 

I have tested on one of affected hardware, it works.

Thanks
-Youquan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-24  4:24 ` [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Yinghai Lu
  2013-07-24  6:22   ` Gleb Natapov
@ 2013-07-24 14:45   ` Konrad Rzeszutek Wilk
  1 sibling, 0 replies; 23+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-07-24 14:45 UTC (permalink / raw)
  To: Yinghai Lu
  Cc: Youquan Song, Gleb Natapov, Sheng Yang,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner,
	Ingo Molnar, Youquan Song

On Tue, Jul 23, 2013 at 09:24:45PM -0700, Yinghai Lu wrote:
> On Thu, Jul 11, 2013 at 6:22 PM, Youquan Song <youquan.song@intel.com> wrote:
> > x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt routed
> > from IOAPIC or delivered in MSI mode will keep 8 bits destination APICID.
> > In order to support x2APIC, the VT-d interrupt remapping is introduced to
> > translate the destination APICID to 32 bits in x2APIC mode and keep the device
> > compatible in this way.
> >
> > x2APIC support both logical and physical mode in destination mode.
> > In logical destination mode, the 32 bits Logical APICID has 2 sub-fields:
> >  16 bits cluster ID and 16 bits logical ID within the cluster and it is
> > required VT-d interrupt remapping in x2APIC cluster mode.
> > In physical destination mode, the 8 bits physical id is compatible with 32
> > bits physical id when CPU number < 256.
> > When interrupt remapping initialization fail on platform with CPU number < 256,
> > current kernel only enables x2APIC physical mode in virutalization environment,
                                                        ^^^^^^^^^^^^^^-virtualization
> > while we also can enable x2APIC physcial mode in native kernel this situation,
                                    ^^^^^^^ physical
> > and the device interrupt will use 8 bits destination APICID in physical mode
> > and be compatible with x2APIC physical when < 256 CPUs.
> >
> > So we can benefit from x2APIC vs xAPIC MMIO:
> >  - x2APIC MSR read/write is faster than xAPIC mmio
> >  - x2APIC only ICR write to deliver interrupt without polling ICR deliver
> >    status bit and xAPIC need poll to read ICR deliver status bit.
> >  - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.
> >
> > Signed-off-by: Youquan Song <youquan.song@intel.com>
> > ---
> >  arch/x86/kernel/apic/apic.c |    7 ++-----
> >  1 files changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > index 904611b..51a065a 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -1603,11 +1603,8 @@ void __init enable_IR_x2apic(void)
> >                 goto skip_x2apic;
> >
> >         if (ret < 0) {
> > -               /* IR is required if there is APIC ID > 255 even when running
> > -                * under KVM
> > -                */
> > -               if (max_physical_apicid > 255 ||
> > -                   !hypervisor_x2apic_available()) {
> > +               /* IR is required if there is APIC ID > 255 */
> > +               if (max_physical_apicid > 255) {
> >                         if (x2apic_preenabled)
> >                                 disable_x2apic();
> >                         goto skip_x2apic;
> 
> Those are kvm and xen related.
> 
> Add more Cc.
> 
> Yinghai

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-24  6:22   ` Gleb Natapov
@ 2013-07-25 14:05     ` Yinghai Lu
  2013-07-29 17:05       ` Youquan Song
  2013-08-02 19:12       ` Konrad Rzeszutek Wilk
  0 siblings, 2 replies; 23+ messages in thread
From: Yinghai Lu @ 2013-07-25 14:05 UTC (permalink / raw)
  To: Gleb Natapov
  Cc: Youquan Song, Sheng Yang, Konrad Rzeszutek Wilk,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner,
	Ingo Molnar, Youquan Song

On Tue, Jul 23, 2013 at 11:22 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Tue, Jul 23, 2013 at 09:24:45PM -0700, Yinghai Lu wrote:
>> On Thu, Jul 11, 2013 at 6:22 PM, Youquan Song <youquan.song@intel.com> wrote:
>> > x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt routed
>> > from IOAPIC or delivered in MSI mode will keep 8 bits destination APICID.
>> > In order to support x2APIC, the VT-d interrupt remapping is introduced to
>> > translate the destination APICID to 32 bits in x2APIC mode and keep the device
>> > compatible in this way.
>> >
>> > x2APIC support both logical and physical mode in destination mode.
>> > In logical destination mode, the 32 bits Logical APICID has 2 sub-fields:
>> >  16 bits cluster ID and 16 bits logical ID within the cluster and it is
>> > required VT-d interrupt remapping in x2APIC cluster mode.
>> > In physical destination mode, the 8 bits physical id is compatible with 32
>> > bits physical id when CPU number < 256.
>> > When interrupt remapping initialization fail on platform with CPU number < 256,
>> > current kernel only enables x2APIC physical mode in virutalization environment,
>> > while we also can enable x2APIC physcial mode in native kernel this situation,
>> > and the device interrupt will use 8 bits destination APICID in physical mode
>> > and be compatible with x2APIC physical when < 256 CPUs.
>> >
>> > So we can benefit from x2APIC vs xAPIC MMIO:
>> >  - x2APIC MSR read/write is faster than xAPIC mmio
>> >  - x2APIC only ICR write to deliver interrupt without polling ICR deliver
>> >    status bit and xAPIC need poll to read ICR deliver status bit.
>> >  - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.
>> >
>> > Signed-off-by: Youquan Song <youquan.song@intel.com>
>> > ---
>> >  arch/x86/kernel/apic/apic.c |    7 ++-----
>> >  1 files changed, 2 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
>> > index 904611b..51a065a 100644
>> > --- a/arch/x86/kernel/apic/apic.c
>> > +++ b/arch/x86/kernel/apic/apic.c
>> > @@ -1603,11 +1603,8 @@ void __init enable_IR_x2apic(void)
>> >                 goto skip_x2apic;
>> >
>> >         if (ret < 0) {
>> > -               /* IR is required if there is APIC ID > 255 even when running
>> > -                * under KVM
>> > -                */
>> > -               if (max_physical_apicid > 255 ||
>> > -                   !hypervisor_x2apic_available()) {
>> > +               /* IR is required if there is APIC ID > 255 */
>> > +               if (max_physical_apicid > 255) {
>> >                         if (x2apic_preenabled)
>> >                                 disable_x2apic();
>> >                         goto skip_x2apic;
>>
>> Those are kvm and xen related.
>>
> This change does not affect kvm or xen since they already enable x2apic
> without IR. But back then when I added the apicid > 255 check I had to
> add the hypervisor check too because I was told by Intel that x2apic
> without IR is not architectural. Seeing that this patch comes from Intel
> engineer it is possible that things changed since then.

Yes. It would be great, if Youquan can point out where is the intel doc
about the change.

Also if the patch can move on,  hypervisor_x2apic_available() related
declaration and define
could be dropped.

Yinghai

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-24 14:04   ` Youquan Song
@ 2013-07-25 22:01     ` Ingo Molnar
  2013-07-29 16:48       ` Youquan Song
  0 siblings, 1 reply; 23+ messages in thread
From: Ingo Molnar @ 2013-07-25 22:01 UTC (permalink / raw)
  To: Youquan Song; +Cc: Youquan Song, linux-kernel, hpa, yinghai, tglx


* Youquan Song <youquan.song@linux.intel.com> wrote:

> On Tue, Jul 23, 2013 at 11:17:29AM +0200, Ingo Molnar wrote:
> > 
> > * Youquan Song <youquan.song@intel.com> wrote:
> > 
> > > x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt 
> > > routed from IOAPIC or delivered in MSI mode will keep 8 bits destination 
> > > APICID. In order to support x2APIC, the VT-d interrupt remapping is 
> > > introduced to translate the destination APICID to 32 bits in x2APIC mode 
> > > and keep the device compatible in this way.
> > > 
> > > x2APIC support both logical and physical mode in destination mode.  In 
> > > logical destination mode, the 32 bits Logical APICID has 2 sub-fields:
> > >  16 bits cluster ID and 16 bits logical ID within the cluster and it is 
> > > required VT-d interrupt remapping in x2APIC cluster mode. In physical 
> > > destination mode, the 8 bits physical id is compatible with 32 bits 
> > > physical id when CPU number < 256. When interrupt remapping 
> > > initialization fail on platform with CPU number < 256, current kernel 
> > > only enables x2APIC physical mode in virutalization environment, while 
> > > we also can enable x2APIC physcial mode in native kernel this situation, 
> > > and the device interrupt will use 8 bits destination APICID in physical 
> > > mode and be compatible with x2APIC physical when < 256 CPUs.
> > >  
> > > So we can benefit from x2APIC vs xAPIC MMIO:
> > >  - x2APIC MSR read/write is faster than xAPIC mmio
> > >  - x2APIC only ICR write to deliver interrupt without polling ICR deliver 
> > >    status bit and xAPIC need poll to read ICR deliver status bit.
> > >  - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.
> > 
> > That looks interesting. How many systems are affected by this change in 
> > practice? Have you tested it on affected hardware?
> 
> Thanks Ingo!
> The machines will be affected: CPU support x2APIC and CPU number < 256,
> chipset does not support VT-d2 or VT-d is disabled in BIOS. 

I mean, can you guess what rough percentage of new systems 
shipping (or significant number of older systems already 
shipped) will be affected by this?

My feeling is that this should be relatively rare (only 
when a user reconfigures the BIOS, etc.), but I might be 
wrong.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-25 22:01     ` Ingo Molnar
@ 2013-07-29 16:48       ` Youquan Song
  0 siblings, 0 replies; 23+ messages in thread
From: Youquan Song @ 2013-07-29 16:48 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Youquan Song, Youquan Song, linux-kernel, hpa, yinghai, tglx

> > Thanks Ingo!
> > The machines will be affected: CPU support x2APIC and CPU number < 256,
> > chipset does not support VT-d2 or VT-d is disabled in BIOS. 
> 
> I mean, can you guess what rough percentage of new systems 
> shipping (or significant number of older systems already 
> shipped) will be affected by this?
> 
> My feeling is that this should be relatively rare (only 
> when a user reconfigures the BIOS, etc.), but I might be 
> wrong.

Sorry. I do not know what percentage of system shipped be affected.
I have encountered one affected machine which CPU support x2APIC but its
BIOS not support VT-d (BIOS also has no item to enable it). After apply
the patch, it works with X2APIC physical mode.

Of course, most of machine affected are in the case of disable VT-d in BIOS
 by option or add intremap=off kernel option. 

>From what I understand, the x2APIC physical mode should be compatiable
with legacy mode when CPU < 256 without support interrupt remapping.

I have tested many machines, both old and most recent machines and from desktop
 to server, x2APIC physical mode works without interrupt remapping when CPU < 256. 

Thanks
-Youquan
 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-25 14:05     ` Yinghai Lu
@ 2013-07-29 17:05       ` Youquan Song
  2013-08-14 18:40         ` Youquan Song
  2013-08-02 19:12       ` Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 23+ messages in thread
From: Youquan Song @ 2013-07-29 17:05 UTC (permalink / raw)
  To: Yinghai Lu
  Cc: Gleb Natapov, Youquan Song, Sheng Yang, Konrad Rzeszutek Wilk,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner,
	Ingo Molnar, Youquan Song

> Yes. It would be great, if Youquan can point out where is the intel doc
> about the change.
> 
> Also if the patch can move on,  hypervisor_x2apic_available() related
> declaration and define
> could be dropped.

Hi Yinghai,

Sorry I do not know the document change but I also do not find the
words/description/explanation that x2APIC physical mode also need interrupt
 remapping support when CPU < 256. Of course, X2APIC cluster mode must
has interrupt remapping support. 

I have tested many machines, both old and most recent machines and from
desktop to server, x2APIC physical mode works without interrupt
remapping when CPU < 256.

In theory and real test, I do not find any issue about the patch.

In order to make sure the patch without involving unexpected issues beyond
I can understand, I will confirm with our expert about it.

so please pend the patch going to mainline. If the patch can move on, I
think I will also provide other patch changing, like direct EOI.

Thanks
-Youquan
 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-25 14:05     ` Yinghai Lu
  2013-07-29 17:05       ` Youquan Song
@ 2013-08-02 19:12       ` Konrad Rzeszutek Wilk
  1 sibling, 0 replies; 23+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-08-02 19:12 UTC (permalink / raw)
  To: Yinghai Lu, zhenzhong.duan
  Cc: Gleb Natapov, Youquan Song, Sheng Yang,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner,
	Ingo Molnar, Youquan Song

On Thu, Jul 25, 2013 at 07:05:15AM -0700, Yinghai Lu wrote:
> On Tue, Jul 23, 2013 at 11:22 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Jul 23, 2013 at 09:24:45PM -0700, Yinghai Lu wrote:
> >> On Thu, Jul 11, 2013 at 6:22 PM, Youquan Song <youquan.song@intel.com> wrote:
> >> > x2APIC extends APICID from 8 bits to 32 bits, but the device interrupt routed
> >> > from IOAPIC or delivered in MSI mode will keep 8 bits destination APICID.
> >> > In order to support x2APIC, the VT-d interrupt remapping is introduced to
> >> > translate the destination APICID to 32 bits in x2APIC mode and keep the device
> >> > compatible in this way.
> >> >
> >> > x2APIC support both logical and physical mode in destination mode.
> >> > In logical destination mode, the 32 bits Logical APICID has 2 sub-fields:
> >> >  16 bits cluster ID and 16 bits logical ID within the cluster and it is
> >> > required VT-d interrupt remapping in x2APIC cluster mode.
> >> > In physical destination mode, the 8 bits physical id is compatible with 32
> >> > bits physical id when CPU number < 256.
> >> > When interrupt remapping initialization fail on platform with CPU number < 256,
> >> > current kernel only enables x2APIC physical mode in virutalization environment,
> >> > while we also can enable x2APIC physcial mode in native kernel this situation,
> >> > and the device interrupt will use 8 bits destination APICID in physical mode
> >> > and be compatible with x2APIC physical when < 256 CPUs.
> >> >
> >> > So we can benefit from x2APIC vs xAPIC MMIO:
> >> >  - x2APIC MSR read/write is faster than xAPIC mmio
> >> >  - x2APIC only ICR write to deliver interrupt without polling ICR deliver
> >> >    status bit and xAPIC need poll to read ICR deliver status bit.
> >> >  - x2APIC 64 bits ICR access instead of xAPIC two 32 bits access.
> >> >
> >> > Signed-off-by: Youquan Song <youquan.song@intel.com>
> >> > ---
> >> >  arch/x86/kernel/apic/apic.c |    7 ++-----
> >> >  1 files changed, 2 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> >> > index 904611b..51a065a 100644
> >> > --- a/arch/x86/kernel/apic/apic.c
> >> > +++ b/arch/x86/kernel/apic/apic.c
> >> > @@ -1603,11 +1603,8 @@ void __init enable_IR_x2apic(void)
> >> >                 goto skip_x2apic;
> >> >
> >> >         if (ret < 0) {
> >> > -               /* IR is required if there is APIC ID > 255 even when running
> >> > -                * under KVM
> >> > -                */
> >> > -               if (max_physical_apicid > 255 ||
> >> > -                   !hypervisor_x2apic_available()) {
> >> > +               /* IR is required if there is APIC ID > 255 */
> >> > +               if (max_physical_apicid > 255) {
> >> >                         if (x2apic_preenabled)
> >> >                                 disable_x2apic();
> >> >                         goto skip_x2apic;
> >>
> >> Those are kvm and xen related.
> >>
> > This change does not affect kvm or xen since they already enable x2apic
> > without IR. But back then when I added the apicid > 255 check I had to
> > add the hypervisor check too because I was told by Intel that x2apic
> > without IR is not architectural. Seeing that this patch comes from Intel
> > engineer it is possible that things changed since then.
> 
> Yes. It would be great, if Youquan can point out where is the intel doc
> about the change.
> 
> Also if the patch can move on,  hypervisor_x2apic_available() related
> declaration and define
> could be dropped.

Adding Duan here, who says:

"But he didn't consider about VMware and Microsoft HyperV that may not
have emulation of x2apic.

About xen, if hvm pirq and vector callback is supported, x2apic could
be closed too."

> 
> Yinghai

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-14 18:40         ` Youquan Song
@ 2013-08-14 11:11           ` Ingo Molnar
  2013-08-17 13:44             ` Youquan Song
  0 siblings, 1 reply; 23+ messages in thread
From: Ingo Molnar @ 2013-08-14 11:11 UTC (permalink / raw)
  To: Youquan Song
  Cc: Yinghai Lu, Gleb Natapov, Youquan Song, Sheng Yang,
	Konrad Rzeszutek Wilk, Linux Kernel Mailing List, H. Peter Anvin,
	Thomas Gleixner


* Youquan Song <youquan.song@linux.intel.com> wrote:

> > In order to make sure the patch without involving unexpected issues beyond
> > I can understand, I will confirm with our expert about it.
> > 
> > so please pend the patch going to mainline. If the patch can move on, I
> > think I will also provide other patch changing, like direct EOI.
> 
> Hi Yinghai and Ingo,
> 
> I have confirmed with our experts about it. x2APIC without interrupt
> remapping is not architecture and no guarantee it will work in future.
> 
> What's more, there are some words in SDM3, 
> 10.12.7 Initialization by System
> Software Routing of device interrupts to local APIC units operating in
> x2APIC mode requires use of the interrupt-remapping architecture
> specified in the Intel Virtualization Technology for Directed I/O,
> Revision 1.3. Because of this, BIOS must enumerate support for and
> software must enable this interrupt remapping with Extended Interrupt
> Mode Enabled before it enabling x2APIC mode in the local APIC units.
> 
> Ingo, please drop the patch in -tip tree.
> 3d1acb49d22fbbae96524040e9e2d4cbbb3adbef "x86/apic: Enable x2APIC
> physical mode on native hardware too, when there are fewer than 256 
> CPUs" 

Ok, dropped it - it was still the tail commit.

> Sorry for making fuss here and it is my fault.

No problem - you might want to send another patch adding some comments to 
the code, explaining why we don't switch to physical mode, quoting from 
the SDM and so.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-07-29 17:05       ` Youquan Song
@ 2013-08-14 18:40         ` Youquan Song
  2013-08-14 11:11           ` Ingo Molnar
  0 siblings, 1 reply; 23+ messages in thread
From: Youquan Song @ 2013-08-14 18:40 UTC (permalink / raw)
  To: Ingo Molnar, Yinghai Lu
  Cc: Yinghai Lu, Gleb Natapov, Youquan Song, Sheng Yang,
	Konrad Rzeszutek Wilk, Linux Kernel Mailing List, H. Peter Anvin,
	Thomas Gleixner, Ingo Molnar, youquan.song

> In order to make sure the patch without involving unexpected issues beyond
> I can understand, I will confirm with our expert about it.
> 
> so please pend the patch going to mainline. If the patch can move on, I
> think I will also provide other patch changing, like direct EOI.

Hi Yinghai and Ingo,

I have confirmed with our experts about it. x2APIC without interrupt
remapping is not architecture and no guarantee it will work in future.

What's more, there are some words in SDM3, 
10.12.7 Initialization by System
Software Routing of device interrupts to local APIC units operating in
x2APIC mode requires use of the interrupt-remapping architecture
specified in the Intel Virtualization Technology for Directed I/O,
Revision 1.3. Because of this, BIOS must enumerate support for and
software must enable this interrupt remapping with Extended Interrupt
Mode Enabled before it enabling x2APIC mode in the local APIC units.

Ingo, please drop the patch in -tip tree.
3d1acb49d22fbbae96524040e9e2d4cbbb3adbef "x86/apic: Enable x2APIC
physical mode on native hardware too, when there are fewer than 256 
CPUs" 

Sorry for making fuss here and it is my fault. 

Thanks
-Youquan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-17 13:44             ` Youquan Song
@ 2013-08-17  7:42               ` Ingo Molnar
  2013-08-17  8:24                 ` Borislav Petkov
  2013-08-17 19:52                 ` Youquan Song
  0 siblings, 2 replies; 23+ messages in thread
From: Ingo Molnar @ 2013-08-17  7:42 UTC (permalink / raw)
  To: Youquan Song
  Cc: Yinghai Lu, Gleb Natapov, Youquan Song, Sheng Yang,
	Konrad Rzeszutek Wilk, Linux Kernel Mailing List, H. Peter Anvin,
	Thomas Gleixner


* Youquan Song <youquan.song@linux.intel.com> wrote:

> > No problem - you might want to send another patch adding some comments to 
> > the code, explaining why we don't switch to physical mode, quoting from 
> > the SDM and so.
> 
> Here is the revert patch.
> 
> Subject: [PATCH] Revert "x86/apic: Enable x2APIC physical mode on native hardware too, when there are fewer than 256 CPUs"
> 
> x2APIC without interrupt remapping is not architecture and no guarantee it 
> will work in future.
> There are some words in SDM3, 10.12.7 Initialization by System
> Software Routing of device interrupts to local APIC units operating in 
> x2APIC mode requires use of the interrupt-remapping architecture 
> specified in the Intel Virtualization Technology for Directed I/O, 
> Revision 1.3. Because of this, BIOS must enumerate support for and 
> software must enable this interrupt remapping with Extended Interrupt 
> Mode Enabled before it enabling x2APIC mode in the local APIC units.
> 
> This reverts commit 3d1acb49d22fbbae96524040e9e2d4cbbb3adbef, do not use
> x2apic_pysical mode if interrupt remapping is not enabled even at CPU
> number fewer than 256.
> 
> Signed-off-by: Youquan Song <youquan.song@intel.com>
> ---
>  arch/x86/kernel/apic/apic.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index d9dd5a6..eca89c5 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1622,8 +1622,11 @@ void __init enable_IR_x2apic(void)
>  		goto skip_x2apic;
>  
>  	if (ret < 0) {
> -		/* IR is required if there is APIC ID > 255 */
> -		if (max_physical_apicid > 255) {
> +		/* IR is required if there is APIC ID > 255 even when running
> +		 * under KVM
> +		 */
> +		if (max_physical_apicid > 255 ||
> +		    !hypervisor_x2apic_available()) {

Firstly, please use the customary (multi-line) comment 
style:

  /*
   * Comment .....
   * ...... goes here.
   */

specified in Documentation/CodingStyle.

Secondly, please send a patch against a vanilla (e.g. 
v3.11-rc5) kernel, as I've already zapped your previous 
patch from tip:x86/apic per your request.

Thanks,

        Ingo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-17  7:42               ` Ingo Molnar
@ 2013-08-17  8:24                 ` Borislav Petkov
  2013-08-17  9:03                   ` Joe Perches
  2013-08-17 19:52                 ` Youquan Song
  1 sibling, 1 reply; 23+ messages in thread
From: Borislav Petkov @ 2013-08-17  8:24 UTC (permalink / raw)
  To: Ingo Molnar, Joe Perches
  Cc: Youquan Song, Yinghai Lu, Gleb Natapov, Youquan Song, Sheng Yang,
	Konrad Rzeszutek Wilk, Linux Kernel Mailing List, H. Peter Anvin,
	Thomas Gleixner

On Sat, Aug 17, 2013 at 09:42:56AM +0200, Ingo Molnar wrote:
> 
> * Youquan Song <youquan.song@linux.intel.com> wrote:
> 
> > > No problem - you might want to send another patch adding some comments to 
> > > the code, explaining why we don't switch to physical mode, quoting from 
> > > the SDM and so.
> > 
> > Here is the revert patch.
> > 
> > Subject: [PATCH] Revert "x86/apic: Enable x2APIC physical mode on native hardware too, when there are fewer than 256 CPUs"
> > 
> > x2APIC without interrupt remapping is not architecture and no guarantee it 
> > will work in future.
> > There are some words in SDM3, 10.12.7 Initialization by System
> > Software Routing of device interrupts to local APIC units operating in 
> > x2APIC mode requires use of the interrupt-remapping architecture 
> > specified in the Intel Virtualization Technology for Directed I/O, 
> > Revision 1.3. Because of this, BIOS must enumerate support for and 
> > software must enable this interrupt remapping with Extended Interrupt 
> > Mode Enabled before it enabling x2APIC mode in the local APIC units.
> > 
> > This reverts commit 3d1acb49d22fbbae96524040e9e2d4cbbb3adbef, do not use
> > x2apic_pysical mode if interrupt remapping is not enabled even at CPU
> > number fewer than 256.
> > 
> > Signed-off-by: Youquan Song <youquan.song@intel.com>
> > ---
> >  arch/x86/kernel/apic/apic.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > index d9dd5a6..eca89c5 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -1622,8 +1622,11 @@ void __init enable_IR_x2apic(void)
> >  		goto skip_x2apic;
> >  
> >  	if (ret < 0) {
> > -		/* IR is required if there is APIC ID > 255 */
> > -		if (max_physical_apicid > 255) {
> > +		/* IR is required if there is APIC ID > 255 even when running
> > +		 * under KVM
> > +		 */
> > +		if (max_physical_apicid > 255 ||
> > +		    !hypervisor_x2apic_available()) {
> 
> Firstly, please use the customary (multi-line) comment 
> style:
> 
>   /*
>    * Comment .....
>    * ...... goes here.
>    */
> 
> specified in Documentation/CodingStyle.

This keeps popping up. Maybe it should be in checkpatch, Joe?

With the special handling of net/ and drivers/net/ of course.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-17  8:24                 ` Borislav Petkov
@ 2013-08-17  9:03                   ` Joe Perches
  2013-08-17 15:44                     ` Borislav Petkov
  0 siblings, 1 reply; 23+ messages in thread
From: Joe Perches @ 2013-08-17  9:03 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ingo Molnar, Youquan Song, Yinghai Lu, Gleb Natapov,
	Youquan Song, Sheng Yang, Konrad Rzeszutek Wilk,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner

On Sat, 2013-08-17 at 10:24 +0200, Borislav Petkov wrote:
> On Sat, Aug 17, 2013 at 09:42:56AM +0200, Ingo Molnar wrote:
> > 
> > * Youquan Song <youquan.song@linux.intel.com> wrote:
[]
> > Firstly, please use the customary (multi-line) comment 
> > style:
> > 
> >   /*
> >    * Comment .....
> >    * ...... goes here.
> >    */
> > 
> > specified in Documentation/CodingStyle.
> 
> This keeps popping up. Maybe it should be in checkpatch, Joe?
> With the special handling of net/ and drivers/net/ of course.

I don't feel that's useful.
You're welcome to add it if you want to.

checkpatch tends to be used for firs patch submissions and
adding it would only encourage a new wave of trivial whitespace
patches.

I think there are already way, _way_ too many existing instances
of comments that are of the form
	/* foo
	 * ...
to add that.

There are 10s of thousands outside of net/ and drivers/net/.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-14 11:11           ` Ingo Molnar
@ 2013-08-17 13:44             ` Youquan Song
  2013-08-17  7:42               ` Ingo Molnar
  0 siblings, 1 reply; 23+ messages in thread
From: Youquan Song @ 2013-08-17 13:44 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Youquan Song, Yinghai Lu, Gleb Natapov, Youquan Song, Sheng Yang,
	Konrad Rzeszutek Wilk, Linux Kernel Mailing List, H. Peter Anvin,
	Thomas Gleixner

> No problem - you might want to send another patch adding some comments to 
> the code, explaining why we don't switch to physical mode, quoting from 
> the SDM and so.

Here is the revert patch.

Subject: [PATCH] Revert "x86/apic: Enable x2APIC physical mode on native hardware too, when there are fewer than 256 CPUs"

x2APIC without interrupt remapping is not architecture and no guarantee it 
will work in future.
There are some words in SDM3, 10.12.7 Initialization by System
Software Routing of device interrupts to local APIC units operating in 
x2APIC mode requires use of the interrupt-remapping architecture 
specified in the Intel Virtualization Technology for Directed I/O, 
Revision 1.3. Because of this, BIOS must enumerate support for and 
software must enable this interrupt remapping with Extended Interrupt 
Mode Enabled before it enabling x2APIC mode in the local APIC units.

This reverts commit 3d1acb49d22fbbae96524040e9e2d4cbbb3adbef, do not use
x2apic_pysical mode if interrupt remapping is not enabled even at CPU
number fewer than 256.

Signed-off-by: Youquan Song <youquan.song@intel.com>
---
 arch/x86/kernel/apic/apic.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index d9dd5a6..eca89c5 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1622,8 +1622,11 @@ void __init enable_IR_x2apic(void)
 		goto skip_x2apic;
 
 	if (ret < 0) {
-		/* IR is required if there is APIC ID > 255 */
-		if (max_physical_apicid > 255) {
+		/* IR is required if there is APIC ID > 255 even when running
+		 * under KVM
+		 */
+		if (max_physical_apicid > 255 ||
+		    !hypervisor_x2apic_available()) {
 			if (x2apic_preenabled)
 				disable_x2apic();
 			goto skip_x2apic;
-- 
1.6.4.2

 

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-17  9:03                   ` Joe Perches
@ 2013-08-17 15:44                     ` Borislav Petkov
  2013-08-17 16:26                       ` Joe Perches
  0 siblings, 1 reply; 23+ messages in thread
From: Borislav Petkov @ 2013-08-17 15:44 UTC (permalink / raw)
  To: Joe Perches
  Cc: Ingo Molnar, Youquan Song, Yinghai Lu, Gleb Natapov,
	Youquan Song, Sheng Yang, Konrad Rzeszutek Wilk,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner

On Sat, Aug 17, 2013 at 02:03:51AM -0700, Joe Perches wrote:
> checkpatch tends to be used for firs patch submissions and
> adding it would only encourage a new wave of trivial whitespace
> patches.

Nope, we definitely don't want that...

> I think there are already way, _way_ too many existing instances
> of comments that are of the form
> 	/* foo
> 	 * ...
> to add that.
> 
> There are 10s of thousands outside of net/ and drivers/net/.

... unless there's a way to detect new submissions and scream only
for those. I.e., look at lines starting with "+" which don't have
corresponding "-" lines.

This would need a bit of experimenting and is not trivial though, maybe
Algorithm::Diff could even help there.

Sounds like a mini-project for a perl dude :-)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-17 15:44                     ` Borislav Petkov
@ 2013-08-17 16:26                       ` Joe Perches
  2013-08-18 10:02                         ` Borislav Petkov
  0 siblings, 1 reply; 23+ messages in thread
From: Joe Perches @ 2013-08-17 16:26 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ingo Molnar, Youquan Song, Yinghai Lu, Gleb Natapov,
	Youquan Song, Sheng Yang, Konrad Rzeszutek Wilk,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner

On Sat, 2013-08-17 at 17:44 +0200, Borislav Petkov wrote:
> On Sat, Aug 17, 2013 at 02:03:51AM -0700, Joe Perches wrote:
> > checkpatch tends to be used for firs patch submissions and
> > adding it would only encourage a new wave of trivial whitespace
> > patches.
> 
> Nope, we definitely don't want that...
> 
> > I think there are already way, _way_ too many existing instances
> > of comments that are of the form
> > 	/* foo
> > 	 * ...
> > to add that.
> > 
> > There are 10s of thousands outside of net/ and drivers/net/.
> 
> ... unless there's a way to detect new submissions and scream only
> for those. I.e., look at lines starting with "+" which don't have
> corresponding "-" lines.

No, that's not the right way to do that.
That bit's relatively easy because checkpatch only looks
at files when there's a specific command line -f flag.
So just check the existence of the -f command line flag
(!$file) and for an added comment that starts "/* foo" without
a comment termination */ after it on the same line.

> This would need a bit of experimenting and is not trivial though, maybe
> Algorithm::Diff could even help there.
> 
> Sounds like a mini-project for a perl dude :-)

I'm not one of those...

This might work though

 scripts/checkpatch.pl | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9ba4fc4..abe820a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2119,6 +2119,15 @@ sub process {
 			}
 		}
 
+		if (!$file &&
+		    $realfile !~ m@^(drivers/net/|net/)@ &&
+		    $rawline =~ /^\+/ &&	#Added line
+		    $rawline !~ m@/\*.*\*/@ &&	#whole comments
+		    $rawline =~ m@/\*+.+@) {	#unterminated comment
+			WARN("COMMENT_STYLE",
+			     "block comments use an empty /* line\n" . $herecurr);
+		}
+
 		if ($realfile =~ m@^(drivers/net/|net/)@ &&
 		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
 		    $rawline =~ /^\+[ \t]*\*/) {



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-17  7:42               ` Ingo Molnar
  2013-08-17  8:24                 ` Borislav Petkov
@ 2013-08-17 19:52                 ` Youquan Song
  2013-08-19  7:11                   ` Ingo Molnar
  1 sibling, 1 reply; 23+ messages in thread
From: Youquan Song @ 2013-08-17 19:52 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Youquan Song, Yinghai Lu, Gleb Natapov, Youquan Song, Sheng Yang,
	Konrad Rzeszutek Wilk, Linux Kernel Mailing List, H. Peter Anvin,
	Thomas Gleixner

> Firstly, please use the customary (multi-line) comment 
> style:
> 
>   /*
>    * Comment .....
>    * ...... goes here.
>    */
> 
> specified in Documentation/CodingStyle.
> 
> Secondly, please send a patch against a vanilla (e.g. 
> v3.11-rc5) kernel, as I've already zapped your previous 
> patch from tip:x86/apic per your request.
Hi Ingo,

latest vanilla has no includes the patch yet, so I think it
 will be fine by only dropping it from tip tree.

Thanks
-Youquan
 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-17 16:26                       ` Joe Perches
@ 2013-08-18 10:02                         ` Borislav Petkov
  0 siblings, 0 replies; 23+ messages in thread
From: Borislav Petkov @ 2013-08-18 10:02 UTC (permalink / raw)
  To: Joe Perches
  Cc: Ingo Molnar, Youquan Song, Yinghai Lu, Gleb Natapov,
	Youquan Song, Sheng Yang, Konrad Rzeszutek Wilk,
	Linux Kernel Mailing List, H. Peter Anvin, Thomas Gleixner

On Sat, Aug 17, 2013 at 09:26:43AM -0700, Joe Perches wrote:
> > ... unless there's a way to detect new submissions and scream only
> > for those. I.e., look at lines starting with "+" which don't have
> > corresponding "-" lines.
> 
> No, that's not the right way to do that.
> That bit's relatively easy because checkpatch only looks
> at files when there's a specific command line -f flag.
> So just check the existence of the -f command line flag
> (!$file) and for an added comment that starts "/* foo" without
> a comment termination */ after it on the same line.

Right, your way is simpler.

> > This would need a bit of experimenting and is not trivial though, maybe
> > Algorithm::Diff could even help there.
> > 
> > Sounds like a mini-project for a perl dude :-)
> 
> I'm not one of those...
> 
> This might work though
> 
>  scripts/checkpatch.pl | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9ba4fc4..abe820a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2119,6 +2119,15 @@ sub process {
>  			}
>  		}
>  
> +		if (!$file &&
> +		    $realfile !~ m@^(drivers/net/|net/)@ &&
> +		    $rawline =~ /^\+/ &&	#Added line
> +		    $rawline !~ m@/\*.*\*/@ &&	#whole comments
> +		    $rawline =~ m@/\*+.+@) {	#unterminated comment
> +			WARN("COMMENT_STYLE",
> +			     "block comments use an empty /* line\n" . $herecurr);
> +		}
> +
>  		if ($realfile =~ m@^(drivers/net/|net/)@ &&
>  		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
>  		    $rawline =~ /^\+[ \t]*\*/) {

Almost. It needs to look only on *.[ch] files because otherwise it fires on that
	very same diff to checkpatch.pl too:

$ git diff | ./scripts/checkpatch.pl -

...

WARNING: block comments use an empty /* line
#15: FILE: scripts/checkpatch.pl:2059:
+                            "block comments use an empty /* line\n" . $herecurr);


Also, we need to catch this obvious case:

        /*
         * This is a wrong comment. */


IOW, something like a modified version of yours below. It doesn't catch
every wrong comment form out there but it should be a start.

--
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2ee9eb750560..111b6615d74d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2050,6 +2050,16 @@ sub process {
 			}
 		}
 
+		if (!$file &&
+		    $realfile !~ m@^(drivers/net/|net/)@ &&
+		    $rawline =~ /^\+/ &&		# Added line
+		    $rawline !~ m@/\*.*\*/@ &&		# whole comments
+		    ($rawline =~ m@/\*+.+@ ||		# unterminated opening
+		     $rawline =~ m@\s*\*+.+\*+/@)) {	# unterminated closing
+			WARN("COMMENT_STYLE",
+			     "Multi-line comments use an empty /* opening and */ closing line\n" . $herecurr);
+		}
+
 		if ($realfile =~ m@^(drivers/net/|net/)@ &&
 		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
 		    $rawline =~ /^\+[ \t]*\*/) {

--

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native
  2013-08-17 19:52                 ` Youquan Song
@ 2013-08-19  7:11                   ` Ingo Molnar
  0 siblings, 0 replies; 23+ messages in thread
From: Ingo Molnar @ 2013-08-19  7:11 UTC (permalink / raw)
  To: Youquan Song
  Cc: Yinghai Lu, Gleb Natapov, Youquan Song, Sheng Yang,
	Konrad Rzeszutek Wilk, Linux Kernel Mailing List, H. Peter Anvin,
	Thomas Gleixner


* Youquan Song <youquan.song@linux.intel.com> wrote:

> > Firstly, please use the customary (multi-line) comment 
> > style:
> > 
> >   /*
> >    * Comment .....
> >    * ...... goes here.
> >    */
> > 
> > specified in Documentation/CodingStyle.
> > 
> > Secondly, please send a patch against a vanilla (e.g. 
> > v3.11-rc5) kernel, as I've already zapped your previous 
> > patch from tip:x86/apic per your request.
> Hi Ingo,
> 
> latest vanilla has no includes the patch yet, so I think it
>  will be fine by only dropping it from tip tree.

I know that it's not included yet - because I have not sent 
it to Linus.

My suggestion was to document the circumstances here via a 
single patch, i.e. not a patch + revert-and-add-comments 
patch, but via a simple add-comments patch. Agreed?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2013-08-19  7:11 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-12  1:22 [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Youquan Song
2013-07-23  9:17 ` Ingo Molnar
2013-07-24 14:04   ` Youquan Song
2013-07-25 22:01     ` Ingo Molnar
2013-07-29 16:48       ` Youquan Song
2013-07-24  3:55 ` [tip:x86/apic] x86/apic: Enable x2APIC physical mode on native hardware too, when there are fewer than 256 CPUs tip-bot for Youquan Song
2013-07-24  4:24 ` [PATCH] x86, apic: Enable x2APIC physical when cpu < 256 native Yinghai Lu
2013-07-24  6:22   ` Gleb Natapov
2013-07-25 14:05     ` Yinghai Lu
2013-07-29 17:05       ` Youquan Song
2013-08-14 18:40         ` Youquan Song
2013-08-14 11:11           ` Ingo Molnar
2013-08-17 13:44             ` Youquan Song
2013-08-17  7:42               ` Ingo Molnar
2013-08-17  8:24                 ` Borislav Petkov
2013-08-17  9:03                   ` Joe Perches
2013-08-17 15:44                     ` Borislav Petkov
2013-08-17 16:26                       ` Joe Perches
2013-08-18 10:02                         ` Borislav Petkov
2013-08-17 19:52                 ` Youquan Song
2013-08-19  7:11                   ` Ingo Molnar
2013-08-02 19:12       ` Konrad Rzeszutek Wilk
2013-07-24 14:45   ` Konrad Rzeszutek Wilk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).