* [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized
@ 2020-01-31 15:56 Thadeu Lima de Souza Cascardo
2020-02-03 9:59 ` Vitaly Kuznetsov
2020-02-05 14:28 ` Paolo Bonzini
0 siblings, 2 replies; 6+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2020-01-31 15:56 UTC (permalink / raw)
To: kvm
Cc: pbonzini, sean.j.christopherson, wanpengli, vkuznets, jmattson,
joro, tglx, mingo, bp, hpa, x86, linux-kernel
kvm_setup_pv_tlb_flush will waste memory and print a misguiding message
when KVM paravirtualization is not available.
Intel SDM says that the when cpuid is used with EAX higher than the
maximum supported value for basic of extended function, the data for the
highest supported basic function will be returned.
So, in some systems, kvm_arch_para_features will return bogus data,
causing kvm_setup_pv_tlb_flush to detect support for pv tlb flush.
Testing for kvm_para_available will work as it checks for the hypervisor
signature.
Besides, when the "nopv" command line parameter is used, it should not
continue as well, as kvm_guest_init will no be called in that case.
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
arch/x86/kernel/kvm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 81045aabb6f4..d817f255aed8 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -736,6 +736,9 @@ static __init int kvm_setup_pv_tlb_flush(void)
{
int cpu;
+ if (!kvm_para_available() || nopv)
+ return 0;
+
if (kvm_para_has_feature(KVM_FEATURE_PV_TLB_FLUSH) &&
!kvm_para_has_hint(KVM_HINTS_REALTIME) &&
kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized
2020-01-31 15:56 [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized Thadeu Lima de Souza Cascardo
@ 2020-02-03 9:59 ` Vitaly Kuznetsov
2020-02-03 10:15 ` Thadeu Lima de Souza Cascardo
2020-02-05 14:28 ` Paolo Bonzini
1 sibling, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-03 9:59 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo
Cc: pbonzini, sean.j.christopherson, wanpengli, jmattson, joro, tglx,
mingo, bp, hpa, x86, linux-kernel, kvm
Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
> kvm_setup_pv_tlb_flush will waste memory and print a misguiding message
> when KVM paravirtualization is not available.
>
> Intel SDM says that the when cpuid is used with EAX higher than the
> maximum supported value for basic of extended function, the data for the
> highest supported basic function will be returned.
>
> So, in some systems, kvm_arch_para_features will return bogus data,
> causing kvm_setup_pv_tlb_flush to detect support for pv tlb flush.
>
> Testing for kvm_para_available will work as it checks for the hypervisor
> signature.
>
> Besides, when the "nopv" command line parameter is used, it should not
> continue as well, as kvm_guest_init will no be called in that case.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
> arch/x86/kernel/kvm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 81045aabb6f4..d817f255aed8 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -736,6 +736,9 @@ static __init int kvm_setup_pv_tlb_flush(void)
> {
> int cpu;
>
> + if (!kvm_para_available() || nopv)
> + return 0;
> +
> if (kvm_para_has_feature(KVM_FEATURE_PV_TLB_FLUSH) &&
> !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
> kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
The patch will fix the immediate issue, but why kvm_setup_pv_tlb_flush()
is just an arch_initcall() which will be executed regardless of the fact
if we are running on KVM or not?
In Hyper-V we setup PV TLB flush from ms_hyperv_init_platform() -- which
only happens if Hyper-V platform was detected. Why don't we do it from
kvm_init_platform() in KVM?
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized
2020-02-03 9:59 ` Vitaly Kuznetsov
@ 2020-02-03 10:15 ` Thadeu Lima de Souza Cascardo
2020-02-03 12:25 ` Wanpeng Li
2020-02-03 12:42 ` Vitaly Kuznetsov
0 siblings, 2 replies; 6+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2020-02-03 10:15 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: pbonzini, sean.j.christopherson, wanpengli, jmattson, joro, tglx,
mingo, bp, hpa, x86, linux-kernel, kvm
On Mon, Feb 03, 2020 at 10:59:10AM +0100, Vitaly Kuznetsov wrote:
> Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
>
> > kvm_setup_pv_tlb_flush will waste memory and print a misguiding message
> > when KVM paravirtualization is not available.
> >
> > Intel SDM says that the when cpuid is used with EAX higher than the
> > maximum supported value for basic of extended function, the data for the
> > highest supported basic function will be returned.
> >
> > So, in some systems, kvm_arch_para_features will return bogus data,
> > causing kvm_setup_pv_tlb_flush to detect support for pv tlb flush.
> >
> > Testing for kvm_para_available will work as it checks for the hypervisor
> > signature.
> >
> > Besides, when the "nopv" command line parameter is used, it should not
> > continue as well, as kvm_guest_init will no be called in that case.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > ---
> > arch/x86/kernel/kvm.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> > index 81045aabb6f4..d817f255aed8 100644
> > --- a/arch/x86/kernel/kvm.c
> > +++ b/arch/x86/kernel/kvm.c
> > @@ -736,6 +736,9 @@ static __init int kvm_setup_pv_tlb_flush(void)
> > {
> > int cpu;
> >
> > + if (!kvm_para_available() || nopv)
> > + return 0;
> > +
> > if (kvm_para_has_feature(KVM_FEATURE_PV_TLB_FLUSH) &&
> > !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
> > kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
>
> The patch will fix the immediate issue, but why kvm_setup_pv_tlb_flush()
> is just an arch_initcall() which will be executed regardless of the fact
> if we are running on KVM or not?
>
> In Hyper-V we setup PV TLB flush from ms_hyperv_init_platform() -- which
> only happens if Hyper-V platform was detected. Why don't we do it from
> kvm_init_platform() in KVM?
>
> --
> Vitaly
>
Because we can't call the allocator that early.
Also, see the thread where this was "decided", the v6 of the original patch:
https://lore.kernel.org/kvm/20171129162118.GA10661@flask/
Regards.
Cascardo.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized
2020-02-03 10:15 ` Thadeu Lima de Souza Cascardo
@ 2020-02-03 12:25 ` Wanpeng Li
2020-02-03 12:42 ` Vitaly Kuznetsov
1 sibling, 0 replies; 6+ messages in thread
From: Wanpeng Li @ 2020-02-03 12:25 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo
Cc: Vitaly Kuznetsov, Paolo Bonzini, Sean Christopherson, Wanpeng Li,
Jim Mattson, Joerg Roedel, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, LKML,
kvm
On Mon, 3 Feb 2020 at 18:31, Thadeu Lima de Souza Cascardo
<cascardo@canonical.com> wrote:
>
> On Mon, Feb 03, 2020 at 10:59:10AM +0100, Vitaly Kuznetsov wrote:
> > Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
> >
> > > kvm_setup_pv_tlb_flush will waste memory and print a misguiding message
> > > when KVM paravirtualization is not available.
> > >
> > > Intel SDM says that the when cpuid is used with EAX higher than the
> > > maximum supported value for basic of extended function, the data for the
> > > highest supported basic function will be returned.
> > >
> > > So, in some systems, kvm_arch_para_features will return bogus data,
> > > causing kvm_setup_pv_tlb_flush to detect support for pv tlb flush.
> > >
> > > Testing for kvm_para_available will work as it checks for the hypervisor
> > > signature.
> > >
> > > Besides, when the "nopv" command line parameter is used, it should not
> > > continue as well, as kvm_guest_init will no be called in that case.
> > >
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > ---
> > > arch/x86/kernel/kvm.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> > > index 81045aabb6f4..d817f255aed8 100644
> > > --- a/arch/x86/kernel/kvm.c
> > > +++ b/arch/x86/kernel/kvm.c
> > > @@ -736,6 +736,9 @@ static __init int kvm_setup_pv_tlb_flush(void)
> > > {
> > > int cpu;
> > >
> > > + if (!kvm_para_available() || nopv)
> > > + return 0;
> > > +
> > > if (kvm_para_has_feature(KVM_FEATURE_PV_TLB_FLUSH) &&
> > > !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
> > > kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
> >
> > The patch will fix the immediate issue, but why kvm_setup_pv_tlb_flush()
> > is just an arch_initcall() which will be executed regardless of the fact
> > if we are running on KVM or not?
> >
> > In Hyper-V we setup PV TLB flush from ms_hyperv_init_platform() -- which
> > only happens if Hyper-V platform was detected. Why don't we do it from
> > kvm_init_platform() in KVM?
> >
> > --
> > Vitaly
> >
>
> Because we can't call the allocator that early.
>
> Also, see the thread where this was "decided", the v6 of the original patch:
>
> https://lore.kernel.org/kvm/20171129162118.GA10661@flask/
A little change to this function.
https://lore.kernel.org/kvm/CANRm+CwK0Cg45mktda9Yz9fsjPCvtuB8O+fma5L3tV725ki1qw@mail.gmail.com
Testing is a great appreciated. (Still in vacation)
Wanpeng
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized
2020-02-03 10:15 ` Thadeu Lima de Souza Cascardo
2020-02-03 12:25 ` Wanpeng Li
@ 2020-02-03 12:42 ` Vitaly Kuznetsov
1 sibling, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-03 12:42 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo
Cc: pbonzini, sean.j.christopherson, wanpengli, jmattson, joro, tglx,
mingo, bp, hpa, x86, linux-kernel, kvm
Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
> On Mon, Feb 03, 2020 at 10:59:10AM +0100, Vitaly Kuznetsov wrote:
>> Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
>>
>> > kvm_setup_pv_tlb_flush will waste memory and print a misguiding message
>> > when KVM paravirtualization is not available.
>> >
>> > Intel SDM says that the when cpuid is used with EAX higher than the
>> > maximum supported value for basic of extended function, the data for the
>> > highest supported basic function will be returned.
>> >
>> > So, in some systems, kvm_arch_para_features will return bogus data,
>> > causing kvm_setup_pv_tlb_flush to detect support for pv tlb flush.
>> >
>> > Testing for kvm_para_available will work as it checks for the hypervisor
>> > signature.
>> >
>> > Besides, when the "nopv" command line parameter is used, it should not
>> > continue as well, as kvm_guest_init will no be called in that case.
>> >
>> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>> > ---
>> > arch/x86/kernel/kvm.c | 3 +++
>> > 1 file changed, 3 insertions(+)
>> >
>> > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> > index 81045aabb6f4..d817f255aed8 100644
>> > --- a/arch/x86/kernel/kvm.c
>> > +++ b/arch/x86/kernel/kvm.c
>> > @@ -736,6 +736,9 @@ static __init int kvm_setup_pv_tlb_flush(void)
>> > {
>> > int cpu;
>> >
>> > + if (!kvm_para_available() || nopv)
>> > + return 0;
>> > +
>> > if (kvm_para_has_feature(KVM_FEATURE_PV_TLB_FLUSH) &&
>> > !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
>> > kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
>>
>> The patch will fix the immediate issue, but why kvm_setup_pv_tlb_flush()
>> is just an arch_initcall() which will be executed regardless of the fact
>> if we are running on KVM or not?
>>
>> In Hyper-V we setup PV TLB flush from ms_hyperv_init_platform() -- which
>> only happens if Hyper-V platform was detected. Why don't we do it from
>> kvm_init_platform() in KVM?
>>
>> --
>> Vitaly
>>
>
> Because we can't call the allocator that early.
>
> Also, see the thread where this was "decided", the v6 of the original patch:
>
> https://lore.kernel.org/kvm/20171129162118.GA10661@flask/
Ok, I see, it's basically about what we prioritize: shorter boot time vs
smaller memory footprint. I'd personally vote for the former but the
opposite opinion is equally valid. Let's preserve the status quo.
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized
2020-01-31 15:56 [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized Thadeu Lima de Souza Cascardo
2020-02-03 9:59 ` Vitaly Kuznetsov
@ 2020-02-05 14:28 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-02-05 14:28 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo, kvm
Cc: sean.j.christopherson, wanpengli, vkuznets, jmattson, joro, tglx,
mingo, bp, hpa, x86, linux-kernel
On 31/01/20 16:56, Thadeu Lima de Souza Cascardo wrote:
> kvm_setup_pv_tlb_flush will waste memory and print a misguiding message
> when KVM paravirtualization is not available.
>
> Intel SDM says that the when cpuid is used with EAX higher than the
> maximum supported value for basic of extended function, the data for the
> highest supported basic function will be returned.
>
> So, in some systems, kvm_arch_para_features will return bogus data,
> causing kvm_setup_pv_tlb_flush to detect support for pv tlb flush.
>
> Testing for kvm_para_available will work as it checks for the hypervisor
> signature.
>
> Besides, when the "nopv" command line parameter is used, it should not
> continue as well, as kvm_guest_init will no be called in that case.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
> arch/x86/kernel/kvm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 81045aabb6f4..d817f255aed8 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -736,6 +736,9 @@ static __init int kvm_setup_pv_tlb_flush(void)
> {
> int cpu;
>
> + if (!kvm_para_available() || nopv)
> + return 0;
> +
> if (kvm_para_has_feature(KVM_FEATURE_PV_TLB_FLUSH) &&
> !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
> kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-05 14:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-31 15:56 [PATCH] x86/kvm: do not setup pv tlb flush when not paravirtualized Thadeu Lima de Souza Cascardo
2020-02-03 9:59 ` Vitaly Kuznetsov
2020-02-03 10:15 ` Thadeu Lima de Souza Cascardo
2020-02-03 12:25 ` Wanpeng Li
2020-02-03 12:42 ` Vitaly Kuznetsov
2020-02-05 14:28 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox