From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hollis Blanchard Subject: Re: [RFC] Paravirt timer for KVM Date: Fri, 12 Oct 2007 17:09:59 -0500 Message-ID: <1192226999.14891.124.camel@basalt> References: <5d6222a80710120908s6b1f5845head84e7b7a463cd1@mail.gmail.com> Reply-To: Hollis Blanchard Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel , Jeremy Fitzhardinge , Avi Kivity To: Glauber de Oliveira Costa Return-path: In-Reply-To: <5d6222a80710120908s6b1f5845head84e7b7a463cd1-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org On Fri, 2007-10-12 at 13:08 -0300, Glauber de Oliveira Costa wrote: > + > +/* The hypervisor will put information about time periodically here */ > +struct kvm_hv_clock hv_clock; ... > +void __init kvmclock_init(void) > +{ > + > + unsigned long shared_page = (unsigned long)&hv_clock; What makes you think this is page-aligned? > + /* > + * If we can't use the paravirt clock, just go with > + * the usual timekeeping > + */ > + if (!kvm_para_available() || no_kvmclock) > + return; > + > + if (kvm_hypercall1(KVM_HCALL_SET_SHARED_PAGE, shared_page)) > + return; Here you're passing a virtual address across the guest/host interface, which is something we've previously agreed is bad. > @@ -1406,6 +1408,7 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu) > } > EXPORT_SYMBOL_GPL(kvm_emulate_halt); > > +static struct kvm_hv_clock hv_clock; > int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > { > unsigned long nr, a0, a1, a2, a3, ret; > @@ -1426,7 +1429,32 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > a3 &= 0xFFFFFFFF; > } > > + ret = 0; > switch (nr) { > + case KVM_HCALL_SET_SHARED_PAGE: > + if (!irqchip_in_kernel(vcpu->kvm)) { > + ret = -1; > + break; > + } > + > + vcpu->kvm->clock_addr = a0; > + getnstimeofday(&hv_clock.wc); > + hv_clock.stable_tsc = check_tsc_unstable(); > + hv_clock.tsc_mult = clocksource_khz2mult(tsc_khz, 22); > + rdtscll(hv_clock.last_tsc); > + emulator_write_emulated(vcpu->kvm->clock_addr, &hv_clock, > + sizeof(hv_clock), vcpu); Are you intending this shared page to contain other data in the future, or is it just poorly named? > +struct kvm_hv_clock { > + int stable_tsc; /* use raw tsc for clock_read */ > + unsigned long tsc_mult; > + struct timespec now; > + u64 last_tsc; > + /* That's the wall clock, not the water closet */ > + struct timespec wc; > +}; This isn't even good for x86: you're using "long" in a binary interface! You're also using timespec, and what sort of binary compatibility guarantees would you like to make about that? -- Hollis Blanchard IBM Linux Technology Center ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/