On 07/03/2017 10:55, David Hildenbrand wrote:
> Am 06.03.2017 um 19:08 schrieb Paolo Bonzini:
>>> - bool ret = kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE;
>>> + bool ret = kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL ||
>>> + kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
>>
>> I suspect that if you phrase it the other way round (!= NONE && !=
>> KERNEL_INIT) you'll get infinitesimally better code, because it can be
>> compiled to an unsigned comparison with 1.
>
> However, adding new modes can silently make this check wrong (e.g.
> grepping for KVM_IRQCHIP_KERNEL will no longer identify all users). Do
> you think the optimization is worth it?
I don't think we want to add new modes.
>>
>>> /* Matches with wmb after initializing kvm->irq_routing. */
>>> smp_rmb();
>>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>>> index b96d389..4e4a67a 100644
>>> --- a/arch/x86/kvm/irq_comm.c
>>> +++ b/arch/x86/kvm/irq_comm.c
>>> @@ -282,22 +282,18 @@ int kvm_set_routing_entry(struct kvm *kvm,
>>>
>>> switch (ue->type) {
>>> case KVM_IRQ_ROUTING_IRQCHIP:
>>> + if (!irqchip_kernel(kvm) && !irqchip_kernel_init(kvm))
>>> + goto out;
>>> delta = 0;
>>
>> This can be irqchip_in_kernel, after which irqchip_kernel_init can be
>> removed.
>
> irqchip_in_kernel in its current form would allow KVM_IRQCHIP_SPLIT,
> which is not what we want here, or am I missing something?
Hmm, perhaps we can split the checks to rule out KVM_IRQCHIP_NONE
outside the switch, and KVM_IRQCHIP_SPLIT here?
>> Should the code to enable split irqchip also use KVM_IRQCHIP_KERNEL_INIT?
>
> a) checking against KVM_IRQCHIP_KERNEL_INIT shouldn't be necessary due
> to the kvm->lock (this code path will never see KVM_IRQCHIP_KERNEL_INIT).
>
> b) setting KVM_IRQCHIP_KERNEL_INIT could be done. We only initialize an
> empty irq routing. But also that should never be allowed to set up
> routings targeted at pic/ioapic. However that would in its current form
> never happen.
You're right, it'd be just a matter of keeping the code similar between
the two cases. You can try it and decide when you post the next version.
Thanks!
Paolo
