Re: [Qemu-devel] [PATCH] target-i386: add pcid to both Sandy Bridge and Ivy Bridge

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Jan 08, 2018 at 10:51:48PM +0100, Vincent Bernat wrote:
>  ❦  8 janvier 2018 19:16 -0200, Eduardo Habkost <ehabkost@redhat.com> :
> 
> > One possible way to work around this problem is to declare that
> > QEMU 2.12 with KVM will require Linux v3.6 and newer (because we
> > need Linux kernel commit ad756a1603c5 "KVM: VMX: Implement
> > PCID/INVPCID for guests with EPT").  I have proposed something
> > similar to allow us to enable kvm_pv_eoi by default, some time
> > ago:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg486559.html
> > ("qemu-doc: Document minimum kernel version for KVM in x86_64").
> 
> I don't see a way to probe KVM to know what's supported, so yes.

We do have a way to probe KVM: GET_SUPPORTED_CPUID.  The problem
here is breaking libvirt and management software expectations.

libvirt assumes "stable runnability": a CPU model that is
runnable on a host using QEMU/machine-type version will stay
runnable on the same host after a QEMU or machine-type upgrade.


> Should
> I add a paragraph similar to yours or would your patch be merged soon?

My patch was dropped because we decided to wait a bit before
enabling kvm_pv_eoi by default.  My paragraph could be improved
by a description of what could happen if an older kernel version
is used (see below).


> What are the consequences of running a too old kernel? Would KVM just
> hide PCID flag?

On an old kernel, the SandyBridge and IvyBridge CPU models will
be unexpectedly become not runnable.


> 
> > Second, we need compatibility entries setting pcid=off on
> > PC_COMPAT_2_10 so we don't break compatibility on older
> > machine-types.

(Oops, I should have said PC_COMPAT_2_11 here)

> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 6f77eb066587..da5bd8304eb0 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -327,6 +327,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "x-hv-max-vps",\
>          .value    = "0x40",\
> +    },{\
> +        .driver   = "SandyBridge-" TYPE_X86_CPU,\
> +        .property = "pcid",\
> +        .value    = "off",\
> +    },{\
> +        .driver   = "IvyBridge-" TYPE_X86_CPU,\
> +        .property = "pcid",\
> +        .value    = "off",\
>      },{\
>          .driver   = "i440FX-pcihost",\
>          .property = "x-pci-hole64-fix",\

This is correct, but it should be done on PC_COMPAT_2_11 instead
(sorry for my confusion above).

If you don't find PC_COMPAT_2_11 on master, please look for the
"pc: add 2.12 machine types" patch.  I thought it was already
merged on master.  I just queued it on my x86-next tree[1].


[1] https://github.com/ehabkost/qemu x86-next

> 
> I'll resend a proper patch once the first point is cleared.

Thanks!

-- 
Eduardo