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
