From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dong, Eddie" Subject: Re: [PATCH 4/6] KVM: in-kernel LAPIC save and restore support Date: Sat, 4 Aug 2007 09:26:01 +0800 Message-ID: <10EA09EFD8728347A513008B6B0DA77A01E0088F@pdsmsx411.ccr.corp.intel.com> References: <46B35B48.9090907@qumranet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel To: "Avi Kivity" , "He, Qing" Return-path: Content-class: urn:content-classes:message In-Reply-To: <46B35B48.9090907-atKUWr5tajBWk0Htik3J/w@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 kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org wrote: > He, Qing wrote: >> KVM: in-kernel LAPIC save and restore support >> >> This patch adds a new vcpu-based IOCTL to save and restore the >> local apic registers for a single vcpu. The kernel only copies >> the apic page as a whole, extraction of registers is left to >> userspace side. On restore, the APIC timer is restarted from the >> initial count, this introduces a little delay, but works fine >> >> Signed-off-by: Yaozu (Eddie) Dong >> Signed-off-by: Qing He >> >> static long kvm_vcpu_ioctl(struct file *filp, >> unsigned int ioctl, unsigned long arg) >> { >> @@ -2805,6 +2826,31 @@ static long kvm_vcpu_ioctl(struct file *filp, >> r = 0; break; >> } >> + case KVM_GET_LAPIC: { >> + struct kvm_lapic_state lapic; >> + >> + memset(&lapic, 0, sizeof lapic); >> + r = kvm_vcpu_ioctl_get_lapic(vcpu, &lapic); >> + if (r) >> + goto out; >> + r = -EFAULT; >> + if (copy_to_user(argp, &lapic, sizeof lapic)) >> + goto out; >> + r = 0; >> + break; >> + } >> >> +/* for KVM_GET_LAPIC and KVM_SET_LAPIC */ >> +#define KVM_APIC_REG_SIZE 0x400 >> +struct kvm_lapic_state { >> + char regs[KVM_APIC_REG_SIZE]; >> +}; >> + >> > > kvm_lapic_state is 1KB in size, and you are allocating it on > the stack. > On i386, the stack can be 4KB, and you're allocating 25% of it... > How about we use dynamic allocate like kzalloc? > While it's true that this code path is short and can't be compounded > with others like I/O code paths, still it's not a good idea to > allocate so much stack space. I suggest defining kvm_lapic_state as > > #define KVM_LAPIC_NR_REGS 0x40 > struct kvm_lapic_state { > u32 regs[KVM_LAPIC_NR_REGS]; > }; > > That reduces the state size to 256 bytes. It could. The benefit of defining whole 1K register is that it is exactly same with real hardware which means we never need to worry about future data structure change no matter in user level or kernel level, they will always be able to fit into this state. Today kernel level use full hardware register state due to hardware acceleration consideration while user level use similar with what you suggested here. Leave to your decision :-) thx,eddie ------------------------------------------------------------------------- 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/