[RFC,02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry()
diff mbox

Message ID 20170306131815.12033-3-david@redhat.com
State New
Headers show

Commit Message

David Hildenbrand March 6, 2017, 1:17 p.m. UTC
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(-)

Comments

Paolo Bonzini March 6, 2017, 6:08 p.m. UTC | #1
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
David Hildenbrand March 7, 2017, 9:55 a.m. UTC | #2
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
Paolo Bonzini March 7, 2017, 10:53 a.m. UTC | #3
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
Radim Krčmář March 7, 2017, 2:40 p.m. UTC | #4
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.
David Hildenbrand March 7, 2017, 3:32 p.m. UTC | #5
>>> 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!

Patch
diff mbox

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);