[PATCH] kvm-userspace: fix memslot assignment

[PATCH] kvm-userspace: fix memslot assignment

[Christian Borntraeger]

Hello Avi,

seems that I mixed up the slot initialization, instead of making the first 
slot always 0 I made it always 1. Lets go back to Carstens variant, since I 
dont like nested ifdefs. The compiler will remove the dead code anyway.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

---
 libkvm.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: libkvm/libkvm.c
===================================================================
--- libkvm.orig/libkvm.c
+++ libkvm/libkvm.c
@@ -78,7 +78,7 @@ int get_free_slot(kvm_context_t kvm)
 	int i;
 	int tss_ext;
 
-#if defined(KVM_CAP_SET_TSS_ADDR) && !defined(__s390__)
+#if defined(KVM_CAP_SET_TSS_ADDR)
 	tss_ext = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
 #else
 	tss_ext = 0;
@@ -92,8 +92,11 @@ int get_free_slot(kvm_context_t kvm)
 	if (tss_ext > 0)
 		i = 0;
 	else
+#if !defined(__s390__)
 		i = 1;
-
+#else
+		i = 0;
+#endif
 	for (; i < KVM_MAX_NUM_MEM_REGIONS; ++i)
 		if (!slots[i].len)
 			return i;

Re: [PATCH] kvm-userspace: fix memslot assignment

[Avi Kivity]

Christian Borntraeger wrote:
> Hello Avi,
>
> seems that I mixed up the slot initialization, instead of making the first 
> slot always 0 I made it always 1. Lets go back to Carstens variant, since I 
> dont like nested ifdefs. The compiler will remove the dead code anyway.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> ---
>  libkvm.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> Index: libkvm/libkvm.c
> ===================================================================
> --- libkvm.orig/libkvm.c
> +++ libkvm/libkvm.c
> @@ -78,7 +78,7 @@ int get_free_slot(kvm_context_t kvm)
>  	int i;
>  	int tss_ext;
>  
> -#if defined(KVM_CAP_SET_TSS_ADDR) && !defined(__s390__)
> +#if defined(KVM_CAP_SET_TSS_ADDR)
>  	tss_ext = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
>  #else
>  	tss_ext = 0;
>   

This is really a no-op, since s390 will nack a KVM_CAP_SET_TSS_ADDR 
query.  Of course, the change is an improvement.

> @@ -92,8 +92,11 @@ int get_free_slot(kvm_context_t kvm)
>  	if (tss_ext > 0)
>  		i = 0;
>  	else
> +#if !defined(__s390__)
>  		i = 1;
> -
> +#else
> +		i = 0;
> +#endif
>   

ppc and ia64 also don't care about TSS_ADDR.  Care to make this depend 
on x86, rather than on s390?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm-userspace: fix memslot assignment

[Christian Borntraeger]

Am Sonntag, 27. Juli 2008 schrieb Avi Kivity:
> This is really a no-op, since s390 will nack a KVM_CAP_SET_TSS_ADDR 
> query.  Of course, the change is an improvement.
> 
> > @@ -92,8 +92,11 @@ int get_free_slot(kvm_context_t kvm)
> >  	if (tss_ext > 0)
> >  		i = 0;
> >  	else
> > +#if !defined(__s390__)
> >  		i = 1;
> > -
> > +#else
> > +		i = 0;
> > +#endif
> >   
> 
> ppc and ia64 also don't care about TSS_ADDR.  Care to make this depend 
> on x86, rather than on s390?

Can do. Should ppc and ia64 start with i=1 or i=0, do they need a reserved 
slot?

Christian

Re: [PATCH] kvm-userspace: fix memslot assignment

[Avi Kivity]

Christian Borntraeger wrote:
> Am Sonntag, 27. Juli 2008 schrieb Avi Kivity:
>   
>> This is really a no-op, since s390 will nack a KVM_CAP_SET_TSS_ADDR 
>> query.  Of course, the change is an improvement.
>>
>>     
>>> @@ -92,8 +92,11 @@ int get_free_slot(kvm_context_t kvm)
>>>  	if (tss_ext > 0)
>>>  		i = 0;
>>>  	else
>>> +#if !defined(__s390__)
>>>  		i = 1;
>>> -
>>> +#else
>>> +		i = 0;
>>> +#endif
>>>   
>>>       
>> ppc and ia64 also don't care about TSS_ADDR.  Care to make this depend 
>> on x86, rather than on s390?
>>     
>
> Can do. Should ppc and ia64 start with i=1 or i=0, do they need a reserved 
> slot?
>
>   

They all start with 0 AFAIK (kvm also starts with 0, it just wants it to 
be a special slot).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm-userspace: fix memslot assignment

[Christian Borntraeger]

Am Dienstag, 29. Juli 2008 schrieb Avi Kivity:
> They all start with 0 AFAIK (kvm also starts with 0, it just wants it to 
> be a special slot).

Now my brain hurts....
Ok, so I read this as: ppc, ia64 and s390 can start with slot 0 and it is not a special slot.
On x86 slot 0 is special, if
* KVM_CAP_SET_TSS_ADDR is not available
or
* The ioctl VM_CAP_SET_TSS_ADDR returns <=0

Since my opteron box died recently I cannot test that on x86 at the moment.
What do you think about the following patch

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

---
 libkvm/libkvm.c |   28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

Index: kvm-userspace/libkvm/libkvm.c
===================================================================
--- kvm-userspace.orig/libkvm/libkvm.c
+++ kvm-userspace/libkvm/libkvm.c
@@ -75,25 +75,21 @@ void init_slots(void)
 
 int get_free_slot(kvm_context_t kvm)
 {
-	int i;
-	int tss_ext;
-
-#if defined(KVM_CAP_SET_TSS_ADDR) && !defined(__s390__)
-	tss_ext = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
-#else
-	tss_ext = 0;
-#endif
+	int i = 0;
 
 	/*
-	 * on older kernels where the set tss ioctl is not supprted we must save
-	 * slot 0 to hold the extended memory, as the vmx will use the last 3
-	 * pages of this slot.
+	 * on older x86 kernels where the set tss ioctl is not supported we
+	 * must save slot 0 to hold the extended memory, as the vmx will
+	 * use the last 3 pages of this slot.
 	 */
-	if (tss_ext > 0)
-		i = 0;
-	else
-		i = 1;
-
+#if defined(__x86_64__) || defined(__i386__)
+#if defined(KVM_CAP_SET_TSS_ADDR)
+	if (ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR) > 0)
+		i++;
+#else
+	i++;
+#endif
+#endif
 	for (; i < KVM_MAX_NUM_MEM_REGIONS; ++i)
 		if (!slots[i].len)
 			return i;

Re: [PATCH] kvm-userspace: fix memslot assignment

[Avi Kivity]

Christian Borntraeger wrote:
> Am Dienstag, 29. Juli 2008 schrieb Avi Kivity:
>   
>> They all start with 0 AFAIK (kvm also starts with 0, it just wants it to 
>> be a special slot).
>>     
>
> Now my brain hurts....
> Ok, so I read this as: ppc, ia64 and s390 can start with slot 0 and it is not a special slot.
> On x86 slot 0 is special, if
> * KVM_CAP_SET_TSS_ADDR is not available
> or
> * The ioctl VM_CAP_SET_TSS_ADDR returns <=0
>
>   

I think this is right.  Izik can you confirm?

> +#if defined(__x86_64__) || defined(__i386__)
> +#if defined(KVM_CAP_SET_TSS_ADDR)
> +	if (ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR) > 0)
>   

Should be <= here?

> +		i++;
> +#else
> +	i++;
> +#endif
> +#endif
>   

Suggest a helper, kvm_supports_set_tss_addr(), to reduce further 
braindamage.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm-userspace: fix memslot assignment

[Izik Eidos]

Avi Kivity wrote:
> Christian Borntraeger wrote:
>> Am Dienstag, 29. Juli 2008 schrieb Avi Kivity:
>>  
>>> They all start with 0 AFAIK (kvm also starts with 0, it just wants 
>>> it to be a special slot).
>>>     
>>
>> Now my brain hurts....
>> Ok, so I read this as: ppc, ia64 and s390 can start with slot 0 and 
>> it is not a special slot.
>> On x86 slot 0 is special, if
>> * KVM_CAP_SET_TSS_ADDR is not available
>> or
>> * The ioctl VM_CAP_SET_TSS_ADDR returns <=0
>>
>>   
>
> I think this is right.  Izik can you confirm?

yea kernels that dont have KVM_CAP_SET_TSS_ADDR should be treated like that.

>
>> +#if defined(__x86_64__) || defined(__i386__)
>> +#if defined(KVM_CAP_SET_TSS_ADDR)
>> +    if (ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR) > 0)
>>   
>
> Should be <= here?
>
>> +        i++;
>> +#else
>> +    i++;
>> +#endif
>> +#endif
>>   
>
> Suggest a helper, kvm_supports_set_tss_addr(), to reduce further 
> braindamage.
>

Re: [PATCH] kvm-userspace: fix memslot assignment

[Christian Borntraeger]

Am Donnerstag, 31. Juli 2008 schrieb Avi Kivity:
> > +#if defined(__x86_64__) || defined(__i386__)
> > +#if defined(KVM_CAP_SET_TSS_ADDR)
> > +	if (ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR) > 0)
> >   
> 
> Should be <= here?

yes. But with the wrapper you suggest below its again >.

> 
> > +		i++;
> > +#else
> > +	i++;
> > +#endif
> > +#endif
> >   
> 
> Suggest a helper, kvm_supports_set_tss_addr(), to reduce further 
> braindamage.

Something like that? But then I get the following warning on non-x86:
libkvm.c:77: warning: 'kvm_supports_set_tss_addr' defined but not used

Should I mask the wrapper with config x86 as well?


---
 libkvm.c |   32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

Index: libkvm/libkvm.c
===================================================================
--- libkvm.orig/libkvm.c
+++ libkvm/libkvm.c
@@ -73,27 +73,29 @@ void init_slots(void)
 		slots[i].len = 0;
 }
 
+static int kvm_supports_set_tss_addr(kvm_context_t kvm)
+{
+#if defined(KVM_CAP_SET_TSS_ADDR)
+	if (ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR) > 0)
+		return 1;
+#endif
+	return 0;
+}
+
 int get_free_slot(kvm_context_t kvm)
 {
 	int i;
-	int tss_ext;
-
-#if defined(KVM_CAP_SET_TSS_ADDR) && !defined(__s390__)
-	tss_ext = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
-#else
-	tss_ext = 0;
-#endif
 
 	/*
-	 * on older kernels where the set tss ioctl is not supprted we must save
-	 * slot 0 to hold the extended memory, as the vmx will use the last 3
-	 * pages of this slot.
+	 * on older x86 kernels where the set tss ioctl is not supported we
+	 * must save slot 0 to hold the extended memory, as the vmx will
+	 * use the last 3 pages of this slot.
 	 */
-	if (tss_ext > 0)
-		i = 0;
-	else
-		i = 1;
-
+#if defined(__x86_64__) || defined(__i386__)
+	i = !kvm_supports_set_tss_addr(kvm);
+#else
+	i = 0;
+#endif
 	for (; i < KVM_MAX_NUM_MEM_REGIONS; ++i)
 		if (!slots[i].len)
 			return i;

Re: [PATCH] kvm-userspace: fix memslot assignment

[Avi Kivity]

Christian Borntraeger wrote:
> Something like that? But then I get the following warning on non-x86:
> libkvm.c:77: warning: 'kvm_supports_set_tss_addr' defined but not used
>
> Should I mask the wrapper with config x86 as well?
>   

If we rename the helper, kvm_wants_special_memslot_0() (reversing the 
meaning), we can have non-x86 return 0, x86 with KVM_CAP_SET_TSS_ADDR 
return  0, and otherwise return 1.  No ifdefs outside the helper this way.

>
> ---
>  libkvm.c |   32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
>   
(missing signoff)


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] kvm-userspace: fix memslot assignment

[Christian Borntraeger]

Am Donnerstag, 31. Juli 2008 schrieb Avi Kivity:
> If we rename the helper, kvm_wants_special_memslot_0() (reversing the 
> meaning), we can have non-x86 return 0, x86 with KVM_CAP_SET_TSS_ADDR 
> return  0, and otherwise return 1.  No ifdefs outside the helper this way.
Having two ifdefs is not going to be very pretty, but here is the latest
version:


ppc, ia64 and s390 can start with slot 0 and it is not a special slot.
On x86 slot 0 is special, if
* KVM_CAP_SET_TSS_ADDR is not available
or
* The ioctl VM_CAP_SET_TSS_ADDR returns <=0
Lets introduce a helper that checks if we need a special memslot.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

---
 libkvm.c |   35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

Index: libkvm/libkvm.c
===================================================================
--- libkvm.orig/libkvm.c
+++ libkvm/libkvm.c
@@ -73,26 +73,29 @@ void init_slots(void)
 		slots[i].len = 0;
 }
 
-int get_free_slot(kvm_context_t kvm)
+static int kvm_wants_special_memslot_0(kvm_context_t kvm)
 {
-	int i;
-	int tss_ext;
-
-#if defined(KVM_CAP_SET_TSS_ADDR) && !defined(__s390__)
-	tss_ext = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
+#if !defined(__x86_64__) && !defined(__i386__)
+	return 0;
 #else
-	tss_ext = 0;
-#endif
-
 	/*
-	 * on older kernels where the set tss ioctl is not supprted we must save
-	 * slot 0 to hold the extended memory, as the vmx will use the last 3
-	 * pages of this slot.
+	 * on older x86 kernels where the set tss ioctl is not supported we
+	 * must save slot 0 to hold the extended memory, as the vmx will
+	 * use the last 3 pages of this slot.
 	 */
-	if (tss_ext > 0)
-		i = 0;
-	else
-		i = 1;
+#if defined(KVM_CAP_SET_TSS_ADDR)
+	if (ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR) > 0)
+		return 0;
+#endif
+	return 1;
+#endif
+}
+
+int get_free_slot(kvm_context_t kvm)
+{
+	int i;
+
+	i = kvm_wants_special_memslot_0(kvm);
 
 	for (; i < KVM_MAX_NUM_MEM_REGIONS; ++i)
 		if (!slots[i].len)