Re: [PATCH] kvm-userspace: fix memslot assignment
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 <[EMAIL PROTECTED]>
---
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)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-userspace: fix memslot assignment
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-userspace: fix memslot assignment
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;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-userspace: fix memslot assignment
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-userspace: fix memslot assignment
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-userspace: fix memslot assignment
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 <[EMAIL PROTECTED]>
---
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;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-userspace: fix memslot assignment
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-userspace: fix memslot assignment
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-userspace: fix memslot assignment
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 <[EMAIL PROTECTED]> --- 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
