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 <[email protected]>
---
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
* Youquan Song <[email protected]> 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
On Tue, Jul 23, 2013 at 11:17:29AM +0200, Ingo Molnar wrote:
>
> * Youquan Song <[email protected]> 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
Commit-ID: 3d1acb49d22fbbae96524040e9e2d4cbbb3adbef
Gitweb: http://git.kernel.org/tip/3d1acb49d22fbbae96524040e9e2d4cbbb3adbef
Author: Youquan Song <[email protected]>
AuthorDate: Thu, 11 Jul 2013 21:22:39 -0400
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: Youquan Song <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
On Thu, Jul 11, 2013 at 6:22 PM, Youquan Song <[email protected]> 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 <[email protected]>
> ---
> 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
On Tue, Jul 23, 2013 at 09:24:45PM -0700, Yinghai Lu wrote:
> On Thu, Jul 11, 2013 at 6:22 PM, Youquan Song <[email protected]> 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 <[email protected]>
> > ---
> > 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.
On Tue, Jul 23, 2013 at 09:24:45PM -0700, Yinghai Lu wrote:
> On Thu, Jul 11, 2013 at 6:22 PM, Youquan Song <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Tue, Jul 23, 2013 at 11:22 PM, Gleb Natapov <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> > ---
>> > 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
* Youquan Song <[email protected]> wrote:
> On Tue, Jul 23, 2013 at 11:17:29AM +0200, Ingo Molnar wrote:
> >
> > * Youquan Song <[email protected]> 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
> > 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
> 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
On Thu, Jul 25, 2013 at 07:05:15AM -0700, Yinghai Lu wrote:
> On Tue, Jul 23, 2013 at 11:22 PM, Gleb Natapov <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> > ---
> >> > 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
> 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
* Youquan Song <[email protected]> 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
> 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 <[email protected]>
---
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
* Youquan Song <[email protected]> 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 <[email protected]>
> ---
> 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
> 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
On Sat, Aug 17, 2013 at 09:42:56AM +0200, Ingo Molnar wrote:
>
> * Youquan Song <[email protected]> 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 <[email protected]>
> > ---
> > 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.
--
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 <[email protected]> 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/.
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.
--
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]*\*/) {
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.
--
* Youquan Song <[email protected]> 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