From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] KVM: Add CPUID support for VIA CPU Date: Thu, 14 Apr 2011 13:07:01 +0300 Message-ID: <4DA6C745.9070406@redhat.com> References: <4DA565D8.8050504@redhat.com> <4DA589DF.8060002@redhat.com> <4DA6A6C4.4040507@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: mtosatti@redhat.com, kvm@vger.kernel.org To: BrillyWu@viatech.com.cn Return-path: Received: from mx1.redhat.com ([209.132.183.28]:19468 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758694Ab1DNKHF (ORCPT ); Thu, 14 Apr 2011 06:07:05 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 04/14/2011 12:54 PM, BrillyWu@viatech.com.cn wrote: > > On 04/14/2011 06:14 AM, BrillyWu@viatech.com.cn wrote: > > > On 04/13/2011 02:05 PM, BrillyWu@viatech.com.cn wrote: > > > > > >> > > > > > >> + /* cpuid 0xC0000001.edx */ > > > > > >> + const u32 kvm_supported_word5_x86_features = > > > > > >> + F(XSTORE) | F(XSTORE_EN) | > > F(XCRYPT) | F(XCRYPT_EN) | > > > > > >> + F(ACE2) | F(ACE2_EN) | F(PHE) | > > F(PHE_EN) | > > > > > >> + F(PMM) | F(PMM_EN); > > > > > >> + > > > > > > > > > > Are all of these features save wrt save/restore? > > (do they all act > > > > > >on state in standard registers?) Do they need any control > > > > register> >bits to be active or MSRs to configure? > > > > > > > > > These features depend on instructions for the PadLock > > hardware engine of VIA CPU. > > > > > The PadLock instructions just act on standard > > registers like general X86 instructions, and the features have been > > enabled when the CPU leave the factory, so there is no need to > > activate any control register bits or configure MSRs. > > > > > > > I see there is a dependency on EFLAGS[30]. Does a VM > > entry clear this bit? If not, we have to do it ourselves. > > > > > > Yes, PadLock hardware engine has some association with > > EFLAGS[30], but it just required that the EFLAGS[30] should be set to > > "0" > > > before using PadLock ACE instructions. It is recommanded > > that execute > > > instruction sequence "pushf;popf" to clear this bit before > > using ACE instructions. > > > AFAIK, the VM entry never sets the EFLAGS[30] bit, so it > > seems that we do not have to do it ourselves. > > > > I don't think we need to. kvm kernel code doesn't use padlock; other > > sources which might set EFLAGS[30] are: > > > > - the guest; but VMEXIT clears EFLAGS > > - userspace; but syscall/sysenter/int instructions clear EFLAGS[30] > > - another kernel thread; there is likely a popf in the context switch > > path somewhere (we should verify this) > > Thank you very much for telling me where the EFLAGS[30] might > be set and cleared. > In fact, adding the CPUID support into kvm > kernel code is just to provide correct result for the calling of > the "kvm_arch_get_supported_cpuid" function in Qemu-kvm > application. That may not be sufficient for correct operation. Consider: - guest executes a padlock instruction - cpu sets EFLAGS[30] - external interrupt - VMEXIT (saves EFLAGS in GUEST_RFLAGS with EFLAGS[30] set) - external interrupt is processed, causes a task switch - EFLAGS[30] is cleared - some other process uses padlock instructions, which causes a reload of key information - switch back to kvm - VM entry (loads EFLAGS from GUEST_RFLAGS; still has EFLAGS[30] set) - guest executes following padlock instruction, doesn't reload key information so I think the code as is causes data corruption. > There is no dependency on EFLAGS when calling VIA CPUID > instruction. > > Before using padlock engine functions, the application first need detect > is the features exist through cpuid instruction, then use ACE and other > instructions such as PHE/RNG/PMM. > > And as previously said, only the ACE instructions required > that the EFLAGS[30] should be set to "0", It is recommanded > that execute instruction sequence "pushf;popf" to clear this bit > before using ACE instructions. The problem is that VM entry reloads eflags from saved state and is not affected by popf. > I have re-submitted this patch, please check it. Thanks! wrt cpuid it seems reasonable but that's we need to clear the EFLAGS[30] issue first. -- error compiling committee.c: too many arguments to function