Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov <gleb@redhat.com>]

On Sun, Apr 21, 2013 at 01:30:51PM +0200, Borislav Petkov wrote:
> On Sun, Apr 21, 2013 at 12:46:51PM +0300, Gleb Natapov wrote:
> > There may be userspaces that set ->function to 0xffffffff (just
> > because they do not init the buffer before calling into the kernel)
> > and this will break them.
> 
> You don't mean 0xffffffff here but rather something random which is not
> properly initialized and by chance is a valid CPUID leaf. Then, if some
> of the bits in the register variables e[abcd]x are also set, we return
> with emulated bits set.
> 
Yes.

> > > For each set bit, we check whether we emulate the respective feature
> > > and if so, we leave it untouched before returning it to userspace.
> > > Otherwise, we clear it before OR-ing in the host bits and the
> > > good-emulated bits like x2apic.
> > > 
> > > Yeah, semantics need to be handled carefully, but it has this
> > > knock-on-door aspect where kvm says that it actually emulates a feature
> > > only if asked, i.e. with the -cpu ...,+<feature> syntax.
> > > 
> > > * new ioctl KVM_GET_EMULATED_CPUID
> > > 
> > > Might be overkill and might be used only in a limited fashion since we
> > > don't want to emulate *every* feature in kvm.
> > > 
> > > Hmmm. I kinda like the first one more while the second one is cleaner.
> > > 
> > The first one needs explicit userspace support to work correctly. This
> > should be other way around: old userspace should do the right thing, but may
> > not support new features, new userspace should be able to support new
> > feature.
> 
> Crap, not even qemu is handing in cleared buffers with that g_malloc0()
> thing, AFAICT.
> 
> > We may extend KVM_GET_SUPPORTED_CPUID instead of adding new one. There
> > is a padding field in kvm_cpuid2 that we could have reused as flags,
> 
> That would've been lovely.
> 
> > but unfortunately current implementation does not error if the field is
> > not zero, so if there is a userspace that does not zero the padding it may
> > break. Another options is to reuse high bits of nent as flags (not very
> > nice, but will work).
> 
> Not nice?! It is a very nasty hack - that's what it is. :-)
> 
Agree, but not nastier than expecting ->function to have special value :)

> Frankly speaking, I'd rather prefer adding a new ioctl. Since old
> userspace won't support the new features, then we just as well can
> simply add the new ioctl.
> 
> In all the cases we need to touch userspace - be it to OR in the flags
> into ->nent or to implement the new ioctl. So why not do it in a clean
> manner from the get-go?
> 
Agree here too.

--
			Gleb.