2018-09-28 16:44:01

by Peng Hao

Subject: [PATCH] kvm/x86 : set meaningful return value

From: Peng Hao <[email protected]>

kvm_irq_delivery_to_apic_fast()-->
kvm_apic_map_get_dest_lapic()-->
kvm_apic_disabled_lapic_found()
kvm_apic_map_get_dest_lapic return with bitmap==0 and dst[i]==NULL,
then (*r == -1) will be returned to qemu and "KVM: injection failed,
MSI lost(Operation not permitted)" will be recorded in qemu log. The
output is puzzling.

Signed-off-by: Peng Hao <[email protected]>
---
arch/x86/kvm/lapic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index fbb0e6d..a8896b3 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -944,7 +944,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
int i;
bool ret;

- *r = -1;
+ *r = -ENXIO;

if (irq->shorthand == APIC_DEST_SELF) {
*r = kvm_apic_set_irq(src->vcpu, irq, dest_map);
--
1.8.3.1

2018-10-01 14:07:25

by Paolo Bonzini

Subject: Re: [PATCH] kvm/x86 : set meaningful return value

On 28/09/2018 18:41, Peng Hao wrote:
> From: Peng Hao <[email protected]>
>
> kvm_irq_delivery_to_apic_fast()-->
> kvm_apic_map_get_dest_lapic()-->
> kvm_apic_disabled_lapic_found()
> kvm_apic_map_get_dest_lapic return with bitmap==0 and dst[i]==NULL,
> then (*r == -1) will be returned to qemu and "KVM: injection failed,
> MSI lost(Operation not permitted)" will be recorded in qemu log. The
> output is puzzling.

This makes sense, but I am not sure that ENXIO is any better...

In fact, in this case I think it's okay to just return zero. What about
this fix:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index b756f1237ce3..d02515b8018f 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -957,14 +957,14 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
*kvm, struct kvm_lapic *src,
map = rcu_dereference(kvm->arch.apic_map);

ret = kvm_apic_map_get_dest_lapic(kvm, &src, irq, map, &dst, &bitmap);
- if (ret)
+ if (ret) {
+ *r = 0;
for_each_set_bit(i, &bitmap, 16) {
if (!dst[i])
continue;
- if (*r < 0)
- *r = 0;
*r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
}
+ }

rcu_read_unlock();
return ret;

Paolo

> Signed-off-by: Peng Hao <[email protected]>
> ---
> arch/x86/kvm/lapic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index fbb0e6d..a8896b3 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -944,7 +944,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> int i;
> bool ret;
>
> - *r = -1;
> + *r = -ENXIO;
>
> if (irq->shorthand == APIC_DEST_SELF) {
> *r = kvm_apic_set_irq(src->vcpu, irq, dest_map);
>