Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry()

["Radim Krčmář" <rkrcmar@redhat.com>]

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.