linux-kernel - Re: [PATCH 4/4] kvm: x86: export TSC offset to user-space

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b1b2db6c-e506-bd9c-31ec-d9d88c12fe9d@redhat.com>
Date:   Mon, 5 Sep 2016 10:10:39 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Luiz Capitulino <lcapitulino@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        rkrcmar@...hat.com, rostedt@...dmis.org, mhiramat@...nel.org,
        mtosatti@...hat.com
Subject: Re: [PATCH 4/4] kvm: x86: export TSC offset to user-space



On 02/09/2016 19:31, Luiz Capitulino wrote:
> On Fri, 2 Sep 2016 19:00:41 +0200
> Paolo Bonzini <pbonzini@...hat.com> wrote:
> 
>> On 31/08/2016 19:05, Luiz Capitulino wrote:
>>>   vcpu0: 18446742405270834952
>>>   vcpu1: 18446742405270834952
>>>   vcpu2: 18446742405270834952
>>>   vcpu3: 18446742405270834952
>>>
>>>  - We'll probably need to export the TSC multiplier too.
>>>    However, I've been using only the TSC offset for now.
>>>    So, let's get this merged first and do the TSC multiplier
>>>    as a second step  
>>
>> You'll need to export the number of fractional bits in the multiplier,
>> too.  It's going to be a very simple patch, so please do everything now.
> 
> I didn't want to expose the multiplier before testing our tracing
> procedure with it.

Exposing the multiplier should be independent of tracing.  I can test it
for you.

Paolo

> So far we've been only using the TSC offset (and
> it works great). I don't even know if I have a machine around to
> test it, so it could take a bit.
> 
>> arch/x86/kvm/x86.c is huge; please create a new file arch/x86/kvm/debugfs.c.
> 
> Will do.
> 
>>> Signed-off-by: Luiz Capitulino <lcapitulino@...hat.com>
>>> ---
>>>  arch/x86/include/asm/kvm_host.h |  1 +
>>>  arch/x86/kvm/svm.c              |  1 +
>>>  arch/x86/kvm/vmx.c              |  8 ++++++++
>>>  arch/x86/kvm/x86.c              | 30 ++++++++++++++++++++++++++++++
>>>  4 files changed, 40 insertions(+)
>>>
>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>> index 33ae3a4..5714bbd 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>> @@ -952,6 +952,7 @@ struct kvm_x86_ops {
>>>  	bool (*has_wbinvd_exit)(void);
>>>  
>>>  	u64 (*read_tsc_offset)(struct kvm_vcpu *vcpu);
>>> +	u64 (*read_cached_tsc_offset)(struct kvm_vcpu *vcpu);
>>>  	void (*write_tsc_offset)(struct kvm_vcpu *vcpu, u64 offset);
>>>  
>>>  	u64 (*read_l1_tsc)(struct kvm_vcpu *vcpu, u64 host_tsc);
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index af523d8..c851477 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -5065,6 +5065,7 @@ static struct kvm_x86_ops svm_x86_ops = {
>>>  	.has_wbinvd_exit = svm_has_wbinvd_exit,
>>>  
>>>  	.read_tsc_offset = svm_read_tsc_offset,
>>> +	.read_cached_tsc_offset = svm_read_tsc_offset,
>>>  	.write_tsc_offset = svm_write_tsc_offset,
>>>  	.adjust_tsc_offset_guest = svm_adjust_tsc_offset_guest,
>>>  	.read_l1_tsc = svm_read_l1_tsc,
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 5cede40..82dfe42 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -616,6 +616,7 @@ struct vcpu_vmx {
>>>  	u64 hv_deadline_tsc;
>>>  
>>>  	u64 current_tsc_ratio;
>>> +	u64 cached_tsc_offset;
>>>  
>>>  	bool guest_pkru_valid;
>>>  	u32 guest_pkru;
>>> @@ -2608,6 +2609,11 @@ static u64 vmx_read_tsc_offset(struct kvm_vcpu *vcpu)
>>>  	return vmcs_read64(TSC_OFFSET);
>>>  }
>>>  
>>> +static u64 vmx_read_cached_tsc_offset(struct kvm_vcpu *vcpu)
>>> +{
>>> +	return to_vmx(vcpu)->cached_tsc_offset;
>>> +}
>>> +
>>>  /*
>>>   * writes 'offset' into guest's timestamp counter offset register
>>>   */
>>> @@ -2632,6 +2638,7 @@ static void vmx_write_tsc_offset(struct kvm_vcpu *vcpu, u64 offset)
>>>  					   vmcs_read64(TSC_OFFSET), offset);
>>>  		vmcs_write64(TSC_OFFSET, offset);
>>>  	}
>>> +	to_vmx(vcpu)->cached_tsc_offset = offset;
>>>  }
>>>  
>>>  static void vmx_adjust_tsc_offset_guest(struct kvm_vcpu *vcpu, s64 adjustment)
>>> @@ -11275,6 +11282,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
>>>  	.has_wbinvd_exit = cpu_has_vmx_wbinvd_exit,
>>>  
>>>  	.read_tsc_offset = vmx_read_tsc_offset,
>>> +	.read_cached_tsc_offset = vmx_read_cached_tsc_offset,
>>>  	.write_tsc_offset = vmx_write_tsc_offset,
>>>  	.adjust_tsc_offset_guest = vmx_adjust_tsc_offset_guest,
>>>  	.read_l1_tsc = vmx_read_l1_tsc,  
>>
>> You need to handle SVM as well.  So you might as well simplify the code:
> 
> SVM is handled:
> 
> 	+	.read_cached_tsc_offset = svm_read_tsc_offset,
> 
>> - add a kvm_vcpu_write_tsc_offset wrapper for kvm_x86_ops->write_tsc_offset
>>
>> - add a tsc_offset field in struct kvm_vcpu_arch
>>
>> - replace kvm_x86_ops->read_tsc_offset with accesses to the new field
> 
> Given that SVM is handled, you still want me to do this?
> 
>> Then in a fifth patch export the TSC offset (and multiplier ;)) to
>> userspace.
>>
>> I'm not very happy about having a single file for all TSC offsets.
>> Creating subdirectories under the PID-FD per-VM directory would be nicer
>> in the long run.
> 
> I think Steven would also prefer that, but some people raised the
> concern at KVM Forum that creating per vcpu dirs in debugfs may
> consume considerable memory for a system running several dozen
> if not hundreds of VMs. This concern seems valid to me, but I
> can do either way.
>