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

[Paolo Bonzini <pbonzini@redhat.com>]

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