On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> From: Mukesh Rathor <[email protected]>
>
> In the bootup code for PVH we can trap cpuid via vmexit, so don't
> need to use emulated prefix call. We also check for vector callback
> early on, as it is a required feature. PVH also runs at default kernel
> IOPL.
>
> Finally, pure PV settings are moved to a separate function that are
> only called for pure PV, ie, pv with pvmmu. They are also #ifdef
> with CONFIG_XEN_PVMMU.
[...]
> @@ -331,12 +333,15 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
> break;
> }
>
> - asm(XEN_EMULATE_PREFIX "cpuid"
> - : "=a" (*ax),
> - "=b" (*bx),
> - "=c" (*cx),
> - "=d" (*dx)
> - : "0" (*ax), "2" (*cx));
> + if (xen_pvh_domain())
> + native_cpuid(ax, bx, cx, dx);
> + else
> + asm(XEN_EMULATE_PREFIX "cpuid"
> + : "=a" (*ax),
> + "=b" (*bx),
> + "=c" (*cx),
> + "=d" (*dx)
> + : "0" (*ax), "2" (*cx));
For this one off cpuid call it seems preferrable to me to use the
emulate prefix rather than diverge from PV.
> @@ -1431,13 +1449,18 @@ asmlinkage void __init xen_start_kernel(void)
>
> xen_domain_type = XEN_PV_DOMAIN;
>
> + xen_setup_features();
> + xen_pvh_early_guest_init();
> xen_setup_machphys_mapping();
>
> /* Install Xen paravirt ops */
> pv_info = xen_info;
> pv_init_ops = xen_init_ops;
> - pv_cpu_ops = xen_cpu_ops;
> pv_apic_ops = xen_apic_ops;
> + if (xen_pvh_domain())
> + pv_cpu_ops.cpuid = xen_cpuid;
> + else
> + pv_cpu_ops = xen_cpu_ops;
If cpuid is trapped for PVH guests why does PVH need non-native cpuid op?
David
On Thu, Jan 02, 2014 at 03:32:33PM +0000, David Vrabel wrote:
> On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> > From: Mukesh Rathor <[email protected]>
> >
> > In the bootup code for PVH we can trap cpuid via vmexit, so don't
> > need to use emulated prefix call. We also check for vector callback
> > early on, as it is a required feature. PVH also runs at default kernel
> > IOPL.
> >
> > Finally, pure PV settings are moved to a separate function that are
> > only called for pure PV, ie, pv with pvmmu. They are also #ifdef
> > with CONFIG_XEN_PVMMU.
> [...]
> > @@ -331,12 +333,15 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
> > break;
> > }
> >
> > - asm(XEN_EMULATE_PREFIX "cpuid"
> > - : "=a" (*ax),
> > - "=b" (*bx),
> > - "=c" (*cx),
> > - "=d" (*dx)
> > - : "0" (*ax), "2" (*cx));
> > + if (xen_pvh_domain())
> > + native_cpuid(ax, bx, cx, dx);
> > + else
> > + asm(XEN_EMULATE_PREFIX "cpuid"
> > + : "=a" (*ax),
> > + "=b" (*bx),
> > + "=c" (*cx),
> > + "=d" (*dx)
> > + : "0" (*ax), "2" (*cx));
>
> For this one off cpuid call it seems preferrable to me to use the
> emulate prefix rather than diverge from PV.
This was before the PV cpuid was deemed OK to be used on PVH.
Will rip this out to use the same version.
>
> > @@ -1431,13 +1449,18 @@ asmlinkage void __init xen_start_kernel(void)
> >
> > xen_domain_type = XEN_PV_DOMAIN;
> >
> > + xen_setup_features();
> > + xen_pvh_early_guest_init();
> > xen_setup_machphys_mapping();
> >
> > /* Install Xen paravirt ops */
> > pv_info = xen_info;
> > pv_init_ops = xen_init_ops;
> > - pv_cpu_ops = xen_cpu_ops;
> > pv_apic_ops = xen_apic_ops;
> > + if (xen_pvh_domain())
> > + pv_cpu_ops.cpuid = xen_cpuid;
> > + else
> > + pv_cpu_ops = xen_cpu_ops;
>
> If cpuid is trapped for PVH guests why does PVH need non-native cpuid op?
There are a couple of filtering done on the cpuid. But with HVM I am
not entirely sure if it is worth preserving those or not.
My fear is that if we switch over to the native one without the
filtering that the kernel does we open up a can of worms that had been
closed in the past. The reason is that for dom0 - there is no cpuid
filtering being done. So it gets everything that the hypervisor sees.
Which we don't want to do for APERF (b/c the generic scheduler code will
try to do those MSRs), and then there is the ACPI extended C-states.
Perhaps a better thing is just to still have the xen_cpuid but
but a big comment saying: "/* We should use native, but we need to
filter some cpuid's out. TODO */
?
>
> David
On Thu, 2 Jan 2014 13:32:21 -0500
Konrad Rzeszutek Wilk <[email protected]> wrote:
> On Thu, Jan 02, 2014 at 03:32:33PM +0000, David Vrabel wrote:
> > On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> > > From: Mukesh Rathor <[email protected]>
> > >
> > > In the bootup code for PVH we can trap cpuid via vmexit, so don't
> > > need to use emulated prefix call. We also check for vector
> > > callback early on, as it is a required feature. PVH also runs at
> > > default kernel IOPL.
> > >
> > > Finally, pure PV settings are moved to a separate function that
> > > are only called for pure PV, ie, pv with pvmmu. They are also
> > > #ifdef with CONFIG_XEN_PVMMU.
> > [...]
> > > @@ -331,12 +333,15 @@ static void xen_cpuid(unsigned int *ax,
> > > unsigned int *bx, break;
> > > }
> > >
> > > - asm(XEN_EMULATE_PREFIX "cpuid"
> > > - : "=a" (*ax),
> > > - "=b" (*bx),
> > > - "=c" (*cx),
> > > - "=d" (*dx)
> > > - : "0" (*ax), "2" (*cx));
> > > + if (xen_pvh_domain())
> > > + native_cpuid(ax, bx, cx, dx);
> > > + else
> > > + asm(XEN_EMULATE_PREFIX "cpuid"
> > > + : "=a" (*ax),
> > > + "=b" (*bx),
> > > + "=c" (*cx),
> > > + "=d" (*dx)
> > > + : "0" (*ax), "2" (*cx));
> >
> > For this one off cpuid call it seems preferrable to me to use the
> > emulate prefix rather than diverge from PV.
>
> This was before the PV cpuid was deemed OK to be used on PVH.
> Will rip this out to use the same version.
Whats wrong with using native cpuid? That is one of the benefits that
cpuid can be trapped via vmexit, and also there is talk of making PV
cpuid trap obsolete in the future. I suggest leaving it native.
Mukesh
On 02/01/14 18:32, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 02, 2014 at 03:32:33PM +0000, David Vrabel wrote:
>> On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
>>> From: Mukesh Rathor <[email protected]>
>>>
>>> In the bootup code for PVH we can trap cpuid via vmexit, so don't
>>> need to use emulated prefix call. We also check for vector callback
>>> early on, as it is a required feature. PVH also runs at default kernel
>>> IOPL.
>>>
>>> Finally, pure PV settings are moved to a separate function that are
>>> only called for pure PV, ie, pv with pvmmu. They are also #ifdef
>>> with CONFIG_XEN_PVMMU.
>> [...]
>>> @@ -331,12 +333,15 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>>> break;
>>> }
>>>
>>> - asm(XEN_EMULATE_PREFIX "cpuid"
>>> - : "=a" (*ax),
>>> - "=b" (*bx),
>>> - "=c" (*cx),
>>> - "=d" (*dx)
>>> - : "0" (*ax), "2" (*cx));
>>> + if (xen_pvh_domain())
>>> + native_cpuid(ax, bx, cx, dx);
>>> + else
>>> + asm(XEN_EMULATE_PREFIX "cpuid"
>>> + : "=a" (*ax),
>>> + "=b" (*bx),
>>> + "=c" (*cx),
>>> + "=d" (*dx)
>>> + : "0" (*ax), "2" (*cx));
>>
>> For this one off cpuid call it seems preferrable to me to use the
>> emulate prefix rather than diverge from PV.
>
> This was before the PV cpuid was deemed OK to be used on PVH.
> Will rip this out to use the same version.
>
>>
>>> @@ -1431,13 +1449,18 @@ asmlinkage void __init xen_start_kernel(void)
>>>
>>> xen_domain_type = XEN_PV_DOMAIN;
>>>
>>> + xen_setup_features();
>>> + xen_pvh_early_guest_init();
>>> xen_setup_machphys_mapping();
>>>
>>> /* Install Xen paravirt ops */
>>> pv_info = xen_info;
>>> pv_init_ops = xen_init_ops;
>>> - pv_cpu_ops = xen_cpu_ops;
>>> pv_apic_ops = xen_apic_ops;
>>> + if (xen_pvh_domain())
>>> + pv_cpu_ops.cpuid = xen_cpuid;
>>> + else
>>> + pv_cpu_ops = xen_cpu_ops;
>>
>> If cpuid is trapped for PVH guests why does PVH need non-native cpuid op?
>
> There are a couple of filtering done on the cpuid. But with HVM I am
> not entirely sure if it is worth preserving those or not.
I think we should behave like HVM for cpuid and any cpuid
policy/filtering should be set up by the toolstack.
> My fear is that if we switch over to the native one without the
> filtering that the kernel does we open up a can of worms that had been
> closed in the past. The reason is that for dom0 - there is no cpuid
> filtering being done. So it gets everything that the hypervisor sees.
I think we should switch to using the native cpuid pv-op and fix up any
problems as we encounter them (by fixing the toolstack to set up the
cpuid stuff properly).
dom0 isn't supported yet so that's not an issue. In the future dom0
could be handled by either: a) setting the cpuid policy in the
hypervisor during dom0 create; or b) the kernel can set this up during
early boot. In both cases using native cpuid should do the right thing.
David
On 03/01/14 01:34, Mukesh Rathor wrote:
> On Thu, 2 Jan 2014 13:32:21 -0500
> Konrad Rzeszutek Wilk <[email protected]> wrote:
>
>> On Thu, Jan 02, 2014 at 03:32:33PM +0000, David Vrabel wrote:
>>> On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
>>>> From: Mukesh Rathor <[email protected]>
>>>>
>>>> In the bootup code for PVH we can trap cpuid via vmexit, so don't
>>>> need to use emulated prefix call. We also check for vector
>>>> callback early on, as it is a required feature. PVH also runs at
>>>> default kernel IOPL.
>>>>
>>>> Finally, pure PV settings are moved to a separate function that
>>>> are only called for pure PV, ie, pv with pvmmu. They are also
>>>> #ifdef with CONFIG_XEN_PVMMU.
>>> [...]
>>>> @@ -331,12 +333,15 @@ static void xen_cpuid(unsigned int *ax,
>>>> unsigned int *bx, break;
>>>> }
>>>>
>>>> - asm(XEN_EMULATE_PREFIX "cpuid"
>>>> - : "=a" (*ax),
>>>> - "=b" (*bx),
>>>> - "=c" (*cx),
>>>> - "=d" (*dx)
>>>> - : "0" (*ax), "2" (*cx));
>>>> + if (xen_pvh_domain())
>>>> + native_cpuid(ax, bx, cx, dx);
>>>> + else
>>>> + asm(XEN_EMULATE_PREFIX "cpuid"
>>>> + : "=a" (*ax),
>>>> + "=b" (*bx),
>>>> + "=c" (*cx),
>>>> + "=d" (*dx)
>>>> + : "0" (*ax), "2" (*cx));
>>>
>>> For this one off cpuid call it seems preferrable to me to use the
>>> emulate prefix rather than diverge from PV.
>>
>> This was before the PV cpuid was deemed OK to be used on PVH.
>> Will rip this out to use the same version.
>
> Whats wrong with using native cpuid? That is one of the benefits that
> cpuid can be trapped via vmexit, and also there is talk of making PV
> cpuid trap obsolete in the future. I suggest leaving it native.
It should either use the PV interface or the HVM one, not a hybrid of
the two.
David
On Fri, 3 Jan 2014, David Vrabel wrote:
> On 03/01/14 01:34, Mukesh Rathor wrote:
> > On Thu, 2 Jan 2014 13:32:21 -0500
> > Konrad Rzeszutek Wilk <[email protected]> wrote:
> >
> >> On Thu, Jan 02, 2014 at 03:32:33PM +0000, David Vrabel wrote:
> >>> On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> >>>> From: Mukesh Rathor <[email protected]>
> >>>>
> >>>> In the bootup code for PVH we can trap cpuid via vmexit, so don't
> >>>> need to use emulated prefix call. We also check for vector
> >>>> callback early on, as it is a required feature. PVH also runs at
> >>>> default kernel IOPL.
> >>>>
> >>>> Finally, pure PV settings are moved to a separate function that
> >>>> are only called for pure PV, ie, pv with pvmmu. They are also
> >>>> #ifdef with CONFIG_XEN_PVMMU.
> >>> [...]
> >>>> @@ -331,12 +333,15 @@ static void xen_cpuid(unsigned int *ax,
> >>>> unsigned int *bx, break;
> >>>> }
> >>>>
> >>>> - asm(XEN_EMULATE_PREFIX "cpuid"
> >>>> - : "=a" (*ax),
> >>>> - "=b" (*bx),
> >>>> - "=c" (*cx),
> >>>> - "=d" (*dx)
> >>>> - : "0" (*ax), "2" (*cx));
> >>>> + if (xen_pvh_domain())
> >>>> + native_cpuid(ax, bx, cx, dx);
> >>>> + else
> >>>> + asm(XEN_EMULATE_PREFIX "cpuid"
> >>>> + : "=a" (*ax),
> >>>> + "=b" (*bx),
> >>>> + "=c" (*cx),
> >>>> + "=d" (*dx)
> >>>> + : "0" (*ax), "2" (*cx));
> >>>
> >>> For this one off cpuid call it seems preferrable to me to use the
> >>> emulate prefix rather than diverge from PV.
> >>
> >> This was before the PV cpuid was deemed OK to be used on PVH.
> >> Will rip this out to use the same version.
> >
> > Whats wrong with using native cpuid? That is one of the benefits that
> > cpuid can be trapped via vmexit, and also there is talk of making PV
> > cpuid trap obsolete in the future. I suggest leaving it native.
>
> It should either use the PV interface or the HVM one, not a hybrid of
> the two.
I agree
On Thu, Jan 02, 2014 at 05:34:38PM -0800, Mukesh Rathor wrote:
> On Thu, 2 Jan 2014 13:32:21 -0500
> Konrad Rzeszutek Wilk <[email protected]> wrote:
>
> > On Thu, Jan 02, 2014 at 03:32:33PM +0000, David Vrabel wrote:
> > > On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> > > > From: Mukesh Rathor <[email protected]>
> > > >
> > > > In the bootup code for PVH we can trap cpuid via vmexit, so don't
> > > > need to use emulated prefix call. We also check for vector
> > > > callback early on, as it is a required feature. PVH also runs at
> > > > default kernel IOPL.
> > > >
> > > > Finally, pure PV settings are moved to a separate function that
> > > > are only called for pure PV, ie, pv with pvmmu. They are also
> > > > #ifdef with CONFIG_XEN_PVMMU.
> > > [...]
> > > > @@ -331,12 +333,15 @@ static void xen_cpuid(unsigned int *ax,
> > > > unsigned int *bx, break;
> > > > }
> > > >
> > > > - asm(XEN_EMULATE_PREFIX "cpuid"
> > > > - : "=a" (*ax),
> > > > - "=b" (*bx),
> > > > - "=c" (*cx),
> > > > - "=d" (*dx)
> > > > - : "0" (*ax), "2" (*cx));
> > > > + if (xen_pvh_domain())
> > > > + native_cpuid(ax, bx, cx, dx);
> > > > + else
> > > > + asm(XEN_EMULATE_PREFIX "cpuid"
> > > > + : "=a" (*ax),
> > > > + "=b" (*bx),
> > > > + "=c" (*cx),
> > > > + "=d" (*dx)
> > > > + : "0" (*ax), "2" (*cx));
> > >
> > > For this one off cpuid call it seems preferrable to me to use the
> > > emulate prefix rather than diverge from PV.
> >
> > This was before the PV cpuid was deemed OK to be used on PVH.
> > Will rip this out to use the same version.
>
> Whats wrong with using native cpuid? That is one of the benefits that
> cpuid can be trapped via vmexit, and also there is talk of making PV
> cpuid trap obsolete in the future. I suggest leaving it native.
I chatted with David, Andrew and Roger on IRC about this. I like the
idea of using xen_cpuid because:
1) It filters some of the CPUID flags that guests should not use. There is
the 'aperfmperf,'x2apic', 'xsave', and whether the MWAIT_LEAF
should be exposed (so that the ACPI AML code can call the right
initialization code to use the extended C3 states instead of the
legacy IOPORT ones). All of that is in xen_cpuid.
2) It works, while we can concentrate on making 1) work in the
hypervisor/toolstack.
Meaning that the future way would be to use the native cpuid and have
the hypervisor/toolstack setup the proper cpuid. In other words - use
the xen_cpuid as is until that code for filtering is in the hypervisor.
Except that PVH does not work the PV cpuid at all. I get a triple fault.
The instruction it fails at is at the 'XEN_EMULATE_PREFIX'.
Mukesh, can you point me to the patch where the PV cpuid functionality
is enabled?
Anyhow, as it stands, I will just use the native cpuid.
>
> Mukesh
>
On Fri, 3 Jan 2014 12:35:55 -0500
Konrad Rzeszutek Wilk <[email protected]> wrote:
> On Thu, Jan 02, 2014 at 05:34:38PM -0800, Mukesh Rathor wrote:
> > On Thu, 2 Jan 2014 13:32:21 -0500
> > Konrad Rzeszutek Wilk <[email protected]> wrote:
> >
> > > On Thu, Jan 02, 2014 at 03:32:33PM +0000, David Vrabel wrote:
> > > > On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> > > > > From: Mukesh Rathor <[email protected]>
> > > > >
> > > > > In the bootup code for PVH we can trap cpuid via vmexit, so
> > > > > don't need to use emulated prefix call. We also check for
> > > > > vector callback early on, as it is a required feature. PVH
> > > > > also runs at default kernel IOPL.
> > > > >
> > > > > Finally, pure PV settings are moved to a separate function
> > > > > that are only called for pure PV, ie, pv with pvmmu. They are
> > > > > also #ifdef with CONFIG_XEN_PVMMU.
> > > > [...]
> > > > > @@ -331,12 +333,15 @@ static void xen_cpuid(unsigned int *ax,
> > > > > unsigned int *bx, break;
> > > > > }
> > > > >
> > > > > - asm(XEN_EMULATE_PREFIX "cpuid"
> > > > > - : "=a" (*ax),
> > > > > - "=b" (*bx),
> > > > > - "=c" (*cx),
> > > > > - "=d" (*dx)
> > > > > - : "0" (*ax), "2" (*cx));
> > > > > + if (xen_pvh_domain())
> > > > > + native_cpuid(ax, bx, cx, dx);
> > > > > + else
> > > > > + asm(XEN_EMULATE_PREFIX "cpuid"
> > > > > + : "=a" (*ax),
> > > > > + "=b" (*bx),
> > > > > + "=c" (*cx),
> > > > > + "=d" (*dx)
> > > > > + : "0" (*ax), "2" (*cx));
> > > >
> > > > For this one off cpuid call it seems preferrable to me to use
> > > > the emulate prefix rather than diverge from PV.
> > >
> > > This was before the PV cpuid was deemed OK to be used on PVH.
> > > Will rip this out to use the same version.
> >
> > Whats wrong with using native cpuid? That is one of the benefits
> > that cpuid can be trapped via vmexit, and also there is talk of
> > making PV cpuid trap obsolete in the future. I suggest leaving it
> > native.
>
> I chatted with David, Andrew and Roger on IRC about this. I like the
> idea of using xen_cpuid because:
> 1) It filters some of the CPUID flags that guests should not use.
> There is the 'aperfmperf,'x2apic', 'xsave', and whether the MWAIT_LEAF
> should be exposed (so that the ACPI AML code can call the right
> initialization code to use the extended C3 states instead of the
> legacy IOPORT ones). All of that is in xen_cpuid.
>
> 2) It works, while we can concentrate on making 1) work in the
> hypervisor/toolstack.
>
> Meaning that the future way would be to use the native cpuid and have
> the hypervisor/toolstack setup the proper cpuid. In other words - use
> the xen_cpuid as is until that code for filtering is in the
> hypervisor.
>
>
> Except that PVH does not work the PV cpuid at all. I get a triple
> fault. The instruction it fails at is at the 'XEN_EMULATE_PREFIX'.
>
> Mukesh, can you point me to the patch where the PV cpuid functionality
> is enabled?
>
> Anyhow, as it stands, I will just use the native cpuid.
I am referring to using "cpuid" instruction instead of XEN_EMULATE_PREFIX.
cpuid is faster and long term better... there is no benefit using
XEN_EMULATE_PREFIX IMO. We can look at removing xen_cpuid() altogether for
PVH when/after pvh 32bit work gets done IMO.
The triple fault seems to be a new bug... I can create a bug, but for
now, with using cpuid instruction, that won't be an issue.
thanks
mukesh