| Message ID | 1506647099-2688-5-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-09-28 18:04-0700, Wanpeng Li: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > Vectors 0-15 are reserved, and a physical LAPIC - upon sending or > receiving one - would generate an APIC error instead of doing the > requested action. Make our emulation behave similarly. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > --- > arch/x86/kvm/lapic.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 6bafd06..a779ba9 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -935,6 +935,25 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq, > return ret; > } > > +static void apic_error(struct kvm_lapic *apic, unsigned long errmask) > +{ > + uint32_t esr; > + > + esr = kvm_lapic_get_reg(apic, APIC_ESR); > + > + if ((esr & errmask) != errmask) { The spec makes me think that there is going to be only 1 interrupt (regardless of the number errors) until the software writes 0 to APIC_ESR. Is there a better description than the following 10.5.3? The ESR is a write/read register. Before attempt to read from the ESR, software should first write to it. (The value written does not affect the values read subsequently; only zero may be written in x2APIC mode.) This write clears any previously logged errors and updates the ESR with any errors detected since the last write to the ESR. This write also rearms the APIC error interrupt triggering mechanism. This also describes a different handling of APIC_ESR -- APIC_ESR is updated only on software writes to APIC_ESR. All errors in between seem to be logged internally (not sure where to migrate it). > + uint32_t lvterr = kvm_lapic_get_reg(apic, APIC_LVTERR); > + > + kvm_lapic_set_reg(apic, APIC_ESR, esr | errmask); > + if (!(lvterr & APIC_LVT_MASKED)) { > + struct kvm_lapic_irq irq; > + > + irq.vector = lvterr & 0xff; > + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); > + } > + } > +} > + > /* > * Add a pending IRQ into lapic. > * Return 1 if successfully added and 0 if discarded. > @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > int result = 0; > struct kvm_vcpu *vcpu = apic->vcpu; > > + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { > + apic_error(apic, APIC_ESR_RECVILL); The error is also triggered if lowest priority is supported and tries to deliver an invalid vector. > + return 0; > + } > + > trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, > trig_mode, vector); > switch (delivery_mode) { > @@ -1146,7 +1170,10 @@ static void apic_send_ipi(struct kvm_lapic *apic) > irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode, > irq.vector, irq.msi_redir_hint); > > - kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); > + if (unlikely(irq.vector < 16 && irq.delivery_mode == APIC_DM_FIXED)) Please check how APICv self-IPI acceleration behaves, so we're consistent. Thanks. > + apic_error(apic, APIC_ESR_SENDILL); > + else > + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); > } > > static u32 apic_get_tmcct(struct kvm_lapic *apic) > @@ -1734,7 +1761,6 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > case APIC_LVTPC: > case APIC_LVT1: > case APIC_LVTERR: > - /* TODO: Check vector */ > if (!kvm_apic_sw_enabled(apic)) > val |= APIC_LVT_MASKED; > > -- > 2.7.4 >
2017-10-04 1:53 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2017-09-28 18:04-0700, Wanpeng Li: >> From: Wanpeng Li <wanpeng.li@hotmail.com> >> >> Vectors 0-15 are reserved, and a physical LAPIC - upon sending or >> receiving one - would generate an APIC error instead of doing the >> requested action. Make our emulation behave similarly. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >> --- >> arch/x86/kvm/lapic.c | 30 ++++++++++++++++++++++++++++-- >> 1 file changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index 6bafd06..a779ba9 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -935,6 +935,25 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq, >> return ret; >> } >> >> +static void apic_error(struct kvm_lapic *apic, unsigned long errmask) >> +{ >> + uint32_t esr; >> + >> + esr = kvm_lapic_get_reg(apic, APIC_ESR); >> + >> + if ((esr & errmask) != errmask) { > > The spec makes me think that there is going to be only 1 interrupt > (regardless of the number errors) until the software writes 0 to > APIC_ESR. Is there a better description than the following 10.5.3? > > The ESR is a write/read register. Before attempt to read from the ESR, > software should first write to it. (The value written does not affect > the values read subsequently; only zero may be written in x2APIC > mode.) This write clears any previously logged errors and updates the > ESR with any errors detected since the last write to the ESR. This > write also rearms the APIC error interrupt triggering mechanism. > > This also describes a different handling of APIC_ESR -- APIC_ESR is > updated only on software writes to APIC_ESR. All errors in between seem > to be logged internally (not sure where to migrate it). Is there any thing need to be changed in this function? > >> + uint32_t lvterr = kvm_lapic_get_reg(apic, APIC_LVTERR); >> + >> + kvm_lapic_set_reg(apic, APIC_ESR, esr | errmask); >> + if (!(lvterr & APIC_LVT_MASKED)) { >> + struct kvm_lapic_irq irq; >> + >> + irq.vector = lvterr & 0xff; >> + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); >> + } >> + } >> +} >> + >> /* >> * Add a pending IRQ into lapic. >> * Return 1 if successfully added and 0 if discarded. >> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, >> int result = 0; >> struct kvm_vcpu *vcpu = apic->vcpu; >> >> + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { >> + apic_error(apic, APIC_ESR_RECVILL); > > The error is also triggered if lowest priority is supported and tries to > deliver an invalid vector. Could you point out this in SDM? :) > >> + return 0; >> + } >> + >> trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, >> trig_mode, vector); >> switch (delivery_mode) { >> @@ -1146,7 +1170,10 @@ static void apic_send_ipi(struct kvm_lapic *apic) >> irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode, >> irq.vector, irq.msi_redir_hint); >> >> - kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); >> + if (unlikely(irq.vector < 16 && irq.delivery_mode == APIC_DM_FIXED)) > > Please check how APICv self-IPI acceleration behaves, so we're > consistent. There is no vmexit for APICv self-IPI, so I think we can't intercept the vector. Regards, Wanpeng Li > > Thanks. > >> + apic_error(apic, APIC_ESR_SENDILL); >> + else >> + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); >> } >> >> static u32 apic_get_tmcct(struct kvm_lapic *apic) >> @@ -1734,7 +1761,6 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) >> case APIC_LVTPC: >> case APIC_LVT1: >> case APIC_LVTERR: >> - /* TODO: Check vector */ >> if (!kvm_apic_sw_enabled(apic)) >> val |= APIC_LVT_MASKED; >> >> -- >> 2.7.4 >>
2017-10-04 15:56+0800, Wanpeng Li: > 2017-10-04 1:53 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > > 2017-09-28 18:04-0700, Wanpeng Li: > >> From: Wanpeng Li <wanpeng.li@hotmail.com> > >> > >> Vectors 0-15 are reserved, and a physical LAPIC - upon sending or > >> receiving one - would generate an APIC error instead of doing the > >> requested action. Make our emulation behave similarly. > >> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Cc: Radim Krčmář <rkrcmar@redhat.com> > >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > >> --- > >> arch/x86/kvm/lapic.c | 30 ++++++++++++++++++++++++++++-- > >> 1 file changed, 28 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > >> index 6bafd06..a779ba9 100644 > >> --- a/arch/x86/kvm/lapic.c > >> +++ b/arch/x86/kvm/lapic.c > >> @@ -935,6 +935,25 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq, > >> return ret; > >> } > >> > >> +static void apic_error(struct kvm_lapic *apic, unsigned long errmask) > >> +{ > >> + uint32_t esr; > >> + > >> + esr = kvm_lapic_get_reg(apic, APIC_ESR); > >> + > >> + if ((esr & errmask) != errmask) { > > > > The spec makes me think that there is going to be only 1 interrupt > > (regardless of the number errors) until the software writes 0 to > > APIC_ESR. Is there a better description than the following 10.5.3? > > > > The ESR is a write/read register. Before attempt to read from the ESR, > > software should first write to it. (The value written does not affect > > the values read subsequently; only zero may be written in x2APIC > > mode.) This write clears any previously logged errors and updates the > > ESR with any errors detected since the last write to the ESR. This > > write also rearms the APIC error interrupt triggering mechanism. > > > > This also describes a different handling of APIC_ESR -- APIC_ESR is > > updated only on software writes to APIC_ESR. All errors in between seem > > to be logged internally (not sure where to migrate it). > > Is there any thing need to be changed in this function? Yes. For the first part, it should really be tested on bare-metal and modelled upon that. SDM mentions some kind of rearming and APM doesn't so we maybe could just send the interrupt every time (if unmasked). And maybe vectors from external interrupts trigger the error too, but we definitely don't need to sort that out immediately. For the second part, the LAPIC error doesn't cause a write to APIC_ESR. We need to add a state for pending errors (and somehow migrate it) that gets copied to APIC_ESR after a write. > >> + uint32_t lvterr = kvm_lapic_get_reg(apic, APIC_LVTERR); > >> + > >> + kvm_lapic_set_reg(apic, APIC_ESR, esr | errmask); > >> + if (!(lvterr & APIC_LVT_MASKED)) { > >> + struct kvm_lapic_irq irq; > >> + > >> + irq.vector = lvterr & 0xff; > >> + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); > >> + } > >> + } > >> +} > >> + > >> /* > >> * Add a pending IRQ into lapic. > >> * Return 1 if successfully added and 0 if discarded. > >> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > >> int result = 0; > >> struct kvm_vcpu *vcpu = apic->vcpu; > >> > >> + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { > >> + apic_error(apic, APIC_ESR_RECVILL); > > > > The error is also triggered if lowest priority is supported and tries to > > deliver an invalid vector. > > Could you point out this in SDM? :) In section 10.5.3 Error Handling: If the local APIC does not support the sending of lowest-priority IPIs and software writes the ICR to send a lowest-priority IPI with an illegal vector, the local APIC sets only the “redirectible IPI” error bit. Hence, if local APIC does support lowest-priority, then it throws the same error as fixed. (KVM does support lowest-priority.) > > > >> + return 0; > >> + } > >> + > >> trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, > >> trig_mode, vector); > >> switch (delivery_mode) { > >> @@ -1146,7 +1170,10 @@ static void apic_send_ipi(struct kvm_lapic *apic) > >> irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode, > >> irq.vector, irq.msi_redir_hint); > >> > >> - kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); > >> + if (unlikely(irq.vector < 16 && irq.delivery_mode == APIC_DM_FIXED)) > > > > Please check how APICv self-IPI acceleration behaves, so we're > > consistent. > > There is no vmexit for APICv self-IPI, so I think we can't intercept the vector. Right, so does it deliver the 0-15 vector? If yes, then we should do that as well. Otherwise, where does it save the error flag and does it send an error interrupt? Or do we get a VM exit after all? Thanks.
2017-10-04 20:01 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2017-10-04 15:56+0800, Wanpeng Li: >> 2017-10-04 1:53 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> > 2017-09-28 18:04-0700, Wanpeng Li: >> >> From: Wanpeng Li <wanpeng.li@hotmail.com> >> >> >> >> Vectors 0-15 are reserved, and a physical LAPIC - upon sending or >> >> receiving one - would generate an APIC error instead of doing the >> >> requested action. Make our emulation behave similarly. >> >> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >> >> --- >> >> arch/x86/kvm/lapic.c | 30 ++++++++++++++++++++++++++++-- >> >> 1 file changed, 28 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> >> index 6bafd06..a779ba9 100644 >> >> --- a/arch/x86/kvm/lapic.c >> >> +++ b/arch/x86/kvm/lapic.c >> >> @@ -935,6 +935,25 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq, >> >> return ret; >> >> } >> >> >> >> +static void apic_error(struct kvm_lapic *apic, unsigned long errmask) >> >> +{ >> >> + uint32_t esr; >> >> + >> >> + esr = kvm_lapic_get_reg(apic, APIC_ESR); >> >> + >> >> + if ((esr & errmask) != errmask) { >> > >> > The spec makes me think that there is going to be only 1 interrupt >> > (regardless of the number errors) until the software writes 0 to >> > APIC_ESR. Is there a better description than the following 10.5.3? >> > >> > The ESR is a write/read register. Before attempt to read from the ESR, >> > software should first write to it. (The value written does not affect >> > the values read subsequently; only zero may be written in x2APIC >> > mode.) This write clears any previously logged errors and updates the >> > ESR with any errors detected since the last write to the ESR. This >> > write also rearms the APIC error interrupt triggering mechanism. >> > >> > This also describes a different handling of APIC_ESR -- APIC_ESR is >> > updated only on software writes to APIC_ESR. All errors in between seem >> > to be logged internally (not sure where to migrate it). >> >> Is there any thing need to be changed in this function? > > Yes. For the first part, it should really be tested on bare-metal and > modelled upon that. SDM mentions some kind of rearming and APM doesn't > so we maybe could just send the interrupt every time (if unmasked). > And maybe vectors from external interrupts trigger the error too, but > we definitely don't need to sort that out immediately. > > For the second part, the LAPIC error doesn't cause a write to APIC_ESR. > We need to add a state for pending errors (and somehow migrate it) that > gets copied to APIC_ESR after a write. > >> >> + uint32_t lvterr = kvm_lapic_get_reg(apic, APIC_LVTERR); >> >> + >> >> + kvm_lapic_set_reg(apic, APIC_ESR, esr | errmask); >> >> + if (!(lvterr & APIC_LVT_MASKED)) { >> >> + struct kvm_lapic_irq irq; >> >> + >> >> + irq.vector = lvterr & 0xff; >> >> + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); >> >> + } >> >> + } >> >> +} >> >> + >> >> /* >> >> * Add a pending IRQ into lapic. >> >> * Return 1 if successfully added and 0 if discarded. >> >> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, >> >> int result = 0; >> >> struct kvm_vcpu *vcpu = apic->vcpu; >> >> >> >> + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { >> >> + apic_error(apic, APIC_ESR_RECVILL); >> > >> > The error is also triggered if lowest priority is supported and tries to >> > deliver an invalid vector. >> >> Could you point out this in SDM? :) > > In section 10.5.3 Error Handling: > > If the local APIC does not support the sending of lowest-priority IPIs > and software writes the ICR to send a lowest-priority IPI with an > illegal vector, the local APIC sets only the “redirectible IPI” error > bit. > > Hence, if local APIC does support lowest-priority, then it throws the > same error as fixed. (KVM does support lowest-priority.) Yeah, I read the section before but I misunderstand it. It seems that the section means it just occurs when the local APIC does not support the sending of lowest-priority IPIs? Regards, Wanpeng Li > >> > >> >> + return 0; >> >> + } >> >> + >> >> trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, >> >> trig_mode, vector); >> >> switch (delivery_mode) { >> >> @@ -1146,7 +1170,10 @@ static void apic_send_ipi(struct kvm_lapic *apic) >> >> irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode, >> >> irq.vector, irq.msi_redir_hint); >> >> >> >> - kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); >> >> + if (unlikely(irq.vector < 16 && irq.delivery_mode == APIC_DM_FIXED)) >> > >> > Please check how APICv self-IPI acceleration behaves, so we're >> > consistent. >> >> There is no vmexit for APICv self-IPI, so I think we can't intercept the vector. > > Right, so does it deliver the 0-15 vector? If yes, then we should do > that as well. Otherwise, where does it save the error flag and does it > send an error interrupt? Or do we get a VM exit after all? > > Thanks.
2017-10-04 22:16+0800, Wanpeng Li: > 2017-10-04 20:01 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > > 2017-10-04 15:56+0800, Wanpeng Li: > >> 2017-10-04 1:53 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > >> > 2017-09-28 18:04-0700, Wanpeng Li: > >> >> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > >> >> int result = 0; > >> >> struct kvm_vcpu *vcpu = apic->vcpu; > >> >> > >> >> + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { > >> >> + apic_error(apic, APIC_ESR_RECVILL); > >> > > >> > The error is also triggered if lowest priority is supported and tries to > >> > deliver an invalid vector. > >> > >> Could you point out this in SDM? :) > > > > In section 10.5.3 Error Handling: > > > > If the local APIC does not support the sending of lowest-priority IPIs > > and software writes the ICR to send a lowest-priority IPI with an > > illegal vector, the local APIC sets only the “redirectible IPI” error > > bit. > > > > Hence, if local APIC does support lowest-priority, then it throws the > > same error as fixed. (KVM does support lowest-priority.) > > Yeah, I read the section before but I misunderstand it. It seems that > the section means it just occurs when the local APIC does not support > the sending of lowest-priority IPIs? I think so.
2017-10-04 22:44 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2017-10-04 22:16+0800, Wanpeng Li: >> 2017-10-04 20:01 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> > 2017-10-04 15:56+0800, Wanpeng Li: >> >> 2017-10-04 1:53 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> >> > 2017-09-28 18:04-0700, Wanpeng Li: >> >> >> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, >> >> >> int result = 0; >> >> >> struct kvm_vcpu *vcpu = apic->vcpu; >> >> >> >> >> >> + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { >> >> >> + apic_error(apic, APIC_ESR_RECVILL); >> >> > >> >> > The error is also triggered if lowest priority is supported and tries to >> >> > deliver an invalid vector. >> >> >> >> Could you point out this in SDM? :) >> > >> > In section 10.5.3 Error Handling: >> > >> > If the local APIC does not support the sending of lowest-priority IPIs >> > and software writes the ICR to send a lowest-priority IPI with an >> > illegal vector, the local APIC sets only the “redirectible IPI” error >> > bit. >> > >> > Hence, if local APIC does support lowest-priority, then it throws the >> > same error as fixed. (KVM does support lowest-priority.) >> >> Yeah, I read the section before but I misunderstand it. It seems that >> the section means it just occurs when the local APIC does not support >> the sending of lowest-priority IPIs? > > I think so. I see Virtualbox just captures Fixed delivery mode for error handling. Regards, Wanpeng Li
2017-10-13 09:17+0800, Wanpeng Li: > 2017-10-04 22:44 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > > 2017-10-04 22:16+0800, Wanpeng Li: > >> 2017-10-04 20:01 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > >> > 2017-10-04 15:56+0800, Wanpeng Li: > >> >> 2017-10-04 1:53 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > >> >> > 2017-09-28 18:04-0700, Wanpeng Li: > >> >> >> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > >> >> >> int result = 0; > >> >> >> struct kvm_vcpu *vcpu = apic->vcpu; > >> >> >> > >> >> >> + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { > >> >> >> + apic_error(apic, APIC_ESR_RECVILL); > >> >> > > >> >> > The error is also triggered if lowest priority is supported and tries to > >> >> > deliver an invalid vector. > >> >> > >> >> Could you point out this in SDM? :) > >> > > >> > In section 10.5.3 Error Handling: > >> > > >> > If the local APIC does not support the sending of lowest-priority IPIs > >> > and software writes the ICR to send a lowest-priority IPI with an > >> > illegal vector, the local APIC sets only the “redirectible IPI” error > >> > bit. > >> > > >> > Hence, if local APIC does support lowest-priority, then it throws the > >> > same error as fixed. (KVM does support lowest-priority.) > >> > >> Yeah, I read the section before but I misunderstand it. It seems that > >> the section means it just occurs when the local APIC does not support > >> the sending of lowest-priority IPIs? > > > > I think so. > > I see Virtualbox just captures Fixed delivery mode for error handling. Hm, it doesn't even inject an error on destination of the lowest-priority interrupt and just drop it? I can't interpret the SDM in any other way, though: When an interrupt vector in the range of 0 to 15 is sent or received through the local APIC, the APIC indicates an illegal vector in its Error Status Register (see Section 10.5.3, “Error Handling”). and we support lowest-priority interrupts, because if we didn't, then The interrupt is not processed and hence the “Send Illegal Vector” bit is not set in the ESR. I'll go for a quick bare-metal test ...
2017-10-13 19:36+0200, Radim Krčmář: > 2017-10-13 09:17+0800, Wanpeng Li: > > 2017-10-04 22:44 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > > > 2017-10-04 22:16+0800, Wanpeng Li: > > >> 2017-10-04 20:01 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > > >> > 2017-10-04 15:56+0800, Wanpeng Li: > > >> >> 2017-10-04 1:53 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > > >> >> > 2017-09-28 18:04-0700, Wanpeng Li: > > >> >> >> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > > >> >> >> int result = 0; > > >> >> >> struct kvm_vcpu *vcpu = apic->vcpu; > > >> >> >> > > >> >> >> + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { > > >> >> >> + apic_error(apic, APIC_ESR_RECVILL); > > >> >> > > > >> >> > The error is also triggered if lowest priority is supported and tries to > > >> >> > deliver an invalid vector. > > >> >> > > >> >> Could you point out this in SDM? :) > > >> > > > >> > In section 10.5.3 Error Handling: > > >> > > > >> > If the local APIC does not support the sending of lowest-priority IPIs > > >> > and software writes the ICR to send a lowest-priority IPI with an > > >> > illegal vector, the local APIC sets only the “redirectible IPI” error > > >> > bit. > > >> > > > >> > Hence, if local APIC does support lowest-priority, then it throws the > > >> > same error as fixed. (KVM does support lowest-priority.) > > >> > > >> Yeah, I read the section before but I misunderstand it. It seems that > > >> the section means it just occurs when the local APIC does not support > > >> the sending of lowest-priority IPIs? > > > > > > I think so. > > > > I see Virtualbox just captures Fixed delivery mode for error handling. > > Hm, it doesn't even inject an error on destination of the > lowest-priority interrupt and just drop it? > > I can't interpret the SDM in any other way, though: > > When an interrupt vector in the range of 0 to 15 is sent or received > through the local APIC, the APIC indicates an illegal vector in its > Error Status Register (see Section 10.5.3, “Error Handling”). > > and we support lowest-priority interrupts, because if we didn't, then > > The interrupt is not processed and hence the “Send Illegal Vector” bit > is not set in the ESR. > > I'll go for a quick bare-metal test ... Turns out my machine doesn't support for lowest priority IPIs (probably got killed with FSB), so all I get is error 0x10.
2017-10-14 4:31 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2017-10-13 19:36+0200, Radim Krčmář: >> 2017-10-13 09:17+0800, Wanpeng Li: >> > 2017-10-04 22:44 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> > > 2017-10-04 22:16+0800, Wanpeng Li: >> > >> 2017-10-04 20:01 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> > >> > 2017-10-04 15:56+0800, Wanpeng Li: >> > >> >> 2017-10-04 1:53 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> > >> >> > 2017-09-28 18:04-0700, Wanpeng Li: >> > >> >> >> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, >> > >> >> >> int result = 0; >> > >> >> >> struct kvm_vcpu *vcpu = apic->vcpu; >> > >> >> >> >> > >> >> >> + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { >> > >> >> >> + apic_error(apic, APIC_ESR_RECVILL); >> > >> >> > >> > >> >> > The error is also triggered if lowest priority is supported and tries to >> > >> >> > deliver an invalid vector. >> > >> >> >> > >> >> Could you point out this in SDM? :) >> > >> > >> > >> > In section 10.5.3 Error Handling: >> > >> > >> > >> > If the local APIC does not support the sending of lowest-priority IPIs >> > >> > and software writes the ICR to send a lowest-priority IPI with an >> > >> > illegal vector, the local APIC sets only the “redirectible IPI” error >> > >> > bit. >> > >> > >> > >> > Hence, if local APIC does support lowest-priority, then it throws the >> > >> > same error as fixed. (KVM does support lowest-priority.) >> > >> >> > >> Yeah, I read the section before but I misunderstand it. It seems that >> > >> the section means it just occurs when the local APIC does not support >> > >> the sending of lowest-priority IPIs? >> > > >> > > I think so. >> > >> > I see Virtualbox just captures Fixed delivery mode for error handling. >> >> Hm, it doesn't even inject an error on destination of the >> lowest-priority interrupt and just drop it? I think so. >> >> I can't interpret the SDM in any other way, though: >> >> When an interrupt vector in the range of 0 to 15 is sent or received >> through the local APIC, the APIC indicates an illegal vector in its >> Error Status Register (see Section 10.5.3, “Error Handling”). >> >> and we support lowest-priority interrupts, because if we didn't, then >> >> The interrupt is not processed and hence the “Send Illegal Vector” bit >> is not set in the ESR. >> >> I'll go for a quick bare-metal test ... > > Turns out my machine doesn't support for lowest priority IPIs (probably > got killed with FSB), so all I get is error 0x10.
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 6bafd06..a779ba9 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -935,6 +935,25 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq, return ret; } +static void apic_error(struct kvm_lapic *apic, unsigned long errmask) +{ + uint32_t esr; + + esr = kvm_lapic_get_reg(apic, APIC_ESR); + + if ((esr & errmask) != errmask) { + uint32_t lvterr = kvm_lapic_get_reg(apic, APIC_LVTERR); + + kvm_lapic_set_reg(apic, APIC_ESR, esr | errmask); + if (!(lvterr & APIC_LVT_MASKED)) { + struct kvm_lapic_irq irq; + + irq.vector = lvterr & 0xff; + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); + } + } +} + /* * Add a pending IRQ into lapic. * Return 1 if successfully added and 0 if discarded. @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, int result = 0; struct kvm_vcpu *vcpu = apic->vcpu; + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { + apic_error(apic, APIC_ESR_RECVILL); + return 0; + } + trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, trig_mode, vector); switch (delivery_mode) { @@ -1146,7 +1170,10 @@ static void apic_send_ipi(struct kvm_lapic *apic) irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode, irq.vector, irq.msi_redir_hint); - kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); + if (unlikely(irq.vector < 16 && irq.delivery_mode == APIC_DM_FIXED)) + apic_error(apic, APIC_ESR_SENDILL); + else + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); } static u32 apic_get_tmcct(struct kvm_lapic *apic) @@ -1734,7 +1761,6 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) case APIC_LVTPC: case APIC_LVT1: case APIC_LVTERR: - /* TODO: Check vector */ if (!kvm_apic_sw_enabled(apic)) val |= APIC_LVT_MASKED;