2017-03-07 11:53+0100, Paolo Bonzini:
> 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.
Yes, it seems that the compiler must assume that an enum can also be a
value that is not enumerated.
>> 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.
I would prefer to write it like:
kvm->arch.irqchip_mode > KVM_IRQCHIP_KERNEL_INIT;
Same assembly with simpler code. Setting KVM_IRQCHIP_KERNEL_INIT before
KVM_IRQCHIP_SPLIT would make it a bit more descriprive (and would allow
the check below as well).
>>>> /* 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?
Right, we don't want MSI or HV without LAPIC in kernel either.
