| 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: <4DF9F5E9.8090901@redhat.com>
Date: Thu, 16 Jun 2011 09:24:09 -0300
From: Glauber Costa <glommer@...hat.com>
To: Gleb Natapov <gleb@...hat.com>
CC: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Rik van Riel <riel@...hat.com>,
Jeremy Fitzhardinge <jeremy.fitzhardinge@...rix.com>,
Peter Zijlstra <peterz@...radead.org>,
Avi Kivity <avi@...hat.com>,
Anthony Liguori <aliguori@...ibm.com>,
Eric B Munson <emunson@...bm.net>
Subject: Re: [PATCH 3/7] KVM-HV: KVM Steal time implementation
On 06/16/2011 09:21 AM, Gleb Natapov wrote:
> On Thu, Jun 16, 2011 at 09:11:42AM -0300, Glauber Costa wrote:
>> On 06/16/2011 08:27 AM, Gleb Natapov wrote:
>>> On Thu, Jun 16, 2011 at 12:31:02AM -0300, Glauber Costa wrote:
>>>> On 06/15/2011 06:09 AM, Gleb Natapov wrote:
>>>>> On Wed, Jun 15, 2011 at 12:09:31AM -0300, Glauber Costa wrote:
>>>>>> On 06/14/2011 04:45 AM, Gleb Natapov wrote:
>>>>>>> On Mon, Jun 13, 2011 at 07:31:33PM -0400, Glauber Costa wrote:
>>>>>>>> To implement steal time, we need the hypervisor to pass the guest information
>>>>>>>> about how much time was spent running other processes outside the VM.
>>>>>>>> This is per-vcpu, and using the kvmclock structure for that is an abuse
>>>>>>>> we decided not to make.
>>>>>>>>
>>>>>>>> In this patchset, I am introducing a new msr, KVM_MSR_STEAL_TIME, that
>>>>>>>> holds the memory area address containing information about steal time
>>>>>>>>
>>>>>>>> This patch contains the hypervisor part for it. I am keeping it separate from
>>>>>>>> the headers to facilitate backports to people who wants to backport the kernel
>>>>>>>> part but not the hypervisor, or the other way around.
>>>>>>>>
>>>>>>>> Signed-off-by: Glauber Costa<glommer@...hat.com>
>>>>>>>> CC: Rik van Riel<riel@...hat.com>
>>>>>>>> CC: Jeremy Fitzhardinge<jeremy.fitzhardinge@...rix.com>
>>>>>>>> CC: Peter Zijlstra<peterz@...radead.org>
>>>>>>>> CC: Avi Kivity<avi@...hat.com>
>>>>>>>> CC: Anthony Liguori<aliguori@...ibm.com>
>>>>>>>> CC: Eric B Munson<emunson@...bm.net>
>>>>>>>> ---
>>>>>>>> arch/x86/include/asm/kvm_host.h | 8 +++++
>>>>>>>> arch/x86/include/asm/kvm_para.h | 4 ++
>>>>>>>> arch/x86/kvm/x86.c | 60 +++++++++++++++++++++++++++++++++++++--
>>>>>>>> 3 files changed, 69 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>>>>>>> index fc38eca..5dce014 100644
>>>>>>>> --- a/arch/x86/include/asm/kvm_host.h
>>>>>>>> +++ b/arch/x86/include/asm/kvm_host.h
>>>>>>>> @@ -388,6 +388,14 @@ struct kvm_vcpu_arch {
>>>>>>>> unsigned int hw_tsc_khz;
>>>>>>>> unsigned int time_offset;
>>>>>>>> struct page *time_page;
>>>>>>>> +
>>>>>>>> + struct {
>>>>>>>> + u64 msr_val;
>>>>>>>> + gpa_t stime;
>>>>>>>> + struct kvm_steal_time steal;
>>>>>>>> + u64 this_time_out;
>>>>>>>> + } st;
>>>>>>>> +
>>>>>>>> u64 last_guest_tsc;
>>>>>>>> u64 last_kernel_ns;
>>>>>>>> u64 last_tsc_nsec;
>>>>>>>> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
>>>>>>>> index ac306c4..0341e61 100644
>>>>>>>> --- a/arch/x86/include/asm/kvm_para.h
>>>>>>>> +++ b/arch/x86/include/asm/kvm_para.h
>>>>>>>> @@ -45,6 +45,10 @@ struct kvm_steal_time {
>>>>>>>> __u32 pad[6];
>>>>>>>> };
>>>>>>>>
>>>>>>>> +#define KVM_STEAL_ALIGNMENT_BITS 5
>>>>>>>> +#define KVM_STEAL_VALID_BITS ((-1ULL<< (KVM_STEAL_ALIGNMENT_BITS + 1)))
>>>>>>>> +#define KVM_STEAL_RESERVED_MASK (((1<< KVM_STEAL_ALIGNMENT_BITS) - 1 )<< 1)
>>>>>>>> +
>>>>>>>> #define KVM_MAX_MMU_OP_BATCH 32
>>>>>>>>
>>>>>>>> #define KVM_ASYNC_PF_ENABLED (1<< 0)
>>>>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>>>>>> index 6645634..10fe028 100644
>>>>>>>> --- a/arch/x86/kvm/x86.c
>>>>>>>> +++ b/arch/x86/kvm/x86.c
>>>>>>>> @@ -797,12 +797,12 @@ EXPORT_SYMBOL_GPL(kvm_get_dr);
>>>>>>>> * kvm-specific. Those are put in the beginning of the list.
>>>>>>>> */
>>>>>>>>
>>>>>>>> -#define KVM_SAVE_MSRS_BEGIN 8
>>>>>>>> +#define KVM_SAVE_MSRS_BEGIN 9
>>>>>>>> static u32 msrs_to_save[] = {
>>>>>>>> MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK,
>>>>>>>> MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW,
>>>>>>>> HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
>>>>>>>> - HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN,
>>>>>>>> + HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME,
>>>>>>>> MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP,
>>>>>>>> MSR_STAR,
>>>>>>>> #ifdef CONFIG_X86_64
>>>>>>>> @@ -1480,6 +1480,34 @@ static void kvmclock_reset(struct kvm_vcpu *vcpu)
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static void record_steal_time(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> + u64 delta;
>>>>>>>> +
>>>>>>>> + if (vcpu->arch.st.stime&& vcpu->arch.st.this_time_out) {
>>>>>>>> +
>>>>>>>> + if (unlikely(kvm_read_guest(vcpu->kvm, vcpu->arch.st.stime,
>>>>>>>> + &vcpu->arch.st.steal, sizeof(struct kvm_steal_time)))) {
>>>>>>>> +
>>>>>>>> + vcpu->arch.st.stime = 0;
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + delta = (get_kernel_ns() - vcpu->arch.st.this_time_out);
>>>>>>>> +
>>>>>>>> + vcpu->arch.st.steal.steal += delta;
>>>>>>>> + vcpu->arch.st.steal.version += 2;
>>>>>>>> +
>>>>>>>> + if (unlikely(kvm_write_guest(vcpu->kvm, vcpu->arch.st.stime,
>>>>>>> Why not use kvm_write_guest_cached() here and introduce kvm_read_guest_cached()
>>>>>>> for the read above?
>>>>>>
>>>>>> Actually, I'd expect most read/writes to benefit from caching, no?
>>>>>> So why don't we just rename kvm_write_guest_cached() to
>>>>>> kvm_write_guest(), and the few places - if any - that need to force
>>>>>> transversing of the gfn mappings, get renamed to
>>>>>> kvm_write_guest_uncached ?
>>>>>>
>>>>> Good idea. I do not see any places where kvm_write_guest_uncached is
>>>>> needed from a brief look. Avi?
>>>>>
>>>>>>>> + &vcpu->arch.st.steal, sizeof(struct kvm_steal_time)))) {
>>>>>>>> +
>>>>>>>> + vcpu->arch.st.stime = 0;
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>>>>>>> {
>>>>>>>> switch (msr) {
>>>>>>>> @@ -1562,6 +1590,23 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>>>>>>> if (kvm_pv_enable_async_pf(vcpu, data))
>>>>>>>> return 1;
>>>>>>>> break;
>>>>>>>> + case MSR_KVM_STEAL_TIME:
>>>>>>>> + vcpu->arch.st.msr_val = data;
>>>>>>>> +
>>>>>>>> + if (!(data& KVM_MSR_ENABLED)) {
>>>>>>>> + vcpu->arch.st.stime = 0;
>>>>>>>> + break;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + if (data& KVM_STEAL_RESERVED_MASK)
>>>>>>>> + return 1;
>>>>>>>> +
>>>>>>>> + vcpu->arch.st.this_time_out = get_kernel_ns();
>>>>>>>> + vcpu->arch.st.stime = data& KVM_STEAL_VALID_BITS;
>>>>>>>> + record_steal_time(vcpu);
>>>>>>>> +
>>>>>>>> + break;
>>>>>>>> +
>>>>>>>> case MSR_IA32_MCG_CTL:
>>>>>>>> case MSR_IA32_MCG_STATUS:
>>>>>>>> case MSR_IA32_MC0_CTL ... MSR_IA32_MC0_CTL + 4 * KVM_MAX_MCE_BANKS - 1:
>>>>>>>> @@ -1847,6 +1892,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>>>>>>>> case MSR_KVM_ASYNC_PF_EN:
>>>>>>>> data = vcpu->arch.apf.msr_val;
>>>>>>>> break;
>>>>>>>> + case MSR_KVM_STEAL_TIME:
>>>>>>>> + data = vcpu->arch.st.msr_val;
>>>>>>>> + break;
>>>>>>>> case MSR_IA32_P5_MC_ADDR:
>>>>>>>> case MSR_IA32_P5_MC_TYPE:
>>>>>>>> case MSR_IA32_MCG_CAP:
>>>>>>>> @@ -2158,6 +2206,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>>>>>>> kvm_migrate_timers(vcpu);
>>>>>>>> vcpu->cpu = cpu;
>>>>>>>> }
>>>>>>>> +
>>>>>>>> + record_steal_time(vcpu);
>>>>>>>> }
>>>>>>>>
>>>>>>>> void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>>>>>>> @@ -2165,6 +2215,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>>>>>>> kvm_x86_ops->vcpu_put(vcpu);
>>>>>>>> kvm_put_guest_fpu(vcpu);
>>>>>>>> kvm_get_msr(vcpu, MSR_IA32_TSC,&vcpu->arch.last_guest_tsc);
>>>>>>>> + vcpu->arch.st.this_time_out = get_kernel_ns();
>>>>>>>> }
>>>>>>>>
>>>>>>> Shouldn't we call record_steal_time(vcpu)/vcpu->arch.st.this_time_out = get_kernel_ns();
>>>>>>> just before/after entering/exiting a guest? vcpu_(put|get) are called
>>>>>>> for each vcpu ioctl, not only VCPU_RUN.
>>>>>> Sorry, missed that the first time I've read your e-mail.
>>>>>>
>>>>>> If done like you said, time spent on the hypervisor is accounted as
>>>>>> steal time. I don't think it is.
>>>>> I thought that this is the point of a steal time. Running other
>>>>> tasks/guests is a hypervisor overhead too after all :) Also what about
>>>>> time spend serving host interrupts between put/get? It will not be
>>>>> accounted as steal time, correct?
>>>>
>>>> This is mostly semantics. I like to compare this to a normal
>>>> process: There is a difference between time the OS spent on your
>>>> behalf, doing your system calls (sys), and time spent by other
>>>> processes. Similar thing here.
>>>>
>>> The problem with this approach is that things like doing "info cpus"
>>> in qemu monitor will change guest scheduling behaviour. Do we want it
>>> to be like that?
>>
>> You mean because it is running in a different thread, and will
>> compete for resources with the cpu thread ?
>>
> No, because it executes GET_REGS ioctl (in vcpu thread). The time
> it takes to execute it is accounted as time the hypervior spent on behave
> of a guest. Not a big deal if it is executed rarely. I tend to use "info
> cpus"/"info register" quite a lot when debugging and would preffer it to
> not affect guest behaviour.
>
>
>>>> Which put/get are you referring to specifically ? You mean
>>>> vcpu_put() vs vcpu_load() ?
>>>>
>>> Yes.
>>>
>>>> If they are after vcpu_put(), they will, because at this time your
>>>> process is officially out of the cpu.
>>>>
>>> And if they are between vcpu_load() and vcpu_put() they will be accounted as
>>> a work done on behalf of a guest although they are likely unrelated.
>> I think the best we can do around it here is record steal time /
>> measure time as late as we can. We could in theory subtract irq
>> time, but it sounds too complicated for little gain.
>>
> Recording steal time/measure time as close as possible to vmentry/vmexit
> is what I propose :) I agree about little gain part.
Well, I don't like it a priori, due to the reasons I've already stated.
If there would be a large noticeable gain, there could be a trade off.
But since you agree with the little gain, I'd prefer to keep it this way.
>>>>
>>>>>>
>>>>>> Steal time is time spent running someone else's job instead of
>>>>>> yours. The name for the time spent in the hypervisor doing something
>>>>>> for *you* is just overhead.
>>>>> OK. That is the question of a definition I guess. If you define it like
>>>>> that the code is correct.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> static int is_efer_nx(void)
>>>>>>>> @@ -2477,7 +2528,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>>>>>>>> (1<< KVM_FEATURE_NOP_IO_DELAY) |
>>>>>>>> (1<< KVM_FEATURE_CLOCKSOURCE2) |
>>>>>>>> (1<< KVM_FEATURE_ASYNC_PF) |
>>>>>>>> - (1<< KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
>>>>>>>> + (1<< KVM_FEATURE_CLOCKSOURCE_STABLE_BIT) |
>>>>>>>> + (1<< KVM_FEATURE_STEAL_TIME);
>>>>>>>> entry->ebx = 0;
>>>>>>>> entry->ecx = 0;
>>>>>>>> entry->edx = 0;
>>>>>>>> @@ -6200,6 +6252,8 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
>>>>>>>>
>>>>>>>> kvmclock_reset(vcpu);
>>>>>>>>
>>>>>>>> + vcpu->arch.st.stime = 0;
>>>>>>>> +
>>>>>>>> kvm_clear_async_pf_completion_queue(vcpu);
>>>>>>>> kvm_async_pf_hash_reset(vcpu);
>>>>>>>> vcpu->arch.apf.halted = false;
>>>>>>>> --
>>>>>>>> 1.7.3.4
>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>>>>>>> the body of a message to majordomo@...r.kernel.org
>>>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>>>
>>>>>>> --
>>>>>>> Gleb.
>>>>>
>>>>> --
>>>>> Gleb.
>>>
>>> --
>>> Gleb.
>
> --
> Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists