| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <36b8df1d-593e-44c0-b34d-eb158e5ebabe@huawei.com>
Date: Tue, 21 May 2024 20:33:59 +0800
From: zhoushuling <zhoushuling@...wei.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: <pbonzini@...hat.com>, <weiqi4@...wei.com>, <wanpengli@...cent.com>,
<kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] KVM: LAPIC: Fix an inversion error when a negative value
assigned to lapic_timer.timer_advance_ns
> On Mon, May 20, 2024, zhoushuling@...wei.com wrote:
>> From: Shuling Zhou <zhoushuling@...wei.com>
>>
>> After 'commit 0e6edceb8f18 ("KVM: LAPIC: Fix lapic_timer_advance_ns
>> parameter overflow")',a negative value can be assigned to
>> lapic_timer_advance_ns, when it is '-1', the kvm_create_lapic()
>> will judge it and turns on adaptive tuning of timer advancement.
>> However, when lapic_timer_advance_ns=-2, it will be assigned to
>> an uint variable apic->lapic_timer.timer_advance_ns, the
>> apic->lapic_timer.timer_advance_ns of each vCPU will become a huge
>> value. When a VM is started, the VM is stuck in the
>> "
>> [ 2.669717] ACPI: Core revision 20130517
>> [ 2.672378] ACPI: All ACPI Tables successfully acquired
>> [ 2.673309] ftrace: allocating 29651 entries in 116 pages
>> [ 2.698797] Enabling x2apic
>> [ 2.699431] Enabled x2apic
>> [ 2.700160] Switched APIC routing to physical x2apic.
>> [ 2.701644] ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
>> [ 2.702575] smpboot: CPU0: Intel(R) Xeon(R) Platinum 8378A CPU @ 3.00GHz (fam: 06, model: 6a, stepping: 06)
>> ..........
>> "
>>
>> 'Fixes: 0e6edceb8f18 ("KVM: LAPIC: Fix lapic_timer_advance_ns
>> parameter overflow")'
> Fixes: 0e6edceb8f18 ("KVM: LAPIC: Fix lapic_timer_advance_ns parameter overflow")
>
>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>> Cc: Sean Christopherson <sean.j.christopherson@...el.com>
>> Cc: Wanpeng Li <wanpengli@...cent.com>
>> Signed-off-by: Shuling Zhou<zhoushuling@...wei.com>
> There should be whitespace between your name and email.
>
>> ---
>> arch/x86/kvm/lapic.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index ebf41023be38..5feeb889ddb6 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2848,7 +2848,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
>> if (timer_advance_ns == -1) {
>> apic->lapic_timer.timer_advance_ns = LAPIC_TIMER_ADVANCE_NS_INIT;
>> lapic_timer_advance_dynamic = true;
>> - } else {
>> + } else if (timer_advance_ns >= 0) {
>> apic->lapic_timer.timer_advance_ns = timer_advance_ns;
>> lapic_timer_advance_dynamic = false;
>> }
> Wouldn't it be more appropriate to treat any negative value as "dynamic"? The
> comment above the module param also needs to be updated.
Thank for your suggestions.
I agree with "treat any negative value as dynamic".
>
> Oof, and lapic_timer_advance_dynamic is a global, which yields behavior that is
> nearly impossible to document. So I think we want this, over two patches?
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index ebf41023be38..3a1bcfbe3e93 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -59,7 +59,6 @@
> #define MAX_APIC_VECTOR 256
> #define APIC_VECTORS_PER_REG 32
>
> -static bool lapic_timer_advance_dynamic __read_mostly;
> #define LAPIC_TIMER_ADVANCE_ADJUST_MIN 100 /* clock cycles */
> #define LAPIC_TIMER_ADVANCE_ADJUST_MAX 10000 /* clock cycles */
> #define LAPIC_TIMER_ADVANCE_NS_INIT 1000
> @@ -1854,7 +1853,7 @@ static void __kvm_wait_lapic_expire(struct kvm_vcpu *vcpu)
> guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
> trace_kvm_wait_lapic_expire(vcpu->vcpu_id, guest_tsc - tsc_deadline);
>
> - if (lapic_timer_advance_dynamic) {
> + if (apic->lapic_timer.timer_advance_dynamic) {
> adjust_lapic_timer_advance(vcpu, guest_tsc - tsc_deadline);
> /*
> * If the timer fired early, reread the TSC to account for the
> @@ -2845,12 +2844,12 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
> hrtimer_init(&apic->lapic_timer.timer, CLOCK_MONOTONIC,
> HRTIMER_MODE_ABS_HARD);
> apic->lapic_timer.timer.function = apic_timer_fn;
> - if (timer_advance_ns == -1) {
> + if (timer_advance_ns < 0) {
> apic->lapic_timer.timer_advance_ns = LAPIC_TIMER_ADVANCE_NS_INIT;
> - lapic_timer_advance_dynamic = true;
> + apic->lapic_timer.timer_advance_dynamic = true;
> } else {
> apic->lapic_timer.timer_advance_ns = timer_advance_ns;
> - lapic_timer_advance_dynamic = false;
> + apic->lapic_timer.timer_advance_dynamic = false;
> }
>
> /*
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 0a0ea4b5dd8c..6fb3b16a2754 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -54,6 +54,7 @@ struct kvm_timer {
> u32 timer_advance_ns;
> atomic_t pending; /* accumulated triggered timers */
> bool hv_timer_in_use;
> + bool timer_advance_dynamic;
> };
However，I do not understand why the global function switch
'lapic_timer_advance_dynamic'
is changed to a local variable in the 'struct kvm_timer'.On a host, the
adaptive tuning
of timer advancement is global function, and
each vcpu->apic->lapic_timer.timer_advance_dynamic
of each VM is the same, different VMs cannot be configured with
different switches.
Is it better to keep the current implementation of the global variable
'lapic_timer_advance_dynamic'?
and just modify like this:
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index ebf41023be38..75469e329b23 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2845,7 +2845,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int
timer_advance_ns)
hrtimer_init(&apic->lapic_timer.timer, CLOCK_MONOTONIC,
HRTIMER_MODE_ABS_HARD);
apic->lapic_timer.timer.function = apic_timer_fn;
- if (timer_advance_ns == -1) {
+ if (timer_advance_ns < 0) {
apic->lapic_timer.timer_advance_ns = LAPIC_TIMER_ADVANCE_NS_INIT;
lapic_timer_advance_dynamic = true;
} else {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 082ac6d95a3a..071342d56ba8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -165,10 +165,11 @@ static u32 __read_mostly tsc_tolerance_ppm = 250;
module_param(tsc_tolerance_ppm, uint, 0644);
/*
- * lapic timer advance (tscdeadline mode only) in nanoseconds. '-1' enables
- * adaptive tuning starting from default advancement of 1000ns. '0'
disables
- * advancement entirely. Any other value is used as-is and disables
adaptive
- * tuning, i.e. allows privileged userspace to set an exact advancement
time.
+ * lapic timer advance (tscdeadline mode only) in nanoseconds. Any negative
+ * value enable adaptive tuning starting from default advancement of
1000ns.
+ * '0' disables advancement entirely. Any postive value is used as-is and
+ * disables adaptive tuning, i.e. allows privileged userspace to set an
exact
+ * advancement time.
*/
static int __read_mostly lapic_timer_advance_ns = -1;
module_param(lapic_timer_advance_ns, int, 0644);
Thanks.
>
> struct kvm_lapic {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a2b62169e09a..60e86607056e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -165,10 +165,11 @@ static u32 __read_mostly tsc_tolerance_ppm = 250;
> module_param(tsc_tolerance_ppm, uint, 0644);
>
> /*
> - * lapic timer advance (tscdeadline mode only) in nanoseconds. '-1' enables
> - * adaptive tuning starting from default advancement of 1000ns. '0' disables
> - * advancement entirely. Any other value is used as-is and disables adaptive
> - * tuning, i.e. allows privileged userspace to set an exact advancement time.
> + * lapic timer advance (tscdeadline mode only) in nanoseconds. Any negative
> + * value enable adaptive tuning starting from default advancement of 1000ns.
> + * '0' disables advancement entirely. Any postive value is used as-is and
> + * disables adaptive tuning, i.e. allows privileged userspace to set an exact
> + * advancement time.
> */
> static int __read_mostly lapic_timer_advance_ns = -1;
> module_param(lapic_timer_advance_ns, int, 0644);
>
Powered by blists - more mailing lists