| Message ID | 20170306131815.12033-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > - 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. > /* 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. Should the code to enable split irqchip also use KVM_IRQCHIP_KERNEL_INIT? Paolo
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
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
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.
>>> 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; Agreed. > > 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). Okay, I added that. > >>>>> /* 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. > Ok, I will see how this turns out! Thanks!
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 74ef58c..c8cdcc3 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -727,6 +727,7 @@ struct kvm_hv { enum kvm_irqchip_mode { KVM_IRQCHIP_NONE, + KVM_IRQCHIP_KERNEL_INIT, /* KVM_CREATE_IRQCHIP in progress */ KVM_IRQCHIP_KERNEL, /* created with KVM_CREATE_IRQCHIP */ KVM_IRQCHIP_SPLIT, /* created with KVM_CAP_SPLIT_IRQCHIP */ }; diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h index 40d5b2c..9ebb6f5 100644 --- a/arch/x86/kvm/irq.h +++ b/arch/x86/kvm/irq.h @@ -96,6 +96,11 @@ static inline int irqchip_split(struct kvm *kvm) return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT; } +static inline int irqchip_kernel_init(struct kvm *kvm) +{ + return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL_INIT; +} + static inline int irqchip_kernel(struct kvm *kvm) { return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL; @@ -103,7 +108,8 @@ static inline int irqchip_kernel(struct kvm *kvm) static inline int irqchip_in_kernel(struct kvm *kvm) { - 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; /* 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; switch (ue->u.irqchip.irqchip) { case KVM_IRQCHIP_PIC_SLAVE: delta = 8; /* fall through */ case KVM_IRQCHIP_PIC_MASTER: - if (!pic_in_kernel(kvm)) - goto out; - e->set = kvm_set_pic_irq; max_pin = PIC_NUM_PINS; break; case KVM_IRQCHIP_IOAPIC: - if (!ioapic_in_kernel(kvm)) - goto out; - max_pin = KVM_IOAPIC_NUM_PINS; e->set = kvm_set_ioapic_irq; break; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b2a4b11..c69940c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4022,8 +4022,10 @@ long kvm_arch_vm_ioctl(struct file *filp, goto create_irqchip_unlock; } + kvm->arch.irqchip_mode = KVM_IRQCHIP_KERNEL_INIT; r = kvm_setup_default_irq_routing(kvm); if (r) { + kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; mutex_lock(&kvm->slots_lock); mutex_lock(&kvm->irq_lock); kvm_ioapic_destroy(kvm);
Let's replace the checks for pic_in_kernel() and ioapic_in_kernel() by checks against irqchip_mode. Add another state, indicating that the caller is currently initializing the irqchip. This is necessary to switch pic_in_kernel() and ioapic_in_kernel() to irqchip_mode, too. Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/irq.h | 8 +++++++- arch/x86/kvm/irq_comm.c | 8 ++------ arch/x86/kvm/x86.c | 2 ++ 4 files changed, 12 insertions(+), 7 deletions(-)