[Qemu-devel] [PATCH] kvm/x86: enlarge number of possible CPUID leaves

[Qemu-devel] [PATCH] kvm/x86: enlarge number of possible CPUID leaves

[Andre Przywara]

Currently the number of CPUID leaves KVM handles is limited to 40.
My desktop machine (AthlonII) already has 35 and future CPUs will
expand this well beyond the limit. Extend the limit to 80 to make
room for future processors.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/include/asm/kvm_host.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Hi,
I found that either KVM or QEMU (possibly both) are broken in respect
to handling more CPUID entries than the limit dictates. KVM will
return -E2BIG, which is the same error as if the user hasn't provided
enough space to hold all entries. Now QEMU will continue to enlarge
the allocated memory until it gets into an out-of-memory condition.
I have tried to fix this with teaching KVM how to deal with a capped
number of entries (there are some bugs in the current code), but this
will limit the number of CPUID entries KVM handles, which will surely
cut of the lastly appended PV leaves.
A proper fix would be to make this allocation dynamic. Is this a
feasible way or will this lead to issues or side-effects?

Regards,
Andre.

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 54e42c8..3cc80c4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -79,7 +79,7 @@
 #define KVM_NUM_MMU_PAGES (1 << KVM_MMU_HASH_SHIFT)
 #define KVM_MIN_FREE_MMU_PAGES 5
 #define KVM_REFILL_PAGES 25
-#define KVM_MAX_CPUID_ENTRIES 40
+#define KVM_MAX_CPUID_ENTRIES 80
 #define KVM_NR_FIXED_MTRR_REGION 88
 #define KVM_NR_VAR_MTRR 8
 
-- 
1.6.4

[Qemu-devel] Re: [PATCH] kvm/x86: enlarge number of possible CPUID leaves

[Avi Kivity]

On 12/01/2010 01:17 PM, Andre Przywara wrote:
> Currently the number of CPUID leaves KVM handles is limited to 40.
> My desktop machine (AthlonII) already has 35 and future CPUs will
> expand this well beyond the limit. Extend the limit to 80 to make
> room for future processors.
>
> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
> ---
>   arch/x86/include/asm/kvm_host.h |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> Hi,
> I found that either KVM or QEMU (possibly both) are broken in respect
> to handling more CPUID entries than the limit dictates. KVM will
> return -E2BIG, which is the same error as if the user hasn't provided
> enough space to hold all entries. Now QEMU will continue to enlarge
> the allocated memory until it gets into an out-of-memory condition.
> I have tried to fix this with teaching KVM how to deal with a capped
> number of entries (there are some bugs in the current code), but this
> will limit the number of CPUID entries KVM handles, which will surely
> cut of the lastly appended PV leaves.
> A proper fix would be to make this allocation dynamic. Is this a
> feasible way or will this lead to issues or side-effects?
>

Well, there has to be some limit, or userspace can allocate unbounded 
kernel memory.

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

[Qemu-devel] Re: [PATCH] kvm/x86: enlarge number of possible CPUID leaves

[Andre Przywara]

Avi Kivity wrote:
> On 12/01/2010 01:17 PM, Andre Przywara wrote:
>> Currently the number of CPUID leaves KVM handles is limited to 40.
>> My desktop machine (AthlonII) already has 35 and future CPUs will
>> expand this well beyond the limit. Extend the limit to 80 to make
>> room for future processors.
>>
>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>> ---
>>   arch/x86/include/asm/kvm_host.h |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> Hi,
>> I found that either KVM or QEMU (possibly both) are broken in respect
>> to handling more CPUID entries than the limit dictates. KVM will
>> return -E2BIG, which is the same error as if the user hasn't provided
>> enough space to hold all entries. Now QEMU will continue to enlarge
>> the allocated memory until it gets into an out-of-memory condition.
>> I have tried to fix this with teaching KVM how to deal with a capped
>> number of entries (there are some bugs in the current code), but this
>> will limit the number of CPUID entries KVM handles, which will surely
>> cut of the lastly appended PV leaves.
>> A proper fix would be to make this allocation dynamic. Is this a
>> feasible way or will this lead to issues or side-effects?
>>
> 
> Well, there has to be some limit, or userspace can allocate unbounded 
> kernel memory.
But this limit should not be a compile-time constant, but a runtime one. 
The needed size depends on the host CPU (plus the KVM PV leaves) and 
thus could be determined once for all VMs and vCPUs at module load-time. 
But then we cannot use the static array allocation we currently have in 
struct kvm_vcpu_arch:
     struct kvm_cpuid_entry2 cpuid_entries[KVM_MAX_CPUID_ENTRIES];
So we would use a kind-of dynamic allocation bounded by the host CPU's 
need. But for the code is does not make much difference to a "real" 
dynamic allocation.

Also we could implement kvm_dev_ioctl_get_supported_cpuid without the 
vmalloc, if we don't care about some dozens of copy_to_user() calls in 
this function. Then we would not need this limit in GET_SUPPORTED_CPUID 
at all, but it will strike us again at KVM_SET_CPUID[2], where we may 
not fulfill the promises we gave earlier.
Having said this, what about that:
kvm_dev_ioctl_get_supported_cpuid is invariant to the VM or vCPU (as it 
is used by a system ioctl), so it could be run once at initialization, 
which would limit the ioctl implementation to a plain bounded copy.
Would you want such a patch (removing the vmalloc and maybe even the limit)?

Regards,
Andre.

-- 
Andre Przywara
AMD-OSRC (Dresden)
Tel: x29712

[Qemu-devel] Re: [PATCH] kvm/x86: enlarge number of possible CPUID leaves

[Avi Kivity]

On 12/01/2010 02:55 PM, Andre Przywara wrote:
> Avi Kivity wrote:
>> On 12/01/2010 01:17 PM, Andre Przywara wrote:
>>> Currently the number of CPUID leaves KVM handles is limited to 40.
>>> My desktop machine (AthlonII) already has 35 and future CPUs will
>>> expand this well beyond the limit. Extend the limit to 80 to make
>>> room for future processors.
>>>
>>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>>> ---
>>>   arch/x86/include/asm/kvm_host.h |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> Hi,
>>> I found that either KVM or QEMU (possibly both) are broken in respect
>>> to handling more CPUID entries than the limit dictates. KVM will
>>> return -E2BIG, which is the same error as if the user hasn't provided
>>> enough space to hold all entries. Now QEMU will continue to enlarge
>>> the allocated memory until it gets into an out-of-memory condition.
>>> I have tried to fix this with teaching KVM how to deal with a capped
>>> number of entries (there are some bugs in the current code), but this
>>> will limit the number of CPUID entries KVM handles, which will surely
>>> cut of the lastly appended PV leaves.
>>> A proper fix would be to make this allocation dynamic. Is this a
>>> feasible way or will this lead to issues or side-effects?
>>>
>>
>> Well, there has to be some limit, or userspace can allocate unbounded 
>> kernel memory.
> But this limit should not be a compile-time constant, but a runtime 
> one. The needed size depends on the host CPU (plus the KVM PV leaves) 
> and thus could be determined once for all VMs and vCPUs at module 
> load-time. But then we cannot use the static array allocation we 
> currently have in struct kvm_vcpu_arch:
>     struct kvm_cpuid_entry2 cpuid_entries[KVM_MAX_CPUID_ENTRIES];
> So we would use a kind-of dynamic allocation bounded by the host CPU's 
> need. But for the code is does not make much difference to a "real" 
> dynamic allocation.
>
> Also we could implement kvm_dev_ioctl_get_supported_cpuid without the 
> vmalloc, if we don't care about some dozens of copy_to_user() calls in 
> this function. Then we would not need this limit in 
> GET_SUPPORTED_CPUID at all, but it will strike us again at 
> KVM_SET_CPUID[2], where we may not fulfill the promises we gave earlier.
> Having said this, what about that:
> kvm_dev_ioctl_get_supported_cpuid is invariant to the VM or vCPU (as 
> it is used by a system ioctl), so it could be run once at 
> initialization, which would limit the ioctl implementation to a plain 
> bounded copy.
> Would you want such a patch (removing the vmalloc and maybe even the 
> limit)?

Making GET_SUPPORTED_CPUID data static would be an improvement, yes.

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

[Qemu-devel] Re: [PATCH] kvm/x86: enlarge number of possible CPUID leaves

[Andre Przywara]

Avi, Marcello,

can you please commit this simple fix? (turning 40 to 80?)
Without it QEMU crashes reliably on our new CPUs (they return 46 leaves) 
and causes pain in our testing, because we have to manually apply this 
patch on each tree.

Thanks!
Andre.

> Currently the number of CPUID leaves KVM handles is limited to 40.
> My desktop machine (AthlonII) already has 35 and future CPUs will
> expand this well beyond the limit. Extend the limit to 80 to make
> room for future processors.
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> ---
>  arch/x86/include/asm/kvm_host.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> Hi,
> I found that either KVM or QEMU (possibly both) are broken in respect
> to handling more CPUID entries than the limit dictates. KVM will
> return -E2BIG, which is the same error as if the user hasn't provided
> enough space to hold all entries. Now QEMU will continue to enlarge
> the allocated memory until it gets into an out-of-memory condition.
> I have tried to fix this with teaching KVM how to deal with a capped
> number of entries (there are some bugs in the current code), but this
> will limit the number of CPUID entries KVM handles, which will surely
> cut of the lastly appended PV leaves.
> A proper fix would be to make this allocation dynamic. Is this a
> feasible way or will this lead to issues or side-effects?
> 
> Regards,
> Andre.
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 54e42c8..3cc80c4 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -79,7 +79,7 @@
>  #define KVM_NUM_MMU_PAGES (1 << KVM_MMU_HASH_SHIFT)
>  #define KVM_MIN_FREE_MMU_PAGES 5
>  #define KVM_REFILL_PAGES 25
> -#define KVM_MAX_CPUID_ENTRIES 40
> +#define KVM_MAX_CPUID_ENTRIES 80
>  #define KVM_NR_FIXED_MTRR_REGION 88
>  #define KVM_NR_VAR_MTRR 8
>  

[Qemu-devel] Re: [PATCH] kvm/x86: enlarge number of possible CPUID leaves

[Avi Kivity]

On 12/08/2010 01:13 PM, Andre Przywara wrote:
> Avi, Marcello,
>
> can you please commit this simple fix? (turning 40 to 80?)
> Without it QEMU crashes reliably on our new CPUs (they return 46 
> leaves) and causes pain in our testing, because we have to manually 
> apply this patch on each tree.

Sorry about that, applied now.

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