Re: [RFC] Paravirt timer for KVM

[Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

On Mon, 2007-10-15 at 14:48 -0300, Glauber de Oliveira Costa wrote:
> On 10/12/07, Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
> > > +       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.
> And that's the reason for the "nothing". It was just the easier way,
> and I'm not aware of any discussion in which it was agreed bad. I'll
> dig into the archives, but it would be a lot easier if you give me
> some pointers ;-)

Here's the original thread:
http://www.mail-archive.com/kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org/msg00755.html

The basic issue is that on many architectures, the hardware MMU does
*not* hold all mappings, and even if it does, performing MMU translation
in software can be prohibitively complicated and slow. As a worst-case
scenario, consider a software-controlled TLB which can hold a small
finite number of simultaneous mappings. On the embedded PowerPC 440
cores I'm working on, that number is 64. There is no hardware table
walker.

That means that if the hypervisor is passed a virtual address, it may
not be present in the MMU at all. Ordinarily the hardware would  deliver
a TLB miss interrupt to the kernel, and the kernel would insert the
appropriate mapping (as calculated or found in software-only data
structures). However, consider the locking nightmare if the hypervisor
delivers an interrupt while "inside" a guest hypercall...

> for the overall picture, it does not really make a difference, so if
> this is all that a bad idea, I can change it, no big deal.

Great! This is very important for portability to non-x86 architectures,
and a long-standing problem on another open source hypervisor, which is
why I'm eager to avoid it from the beginning here.

> > > +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?
> 
> Now seriously, the long is just my bad, but I'm not aware of any
> complications regarding the timespec. You are welcome to enlighten me.
> (Meanwhile, I'll go look at it)

typedef long		__kernel_time_t;
typedef __kernel_time_t		time_t;
struct timespec {
	time_t	tv_sec;		/* seconds */
	long	tv_nsec;	/* nanoseconds */
};

Longs everywhere. :) And not just that, but you are cementing this
particular layout of timespec into the guest ABI. There is nothing to
prevent Linux from changing timespec in some future version...

-- 
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/