Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1545071 > unrolled thread

Re: [PATCH] KVM: x86: check for pic and ioapic presence before use

Started byWanpeng Li <kernellwp@gmail.com>
First post2016-12-20 13:10 +0100
Last post2016-12-22 11:00 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] KVM: x86: check for pic and ioapic presence before use Wanpeng Li <kernellwp@gmail.com> - 2016-12-20 13:10 +0100
    Re: [PATCH] KVM: x86: check for pic and ioapic presence before use Radim Krčmář <rkrcmar@redhat.com> - 2016-12-21 13:50 +0100
      Re: [PATCH] KVM: x86: check for pic and ioapic presence before use Wanpeng Li <kernellwp@gmail.com> - 2016-12-22 11:00 +0100

#1545071 — Re: [PATCH] KVM: x86: check for pic and ioapic presence before use

FromWanpeng Li <kernellwp@gmail.com>
Date2016-12-20 13:10 +0100
SubjectRe: [PATCH] KVM: x86: check for pic and ioapic presence before use
Message-ID<sQroZ-5p6-5@gated-at.bofh.it>
2016-11-24 20:42 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2016-11-23 22:58+0100, Paolo Bonzini:
>> On 23/11/2016 21:25, Radim Krčmář wrote:
>>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>>> index 25810b144b58..ddd63b8b176e 100644
>>> --- a/arch/x86/kvm/irq_comm.c
>>> +++ b/arch/x86/kvm/irq_comm.c
>>> @@ -41,6 +41,15 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>>>                         bool line_status)
>>>  {
>>>      struct kvm_pic *pic = pic_irqchip(kvm);
>>> +
>>> +    /*
>>> +     * XXX: rejecting pic routes when pic isn't in use would be better,
>>> +     * but the default routing table is installed while kvm->arch.vpic is
>>> +     * NULL and KVM_CREATE_IRQCHIP can race with KVM_SET_GSI_ROUTING.
>>> +     */
>>> +    if (!pic)
>>> +            return -1;
>>> +
>>>      return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
>>>  }
>>>
>>
>> Can you explain the race with the default routing table better?  It
>> seems to me that it can only make the routing table go from invalid to
>> valid (there is no KVM_DESTROY_IRQCHIP) so it's benign.
>
> Oops, I wrote the race with wrong IOCTL -- it should be KVM_IRQ_LINE.
>
>  1) set KVM_CAP_SPLIT_IRQCHIP (unlocks KVM_IRQ_LINE)
>  a) call KVM_CREATE_IRQCHIP (creates routes while !kvm->arch.vpic)
>  b) concurrently call KVM_IRQ_LINE for PIO routes (dereferences NULL)

If we should not go through irqfd if irqchip is split?

Regards,
Wanpeng Li

[toc] | [next] | [standalone]

#1545744

FromRadim Krčmář <rkrcmar@redhat.com>
Date2016-12-21 13:50 +0100
Message-ID<sQOvg-3vJ-23@gated-at.bofh.it>
In reply to#1545071
2016-12-20 19:59+0800, Wanpeng Li:
> 2016-11-24 20:42 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
>> 2016-11-23 22:58+0100, Paolo Bonzini:
>>> On 23/11/2016 21:25, Radim Krčmář wrote:
>>>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>>>> index 25810b144b58..ddd63b8b176e 100644
>>>> --- a/arch/x86/kvm/irq_comm.c
>>>> +++ b/arch/x86/kvm/irq_comm.c
>>>> @@ -41,6 +41,15 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>>>>                         bool line_status)
>>>>  {
>>>>      struct kvm_pic *pic = pic_irqchip(kvm);
>>>> +
>>>> +    /*
>>>> +     * XXX: rejecting pic routes when pic isn't in use would be better,
>>>> +     * but the default routing table is installed while kvm->arch.vpic is
>>>> +     * NULL and KVM_CREATE_IRQCHIP can race with KVM_SET_GSI_ROUTING.
>>>> +     */
>>>> +    if (!pic)
>>>> +            return -1;
>>>> +
>>>>      return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
>>>>  }
>>>>
>>>
>>> Can you explain the race with the default routing table better?  It
>>> seems to me that it can only make the routing table go from invalid to
>>> valid (there is no KVM_DESTROY_IRQCHIP) so it's benign.
>>
>> Oops, I wrote the race with wrong IOCTL -- it should be KVM_IRQ_LINE.
>>
>>  1) set KVM_CAP_SPLIT_IRQCHIP (unlocks KVM_IRQ_LINE)
>>  a) call KVM_CREATE_IRQCHIP (creates routes while !kvm->arch.vpic)
>>  b) concurrently call KVM_IRQ_LINE for PIO routes (dereferences NULL)
> 
> If we should not go through irqfd if irqchip is split?

I also remember hearing about that -- do you remember where it was?

The documentation does not say that and irqfd is mostly optimization for
KVM_IRQ_LINE ... QEMU uses KVM_IRQ_LINE_STATUS with split irqchip, so we
can't easily say the opposite now.

[toc] | [prev] | [next] | [standalone]

#1546250

FromWanpeng Li <kernellwp@gmail.com>
Date2016-12-22 11:00 +0100
Message-ID<sR8kh-7Pe-15@gated-at.bofh.it>
In reply to#1545744
2016-12-21 20:44 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
> 2016-12-20 19:59+0800, Wanpeng Li:
>> 2016-11-24 20:42 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
>>> 2016-11-23 22:58+0100, Paolo Bonzini:
>>>> On 23/11/2016 21:25, Radim Krčmář wrote:
>>>>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>>>>> index 25810b144b58..ddd63b8b176e 100644
>>>>> --- a/arch/x86/kvm/irq_comm.c
>>>>> +++ b/arch/x86/kvm/irq_comm.c
>>>>> @@ -41,6 +41,15 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>>>>>                         bool line_status)
>>>>>  {
>>>>>      struct kvm_pic *pic = pic_irqchip(kvm);
>>>>> +
>>>>> +    /*
>>>>> +     * XXX: rejecting pic routes when pic isn't in use would be better,
>>>>> +     * but the default routing table is installed while kvm->arch.vpic is
>>>>> +     * NULL and KVM_CREATE_IRQCHIP can race with KVM_SET_GSI_ROUTING.
>>>>> +     */
>>>>> +    if (!pic)
>>>>> +            return -1;
>>>>> +
>>>>>      return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
>>>>>  }
>>>>>
>>>>
>>>> Can you explain the race with the default routing table better?  It
>>>> seems to me that it can only make the routing table go from invalid to
>>>> valid (there is no KVM_DESTROY_IRQCHIP) so it's benign.
>>>
>>> Oops, I wrote the race with wrong IOCTL -- it should be KVM_IRQ_LINE.
>>>
>>>  1) set KVM_CAP_SPLIT_IRQCHIP (unlocks KVM_IRQ_LINE)
>>>  a) call KVM_CREATE_IRQCHIP (creates routes while !kvm->arch.vpic)
>>>  b) concurrently call KVM_IRQ_LINE for PIO routes (dereferences NULL)
>>
>> If we should not go through irqfd if irqchip is split?
>
> I also remember hearing about that -- do you remember where it was?

Not sure. :)

>
> The documentation does not say that and irqfd is mostly optimization for
> KVM_IRQ_LINE ... QEMU uses KVM_IRQ_LINE_STATUS with split irqchip, so we
> can't easily say the opposite now.

How irqfd optimizes KVM_IRQ_LINE_STATUS? I didn't observe that they
have relationship.

Regards,
Wanpeng Li

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web