* [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) @ 2011-12-19 1:14 Julian Stecklina 2011-12-23 10:40 ` Marcelo Tosatti 0 siblings, 1 reply; 9+ messages in thread From: Julian Stecklina @ 2011-12-19 1:14 UTC (permalink / raw) To: kvm; +Cc: Julian Stecklina If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), it is erroneously treated as INIT de-assert and ignored, but to quote the spec: "For this delivery mode [INIT de-assert], the level flag must be set to 0 and trigger mode flag to 1." Signed-off-by: Julian Stecklina <js@alien8.de> --- arch/x86/kvm/lapic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index a7f3e65..260770d 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -433,7 +433,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, break; case APIC_DM_INIT: - if (level) { + if (!trig_mode || level) { result = 1; vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED; kvm_make_request(KVM_REQ_EVENT, vcpu); -- 1.7.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) 2011-12-19 1:14 [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) Julian Stecklina @ 2011-12-23 10:40 ` Marcelo Tosatti 2011-12-23 14:07 ` Julian Stecklina 2012-01-12 17:07 ` Julian Stecklina 0 siblings, 2 replies; 9+ messages in thread From: Marcelo Tosatti @ 2011-12-23 10:40 UTC (permalink / raw) To: Julian Stecklina; +Cc: kvm On Mon, Dec 19, 2011 at 02:14:27AM +0100, Julian Stecklina wrote: > If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), > it is erroneously treated as INIT de-assert and ignored, but to quote the > spec: "For this delivery mode [INIT de-assert], the level flag must be set to > 0 and trigger mode flag to 1." Yes, the implementation ignores INIT de-assert. Quoting the spec: "(INIT Level De-assert) (Not supported in the Pentium 4 and Intel Xeon processors.)" Your patch below is not improving the implementation to be closer to the spec: it'll trigger the INIT state initialization with trig_mode == 0 (which is not in accordance with your spec quote above). > Signed-off-by: Julian Stecklina <js@alien8.de> > --- > arch/x86/kvm/lapic.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index a7f3e65..260770d 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -433,7 +433,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > break; > > case APIC_DM_INIT: > - if (level) { > + if (!trig_mode || level) { > result = 1; > vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED; > kvm_make_request(KVM_REQ_EVENT, vcpu); > -- > 1.7.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) 2011-12-23 10:40 ` Marcelo Tosatti @ 2011-12-23 14:07 ` Julian Stecklina 2012-01-12 17:07 ` Julian Stecklina 1 sibling, 0 replies; 9+ messages in thread From: Julian Stecklina @ 2011-12-23 14:07 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm [-- Attachment #1: Type: text/plain, Size: 1041 bytes --] On Fr, 2011-12-23 at 08:40 -0200, Marcelo Tosatti wrote: > On Mon, Dec 19, 2011 at 02:14:27AM +0100, Julian Stecklina wrote: > > If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), > > it is erroneously treated as INIT de-assert and ignored, but to quote the > > spec: "For this delivery mode [INIT de-assert], the level flag must be set to > > 0 and trigger mode flag to 1." > > Yes, the implementation ignores INIT de-assert. Quoting the spec: > > "(INIT Level De-assert) (Not supported in the Pentium 4 and Intel Xeon > processors.)" > > Your patch below is not improving the implementation to be closer to the > spec: it'll trigger the INIT state initialization with trig_mode == 0 > (which is not in accordance with your spec quote above). I think our code that triggers this does weird things. Let me check that. Until then ignore that patch and sorry for the noise. :) At least it seems as if real hardware is interpreting the spec in a slightly different way... Regards, Julian [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) 2011-12-23 10:40 ` Marcelo Tosatti 2011-12-23 14:07 ` Julian Stecklina @ 2012-01-12 17:07 ` Julian Stecklina 2012-01-13 10:52 ` Marcelo Tosatti 1 sibling, 1 reply; 9+ messages in thread From: Julian Stecklina @ 2012-01-12 17:07 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm [-- Attachment #1: Type: text/plain, Size: 2144 bytes --] Am Freitag, den 23.12.2011, 08:40 -0200 schrieb Marcelo Tosatti: > On Mon, Dec 19, 2011 at 02:14:27AM +0100, Julian Stecklina wrote: > > If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), > > it is erroneously treated as INIT de-assert and ignored, but to quote the > > spec: "For this delivery mode [INIT de-assert], the level flag must be set to > > 0 and trigger mode flag to 1." > > Yes, the implementation ignores INIT de-assert. Quoting the spec: > > "(INIT Level De-assert) (Not supported in the Pentium 4 and Intel Xeon > processors.)" > > Your patch below is not improving the implementation to be closer to the > spec: it'll trigger the INIT state initialization with trig_mode == 0 > (which is not in accordance with your spec quote above). After reading the spec again and consulting with the guy who wrote the code triggering this, it seems the whole "if (level)" in the code path below is superfluous. Regarding level my spec says: "This flag has no meaning in Pentium 4 and Intel Xeon processors, and will always be issued as a 1." Judging from Section 10.2 this means every CPU where the LAPICs communicate over the system bus (everything after and including the P4). Thus the if can be removed, since its condition is always true (regardless of what the programmer actually programmed into the level field). If there is no objection, I'll submit a revised patch. > > Signed-off-by: Julian Stecklina <js@alien8.de> > > --- > > arch/x86/kvm/lapic.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index a7f3e65..260770d 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -433,7 +433,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > > break; > > > > case APIC_DM_INIT: > > - if (level) { > > + if (!trig_mode || level) { > > result = 1; > > vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED; > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > -- > > 1.7.7.4 > > Regards, Julian [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) 2012-01-12 17:07 ` Julian Stecklina @ 2012-01-13 10:52 ` Marcelo Tosatti 2012-01-13 11:46 ` Julian Stecklina 0 siblings, 1 reply; 9+ messages in thread From: Marcelo Tosatti @ 2012-01-13 10:52 UTC (permalink / raw) To: Julian Stecklina; +Cc: kvm On Thu, Jan 12, 2012 at 06:07:51PM +0100, Julian Stecklina wrote: > Am Freitag, den 23.12.2011, 08:40 -0200 schrieb Marcelo Tosatti: > > On Mon, Dec 19, 2011 at 02:14:27AM +0100, Julian Stecklina wrote: > > > If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), > > > it is erroneously treated as INIT de-assert and ignored, but to quote the > > > spec: "For this delivery mode [INIT de-assert], the level flag must be set to > > > 0 and trigger mode flag to 1." > > > > Yes, the implementation ignores INIT de-assert. Quoting the spec: > > > > "(INIT Level De-assert) (Not supported in the Pentium 4 and Intel Xeon > > processors.)" > > > > Your patch below is not improving the implementation to be closer to the > > spec: it'll trigger the INIT state initialization with trig_mode == 0 > > (which is not in accordance with your spec quote above). > > After reading the spec again and consulting with the guy who wrote the > code triggering this, it seems the whole "if (level)" in the code path > below is superfluous. No. Look at whats inside "if (level)": the mp_state assignment is the internal implementation of "delivers an INIT request to the target processor". According to the spec, the INIT level de-assert "Sends a synchronization message to all the local APICs in the system to set their arbitration IDs (stored in their Arb ID registers) to the values of their APIC IDs (see Section 10.7, “System and APIC Bus Arbitration”)." So if you remove the "if (level)" check, INIT de-assert will be emulated as INIT! > Regarding level my spec says: > > "This flag has no meaning in Pentium 4 and Intel Xeon processors, and > will always be issued as a 1." > > Judging from Section 10.2 this means every CPU where the LAPICs > communicate over the system bus (everything after and including the P4). > Thus the if can be removed, since its condition is always true > (regardless of what the programmer actually programmed into the level > field). > > If there is no objection, I'll submit a revised patch. > > > > Signed-off-by: Julian Stecklina <js@alien8.de> > > > --- > > > arch/x86/kvm/lapic.c | 2 +- > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > > index a7f3e65..260770d 100644 > > > --- a/arch/x86/kvm/lapic.c > > > +++ b/arch/x86/kvm/lapic.c > > > @@ -433,7 +433,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > > > break; > > > > > > case APIC_DM_INIT: > > > - if (level) { > > > + if (!trig_mode || level) { > > > result = 1; > > > vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED; > > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > > -- > > > 1.7.7.4 > > > > > Regards, Julian > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) 2012-01-13 10:52 ` Marcelo Tosatti @ 2012-01-13 11:46 ` Julian Stecklina 2012-01-16 10:18 ` Marcelo Tosatti 0 siblings, 1 reply; 9+ messages in thread From: Julian Stecklina @ 2012-01-13 11:46 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm [-- Attachment #1: Type: text/plain, Size: 2640 bytes --] Am Freitag, den 13.01.2012, 08:52 -0200 schrieb Marcelo Tosatti: > On Thu, Jan 12, 2012 at 06:07:51PM +0100, Julian Stecklina wrote: > > Am Freitag, den 23.12.2011, 08:40 -0200 schrieb Marcelo Tosatti: > > > On Mon, Dec 19, 2011 at 02:14:27AM +0100, Julian Stecklina wrote: > > > > If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), > > > > it is erroneously treated as INIT de-assert and ignored, but to quote the > > > > spec: "For this delivery mode [INIT de-assert], the level flag must be set to > > > > 0 and trigger mode flag to 1." > > > > > > Yes, the implementation ignores INIT de-assert. Quoting the spec: > > > > > > "(INIT Level De-assert) (Not supported in the Pentium 4 and Intel Xeon > > > processors.)" > > > > > > Your patch below is not improving the implementation to be closer to the > > > spec: it'll trigger the INIT state initialization with trig_mode == 0 > > > (which is not in accordance with your spec quote above). > > > > After reading the spec again and consulting with the guy who wrote the > > code triggering this, it seems the whole "if (level)" in the code path > > below is superfluous. > > No. Look at whats inside "if (level)": the mp_state assignment is the > internal implementation of "delivers an INIT request to the target > processor". > > According to the spec, the INIT level de-assert > > "Sends a synchronization message to all the local APICs in the system > to set their arbitration IDs (stored in their Arb ID registers) to the > values of their APIC IDs (see Section 10.7, “System and APIC Bus > Arbitration”)." > > So if you remove the "if (level)" check, INIT de-assert will be emulated > as INIT! Newer processors don't support INIT level de-assert and will interpret this as INIT. Without the "if (level)" check, KVM would behave in the same way, thus not breaking code that actually runs on real processors. For processors that still supported INIT level de-assert: If you look into older specs (243192), you read: 101 (INIT) ... INIT is treated as an edge triggered interrupt even if programmed otherwise. 101 (INIT Level De-assert) The trigger mode must also be set to 1 and level mode to 0. This means that if you don't set trigger mode to 1, you will get an INIT instead of INIT level de-assert. This is where the current code in KVM is wrong. So with my original patch, KVM would behave like the old spec mandates (check for trigger mode). With the "if (level)" check removed, it would behave like recent processors. Either way, the current code is bogus. Regards, Julian [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) 2012-01-13 11:46 ` Julian Stecklina @ 2012-01-16 10:18 ` Marcelo Tosatti 2012-01-16 13:02 ` js 0 siblings, 1 reply; 9+ messages in thread From: Marcelo Tosatti @ 2012-01-16 10:18 UTC (permalink / raw) To: Julian Stecklina; +Cc: kvm On Fri, Jan 13, 2012 at 12:46:19PM +0100, Julian Stecklina wrote: > Am Freitag, den 13.01.2012, 08:52 -0200 schrieb Marcelo Tosatti: > > On Thu, Jan 12, 2012 at 06:07:51PM +0100, Julian Stecklina wrote: > > > Am Freitag, den 23.12.2011, 08:40 -0200 schrieb Marcelo Tosatti: > > > > On Mon, Dec 19, 2011 at 02:14:27AM +0100, Julian Stecklina wrote: > > > > > If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), > > > > > it is erroneously treated as INIT de-assert and ignored, but to quote the > > > > > spec: "For this delivery mode [INIT de-assert], the level flag must be set to > > > > > 0 and trigger mode flag to 1." > > > > > > > > Yes, the implementation ignores INIT de-assert. Quoting the spec: > > > > > > > > "(INIT Level De-assert) (Not supported in the Pentium 4 and Intel Xeon > > > > processors.)" > > > > > > > > Your patch below is not improving the implementation to be closer to the > > > > spec: it'll trigger the INIT state initialization with trig_mode == 0 > > > > (which is not in accordance with your spec quote above). > > > > > > After reading the spec again and consulting with the guy who wrote the > > > code triggering this, it seems the whole "if (level)" in the code path > > > below is superfluous. > > > > No. Look at whats inside "if (level)": the mp_state assignment is the > > internal implementation of "delivers an INIT request to the target > > processor". > > > > According to the spec, the INIT level de-assert > > > > "Sends a synchronization message to all the local APICs in the system > > to set their arbitration IDs (stored in their Arb ID registers) to the > > values of their APIC IDs (see Section 10.7, “System and APIC Bus > > Arbitration”)." > > > > So if you remove the "if (level)" check, INIT de-assert will be emulated > > as INIT! > > Newer processors don't support INIT level de-assert and will interpret > this as INIT. Without the "if (level)" check, KVM would behave in the > same way, thus not breaking code that actually runs on real processors. > > For processors that still supported INIT level de-assert: If you look > into older specs (243192), you read: > > 101 (INIT) ... INIT is treated as an edge triggered interrupt even if > programmed otherwise. > > 101 (INIT Level De-assert) The trigger mode must also be set to 1 and > level mode to 0. > > This means that if you don't set trigger mode to 1, you will get an INIT > instead of INIT level de-assert. This is where the current code in KVM > is wrong. So with my original patch, KVM would behave like the old spec > mandates (check for trigger mode). With the "if (level)" check removed, > it would behave like recent processors. Either way, the current code is > bogus. > > Regards, Julian Yes, the original patch is fine. Please resend it. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) 2012-01-16 10:18 ` Marcelo Tosatti @ 2012-01-16 13:02 ` js 2012-01-24 11:09 ` Marcelo Tosatti 0 siblings, 1 reply; 9+ messages in thread From: js @ 2012-01-16 13:02 UTC (permalink / raw) To: kvm; +Cc: Julian Stecklina From: Julian Stecklina <js@alien8.de> If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), it is erroneously treated as INIT de-assert and ignored, but to quote the spec: "For this delivery mode [INIT de-assert], the level flag must be set to 0 and trigger mode flag to 1." Signed-off-by: Julian Stecklina <js@alien8.de> --- arch/x86/kvm/lapic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index cfdc6e0..3ee1d83 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -433,7 +433,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, break; case APIC_DM_INIT: - if (level) { + if (!trig_mode || level) { result = 1; vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED; kvm_make_request(KVM_REQ_EVENT, vcpu); -- 1.7.8.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) 2012-01-16 13:02 ` js @ 2012-01-24 11:09 ` Marcelo Tosatti 0 siblings, 0 replies; 9+ messages in thread From: Marcelo Tosatti @ 2012-01-24 11:09 UTC (permalink / raw) To: js; +Cc: kvm On Mon, Jan 16, 2012 at 02:02:20PM +0100, js@alien8.de wrote: > From: Julian Stecklina <js@alien8.de> > > If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (edge), > it is erroneously treated as INIT de-assert and ignored, but to quote the > spec: "For this delivery mode [INIT de-assert], the level flag must be set to > 0 and trigger mode flag to 1." > > Signed-off-by: Julian Stecklina <js@alien8.de> Applied, thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-01-24 11:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-19 1:14 [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) Julian Stecklina 2011-12-23 10:40 ` Marcelo Tosatti 2011-12-23 14:07 ` Julian Stecklina 2012-01-12 17:07 ` Julian Stecklina 2012-01-13 10:52 ` Marcelo Tosatti 2012-01-13 11:46 ` Julian Stecklina 2012-01-16 10:18 ` Marcelo Tosatti 2012-01-16 13:02 ` js 2012-01-24 11:09 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox