From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Nat1V-0000nF-Tb for qemu-devel@nongnu.org; Fri, 29 Jan 2010 10:44:41 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Nat1R-0000m9-U6 for qemu-devel@nongnu.org; Fri, 29 Jan 2010 10:44:41 -0500 Received: from [199.232.76.173] (port=56479 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Nat1R-0000m2-NS for qemu-devel@nongnu.org; Fri, 29 Jan 2010 10:44:37 -0500 Received: from mail-iw0-f174.google.com ([209.85.223.174]:54248) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Nat1R-0004yY-EP for qemu-devel@nongnu.org; Fri, 29 Jan 2010 10:44:37 -0500 Received: by iwn4 with SMTP id 4so1202040iwn.18 for ; Fri, 29 Jan 2010 07:44:36 -0800 (PST) Message-ID: <4B630261.40307@codemonkey.ws> Date: Fri, 29 Jan 2010 09:44:33 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [RFC][PATCH] KVM: Introduce modification context for cpu_synchronize_state References: <4B605390.9020709@siemens.com> <4B62F5CD.2040709@codemonkey.ws> <4B62FDD0.4050904@siemens.com> In-Reply-To: <4B62FDD0.4050904@siemens.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Anthony Liguori , kvm , Gleb Natapov , Marcelo Tosatti , Alexander Graf , qemu-devel , Avi Kivity On 01/29/2010 09:25 AM, Jan Kiszka wrote: > Anthony Liguori wrote: > >> On 01/27/2010 08:54 AM, Jan Kiszka wrote: >> >>> This patch originates in the mp_state writeback issue: During runtime >>> and even on reset, we must not write the previously saved VCPU state >>> back into the kernel in an uncontrolled fashion. E.g mp_state should >>> only written on reset or on VCPU setup. Certain clocks (e.g. the TSC) >>> may only be written on setup or after vmload. >>> >>> By introducing additional information about the context of the planned >>> vcpu state manipulation, we can simply skip sensitive states like >>> mp_state when updating the in-kernel state. The planned modifications >>> are defined when calling cpu_synchronize_state. They accumulate, ie. >>> once a full writeback was requested, it will stick until it was >>> performed. >>> >>> This patch already fixes existing writeback issues in upstream KVM by >>> only selectively writing MSR_IA32_TSC, MSR_KVM_SYSTEM_TIME, >>> MSR_KVM_WALL_CLOCK, the mp_state and the vcpu_events. >>> >>> Signed-off-by: Jan Kiszka >>> >>> >> I think the context argument makes the function very difficult to call >> correctly. >> >> I'd suggest making CPU_MODIFY_RUNTIME the behaviour of >> cpu_synchronize_state. I'd suggest adding another function to handle >> init like cpu_init_state(). Likewise, if an explicit reset state is >> needed, I think a cpu_init_state_after_reset() makes sense. >> >> I don't quite understand the context that NONE should be used in. >> > 'context' was the wrong term, it should rather be 'scheduled vcpu state > modifications'. > > >> I think the key point though is to handle RUNTIME mostly transparently >> since it's the most common case. >> > This whole topic is complex and requires at least some cooperation from > the users of this API. Previous attempts to make this transparent caused > way too many bugs. E.g. the idea that writeback could simply be handled > on vcpu exec didn't fly, and qemu-kvm demonstrates the result (lots of > kvm hooks, fragile code). > > So I'm about to carefully remove some transparency. The key to this is > proper announcement of planned and/or performed changes (abstracted to > those three levels "runtime", "reset", and "init"). > > I will think about your suggestions. Maybe it makes sense to > (re-)introduce explicit writeback points as generic services, and we > should keep the common case as is (dropping my optimization > CPU_MODIFY_NONE). > I can understand the argument you're making but if you do decide to keep the context argument, then I'd strongly suggest adding a bucket full of documentation explaining when each state should be used. At the moment, it's not clear at first glance. Regards, Anthony Liguori > Jan > >