Re: [PATCH] KVM: Add CPUID support for VIA CPU

[Avi Kivity <avi@redhat.com>]

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