On 03/07/2007 01:21 PM, Thomas Gleixner wrote:
> On Wed, 2007-03-07 at 11:49 -0800, Dan Hecht wrote:
>> Jeremy, I saw you sent out the Xen version earlier, thanks. Here's ours
>> for reference (please excuse any formating issues); it's also lean.
>> We'll send out a proper patch later after some more testing:
>
> Ah. Bitching loud enough speeds things up. :)
>
We've always planned to do this. We just didn't want to create the
dependency between paravirt_ops and clockevents too early such that they
would depend on each other to merge to main line. Now that they are
both there, we are all for it.
>> /** vmi clockevent */
>>
>> static struct clock_event_device vmi_global_clockevent;
>>
>> static inline u32 vmi_alarm_wiring(struct clock_event_device *evt)
>> {
>> return (evt == &vmi_global_clockevent) ?
>> VMI_ALARM_WIRED_IRQ0 : VMI_ALARM_WIRED_LVTT;
>> }
>>
>> static void vmi_timer_set_mode(enum clock_event_mode mode,
>> struct clock_event_device *evt)
>> {
>> u32 wiring;
>> cycle_t now, cycles_per_hz;
>> BUG_ON(!irqs_disabled());
>>
>> wiring = vmi_alarm_wiring(evt);
>> if (wiring == VMI_ALARM_WIRED_LVTT)
>> /* Route the interrupt to the correct vector */
>> apic_write_around(APIC_LVTT, LOCAL_TIMER_VECTOR);
>
> Wire that in the hypervisor.
>
>> switch (mode) {
>> case CLOCK_EVT_MODE_ONESHOT:
>> break;
>> case CLOCK_EVT_MODE_PERIODIC:
>> cycles_per_hz = vmi_timer_ops.get_cycle_frequency();
>> (void)do_div(cycles_per_hz, HZ);
>> now =
>> vmi_timer_ops.get_cycle_counter(vmi_counter(VMI_PERIODIC));
>> vmi_timer_ops.set_alarm(wiring | VMI_PERIODIC,
>> now, cycles_per_hz);
>
> paravirt_ops->paravirt_clockevent->set_periodic(vcpu, period);
>
Huh? paravirt_ops isn't a hypervisor interface, it's just a linux code
abstraction. The code on both sides of paravirt_ops is *linux* code,
any way you cut it. clockevents is already a linux code abstraction.
why introduce the redundancy?
>> break;
>> case CLOCK_EVT_MODE_UNUSED:
>> case CLOCK_EVT_MODE_SHUTDOWN:
>
> paravirt_ops->paravirt_clockevent->stop_event(vcpu, mode);
>
You would be introducing the same redundancy.
>
>> switch (evt->mode) {
>> case CLOCK_EVT_MODE_ONESHOT:
>> vmi_timer_ops.cancel_alarm(VMI_ONESHOT);
>> break;
>> case CLOCK_EVT_MODE_PERIODIC:
>> vmi_timer_ops.cancel_alarm(VMI_PERIODIC);
>> break;
>> default:
>> break;
>> }
>> break;
>> default:
>> break;
>> }
>> }
>
> This whole vmi_timer_ops thing is horrible. All hypervisors can share
> paravirt_ops->paravirt_clockevent and retrieve the methods on boot.
>
vmi_timer_ops.whatever is where the kernel <-> hypervisor boundary is
crossed for VMI.
>> static int vmi_timer_next_event(unsigned long delta,
>> struct clock_event_device *evt)
>> {
>> /* Unfortunately, set_next_event interface only passes relative
>> * expiry, but we want absolute expiry. It'd be better if were
>> * were passed an aboslute expiry, since a bunch of time may
>> * have been stolen between the time the delta is computed and
>> * when we set the alarm below. */
>> cycle_t now = vmi_timer_ops.get_cycle_counter(vmi_counter(VMI_ONESHOT));
>>
>> BUG_ON(evt->mode != CLOCK_EVT_MODE_ONESHOT);
>> vmi_timer_ops.set_alarm(vmi_alarm_wiring(evt) | VMI_ONESHOT,
>> now + delta, 0);
>> return 0;
>> }
>
> Great. Now we have:
>
> s64 event = startup_offset + ktime_to_ns(evt->next_event);
>
> if (HYPERVISOR_set_timer_op(event) < 0)
> BUG();
> and
>
> vmi_timer_ops.set_alarm(vmi_alarm_wiring(evt) | VMI_ONESHOT, now +
> delta, 0);
>
> How will the next implementations look like ?
>
> lguest_program_timer(delta + lguest_current_time(),
> LGUEST_TIMER_SHOOT_ONCE);
>
> virt_nextgen_ops.set_timer_event(delta, NO_WE_NEED_NO_FLAGS);
>
> .......
>
> This is tinkering of the best. My understanding of the paravirt
> discussion at Kernel Summit was, that paravirt ops are exactly there to
> prevent the above random hackery in the kernel and to allow _ALL_
> hypervisors to interact via a sane interface inside of the kernel.
>
No, that was not the point of paravirt_ops. It is actually the complete
opposite of the intention of paravirt_ops. paravirt_ops' intent is
exactly to allow for *multiple* hypervisor ABIs to exist in the kernel.
At kernel summit, paravirt_ops was proposed to allow for multiple
hypervisor ABI's to be targeted by the kernel. The code on both sides
of paravirt_ops is *linux* code.
> You are just perverting the whole idea of a standartized
> paravirtualization interface.
>
> This things can be done for clocksources, clockevents, interrupts (the
> generic irq code allows this) and probaly for a whole bunch of other
> stuff.
>
> The current paravirt interface is completely insane and will explode
> into an unmaintainable nightmare within no time, if we keep accepting
> that crap further.
>
> No thanks.
>
Again, you are misunderstanding the intent of paravirt_ops and history
behind it's development.
>> #ifdef CONFIG_X86_LOCAL_APIC
>>
>> /* Replacement for lapic timer local clock event.
>> * paravirt_ops.setup_boot_clock = vmi_nop
>> * (continue using global_clock_event on cpu0)
>> * paravirt_ops.setup_secondary_clock = vmi_timer_setup_local_alarm
>> */
>> void __devinit vmi_timer_setup_local_alarm(void)
>> {
>> struct clock_event_device *evt = &__get_cpu_var(local_clock_events);
>>
>> /* Then, start it back up as a local clockevent device. */
>> memcpy(evt, &vmi_clockevent, sizeof(*evt));
>> evt->cpumask = cpumask_of_cpu(smp_processor_id());
>>
>> printk(KERN_WARNING "vmi: registering clock event %s. mult=%lu
>> shift=%u\n",
>> evt->name, evt->mult, evt->shift);
>> clockevents_register_device(evt);
>> }
>>
>> #endif
>
> Why the hell do you need an lapic emulator here? This is exactly the
> kind of crap, we do not want to have. clockevents do not care which
> piece of hardware is calling them and we do not care how a particular
> hypervisor is wiring that hardware.
>
Again, I said in a previous mail that we am fine with introducing our
own interrupt handler rather than using the lapic one.
Dan
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/virtualization
