From mboxrd@z Thu Jan 1 00:00:00 1970 From: Beth Kon Subject: Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7) Date: Fri, 19 Jun 2009 09:54:42 -0400 Message-ID: <4A3B98A2.2000002@us.ibm.com> References: <1245329246-17526-1-git-send-email-eak@us.ibm.com> <1245329246-17526-3-git-send-email-eak@us.ibm.com> <4A3A8FC0.1000606@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: avi@redhat.com, kvm@vger.kernel.org To: Jan Kiszka Return-path: Received: from e9.ny.us.ibm.com ([32.97.182.139]:35589 "EHLO e9.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752636AbZFSNxH (ORCPT ); Fri, 19 Jun 2009 09:53:07 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e9.ny.us.ibm.com (8.13.1/8.13.1) with ESMTP id n5JDemp1001443 for ; Fri, 19 Jun 2009 09:40:48 -0400 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n5JDr9cq243300 for ; Fri, 19 Jun 2009 09:53:09 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n5JDoofQ014262 for ; Fri, 19 Jun 2009 09:50:50 -0400 In-Reply-To: <4A3A8FC0.1000606@web.de> Sender: kvm-owner@vger.kernel.org List-ID: Jan Kiszka wrote: > Beth Kon wrote: > >> When kvm is in hpet_legacy_mode, the hpet is providing the >> timer interrupt and the pit should not be. So in legacy mode, the pit timer is >> destroyed, but the *state* of the pit is maintained. So if kvm or the guest >> tries to modify the state of the pit, this modification is accepted, *except* >> that the timer isn't actually started. When we exit hpet_legacy_mode, >> the current state of the pit (which is up to date since we've been >> accepting modifications) is used to restart the pit timer. >> >> The saved_mode code in kvm_pit_load_count temporarily changes mode to >> 0xff in order to destroy the timer, but then restores the actual value, >> again maintaining "current" state of the pit for possible later reenablement. >> >> changes from v6: >> >> - Added ioctl interface for legacy mode in order not to break the abi. >> >> >> Signed-off-by: Beth Kon >> > > ... > > >> @@ -1986,7 +1987,24 @@ static int kvm_vm_ioctl_set_pit(struct kvm *kvm, struct kvm_pit_state *ps) >> int r = 0; >> >> memcpy(&kvm->arch.vpit->pit_state, ps, sizeof(struct kvm_pit_state)); >> - kvm_pit_load_count(kvm, 0, ps->channels[0].count); >> + kvm_pit_load_count(kvm, 0, ps->channels[0].count, 0); >> + return r; >> +} >> + >> +static int kvm_vm_ioctl_get_hpet_legacy_mode(struct kvm *kvm, u8 *mode) >> +{ >> + int r = 0; >> + *mode = kvm->arch.vpit->pit_state.hpet_legacy_mode; >> + return r; >> +} >> > > This only applies if we go for a separate mode IOCTL: > The legacy mode is not directly modifiable by the guest. Is it planned > to add in-kernel hpet support? Otherwise get_hpet_legacy_mode looks a > bit like overkill given that user space could easily track the state. > Assuming I will at least generalize the ioctl, I'll leave this question for the time being. > >> + >> +static int kvm_vm_ioctl_set_hpet_legacy_mode(struct kvm *kvm, u8 *mode) >> +{ >> + int r = 0, start = 0; >> + if (kvm->arch.vpit->pit_state.hpet_legacy_mode == 0 && *mode == 1) >> > > Here you check more mode == 1, but legacy_mode is only checked for != 0. > I would make this consistent. > > ok >> + start = 1; >> + kvm->arch.vpit->pit_state.hpet_legacy_mode = *mode; >> + kvm_pit_load_count(kvm, 0, kvm->arch.vpit->pit_state.channels[0].count, start); >> return r; >> } >> >> @@ -2047,6 +2065,7 @@ long kvm_arch_vm_ioctl(struct file *filp, >> struct kvm_pit_state ps; >> struct kvm_memory_alias alias; >> struct kvm_pit_config pit_config; >> + u8 hpet_legacy_mode; >> > > Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not > add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And > that struct should also include some flags field and enough padding to > be potentially extended yet again in the future. In that case I see no > problem having also a mode read-back interface. > > I thought about that, but it seemed to add unnecessary complexity, since this legacy control is really outside of normal PIT operation, which is embodied by KVM_GET/SET_PIT. It might be worth making this ioctl more general. Rather than SET_HPET_LEGACY, have SET_PIT_CONTROLS and pass a bit field, one of which is HPET_LEGACY. But, if general consensus is that it would be better to create a kvm_pit_state2, and get/set that, I can do that. > Jan > >