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

[David Hildenbrand <david@redhat.com>]

Am 06.03.2017 um 19:08 schrieb Paolo Bonzini:
> 
> 
> On 06/03/2017 14:17, David Hildenbrand wrote:
>>  	KVM_IRQCHIP_NONE,
>> +	KVM_IRQCHIP_KERNEL_INIT,  /* KVM_CREATE_IRQCHIP in progress */
> 
> Maybe KVM_IRQCHIP_INIT_IN_PROGRESS?

I tried to make it short but I agree, that is more self explaining.

> 
>> -	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?

> 
>>  	/* 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?

> 
> 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.

> Paolo
> 

Thanks!

David